Popcorn contest - cccz's results

Lines of code

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

Vulnerability details

Impact

In MultiRewardStaking.changeRewardSpeed, the end time is adjusted using the balance of rewardTokens, which will include vested but unclaimed rewardTokens and unvested rewardTokens.

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

But in _calcRewardsEnd the unvested rewardTokens are added to the calculation again, which can cause the calculated end time to be inflated and lead to incorrect rewardToken distribution because the rewardToken is not sufficient to cover the end time.

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

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

Proof of Concept

The POC is as follows, the total number of reward tokens is 10 eth, and the initial rewardsPerSecond == 0.1 eth. alice/bob deposit 1 eth respectively. After 10s, the unvested reward is 9 eth, adjust rewardsPerSecond to 0.2. At this time, the end time should be 9/0.2 = 45, but the actual result is greater than 45. And after another 80s, alice's reward is much greater than 5 eth (the actual alice and bob should share the 10 eth reward equally) If alice takes out the reward at this time, bob will not be able to take it out due to insufficient rewards.

  function test__POC() public {
    _addRewardToken(rewardToken1);
    (, , uint32 oldRewardsEndTimestamp, , ) = staking.rewardInfos(iRewardToken1);
    assertEq(uint256(oldRewardsEndTimestamp) - block.timestamp, 100);
    stakingToken.mint(alice, 1 ether);
    stakingToken.mint(bob, 1 ether);

    vm.prank(alice);
    stakingToken.approve(address(staking), 1 ether);
    vm.prank(alice);
    staking.deposit(1 ether);

    vm.prank(bob);
    stakingToken.approve(address(staking), 1 ether);
    vm.prank(bob);
    staking.deposit(1 ether);

    // 10% of rewards paid out
    vm.warp(block.timestamp + 10);
    // Double Accrual (from original)
    staking.changeRewardSpeed(iRewardToken1, 0.2 ether);
    (, , uint32 RewardsEndTimestamp, , ) = staking.rewardInfos(iRewardToken1);
    assertGt(uint256(RewardsEndTimestamp) - block.timestamp, 45);
    vm.warp(block.timestamp + 80);
    vm.prank(alice);
    staking.withdraw(1 ether);
    vm.prank(bob);
    staking.withdraw(1 ether);
    assertGt(staking.accruedRewards(alice, iRewardToken1), 5 ether);

    IERC20[] memory rewardsTokenKeys = new IERC20[](1);
    rewardsTokenKeys[0] = iRewardToken1;
    staking.claimRewards(alice, rewardsTokenKeys);
    // staking.claimRewards(bob, rewardsTokenKeys); @ audit: ERC20: transfer amount exceeds balance
  }

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#L351-L360

Tools Used

None

Recommended Mitigation Steps

  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
+    0
    );
    rewardInfos[rewardToken].rewardsPerSecond = rewardsPerSecond;
    rewardInfos[rewardToken].rewardsEndTimestamp = rewardsEndTimestamp;
  }

Lines of code

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

Vulnerability details

Impact

In MultiRewardStaking.claimRewards, the accruedRewards are reset after the token transfer. If the rewardToken is an ERC777 token, the malicious user can call claimRewards again in the token transfer callback, and since the accruedRewards have not been reset, the user can claim the reward again until the ERC777 reward tokens in the contract are exhausted.

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

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Change to

  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#L253-L257

Vulnerability details

Impact

Vault.deposit/mint/withdraw will cause totalSupply/totalAssets to change, and the syncFeeCheckpoint modifier will be used to update highWaterMark.

    modifier syncFeeCheckpoint() {
        _;
        highWaterMark = convertToAssets(1e18);
    }
