[Lines of code](https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L153-L188) # Vulnerability details ## Impact To claim reward tokens from the MultiRewardStaking contract deployed, a user must call claimRewards(address user, IERC20[] memory _rewardsTokens). The _rewardsTokens array is populated with getAllRewardsTokens() which returns all the reward Tokens the MultiStakingContract has. claimRewards calls will likely revert in many cases, locking funds (of rewardTokens) in the staking contract. ## Proof of Concept The VaultController contract is in charge of deploying Staking contracts. A MultiRewardStaking contract is deployed with deployStaking(IERC20 asset), based on the latest staking template activeTemplateId[STAKING]. Considering a MultiStakingContract with N reward tokens (such as N > 1) IERC20[] public rewardTokens = [iRewardToken1, iRewardToken2, ..., iRewardTokenN], The user must give an IERC20[] memory rewardTokens to call claimRewards(user, rewardTokens). It can be done with getAllRewardsTokens() [claimRewards function in MultiRewardStaking.sol GitHub Link]() However, if the balance of one of the N reward tokens is empty (i.e it exists iRewardToken in rewardTokens such as ERC20(iRewardToken).balanceOf(address(staking)) == 0 is true ) and the other tokens are claimable, claimRewards(user, rewardTokens) will revert without the claimable tokens being claimed. One could say that the user must construct a **sub-array** of rewardTokens, IERC20[] memory claimableRewardTokens which is only made of the current claimable reward tokens interfaces: Every iRewardToken in [iRewardToken1 ; iRewardTokenN] where accruedRewards[user][iRewardToken] > 0 after the call of accrueRewards(msg.sender, user) But there is no logic implemented to get those specific tokens and it would need to compute modifier accrueRewards(msg.sender, user) to know which interfaces to add before calling claimRewards(user, claimableRewardTokens): * accrueRewards(msg.sender, user) is a modifier [accrueRewards modifier GitHub Link]() * _accrueRewards(IERC20 _rewardToken, uint256 accrued) is **internal**. [_accrueRewards internal function GitHub Link]() Creating such a sub-array is impossible. The variables that can set the MultiRewardStaking contract in this locked funds state are **rewardsPerSecond** & **amount**, (rewardTokenAmount). Taking In J such as I != J and I,J in [1;N] rewardsEndTimestampI = block.timestamp + (amount1 / uint256(rewardsPerSecond1)) rewardsEndTimestampJ = block.timestamp + (amount2 / uint256(rewardsPerSecond2)) [_calcRewardsEnd function GitHub Link]() If rewardsEndTimestampI != rewardsEndTimestampJ, one of the reward token claim attempts will revert as seen above. There are multiple ways to set up the reward tokens accordingly: * At the deployment of the staking contract, due to the _Template_ configuration. * After the deployment, intentionally or not by the creator or anyone: * Through changeStakingRewardsSpeeds(): Limited to the vaultCreator. It executes changeRewardsSpeed from the MultiRewardStaking contract. It can change the rewardSpeed in a way that the rewardsEndTimestamps of the rewardTokens don't match. (changing every rewardTokens speed or only some). [changeStakingRewardsSpeeds in VaultController.sol GitHub Link](), [changeRewardSpeed in MultiRewardStaking.sol GitHub Link]() * Through fundStakingRewards(): Anyone can fund any vault. It executes fundReward from the MultiRewardStaking contract. Someone can fund rewards in a way that the rewardsEndTimestamps of the rewardTokens don't match, making some reward tokens not claimable until the rewardsEndTimestamps are adjusted. [fundStakingRewards in VaultController.sol GitHub Link](), [fundRewardin MultiRewardStaking.sol GitHub Link]() As a basic PoC, I've set a staking contract with two reward tokens: * [rewardToken1, rewardToken2] * Same reward speed * Different reward amounts. 1. The first claimRewards call claim 10% of the rewardToken1 and fully claims the rewardToken2. 2. The second claimRewards call tries to claim another 10% of the rewardToken1 funds but reverts due to rewardToken2 empty balance. [![test__claim_multiple_tokens_different_funds\(\) source code](https://camo.githubusercontent.com/efd53e7308bdaf8d8a2a7a7d96ee70fdc9e959cd5f3817a1aa6be12e48fd72d0/68747470733a2f2f692e696d6775722e636f6d2f586b46664651742e706e67)]() [![test__claim_multiple_tokens_different_funds\(\) logs 1](https://camo.githubusercontent.com/2d61276ebd19c2b141dc49651bf46ab6f541bbf550a167d03d752a1139d27556/68747470733a2f2f692e696d6775722e636f6d2f5264597738486d2e706e67)]() [![test__claim_multiple_tokens_different_funds\(\) logs 1](https://camo.githubusercontent.com/12f50810fda2de8550da5fc13d30790fb7fe87f498cba4ce754926d0da2696f6/68747470733a2f2f692e696d6775722e636f6d2f646a72696d54312e706e67)]() ## Tools Used Manual Review ## Recommended Mitigation Steps The claimRewards function from MultiRewardStaking.sol could emit an event instead of reverting, so the claimable rewards can be claimed, regardless of the user input. /// This token has not rewards claimable event ZeroRewards(IERC20 rewardToken); function claimRewards( address user, ERC20[] memory _rewardTokens ) external accrueRewards(msg.sender, user) { for (uint8 i; i < _rewardTokens.length; i++) { uint256 rewardAmount = accruedRewards[user][_rewardTokens[i]]; if (rewardAmount == 0) { emit ZeroRewards(_rewardTokens[i]); } else { EscrowInfo memory escrowInfo = escrowInfos[_rewardTokens[i]]; if (escrowInfo.escrowPercentage > 0) { _lockToken(user, _rewardTokens[i], rewardAmount, escrowInfo); emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, true); } else { _rewardTokens[i].transfer(user, rewardAmount); emit RewardsClaimed( user, _rewardTokens[i], rewardAmount, false ); } accruedRewards[user][_rewardTokens[i]] = 0; } } } --- The text was updated successfully, but these errors were encountered: All reactions