`MultiRewardStaking.addRewardToken` can eventually break the contract

[code423n4]

Lines of code

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

Vulnerability details

When adding a reward token, the token address is added to rewardTokens.

263: rewardTokens.push(rewardToken);

If rewardTokens is large enough, accrueRewards will revert with an out-of-gas error, as it loops through rewardsToken

373:     for (uint8 i; i < _rewardTokens.length; i++) {
374:       IERC20 rewardToken = _rewardTokens[i];
375:       RewardInfo memory rewards = rewardInfos[rewardToken];
376: 
377:       if (rewards.rewardsPerSecond > 0) _accrueRewards(rewardToken, _accrueStatic(rewards));
378:       _accrueUser(_receiver, rewardToken);
379: 
380:       // If a deposit/withdraw operation gets called for another user we should accrue for both of them to avoid potential issues like in the Convex-Vulnerability
381:       if (_receiver != _caller) _accrueUser(_caller, rewardToken);
382:     }

This modifier is used for deposits, withdrawals, meaning in such case the core functions will stop working as there is no way to remove elements from rewardTokens, leading to frozen funds in the contract (users unable to withdraw)

Impact

Medium

Tools Used

Manual Analysis

Mitigation

Add an appropriate limit to rewardTokens.length in addRewardToken()

[c4-judge]

dmvt marked the issue as duplicate of #151

[c4-sponsor]

RedVeil marked the issue as sponsor confirmed

[c4-judge]

dmvt marked the issue as partial-50