The first voter of a new BribeRewarder can drain the funds of BribeRewarder

Summary

The _lastUpdateTimestamp is initially set when the bribeRewarder is funded. The next point of update is when the _modifyfunction is executed when a voter votes.

        if (block.timestamp <= startTime) {
            _lastUpdateTimestamp = startTime;
        }

But this will not get triggered unless the vote has happened exactly on the startTime which may not always be the case. So in the case that _lastUpdateTimestamp hasnt been updated, during the calculation of totalRewardsin _calculateRewards function, the totalRewards can become equal to or exceed the balance of the BribeRewarder.

Vulnerability Detail

function _calculateRewards(uint256 periodId) internal view returns (uint256) {
        (uint256 startTime, uint256 endTime) = IVoter(_caller).getPeriodStartEndtime(periodId);

        if (endTime == 0 || startTime > block.timestamp) {
            return 0;
        }

        uint256 duration = endTime - startTime;
        uint256 emissionsPerSecond = _amountPerPeriod / duration;

        uint256 lastUpdateTimestamp = _lastUpdateTimestamp;
        uint256 timestamp = block.timestamp > endTime ? endTime : block.timestamp;
        return timestamp > lastUpdateTimestamp ? (timestamp - lastUpdateTimestamp) * emissionsPerSecond : 0;
    }

Assume the bribeRewarder function is funded at block.timestamp = 1, with startid = 1000 and lastid = 2000
the current _lastUpdateTimestampis 1
A voter votes in the timeperiod 5, after a single block has elapsed since the new voting period has begun (at block.timestamp = 1000*(number of blocks in 1 epoch) + 1)
this triggers the _notifyBribes function to call the deposit function which in turn calls the _modify function
Since the startTime != block.timestamp (1 block has elapsed since start), the _lastUpdateTimestamp doesnt get updated at line 270.
Now inside the _calculateRewards function, the duration = (number of blocks in 1 epoch),
timestamp = current block.timestamp ( 1000*(number of blocks in 1 epoch) + 1)
this means the returned total rewards will be ( 1000(number of blocks in 1 epoch) + 1) _amountPerPeriod/(number of blocks in 1 epoch) this will equate to approximately 1000 * _amountPerPeriod,

So depending on the startid the first voter of the BribeRewarder can drain all the funds of the contract.

Impact

The first voter of a BribeRewarder, can take more than what he deserves and drain the complete funds.

Code Snippet

https://github.com/sherlock-audit/2024-06-magicsea/blob/42e799446595c542eff9519353d3becc50cdba63/magicsea-staking/src/rewarders/BribeRewarder.sol#L270

https://github.com/sherlock-audit/2024-06-magicsea/blob/42e799446595c542eff9519353d3becc50cdba63/magicsea-staking/src/rewarders/BribeRewarder.sol#L300

Tool used

Manual Review

Recommendation

The _lastUpdateTimestamp must be properly updated when _modify is being called.

Duplicate of #436

sherlock-audit / 2024-06-magicsea-judging

anonymousjoe - The first voter of a new BribeRewarder can drain the funds of BribeRewarder #666