code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L305-L314

Vulnerability details

Impact

In MultiRewardStaking.changeRewardSpeed the new rewardsEndTimetamp is calculated based on the current balance of reward tokens in the contract. However, a fraction of this balance might already be accrued and accounted as reward, but just has not been claimed yet. This can cause more tokens to be claimable than the contract contains, leading to a "first come first serve" scenario for users concerning rewards.

Proof of Concept

The function MultiRewardStaking.changeRewardSpeed uses the current reward token balance of the contract as input for the calculation of the new rewardsEndTimeStamp:

uint256 remainder = rewardToken.balanceOf(address(this));

uint32 prevEndTime = rewards.rewardsEndTimestamp;
uint32 rewardsEndTimestamp = _calcRewardsEnd(
    prevEndTime > block.timestamp ? prevEndTime : block.timestamp.safeCastTo32(),
    rewardsPerSecond,
    remainder
);
rewardInfos[rewardToken].rewardsPerSecond = rewardsPerSecond;
rewardInfos[rewardToken].rewardsEndTimestamp = rewardsEndTimestamp;

The idea is that the current balance of reward tokens is what still needs to be distributed (as the name remainder implies). However, as mentioned under #Impact a fraction of this balance might already be attributed to users and is just unclaimed. Using the full balance will lead to rewardsEndTimeStamp being later than it should be, causing more tokens to be accounted for claiming than the contract holds:

function _calcRewardsEnd(
    ...
    //amount = rewardToken.balanceOf(address(this));
    return (block.timestamp + (amount / uint256(rewardsPerSecond))).safeCastTo32();
}

Tools Used

Manual Review

Recommended Mitigation Steps

Only the fraction of reward tokens that has not yet accrued should be used to calculate the rewardsEndTimestamp when changing the rewardspeed. A separate variable might be necessary to track the amount of tokens that are eligible to be claimed. That variable would need to be incremented on all _accrueStatic calls that occur when totalSupply > 0.