Popcorn contest - supernova's results

Lines of code

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

Vulnerability details

Impact

claimRewards function is prone to ERC777 Reentrancy . Checks -Effects-Interaction Pattern is not followed properly which leads to user being able to claim rewards token till the full contract is empty. At line 186

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

The mapping is updated at the end of the function execution, which leads to reentrancy in the below code

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

Proof of Concept

Tools Used

Manual

Recommended Mitigation Steps

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]];
+      accruedRewards[user][_rewardTokens[i]] = 0;
      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;
    }
  }

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L88

Vulnerability details

Impact

According to the Vault contract https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L523

Fees can never be 1e18 i.e 100%. For this the required checks are present that prevent the following action by the vault's owner .

Proof of Concept

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L525-L537

Although owner cannot set Fee to 100% using the above function , but there is no restriction on the owner to set fee = 1e18(i.e 100%) in the initialize function .

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L88

I consider this as a medium severity as this breaks the invariant that Fees cannot be 100% in any case , which is false.

Tools Used

Recommended Mitigation Steps

In Vault.sol initialize function add the following

+        if (
+            fees_.deposit >= 1e18 ||
+            fees_.withdrawal >= 1e18 ||
+            fees_.management >= 1e18 ||
+            fees_.performance >= 1e18
+.         )