claimRewards is not re-entrancy safe.

[code423n4]

Lines of code

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

Vulnerability details

Impact

In MultiRewardStaking the function claimRewards doesn’t have nonReentrant which makes it possible to re-enter the function. If one of the reward tokens in ERC-777 token, it is possible to re-enter and claim the reward again and again until the contract is drained out of those tokens.

When the function would be re-entered, it would call accrueRewards again which would call _accrueUser for _receiver and _caller, it would update the value of accruedRewards again as accruedRewards[_user][_rewardToken] += supplierDelta; The second time supplierDelta would be zero, but the accruedRewards would remain the same. This value is used later in the claimRewards function as rewardAmount and sent to the user.

POC

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

In the above function the accruedRewards[user][_rewardTokens[i]] is updated after the tokens are transfered to the user. This makes it possible to steal the rewards multiple times in case of ERC-777 (extension of ERC-20) tokens, which have a callback.

Recommendation

Add nonReentrant modifier.

[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