MultiRewardStaking `claimRewards()` reentrancy for ERC-777 reward tokens

[code423n4]

Lines of code

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

Vulnerability details

Impact

A hacker can drain an ERC-777 reward token funds via reentrancy. This is because in the claimRewards() function, the transfer of the reward token which triggers the hacker's ERC-777 hook takes place before setting accruedRewards[user][_rewardTokens[i]] to zero.

Proof of Concept

The attack scenario:

Step 1. Admin adds an ERC-777 hookable token as the reward token. In such a token, a user can set a callback that will be called every time someone sends money to them.

Step 2. Hacker accumulates rewards for this token and calls claimRewards(hackerAddress, [erc777token]). https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L170

Step 3. The function reads the hacker's reward amount from the contract's storage:

uint256 rewardAmount = accruedRewards[user][_rewardTokens[i]];

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

Step 4. The positive amount of tokens is sent to the hacker. Either on the line https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L182

_rewardTokens[i].transfer(user, rewardAmount)

or on the line https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L200

rewardToken.safeTransfer(user, payout);

Step 5. The ERC-777 hook is triggered. The hacker takes control and calls the claimRewards() function again with the same parameters.

Step 6. In the MultiRewardStaking contract, the hacker's state has not changed, and accruedRewards[user][_rewardTokens[i]] is still equal to its original positive value. Therefore, funds are transferred to the hacker again, and the hook is triggered again, from which they can repeat the attack again.

Step 7. After the hacker repeatedly executes the reentrancy, the code flow finally reaches line 186 and sets the hacker's accrued rewards to zero without any revert. https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L186

accruedRewards[user][_rewardTokens[i]] = 0;

Since no errors occur, the hacker gets a profit.

Tools Used

Manual analysis

Recommended Mitigation Steps

Follow the Check-Effect-Interaction pattern and set the state variable accruedRewards[user][_rewardTokens[i]] to zero before doing an external transfer call.

[c4-judge]

dmvt marked the issue as duplicate of #54

[c4-sponsor]

RedVeil marked the issue as sponsor confirmed

[c4-judge]

dmvt marked the issue as partial-50

[c4-judge]

dmvt marked the issue as satisfactory