...
    function deposit(uint256 assets, address receiver)
        public
        nonReentrant
        whenNotPaused
        syncFeeCheckpoint
        returns (uint256 shares)
    {

Vault.redeem also causes totalSupply/totalAssets to change, but does not use the syncFeeCheckpoint modifier to update highWaterMark.

    function redeem(
        uint256 shares,
        address receiver,
        address owner
    ) public nonReentrant returns (uint256 assets) {

This will cause the highWaterMark to be updated incorrectly and thus the PerformanceFee charged will be incorrect

    function accruedPerformanceFee() public view returns (uint256) {
        uint256 highWaterMark_ = highWaterMark;
        uint256 shareValue = convertToAssets(1e18);
        uint256 performanceFee = fees.performance;

        return
            performanceFee > 0 && shareValue > highWaterMark
                ? performanceFee.mulDiv(
                    (shareValue - highWaterMark) * totalSupply(),
                    1e36,
                    Math.Rounding.Down
                )
                : 0;
    }

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Change to

    function redeem(
        uint256 shares,
        address receiver,
        address owner
-   ) public nonReentrant returns (uint256 assets) {
+   ) public nonReentrant syncFeeCheckpoint returns (uint256 assets) {

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/abstracts/AdapterBase.sol#L479-L483

Vulnerability details

Impact

The strategy contract will generally let the Adapter contract use delegatecall to call its functions So IAdapter(address(this)).call is used frequently in strategy contracts, because when the Adapter calls the strategy's functions using delegatecall, address(this) is the Adapter

  function harvest() public override {
    address router = abi.decode(IAdapter(address(this)).strategyConfig(), (address));
    address asset = IAdapter(address(this)).asset();
    ...

But in AdapterBase._verifyAndSetupStrategy, the verifyAdapterSelectorCompatibility/verifyAdapterCompatibility/setUp functions are not called with delegatecall, which causes the context of these functions to be the strategy contract

    function _verifyAndSetupStrategy(bytes4[8] memory requiredSigs) internal {
        strategy.verifyAdapterSelectorCompatibility(requiredSigs);
        strategy.verifyAdapterCompatibility(strategyConfig);
        strategy.setUp(strategyConfig);
    }

and since the strategy contract does not implement the interface of the Adapter contract, these functions will fail, making it impossible to create a Vault using that strategy.

  function verifyAdapterCompatibility(bytes memory data) public override {
    address router = abi.decode(data, (address));
    address asset = IAdapter(address(this)).asset();

More dangerously, if functions such as setup are executed successfully because they do not call the Adapter's functions, they may later error out when calling the havest function because the settings in setup are invalid

Proof of Concept

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/abstracts/AdapterBase.sol#L479-L483

Tools Used

None

Recommended Mitigation Steps

In AdapterBase._verifyAndSetupStrategy, the verifyAdapterSelectorCompatibility/verifyAdapterCompatibility/setUp functions are called using delegatecall

Lines of code

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

Vulnerability details

Impact

In Vault, highWaterMark should represent the highest point of the share price. In accruedPerformanceFee, PerformanceFee is charged when the current share price is higher than highWaterMark.

    function accruedPerformanceFee() public view returns (uint256) {
        uint256 highWaterMark_ = highWaterMark;
        uint256 shareValue = convertToAssets(1e18);
        uint256 performanceFee = fees.performance;

        return
            performanceFee > 0 && shareValue > highWaterMark
                ? performanceFee.mulDiv(
                    (shareValue - highWaterMark) * totalSupply(),
                    1e36,
                    Math.Rounding.Down
                )
                : 0;
    }

In syncFeeCheckpoint, the highWaterMark is updated to the current share price even if the current share price is less than the highWaterMark.

    modifier syncFeeCheckpoint() {
        _;
        highWaterMark = convertToAssets(1e18);
    }

And this means that when the share price fluctuates greatly, a lot of PerformanceFee will be charged!

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

    modifier syncFeeCheckpoint() {
        _;
+      if(highWaterMark < convertToAssets(1e18))
        highWaterMark = convertToAssets(1e18);
    }

Lines of code

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

Vulnerability details

Impact

In Vault, highWaterMark represents the highest point of share price. In accruedPerformanceFee, PerformanceFee is collected when the current share price is higher than highWaterMark.

    function accruedPerformanceFee() public view returns (uint256) {
        uint256 highWaterMark_ = highWaterMark;
        uint256 shareValue = convertToAssets(1e18);
        uint256 performanceFee = fees.performance;

        return
            performanceFee > 0 && shareValue > highWaterMark
                ? performanceFee.mulDiv(
                    (shareValue - highWaterMark) * totalSupply(),
                    1e36,
                    Math.Rounding.Down
                )
                : 0;
    }

However, in syncFeeCheckpoint(when deposit/withdraw is called), highWaterMark is updated to the current share price and no PerformanceFee is charged.

    modifier syncFeeCheckpoint() {
        _;
        highWaterMark = convertToAssets(1e18);
    }

PerformanceFee is only charged when the takeManagementAndPerformanceFees function is called, in the takeFees modifier.

    function takeManagementAndPerformanceFees()
        external
        nonReentrant
        takeFees
    {}

    /// @notice Collect management and performance fees and update vault share high water mark.
    modifier takeFees() {
        uint256 managementFee = accruedManagementFee();
        uint256 totalFee = managementFee + accruedPerformanceFee();
        uint256 currentAssets = totalAssets();
        uint256 shareValue = convertToAssets(1e18);

        if (shareValue > highWaterMark) highWaterMark = shareValue;

        if (managementFee > 0) feesUpdatedAt = block.timestamp;

        if (totalFee > 0 && currentAssets > 0)
            _mint(feeRecipient, convertToShares(totalFee));

        _;
    }

If functions such as deposit/withdraw are called (syncFeeCheckpoint will be called) before takeManagementAndPerformanceFees is called, then as highWaterMark is updated to the current share price takeManagementAndPerformanceFees will not charge any PerformanceFee. Considering that functions such as deposit/withdraw are called frequently, this can make it difficult to collect PerformanceFee

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Consider using takeFees instead of syncFeeCheckpoint in the deposit/withdraw functions. The takeFees modifier will update the highWaterMark while collecting PerformanceFee.

Lines of code

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

Vulnerability details

Impact

When calculating the end time in _calcRewardsEnd, the unvested rewards are calculated first and the end time is calculated afterwards. Both calculations require the use of rewardsPerSecond But note that when rewardsPerSecond changes, the rewardsPerSecond used in these two places should be different, in the first step, the old rewardsPerSecond should be used, while the second step should use the new rewardsPerSecond.

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

However, in the current implementation, the new rewardsPerSecond is used in both steps, which leads to miscalculation of the end time and thus incorrect reward distribution.

  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,

Proof of Concept

Please fix another bug in changeRewardSpeed first.

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
+    0
  );
  rewardInfos[rewardToken].rewardsPerSecond = rewardsPerSecond;
  rewardInfos[rewardToken].rewardsEndTimestamp = rewardsEndTimestamp;
}

The POC is as follows, the total number of reward tokens is 10 eth, and the initial rewardsPerSecond == 0.1 eth. alice/bob deposit 1 eth respectively. Adjust rewardsPerSecond to 0.2. At this time, the end time should be 10/0.2 = 50, but the actual result is greater than 50. And after another 80s, alice's reward is much greater than 5 eth (the actual alice and bob should share the 10 eth reward equally) If alice takes out the reward at this time, bob will not be able to take it out due to insufficient rewards.

  function test__POC() public {
    _addRewardToken(rewardToken1);
    (, , uint32 oldRewardsEndTimestamp, , ) = staking.rewardInfos(iRewardToken1);
    assertEq(uint256(oldRewardsEndTimestamp) - block.timestamp, 100);
    stakingToken.mint(alice, 1 ether);
    stakingToken.mint(bob, 1 ether);

    vm.prank(alice);
    stakingToken.approve(address(staking), 1 ether);
    vm.prank(alice);
    staking.deposit(1 ether);

    vm.prank(bob);
    stakingToken.approve(address(staking), 1 ether);
    vm.prank(bob);
    staking.deposit(1 ether);
    staking.changeRewardSpeed(iRewardToken1, 0.2 ether);
    (, , uint32 RewardsEndTimestamp, , ) = staking.rewardInfos(iRewardToken1);
    assertEq(uint256(RewardsEndTimestamp) - block.timestamp, 50);
    vm.warp(block.timestamp + 80);
    vm.prank(alice);
    staking.withdraw(1 ether);
    vm.prank(bob);
    staking.withdraw(1 ether);
    assertEq(staking.accruedRewards(alice, iRewardToken1), 5 ether);

    IERC20[] memory rewardsTokenKeys = new IERC20[](1);
    rewardsTokenKeys[0] = iRewardToken1;
    staking.claimRewards(alice, rewardsTokenKeys);
    // staking.claimRewards(bob, rewardsTokenKeys); @ audit: ERC20: transfer amount exceeds balance
  }

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

Tools Used

None

Recommended Mitigation Steps

Consider using the oldRewardsPerSecond and newRewardsPerSecond parameters in _calcRewardsEnd and changing the parameters when calling _calcRewardsEnd

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