jsmi - New top pool weights apply to the time before it has been set.

[sherlock-admin3]

jsmi

Medium

New top pool weights apply to the time before it has been set.

Summary

The top pool weights are used to calculate the farm rewards using accrued debt per share of pools.
Voter.sol#setTopPoolIdsWithWeights update the top pool weights but doesn't update the accrued debt per share of pools.
Therefore the newly set pool weights may apply to the time before it has been set.

Vulnerability Detail

Scenario:

1. There are only two pools: pool1 with weight 20% and pool2 with weight 80% respectively.
2. There are no activities changing the accrued debt per share of pools for some time for instance 1 hour.
3. After that, the owner set the pools weight again: pool1 with 80% and pool2 with 20%.
4. However the rewards of master chef are distributed to the pools for the very 1 hour such that 80% for pool1 and 20% for pool2.

Impact

New top pool weights apply to the time before it has been set.

Code Snippet

• magicsea-staking/src/Voter.sol#L260-L298

Tool used

Manual Review

Recommendation

Modify the Voter.sol#setTopPoolIdsWithWeights 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(oldIds);

        ......
    }

Duplicate of #107

[Oot2k]

Escalate

I believe this issue lacks valid POC and description of impact, which makes this issue invalid.

[sherlock-admin3]

Escalate

I believe this issue lacks valid POC and description of impact, which makes this issue invalid.

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.

[jsmi0703]

@Oot2k

First, The following example scenario could replace the POC since it is fully clear to everyone.

Scenario:

1. There are only two pools: pool1 with weight 20% and pool2 with weight 80% respectively.
2. There are no activities changing the accrued debt per share of pools for some time for instance 1 hour.
3. After that, the owner set the pools weight again: pool1 with 80% and pool2 with 20%.
4. However the rewards of master chef are distributed to the pools for the very 1 hour such that 80% for pool1 and 20% for pool2.

Furthermore, I think that not all valid issues are required to have POC.
Of course it will be better to have coded POC like #107 and that's why this issue is a duplicate and not the original target issue.

Second, please compare impact of this issue:

New top pool weights apply to the time before it has been set.

with the impact of original issue #107:

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.

As you can see, they essentially stated the same impact about the new top pools (not old ones) for the past time.
Anyway, the conclusion on validity of this issue is up to the judge.

[WangSecurity]

Yep, I agree that not all issues require POCs, only the ones that fall under the POC requirements. Additionally, POC can be either coded or written, so here it's sufficient I believe.

I agree that the impact inside the Impact section is not very convincing, but the 4th step of the vulnerability path explains it well enough:

However the rewards of master chef are distributed to the pools for the very 1 hour such that 80% for pool1 and 20% for pool2

Hence, I believe it's sufficient duplicate, planning to reject the escalation and leave the issue as it is.

[WangSecurity]

Result:
High
Duplicate of #107

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• PUSH0: rejected