Wrong first parameter for _calcRewardsEnd when changing reward speed

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L308-L312 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L351-L360

Vulnerability details

Impact

The function _calcRewardsEnd is called with the previousEndTime as first parameter in MultiRewardStaking.changeRewardSpeed, which leads to wrong calculation of the new rewardsEndTimestamp, causing it to be later than it should be. This will lead to more rewards being accrued "on paper" than the contract contains.

Proof of Concept

The function MultiRewardStaking.changeRewardSpeed calls the _calcRewardsEnd function like this:

uint32 rewardsEndTimestamp = _calcRewardsEnd(
    prevEndTime > block.timestamp ? prevEndTime : block.timestamp.safeCastTo32(),
    rewardsPerSecond,
    remainder
);

If prevEndTime > block.timestamp == true then the following block in calcRewardsEnd will be executed:

if (rewardsEndTimestamp > block.timestamp)
    amount += uint256(rewardsPerSecond) * (rewardsEndTimestamp - block.timestamp);

A quick calculation why the lines above should not be executed in this case:

function _calcRewardsEnd(
uint32 rewardsEndTimestamp, // @note previousRewardsEndtime = 1000; block.Timestamp=700
uint160 rewardsPerSecond, //@note newRewardsPersecond = 10 (previous = 1) 
uint256 amount //@note currentRewardTokenBalance = 300
) internal returns (uint32) {
if (rewardsEndTimestamp > block.timestamp)
    //@note with values above: amount = 300 + 10*300 = 3300
    // this logic is only compatible with `fundReward` (where `amount` is meant to be added rewards, 
    //while the righthandside calculates current reward amounts left)
    amount += uint256(rewardsPerSecond) * (rewardsEndTimestamp - block.timestamp);

//@note 700 + 3300/10 = 1030 (should be 730, since no new tokens have entered the contract)
    //will try to distribute 330*10 = 3300 tokens even though there are only 300 in the contract
    //enables "theft" of rewards or permanent (?) lock of reward tokens (for some scenarios where each participant is entitled to more than the actual balance)
return (block.timestamp + (amount / uint256(rewardsPerSecond))).safeCastTo32();
}

Tools Used

Manual Review

Recommended Mitigation Steps

Call calcRewardsEnd the same way as is done in the addRewardToken function: _calcRewardsEnd(0, rewardsPerSecond, amount); (first parameter = 0)

[c4-judge]

dmvt marked the issue as duplicate of #156

[c4-sponsor]

RedVeil marked the issue as sponsor confirmed

[c4-judge]

dmvt changed the severity to QA (Quality Assurance)

[Simon-Busch]

Revert back to M as requested by @dmvt

[c4-judge]

dmvt marked the issue as satisfactory