jsmi - A voter lose bribe rewards if another voter voted before claim.

[sherlock-admin3]

jsmi

High

A voter lose bribe rewards if another voter voted before claim.

Summary

A voter lose bribe rewards if another voter voted before claim.

Vulnerability Detail

This problem is related to design architecture. In BribeRewarder.sol, the _lastUpdateTimestamp is used to calculate the unclaimed rewards for periodId, but it is not dependent on periodId. Therefore, once _lastUpdateTimestamp has been updated to the next period, there is no way to calculate the unclaimed rewards for the previous period.

The following is the modified test code for PoC.

    function testDepositMultiple() public {
        ERC20Mock(address(rewardToken)).mint(address(this), 20e18);
        ERC20Mock(address(rewardToken)).approve(address(rewarder), 20e18);

        rewarder.fundAndBribe(1, 2, 10e18);

        _voterMock.setCurrentPeriod(1);
        _voterMock.setStartAndEndTime(0, 100);

        // time: 0
        vm.warp(0);
        vm.prank(address(_voterMock));
        rewarder.deposit(1, 1, 0.2e18);

        assertEq(0, rewarder.getPendingReward(1));

        // time: 50, seconds join
        vm.warp(50);
        vm.prank(address(_voterMock));
        rewarder.deposit(1, 2, 0.2e18);

        // time: 100
        vm.warp(100);
        _voterMock.setCurrentPeriod(2);
        _voterMock.setStartAndEndTime(0, 100);
        _voterMock.setLatestFinishedPeriod(1);

        // @audit-info next period started
        vm.warp(150);

        // 1 -> [0,50] -> 1: 0.5
        // 2 -> [50,100] -> 1: 0.25 + 0.5, 2: 0.25

        assertEq(7500000000000000000, rewarder.getPendingReward(1));
        assertEq(2500000000000000000, rewarder.getPendingReward(2));

        // @audit-info Another voter votes before claim.
        vm.prank(address(_voterMock));
        rewarder.deposit(2, 3, 0.1e18);

        // @audit-info The expected rewards decreased much
        assertEq(5000000000000000000, rewarder.getPendingReward(1));
        assertEq(0, rewarder.getPendingReward(2));

        vm.prank(alice);
        rewarder.claim(1);

        vm.prank(bob);
        rewarder.claim(2);

        // @audit-info The claimed rewards decreased too.
        assertEq(5000000000000000000, rewardToken.balanceOf(alice));
        assertEq(0, rewardToken.balanceOf(bob));

        assertEq(7500000000000000000, rewardToken.balanceOf(alice), "balance of alice should be 75e17 but 50e17");
        assertEq(2500000000000000000, rewardToken.balanceOf(bob), "balance of bob should be 25e17 but 0");
    }

The claimed rewards amount of alice and bob for 1st period are originally 75e17 and 25e17, respectively. But if a voter votes for 2nd period before alice and bob claim their rewards for 1st period, the claimed rewards amount of alice and bob will be decreased to 50e17 and zero, respectively. It means that the rewards for [50,100] of 1st period are will not be claimed.

And the following is the test command and result.

$ forge test --match-test testDepositMultiple

Failing tests:
Encountered 1 failing test in test/BribeRewarder.t.sol:BribeRewarderTest
[FAIL. Reason: balance of alice should be 75e17 but 50e17: 7500000000000000000 != 5000000000000000000] testDepositMultiple() (gas: 767761)

As shown above, if anyone votes before alice and bob claim their rewards, the rewards of alice and bob will be decreased.

Impact

Voters lose bribe rewards if another voter voted before claim. And such cases can occur frequently enough.

Code Snippet

• magicsea-staking/src/rewarders/BribeRewarder.sol#L65
• magicsea-staking/src/rewarders/BribeRewarder.sol#L260-L298

Tool used

Manual Review

Recommendation

Change the _lastUpdateTimestamp state variable to be dependent on periodId. For instance, change it to mapping variable such as mapping(uint256 periodId => uint256) _lastUpdateTimestamp;.

[0xSmartContract]

High severity because this will occur without an attacker and between all Bribe Rewards, resulting in reward loss for all voters. A side effect of this is that rewards sent to Bribe Rewards will be stuck in the contract forever.

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/metropolis-exchange/magicsea-staking/pull/20

[chinmay-farkya]

Escalate

The first thing to remember is that this issue is different from undistributed/ unaccrued rewards getting stuck in the contract due to many reasons like : no voting, precision loss etc. That is a different set of issue reported elsewhere and is meant for “undistributed rewards”. Whereas the current issue set #52 describes a bug that the rewards that users had “already accrued” can not be claimed by them, so this is a problem of claiming accrued rewards rather than undistributed rewards. Fixing either of them will not fix the other, so these are two different sets of issues.

Now lets look at some issues wrongly duped with this #52

303 is not a duplicate and is invalid. When the first call is made totalRewards returned = 0 because totalSupply = 0 and in this first call while setting the reward parameters rewarder.lastUpdateTimestamp gets updated to the startTimestamp of the reward cycle. So the assertion made in this submission “rewards calculated for time when lastUpdateTimestamp was zero will be huge” is wrong. For all later calls, this actual non-zero lastUpdateTimestamp will be used, so the calculation fo rewards is correct.

397 is not a duplicate and is invalid because even though the totalRewards calculation is wrong it has no real impact as the “totalRewards” are manifested into storage using rewarder.update which returns 0 because the oldTotalSupply was zero in the mentioned case of first voter in a briberewarder’s bribe periods. Have a look at rewarder.update here. So oldTotalSupply and oldBalance being zero, this rewards calculation is completely ignored i current logic and no one can earn extra rewards as claimed in the issue. Hence invalid.

