Reentrancy In Claim Rewards

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/utils/MultiRewardStaking.sol#L170-L187

Vulnerability details

Impact

There exists a reentrancy in the claimRewards function which will let the attacker drain all the rewards because the mapping accruedRewards[user][_rewardTokens[i]] = 0; is updated after the transfer.

Proof of Concept

1.) Assume the attacker is claiming rewards using the function claimRewards , the reward amount is calculated through this accruedRewards[user][_rewardTokens[i]] ; 2.) The transfer get called (lets assume we dealing with ERC777 which has a callback hook) , the attacker get's control and call the transfer again with the same reward amount. 3.) The attacker can repeat this till the contract is out of funds as the mapping accruedRewards[user][_rewardTokens[i]] = 0; gets updated at the end.

Tools Used

Manual analysis and Visual Studio

Recommended Mitigation Steps

Follow CIE pattern and use a Reentrancy guard.

[c4-sponsor]

RedVeil marked the issue as sponsor confirmed

[c4-judge]

dmvt marked the issue as unsatisfactory: Invalid