Funds unutilized for rewards may get stranded in BribeRewarder

Summary

When a bribe provider starts the bribe cycle on a BribeRewarder.sol contract, they are required to fund the contract with the total amount of rewardToken required across all the bribePeriods (enforced by the check here), which is amountPerPeriod x number of bribe periods they want to incentivize.

However if any of these funds remain untilized for reward distribution, they will remain stranded in the contract forever as there is no way to recover tokens.

Vulnerability Detail

The bribe cycle can be started on a BribeRewarder contract by calling fundAndBribe() or bribe(), where it is checked that the balance of the rewardToken in this contract is at least equal to the amount required to distribute across all defined bribe periods.

total amount required = amountPerPeriod x number of bribe periods briber wants to incentivize.

Lets say X amount of funds is required and the contract has been funded with exactly X. Now if in any case, the funds do not get utilized, they will remain stuck in the contract forever : as there is no way to sweep these tokens out.

This is possible due to the following reasons :

(1). The full amountPerPeriod will be utilized for distribution even if there is only one voter for the associated poolID in a given period. But if there is no voter, these funds will not be utilized for any other periods too. A pool might not get any voter under normal operations :

The pool is not in the top pool IDs set on the Voter contract, thus not earning any rewards on the MasterChef, so users might want to vote for the top pools to potentially earn more LUM from there.
The associated pool might get hacked, or drained, or emptied by LPs : thus no user would want to vote for such a pool after this happens, instead they would go for better incentives on LP swap fees.

(2). Some amount of tokens might remain unutilized even under normal circumstances and actively running bribing periods due to rounding in calculations. An instance of this is possible when calculating total rewards accrued in BribeRewarder._modify => _calculateRewards :


    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;
    }

Here emissionsPerSecond = _amountPerPeriod / duration : this could round down leading to some portion of the funds unutilized in the rewards calculations, and will be left out of the rewards for whole of the bribing cycle.

Impact

Due to the two reasons mentioned above, it is possible that some amount of rewardToken remains stranded in the contract, but there is no way for the bribe provider to recover these tokens.

These funds will be lost permanently, especially in the first case above, the amount would be large.

Code Snippet

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

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

Tool used

Manual Review

Recommendation

BribeRewarder.sol should have a method to recover unutilized reward tokens, just like the sweep function in BaseRewarder.sol. This will prevent the permanent loss of funds for the briber.

sherlock-admin2 commented 3 weeks ago

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