Popcorn contest - mgf15's results

Lines of code

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

Vulnerability details

##Description

One of the major dangers of calling external contracts is that they can take over the control flow. In the reentrancy attack (a.k.a. recursive call attack), a malicious contract calls back into the calling contract before the first invocation of the function is finished. This may cause the different invocations of the function to interact in undesirable way

Impact

user can claimRewards more than one time !

Proof of Concept

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); // <- Reentrancy 
        emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, false);
      }

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

when the user calm Reward the if condition here will pass as false

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

and the 2nd one

if (escrowInfo.escrowPercentage > 0) {
        _lockToken(user, _rewardTokens[i], rewardAmount, escrowInfo);
        emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, true);
      } 

the bug is right here

_rewardTokens[i].transfer(user, rewardAmount); // <- here 

Tools Used

manually test

Recommended Mitigation Steps

The best practices to avoid Reentrancy weaknesses are:

* Make sure all internal state changes are performed before the call is executed. This is known as the Checks-Effects-Interactions pattern
* Use a reentrancy lock (ie. OpenZeppelin's ReentrancyGuard.

References

https://swcregistry.io/docs/SWC-107#simple_daosol