Popcorn contest - Krace's results

Lines of code

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

Vulnerability details

Impact

In claimRewards, the rewardAmount is cached and then a _rewardTokens[i].transfer is triggered which can lead to exploits if the _rewardTokens[i] is a token that gives control to the sender, like ERC777 tokens.

Proof of Concept

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

Initial state: msg.sender has some ERC777 tokens in accruedRewards[user][_rewardTokens[i]]

1. The msg.sender call claimRewards with his own address and ERC777 token

2. rewardAmount is not equal to zero and escrowInfo.escrowPercentage > 0 is false

3. _rewardTokens[i].transfer(user, rewardAmount); will transfer tokens to the sender and gives control to the sender

4. The sender can call claimRewards again with the same args in step 1, until the contract has no balance.

Tools Used

Recommended Mitigation Steps

Update the accruedRewards[user][_rewardTokens[i]] before transfer.