496 is a duplicate of #164 (note :also read the escalation on #164 as this issue #496 overall describes “undistributed rewards will be stuck” is a duplicate of set 2 of the issues mentioned in that escalation) because some of the issues in set 2 of #164 also describe precision loss as a way of having funds stuck in the contract. For example see #172 also mentioned in the escalation on #164.

582 is also not a duplicate and is invalid because the bug has no impact due to similar reason as #397 as explained above.

588 is invalid due to the same reasons as #397. Ping me if you want to discuss this bug more.

607 is again invalid. It goes over the same assertion that rewards calculation will yield a huge value because lastUpdateTimestamp is a time in past before the period started, but misses the point that this bug never materializes due to how the rewarder2 library works. If the totalsupply before the call was zero (which means this is the first call) these “totalRewards” never accrue in the accDebtPerShare and hence are not distributed. On the next call, these are accrued correctly based on the time interval from this first call to the second call. So the accrued rewards never includes the rewards calculated for time before the first bribe period started. Hence, similar to #397 and invalid.

356 is not a duplicate of this but a duplicate of #164 set 2. It talks about a portion of rewards that was undistributed getting stuck in the BribeRewarder contract. See comment for #496 above.

[sherlock-admin3]

Escalate

The first thing to remember is that this issue is different from undistributed/ unaccrued rewards getting stuck in the contract due to many reasons like : no voting, precision loss etc. That is a different set of issue reported elsewhere and is meant for “undistributed rewards”. Whereas the current issue set #52 describes a bug that the rewards that users had “already accrued” can not be claimed by them, so this is a problem of claiming accrued rewards rather than undistributed rewards. Fixing either of them will not fix the other, so these are two different sets of issues.

Now lets look at some issues wrongly duped with this #52

303 is not a duplicate and is invalid. When the first call is made totalRewards returned = 0 because totalSupply = 0 and in this first call while setting the reward parameters rewarder.lastUpdateTimestamp gets updated to the startTimestamp of the reward cycle. So the assertion made in this submission “rewards calculated for time when lastUpdateTimestamp was zero will be huge” is wrong. For all later calls, this actual non-zero lastUpdateTimestamp will be used, so the calculation fo rewards is correct.

397 is not a duplicate and is invalid because even though the totalRewards calculation is wrong it has no real impact as the “totalRewards” are manifested into storage using rewarder.update which returns 0 because the oldTotalSupply was zero in the mentioned case of first voter in a briberewarder’s bribe periods. Have a look at rewarder.update here. So oldTotalSupply and oldBalance being zero, this rewards calculation is completely ignored i current logic and no one can earn extra rewards as claimed in the issue. Hence invalid.

496 is a duplicate of #164 (note :also read the escalation on #164 as this issue #496 overall describes “undistributed rewards will be stuck” is a duplicate of set 2 of the issues mentioned in that escalation) because some of the issues in set 2 of #164 also describe precision loss as a way of having funds stuck in the contract. For example see #172 also mentioned in the escalation on #164.

582 is also not a duplicate and is invalid because the bug has no impact due to similar reason as #397 as explained above.

588 is invalid due to the same reasons as #397. Ping me if you want to discuss this bug more.

607 is again invalid. It goes over the same assertion that rewards calculation will yield a huge value because lastUpdateTimestamp is a time in past before the period started, but misses the point that this bug never materializes due to how the rewarder2 library works. If the totalsupply before the call was zero (which means this is the first call) these “totalRewards” never accrue in the accDebtPerShare and hence are not distributed. On the next call, these are accrued correctly based on the time interval from this first call to the second call. So the accrued rewards never includes the rewards calculated for time before the first bribe period started. Hence, similar to #397 and invalid.

356 is not a duplicate of this but a duplicate of #164 set 2. It talks about a portion of rewards that was undistributed getting stuck in the BribeRewarder contract. See comment for #496 above.

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.

[0xSmartContract]

303 , #397 , #587 ; The are not a duplicate and is invalid (I aggre)

582 ; This is not a duplicate and is invalid (I aggre)

Issue #582 was related to an incorrect update inside the function _lastUpdateTimestamp _modify(). This was not actually a problem in the code as implemented. Issue #52 highlights a different and real issue in the design of the reward calculation system. This issue relates to how the single _lastUpdateTimestamp variable is used across different periods, which could lead to a loss of rewards for some users.

2 different sets ; I don't agree

[chinmay-farkya]

@0xSmartContract

2 different sets ; I don't agree

I think there is some misunderstanding. I did not say that this issue has 2 sets. I only pointed out specific issues which are invalid and wrongly duped with this. Rest assured, this is only a single issue and not two sets, but please go over the invalid ones and remove them from the dups (list mentioned in original escalation).

[0xSmartContract]

@0xSmartContract

2 different sets ; I don't agree

I think there is some misunderstanding. I did not say that this issue has 2 sets. I only pointed out specific issues which are invalid and wrongly duped with this. Rest assured, this is only a single issue and not two sets, but please go over the invalid ones and remove them from the dups (list mentioned in original escalation).

I already said "I don't agree" for the set part, so my "I aggre" here can be taken into account.

https://github.com/sherlock-audit/2024-06-magicsea-judging/issues/52#issuecomment-2267165669

[WangSecurity]

I agree with the escalation and plan to accept it. Here are the changes I'm going to apply:

1. Invalidate #303, #397, #582, #588, #607.

2. 496 is escalated as well, I agree it's not a duplicate of #52 and will be considered during the #164 escalation.

3. Same for #356.

[WangSecurity]

Result: High Has duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• chinmay-farkya: accepted

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.