Popcorn contest - peanuts's results

Lines of code

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

Vulnerability details

Impact

Reward speed will be calculated incorrectly which will affect rewardsEndTimestamp.

Proof of Concept

changeRewardSpeed() is called by the owner when the owner intends to change the reward speed.

  function changeRewardSpeed(IERC20 rewardToken, uint160 rewardsPerSecond) external onlyOwner {
    RewardInfo memory rewards = rewardInfos[rewardToken];


    if (rewardsPerSecond == 0) revert ZeroAmount();
    if (rewards.lastUpdatedTimestamp == 0) revert RewardTokenDoesntExist(rewardToken);
    if (rewards.rewardsPerSecond == 0) revert RewardsAreDynamic(rewardToken);


    _accrueRewards(rewardToken, _accrueStatic(rewards));


    uint256 remainder = rewardToken.balanceOf(address(this));


    uint32 prevEndTime = rewards.rewardsEndTimestamp;
    uint32 rewardsEndTimestamp = _calcRewardsEnd(
      prevEndTime > block.timestamp ? prevEndTime : block.timestamp.safeCastTo32(),
      rewardsPerSecond,
      remainder
    );
    rewardInfos[rewardToken].rewardsPerSecond = rewardsPerSecond;
    rewardInfos[rewardToken].rewardsEndTimestamp = rewardsEndTimestamp;
  }

The function takes the balance of the MultiRewardStaking contract and puts into the variable remainder. This variable checks how much rewards are left in the contract and tabulates the rewardsEndTimestamp accordingly. However, the function naively assumes that all users will call claimRewards() to withdraw their rewards everytime. For example, if there is 10 ETH of reward tokens being given out at 0.1 ETH per second (as per the test conditions), then after 10 seconds there should be 9 ETH of rewards being given out. However, if no one claims the 1 ETH of reward, the balance in the contract will still stay at 10 ETH. When changeRewardSpeed() is called, the remainder will still be 10 ETH instead of 9 ETH, which affects the calculation of rewardsEndTimestamp.

When remainder is added to _calcRewardsEnd, if remainder does not change at all (no one claim rewards), then end time stamp will not be calculated correctly.

  function _calcRewardsEnd(
    uint32 rewardsEndTimestamp,
    uint160 rewardsPerSecond,
    uint256 amount
  ) internal returns (uint32) {
    if (rewardsEndTimestamp > block.timestamp)
      amount += uint256(rewardsPerSecond) * (rewardsEndTimestamp - block.timestamp);


    return (block.timestamp + (amount / uint256(rewardsPerSecond))).safeCastTo32();
  }

    return (block.timestamp + (amount / uint256(rewardsPerSecond))).safeCastTo32();

block.timestamp + (full amount / altered rewards per second), end timestamp will be greater than what is should be.

Tools Used

VSCode

Recommended Mitigation Steps

Recommend to use the rewardInfos[_rewardToken].index instead in _accrueRewards because this Index is always up to date, whether the user claims their rewards or not.

Lines of code

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

Vulnerability details

Impact

Users can claim more rewards than they are expected to claim.

Proof of Concept

In MultiRewardStaking#claimRewards(), the user loops through a bunch of ERC20 reward tokens and claims the relevant amount for each token. The amount that the user claims is accounted in rewardAmount = accruedRewards[user][_rewardTokens[i]]. If there is no escrow, this rewardAmount will be transferred from the contract to the user. The rewardAmount variable is then set to 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;
    }
  }

This sort of contract writing fails to consider the CEI (check, effects, interaction) pattern. The accruedRewards[user][_rewardTokens[i]] is set to zero at the last part. During the transfer hook, the user can call an attack contract which loops through the claimRewards function again. Because accruedRewards[user][_rewardTokens[i]] is not set to 0 yet, the user can withdraw the rewardAmount again and again until the contract funds is drained. This is a common reentrancy exploit.

      } else {
        _rewardTokens[i].transfer(user, rewardAmount);
        emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, false);
      }


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

Once again, the accruedRewards[user][_rewardTokens[i]] should be set to 0 first before the transfer happens.

Reference: https://hackernoon.com/hack-solidity-reentrancy-attack

Tools Used

Manual Review

Recommended Mitigation Steps

Add a reentrancy guard or set accruedRewards[user][_rewardTokens[i]] = 0 before the transfer pattern.

Lines of code

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

Vulnerability details

Vulnerability details

The calculation of exchange rate for shares in Popcorn Vault is done by dividing the total supply of shares by the totalAssets of the vault. The first depositor can mint a very small number of shares, then donate to the vault to manipulate the share price. When subsequent depositors deposit, they will lose value due to precision loss. This is a common attack vector for almost all shares based liquidity pool contracts using ERC4626.

Impact

First depositor can manipulate shares from later users; later users will not get equivalent shares when converting their underlying asset.

Proof of Concept

1. Malicious user Alice can deposit() with 1 wei of asset token to get 1 wei of shares.

2. Next, Alice sends 10000e18 -1 of asset tokens and inflate the price per share from 1 to 1e22.

3. Subsequent depositor who deposits shares, eg 19999e18 of assets, will only receive 1 wei of shares token.

4. Victim will lose 9999e18 if they redeem() right after deposit() due to precision loss.

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

Relatable issue:

https://github.com/sherlock-audit/2022-08-sentiment-judging#issue-h-1-a-malicious-early-userattacker-can-manipulate-the-ltokens-pricepershare-to-take-an-unfair-share-of-future-users-deposits

Tools Used

Manual Review

Recommended Mitigation Steps

Consider requiring a minimum amount of share tokens to be minted for the first minter or follow Uniswap V2 which mints 10,000 share first to balance liquidity whenever someone wants to open a vault.

https://github.com/Uniswap/v2-core/blob/ee547b17853e71ed4e0101ccfd52e70d5acded58/contracts/UniswapV2Pair.sol#L119-L124