reentrancy in `MultiRewardStaking::claimRewards` for tokens with transfer callbacks, like erc777

[code423n4]

Lines of code

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

Vulnerability details

Impact

An attacker can drain all the tokens from MultiRewardStaking

Proof of Concept

In claimtRewards important state changes are done after interactions with tokens:

File: MultiRewardStaking.sol

  function claimRewards(address user, IERC20[] memory _rewardTokens) external accrueRewards(msg.sender, user) {
    for (uint8 i; i < _rewardTokens.length; i++) {
      uint256 rewardAmount = accruedRewards[user][_rewardTokens[i]];

      if (rewardAmount == 0) revert ZeroRewards(_rewardTokens[i]);

      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;
    }
  }

If an erc777 token (or other ERC20 that supports callbacks for transfers) is used as rewardToken, an attacker could use reentrancy to steal all the tokens since the rewards is zeroed after interactions.

Tools Used

manual audit, vs code

Recommended Mitigation Steps

Follow checks, effects, interactions:

diff --git a/src/utils/MultiRewardStaking.sol b/src/utils/MultiRewardStaking.sol
index 95ebefd..f8433aa 100644
--- a/src/utils/MultiRewardStaking.sol
+++ b/src/utils/MultiRewardStaking.sol
@@ -175,6 +175,8 @@ contract MultiRewardStaking is ERC4626Upgradeable, OwnedUpgradeable {

       EscrowInfo memory escrowInfo = escrowInfos[_rewardTokens[i]];

+      accruedRewards[user][_rewardTokens[i]] = 0;
+
       if (escrowInfo.escrowPercentage > 0) {
         _lockToken(user, _rewardTokens[i], rewardAmount, escrowInfo);
         emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, true);
@@ -182,8 +184,6 @@ contract MultiRewardStaking is ERC4626Upgradeable, OwnedUpgradeable {
         _rewardTokens[i].transfer(user, rewardAmount);
         emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, false);
       }
-
-      accruedRewards[user][_rewardTokens[i]] = 0;
     }
   }

or use an OpenZepplin reentrancy guard.

[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