code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

Anyone can claim other's rewards via MultiRewardStaking.claimRewards() which accepts an array of tokens to be claimed. This enables a race condition where an external malicious actor can frontrun other's claim by just claiming one token of the array making the whole claim call to fail. This same scenario is also enabled in MultiRewardEscrow.

The claimRewards() function loops over all the user input rewards token and reverts if the rewardAmount for one token is zero:


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

The rewardAmount is equal to the accruedRewards mapping evaluated for the user and current reward token. That value is set to zero in the end of the call if the amount is successfully claimed.

A claimRewards() call claiming 10 different reward tokens could easily revert if an attacker frontruns that call by claiming the last token of the array, making the claimer to waste gas.

Proof of Concept

Alice has 10 reward tokens that she needs to claim. She calls claimRewards(Alice, ArrayOfTenTokens = [A, B, C, ... , J])
Bob, was scanning the mempool and calls claimRewards(Alice, [J]) offering more gas fees and his call is mined first.
Alice's call reverts. She realizes this and calls again claimRewards(Alice, ArrayOfNineTokens = [A, B, C, ... , I])
Bob does the same as before.

This scenario is considerably harmful for Alice because if she decides to stop claiming, she will never get the reward tokens. But if she decides to call claimRewards() again, she is condemned to have her calls reverted wasting gas just getting the last token of the claim array instead of all of them.

Tools Used

Manual Review

Recommended Mitigation Steps

Separate the logic in two branches using a zero accrued reward condition. For the zero tokens accrued scenario, emit an event instead of reverting.

c4-sponsor commented 1 year ago

RedVeil marked the issue as sponsor disputed

c4-judge commented 1 year ago

dmvt changed the severity to QA (Quality Assurance)