Popcorn contest - 0xmuxyz's results

Lines of code

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

Vulnerability details

Impact

This lead to the loss of feeShare that is supposed to be minted to the feeRecipient in case of that some amount of assets that is less than certain threshold would be deposited into the vault.

Proof of Concept

Within the Vault# deposit(), the feeShares is calculated like this: https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L143-L145

    /**
     * @notice Deposit exactly `assets` amount of tokens, issuing vault shares to `receiver`.
     * @param assets Quantity of tokens to deposit.
     * @param receiver Receiver of issued vault shares.
     * @return shares Quantity of vault shares issued to `receiver`.
     */
    function deposit(uint256 assets, address receiver)
        public
        nonReentrant
        whenNotPaused
        syncFeeCheckpoint
        returns (uint256 shares)
    {
        if (receiver == address(0)) revert InvalidReceiver();

        uint256 feeShares = convertToShares(
            assets.mulDiv(uint256(fees.deposit), 1e18, Math.Rounding.Down)  /// @audit - The "feeShares" is calculated here.
        );

        shares = convertToShares(assets) - feeShares;

        if (feeShares > 0) _mint(feeRecipient, feeShares);  /// @audit - If "feeShares" is less than 0, the shares based on the fee is not minted to feeRecipient

However, there is no input validation for the assets parameter in the the Vault# deposit() above. Thus, any amount of assets are deposited via the assets parameter in the the Vault# deposit() . If some small amount of assets that is less than certain threshold would be deposited, the feeShares to be minted will be 0.

Example). Let's say:

• assets deposited is 3000 ( 1e3 )
• fees.deposit is 1 BPS ( 1e14) The calculation by using above is like this:

uint256 feeShares = convertToShares(assets.mulDiv(uint256(fees.deposit), 1e18, Math.Rounding.Down))
uint256 feeShares = convertToShares(1e3 * 1e14 / 1e18)
uint256 feeShares = 0

Like the calculation above, the result of 1e3 * 1e14 / 1e18 will be assigned as the argument value of the convertToShares() function. Due to the rounding down in Solidity, the result of 1e3 * 1e14 / 1e18 will be 0. Thus, convertToShares(1e3 * 1e14 / 1e18) will return 0. As a result, the feeShares will be 0.

Tools Used

Manual review

Recommended Mitigation Steps

