User get more reward from BribeRewarder

Summary

Wrong number of reward per period is created, causing the users to claim reward 1 more extra time

Vulnerability Detail

In the function BribeRewarder#_bribe.

...
    function _bribe(uint256 startId, uint256 lastId, uint256 amountPerPeriod) internal {
        _checkAlreadyInitialized();
        if (lastId < startId) revert BribeRewarder__WrongEndId();
        if (amountPerPeriod == 0) revert BribeRewarder__ZeroReward();

        IVoter voter = IVoter(_caller);

        if (startId <= voter.getCurrentVotingPeriod()) {
            revert BribeRewarder__WrongStartId();
        }

        uint256 totalAmount = _calcTotalAmount(startId, lastId, amountPerPeriod);

        uint256 balance = _balanceOfThis(_token());

        if (balance < totalAmount) revert BribeRewarder__InsufficientFunds();

        _startVotingPeriod = startId;
        _lastVotingPeriod = lastId;
        _amountPerPeriod = amountPerPeriod;

        // create rewads per period
        uint256 bribeEpochs = _calcPeriods(startId, lastId);
        for (uint256 i = 0; i <= bribeEpochs; ++i) { // @audit-issue 1 more extra reward added ==> should be i < bribeEpochs
            _rewards.push();
        }
...

bribeEpochs number of period rewards should be used, however bribeEpochs+1 loops are used to create _rewards.

Impact

As result, users will be able to claim reward for bribeEpochs+1 periods. More rewards will be used

Code Snippet

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/rewarders/BribeRewarder.sol#L144

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/rewarders/BribeRewarder.sol#L264-L266

Tool used

Manual Review

Recommendation

Consider change for (uint256 i = 0; i <= bribeEpochs; ++i) to for (uint256 i = 0; i < bribeEpochs; ++i)

0xSmartContract commented 3 months ago

The main reason for this error is that the loop used to calculate the reward periods incorrectly performs an extra iteration. The <= operator used in the loop creates one more reward period than necessary.

Incorrect Code:
```
for (uint256 i = 0; i <= bribeEpochs; ++i) {
_rewards.push();
}
```
This code should create as many reward periods as bribeEpochs, but it creates one more period.
Fix Suggestion:
```
for (uint256 i = 0; i < bribeEpochs; ++i) {
_rewards.push();
}
```
This fix ensures that the loop iterates the correct number of times and prevents the creation of an extra reward period.

0xHans1 commented 3 months ago

https://github.com/metropolis-exchange/magicsea-staking/pull/19

0xSmartContract commented 3 months ago

This issue was initially classified as medium severity, but after further investigation it should be re-evaluated as low severity.

In the BribeRewarder#_bribe function, an extra loop of bribeEpochs + 1 is made in the loop used to calculate the reward periods. This causes an extra reward period to be created.

for (uint256 i = 0; i <= bribeEpochs; ++i) {
_rewards.push();
}

Although an extra reward period is created, it is not possible for users to vote for this extra period and receive rewards from this period.

Risk of Insufficient Funds: The _bribe() function only checks if there are sufficient funds for the periods from startId to lastId. Since calcTotalAmount() works correctly, there is no risk of forcibly overfunding an extra period. That is, bribe is initiated only when there are enough funds for the correct periods.
Vote Restriction: The _bribe() function adds this reward distributor to the list for each valid period by calling Voter.onRegister(). However, getBribePeriods() never touches the incorrect period (lastId+1), it only touches the correct periods.
Invalid Vote Period: Since the bribe rewarder is not registered for the lastId+1 period, the bribe rewarder is not notified when users vote in this period. Therefore, the bribe rewards from this incorrect period will never be distributed because there will be no votes.

The bribe rewards from the incorrect period will never be distributed and the rewards for this period will not be stuck because the admin did not have to send these rewards.
Therefore, creating additional reward periods may only cause unnecessary gas consumption in practice. However, it does not constitute an extra reward for users or a serious security vulnerability for the system.

sherlock-admin2 commented 2 months ago

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