dany.armstrong90 - Total LUM rewards may be distributed for exeeding the _lumPerSecond.

[sherlock-admin2]

dany.armstrong90

High

Total LUM rewards may be distributed for exeeding the _lumPerSecond.

Summary

LUM rewards for a pool are calculated by elapsed time from lastUpdateTimestamp and weights[pid].
Voter.sol#setTopPoolIdsWithWeights function updates the weights[pid] but not update the lastUpdateTimestamp of the pool to the block.timestamp.
Therefore, the total LUM rewards may exceed the _lumPerSecond due to the inflation of elapsed time from lastUpdateTime.

Vulnerability Detail

Scenario:

1. Assume that there are one top pools poolA with weight 100% at time T0.
2. Assume that there is activity on poolA at time T1( > T0), thus the LUM rewards are distributed _lumPerSecond (100%) per second to poolA for time [T0, T1] and the lastUpdateTimestamp of poolA is T1.
3. Assume that there is no activity on poolB during the time [T0, T1], thus the lastUpdateTimestamp of poolB is T0.
4. Assume that administrator update the poolA's weight to 0% and poolB's weight to 100% at time T2 ( > T1).
5. Assume that there is activity on poolB at time T3( > T2). Since the lastUpdateTimestamp of poolB is T0, the Total LUM rewards are distributed _lumPerSecond (100%) per second for the time [T0, T1].
6. As a result, the Total LUM rewards are distributed _lumPerSecond * 2 (200%) per second for the time [T0, T1].

Impact

Total LUM rewards may be distributed for exeeding the _lumPerSecond.

Code Snippet

https://github.com/sherlock-audit/2024-06-magicsea/tree/main/magicsea-staking/src/Voter.sol#L260-L298

Tool used

Manual Review

Recommendation

Modify the Voter.sol#setTopPoolIdsWithWeights function as follows.

    function setTopPoolIdsWithWeights(uint256[] calldata pids, uint256[] calldata weights) external {
        if (msg.sender != _operator) _checkOwner();

        uint256 length = pids.length;
        if (length != weights.length) revert IVoter__InvalidLength();

        uint256[] memory oldIds = _topPids.values();

++      _masterChef.updateAll(oldPids);
++      _masterChef.updateAll(pids);


        if (oldIds.length > 0) {
            // masterchef snould be updated beforehand

            for (uint256 i = oldIds.length; i > 0;) {
                uint256 pid = oldIds[--i];

                _topPids.remove(pid);
                _weights[pid] = 0;
            }
        }

        for (uint256 i; i < length; ++i) {
            uint256 pid = pids[i];
            if (!_topPids.add(pid)) revert IVoter__DuplicatePoolId(pid);
        }

        uint256 totalWeights;
        for (uint256 i; i < length; ++i) {
            uint256 pid = pids[i];

            uint256 weight = weights[i];

            _weights[pid] = weight;

            totalWeights += weight;
        }

        _topPidsTotalWeights = totalWeights;

        emit TopPoolIdsWithWeightsSet(pids, weights);
    }

Duplicate of #107

[web3-master]

Escalate
I think the this issue(Total LUM rewards may be distributed for exeeding the _lumPerSecond) is misjudged and I propose to escalate this issue.
#90 is now incorrectly classified as the duplicate of the #177. As I see, it has no common root cause and impact at all.
I think it should be duplicate of the PUSH0's #107 issue(Wrong call order for setTopPoolIdsWithWeights, resulting in wrong distribution of rewards) .
The PoC of #107 is equal to example of this issue and the impact and the recommendation is exactly the same.

[sherlock-admin3]

Escalate
I think the this issue(Total LUM rewards may be distributed for exeeding the _lumPerSecond) is misjudged and I propose to escalate this issue.
#90 is now incorrectly classified as the duplicate of the #177. As I see, it has no common root cause and impact at all.
I think it should be duplicate of the PUSH0's #107 issue(Wrong call order for setTopPoolIdsWithWeights, resulting in wrong distribution of rewards) .
The PoC of #107 is equal to example of this issue and the impact and the recommendation is exactly the same.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[Oot2k]

I think this report is missing key information like POC and evidence from docs as well as code. For this reason it should not be considered a duplicate.
As per sherlock rules:
Fulfills other submission quality requirements
which is not the case here.

[web3-master]

1. POC is optional especially when the point and argument is obvious.
2. The vulnerability of the issue is from the code but not from comment, so even though there is no comment, the issue is valid.
3. The issue has the exactly the same root cause, vulnerability path and impact. It can be seen from the following.

Impact of #107 (the target issue):
Pools that have just made it into the top pools will have already accrued rewards for time intervals it wasn't in the top pools. Rewards are thus severely inflated.

Impact of #175 (another duplicate of #107):
Setting top pool's weight will cause LUM rewards minting more than expected.

[blockchain555]

Can you show me POC?

[WangSecurity]

I agree this is indeed the duplicate of #107 and not #177. And as correctly mentioned above, POC is not mandatory for each issue and I don’t think it’s mandatory for this issue. Additionally, this report has a textual POC which is also sufficient.

Hence, planning to accept the escalation and duplicate with #107

[WangSecurity]

Result:
High
Duplicate of #107

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• web3-master: accepted