Consider adding the input validation for checking wether or not the amount of assets is more than threshold to the line that is before the calculation of feeShares is executed like this:

    function deposit(uint256 assets, address receiver)
        public
        nonReentrant
        whenNotPaused
        syncFeeCheckpoint
        returns (uint256 shares)
    {
        if (receiver == address(0)) revert InvalidReceiver();

        if (assets * uint256(fees.deposit) < 1e18) revert InvalidAssetsAmount();  /// @audit - Adding this validation to here.
   
        uint256 feeShares = convertToShares(
            assets.mulDiv(uint256(fees.deposit), 1e18, Math.Rounding.Down)  
        );

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/interfaces/vault/IVault.sol#L8-L13 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/interfaces/vault/IVault.sol#L22 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L106 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L132 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L60 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L88

Vulnerability details

Impact

This lead to a bad situation that the invalid rates of fees (i.e. 1000% (1e19)) will be applied to each fees unless the fee will be changed. (NOTE：It takes 2 days at least in order to change the rate of fees) During these invalid rates of fees is applied to each fees, if a user deposit the assets to the Vault, the assets of user will be lost or its transaction will always fail.

Proof of Concept

Within the IVault.sol, the VaultFees struct is defined like below: https://github.com/code-423n4/2023-01-popcorn/blob/main/src/interfaces/vault/IVault.sol#L8-L13

// Fees are set in 1e18 for 100% (1 BPS = 1e14)
struct VaultFees {
    uint64 deposit;
    uint64 withdrawal;
    uint64 management;
    uint64 performance;
}

fees is defined in the form of the VaultFees struct within the VaultInitParams struct like below: https://github.com/code-423n4/2023-01-popcorn/blob/main/src/interfaces/vault/IVault.sol#L22

/// @notice Init data for a Vault
struct VaultInitParams {
    /// @Notice Address of the deposit asset
    IERC20 asset;
    /// @Notice Address of the adapter used by the vault
    IERC4626 adapter;
    /// @Notice Fees used by the vault
    VaultFees fees;  /// @audit
    /// @Notice Address of the recipient of the fees
    address feeRecipient;
    /// @Notice Owner of the vault (Usually the submitter)
    address owner;
}

When new Vault would be deployed, the VaultController.sol# _deployVault() is called within the VaultController.sol# deployVault() https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L106

  function deployVault(
    VaultInitParams memory vaultData,
    DeploymentArgs memory adapterData,
    DeploymentArgs memory strategyData,
    address staking,
    bytes memory rewardsData,
    VaultMetadata memory metadata,
    uint256 initialDeposit
  ) external canCreate returns (address vault) {
    ...
    vault = _deployVault(vaultData, _deploymentController); /// @audit
    ...

Within the VaultController.sol# _deployVault(), new Vault would be deployed with the vaultData in the form of the VaultInitParams struct including the fees in the form of the VaultFee struct above via the Vault# initialize() function like below: https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L132

  /// @notice Deploys a new vault contract using the `activeTemplateId`.
  function _deployVault(VaultInitParams memory vaultData, IDeploymentController _deploymentController)
    internal
    returns (address vault)
  {
    vaultData.owner = address(adminProxy);

    (bool success, bytes memory returnData) = adminProxy.execute(
      address(_deploymentController),
      abi.encodeWithSelector(
        DEPLOY_SIG,
        VAULT,
        activeTemplateId[VAULT],
        abi.encodeWithSelector(IVault.initialize.selector, vaultData)  /// @audit - New Vault would be deployed with the "vaultData" in the form of the VaultInitParams struct including the fees in the form of the VaultFee struct via the Vault# `initialize()` function
      )
    );

    ...

  }

Within the Vault# initialize() , the fees_ in the form of the VaultFees struct would be assigned as a parameter. https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L60

    /**
     * @notice Initialize a new Vault.
     * ...
     * @param fees_ Desired fees in 1e18. (1e18 = 100%, 1e14 = 1 BPS)
     * ...
     */
    function initialize(
        IERC20 asset_,
        IERC4626 adapter_,
        VaultFees calldata fees_,  /// @audit
        address feeRecipient_,
        address owner
    ) external initializer {

However, there is no input validations for the fees_ in the form of the VaultFees struct until the fees_ would be assigned into the variable of the fees within the Vault# initialize() above: https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L88

        fees = fees_;  /// @audit

This allow the owner (deployer) to assign more than 100% (1e18) into the each fees by mistake. (i.e. 1000% (1e19)) Once these invalid rates of fees is set by mistake, the owner needs to wait for proposedFeeTime + quitPeriod (NOTE：According to the condition within the setQuitPeriod() function, quitPeriod is 2 days at least) to change the rates of each fees via the both functions the proposeFees() and the changeFees() https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L541

    function changeFees() external {
        if (block.timestamp < proposedFeeTime + quitPeriod). /// @audit
            revert NotPassedQuitPeriod(quitPeriod);

This lead to a bad situation that the invalid rates of fees (i.e. 1000% (1e19)) will be applied to each fees unless the fee will be changed and it takes 2 days at least like above. During these invalid rates of fees is applied to each fees, if a user deposit the assets to the Vault, the assets of user will be lost or its transaction will always fail.

Tools Used

Manual review

Recommended Mitigation Steps

Consider adding an input validation for the fee_ before it's assigned into the variable of fee within the Vault# initialize() like this:

    function initialize(
        IERC20 asset_,
        IERC4626 adapter_,
        VaultFees calldata fees_,  /// @audit
        address feeRecipient_,
        address owner
    ) external initializer {
        ...

        /// @audit - Consider adding the input validation for the "fee_" before it's assigned into the variable of "fee" like this:
        if (
            fees_.deposit >= 1e18 ||
            fees_.withdrawal >= 1e18 ||
            fees_.management >= 1e18 ||
            fees_.performance >= 1e18
        ) revert InvalidVaultFees();

        fees = fees_;
        ...