Popcorn contest - rbserver's results

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L285-L287 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L247-L252 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/beefy/BeefyAdapter.sol#L108-L115 https://github.com/beefyfinance/beefy-contracts/blob/master/contracts/BIFI/vaults/BeefyVaultV6.sol#L69-L71

Vulnerability details

Impact

When the user is the first depositor for both the Vault contract's vault and the corresponding adapter's used vault, this user is able to manipulate the Vault contract's vault's share price by minting 1 share of the Vault contract's vault, which deposits 1 wei of the asset token into the adapter's used vault, and then transferring a large asset amount to the adapter's used vault. This can greatly increase the adapter's used vault's asset balance, such as beefyVault.balance() used in the BeefyAdapter._totalAssets function, and hugely inflate the values returned by the following AdapterBase.totalAssets and Vault.totalAssets functions. The next user, who deposits an asset amount that is less than the large asset balance owned by the adapter's used vault at that moment, will mint 0 share of the Vault contract's vault due to rounding when calling the Vault.convertToShares function and lose the deposited asset amount to the first depositor because the first depositor still owns the only share of the Vault contract's vault.

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

    function totalAssets() public view returns (uint256) {
        return adapter.convertToAssets(adapter.balanceOf(address(this)));
    }

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

    function totalAssets() public view override returns (uint256) {
        return
            paused()
                ? IERC20(asset()).balanceOf(address(this))
                : _totalAssets();
    }

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/beefy/BeefyAdapter.sol#L108-L115

    function _totalAssets() internal view override returns (uint256) {
        return
            underlyingBalance.mulDiv(
                beefyVault.balance(),
                beefyVault.totalSupply(),
                Math.Rounding.Down
            );
    }

https://github.com/beefyfinance/beefy-contracts/blob/master/contracts/BIFI/vaults/BeefyVaultV6.sol#L69-L71

    function balance() public view returns (uint) {
        return want().balanceOf(address(this)).add(IStrategy(strategy).balanceOf());
    }

Proof of Concept

The following steps can occur for the described scenario.

1. Alice is the first depositor for both the Vault contract's vault and the corresponding adapter's used vault.
2. She calls the Vault.mint function to mint 1 share and deposit 1 wei of the asset token into the adapter's used vault.
3. Then, she transfers a large asset amount, such as 10e24, to the adapter's used vault.
4. Bob calls the Vault.deposit function to deposit a somewhat large asset amount, such as 10e20, but 0 share of the Vault contract's vault is minted to him.
5. Alice calls the Vault.redeem function to redeem the only share of the Vault contract's vault and receive all of the deposited asset amounts, including Bob's.

Tools Used

VSCode

Recommended Mitigation Steps

When the first deposit occurs, a pre-determined minimum number of shares of the Vault contract's vault can be minted to address(0) before minting shares to the first depositor, which is similar to Uniswap V2's implementation.

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L438-L450 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L559-L567 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L523-L542 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L496-L499

Vulnerability details

Impact

In the Vault contract, the Vault.syncFeeCheckpoint would be called to possibly sync its highWaterMark to a value that is higher or lower than the previous highWaterMark when the Vault.deposit, Vault.mint, or Vault.withdraw function is called. However, this is not the case in the AdapterBase contract. When calling the AdapterBase._deposit or AdapterBase._withdraw function, which calls the following AdapterBase.harvest function and triggers the AdapterBase.takeFees modifier below, the highWaterMark only gets updated to shareValue when shareValue > highWaterMark is true. Because highWaterMark is not updated elsewhere except in the AdapterBase.__AdapterBase_init function, highWaterMark can only increase in the AdapterBase contract. As highWaterMark becomes bigger, calling the AdapterBase.accruedPerformanceFee function is less likely to accrue performance fee for the FEE_RECIPIENT. As a result, the more times the AdapterBase.takeFees modifier is triggered, the harder it gets for the FEE_RECIPIENT to receive the performance fee for the adapter.

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

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

        emit Harvested();
    }

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

    modifier takeFees() {
        _;

        uint256 fee = accruedPerformanceFee();
        if (fee > 0) _mint(FEE_RECIPIENT, convertToShares(fee));

        uint256 shareValue = convertToAssets(1e18);
        if (shareValue > highWaterMark) highWaterMark = shareValue;
    }

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

    /**
     * @notice Performance fee that has accrued since last fee harvest.
     * @return Accrued performance fee in underlying `asset` token.
     * @dev Performance fee is based on a high water mark value. If vault share value has increased above the
     *   HWM in a fee period, issue fee shares to the vault equal to the performance fee.
     */
    function accruedPerformanceFee() public view returns (uint256) {
        uint256 highWaterMark_ = highWaterMark;
        uint256 shareValue = convertToAssets(1e18);
        uint256 performanceFee_ = performanceFee;

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

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

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

Proof of Concept

The following steps can occur for the described scenario.

1. The AdapterBase.takeFees modifier is triggered for the first time, the FEE_RECIPIENT receives some performance fee for the adapter, and highWaterMark increases.
2. The AdapterBase.takeFees modifier is triggered for the second time. Because highWaterMark increased in Step 1, shareValue > highWaterMark_ becomes less likely to be true and the FEE_RECIPIENT is less likely to receive the performance fee in this step but still does. highWaterMark increases again.
3. The AdapterBase.takeFees modifier is triggered for couple more times. Now highWaterMark is much higher than the initial highWaterMark, shareValue > highWaterMark_ becomes very unlikely to be true and the FEE_RECIPIENT is very unlikely to receive any performance fee in this step.

Tools Used

VSCode

Recommended Mitigation Steps

Because AdapterBase.accruedPerformanceFee function would return the performance fee that has accrued since last fee harvest, it might make sense to update the AdapterBase.harvest function to sync highWaterMark to convertToAssets(1e18), which is similar to how the Vault.syncFeeCheckpoint modifier does, when the strategy's harvest function can be called.

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/yearn/YearnAdapter.sol#L80-L82 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/yearn/YearnAdapter.sol#L89-L98 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/yearn/YearnAdapter.sol#L101-L103 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/yearn/YearnAdapter.sol#L109-L111 https://github.com/yearn/yearn-vaults/blob/master/contracts/Vault.vy#L809-L813 https://github.com/yearn/yearn-vaults/blob/master/contracts/Vault.vy#L1811-L1838

Vulnerability details

Impact

The YearnAdapter._yTotalAssets function below attempts to resemble Yearn's Vault._totalAssets function but uses IERC20(asset()).balanceOf(address(yVault)) as part of the total quantity of all assets under control of the Yearn vault. In contrast, Yearn's Vault._totalAssets function below uses the self.totalIdle accounting variable instead of the asset balance owned by the vault returned by the asset token's balanceOf function. As shown by Yearn's Vault.sweep function below, it is possible that extra asset token amount is sent to the Yearn vault by accident in which the governance can withdraw such extra amount from the vault.

When such extra asset token amount is transferred to the Yearn vault, the YearnAdapter._yTotalAssets function will return a value that is bigger than it should be because IERC20(asset()).balanceOf(address(yVault)) is bigger than the Yearn vault's self.totalIdle at that moment. This will cause YearnAdapter._totalAssets() to be higher than it should be, which will inflate AdapterBase.totalAssets() and Vault.totalAssets() for the vault that uses the YearnAdapter contract as its adapter. Hence, comparing to if Yearn vault's self.totalIdle is used in the YearnAdapter._yTotalAssets function, calling the Vault.deposit function will mint less shares for depositing a given asset amount, and calling the Vault.mint function will deposit more asset amount for minting a given number of shares. If Yearn vault's governance has withdrawn the extra asset token amount before the Vault.withdraw or Vault.redeem function is called, calling the Vault.withdraw or Vault.redeem function will transfer an asset amount that is less than the deposited amount to the user in this case. As a result, the user loses the difference between the deposited and withdrawn asset amounts.

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/yearn/YearnAdapter.sol#L80-L82

    function _totalAssets() internal view override returns (uint256) {
        return _shareValue(underlyingBalance);
    }

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/yearn/YearnAdapter.sol#L89-L98

    function _shareValue(uint256 yShares) internal view returns (uint256) {
        if (yVault.totalSupply() == 0) return yShares;

        return
            yShares.mulDiv(
                _freeFunds(),
                yVault.totalSupply(),
                Math.Rounding.Down
            );
    }

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/yearn/YearnAdapter.sol#L101-L103

    function _freeFunds() internal view returns (uint256) {
        return _yTotalAssets() - _calculateLockedProfit();
    }

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/yearn/YearnAdapter.sol#L109-L111

    function _yTotalAssets() internal view returns (uint256) {
        return IERC20(asset()).balanceOf(address(yVault)) + yVault.totalDebt();
    }

https://github.com/yearn/yearn-vaults/blob/master/contracts/Vault.vy#L809-L813

@view
@internal
def _totalAssets() -> uint256:
    # See note on `totalAssets()`.
    return self.totalIdle + self.totalDebt

https://github.com/yearn/yearn-vaults/blob/master/contracts/Vault.vy#L1811-L1838

@external
def sweep(token: address, amount: uint256 = MAX_UINT256):
    ...
    assert msg.sender == self.governance
    # Can't be used to steal what this Vault is protecting
    value: uint256 = amount
    if value == MAX_UINT256:
        value = ERC20(token).balanceOf(self)

    if token == self.token.address:
        value = self.token.balanceOf(self) - self.totalIdle

    log Sweep(token, value)
    self.erc20_safe_transfer(token, self.governance, value)

Proof of Concept

The following steps can occur for the described scenario.

1. A vault that uses the YearnAdapter contract as its adapter exists.
2. An extra asset token amount is sent to the used Yearn vault by accident so IERC20(asset()).balanceOf(address(yVault)) is now bigger than the Yearn vault's self.totalIdle.
3. A user calls the Vault.mint function to mint 1 share, which deposits 10e18 asset tokens to the Yearn vault.
4. The Yearn vault's governance calls the Yearn vault's sweep function to withdraw the extra asset token amount from the Yearn vault.
5. The user calls the Vault.redeem function to redeem 1 share but only 9e18 asset tokens are withdrawn. Hence, the user loses 1e18 asset tokens.

Tools Used

VSCode

Recommended Mitigation Steps

The YearnAdapter._yTotalAssets function can be updated to use the Yearn vault's self.totalIdle instead of IERC20(asset()).balanceOf(address(yVault)).

def _totalAssets() -> uint256:
    # See note on `totalAssets()`.
    return self.token.balanceOf(self) + self.totalDebt

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L210-L235 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/yearn/YearnAdapter.sol#L166-L172 https://github.com/yearn/yearn-vaults/blob/master/contracts/Vault.vy#L1028-L1167

Vulnerability details

Impact

For a vault that uses the YearnAdapter contract as its adapter, calling the Vault.withdraw or Vault.redeem function will eventually call the AdapterBase._withdraw and YearnAdapter._protocolWithdraw functions below when the adapter is not paused. When the YearnAdapter._protocolWithdraw function executes yVault.withdraw(convertToUnderlyingShares(assets, shares)), the maxLoss input is not specified when calling the Yearn vault's withdraw function below. Thus, the Yearn vault's withdraw function will be called with its default maxLoss input value that is 0.01%. If the total loss incurred during the withdrawal is more than 0.01%, calling the Yearn vault's withdraw function that executes assert totalLoss <= maxLoss * (value + totalLoss) / MAX_BPS will revert. In a bear market, it is possible that the Yearn vault's strategies do not perform well so the total loss can be more than 0.01% permanently. In this situation, calling the Vault.withdraw or Vault.redeem function will always revert because calling the Yearn vault's withdraw function without specifying the maxLoss input reverts. As a result, users lose the deposited assets that they are unable to withdraw.

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

    function _withdraw(
        address caller,
        address receiver,
        address owner,
        uint256 assets,
        uint256 shares
    ) internal virtual override {
        ...

        if (!paused()) {
            uint256 underlyingBalance_ = _underlyingBalance();  
            _protocolWithdraw(assets, shares);
            // Update the underlying balance to prevent inflation attacks
            underlyingBalance -= underlyingBalance_ - _underlyingBalance();
        }

        ...
    }

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/yearn/YearnAdapter.sol#L166-L172

    function _protocolWithdraw(uint256 assets, uint256 shares)
        internal
        virtual
        override
    {
        yVault.withdraw(convertToUnderlyingShares(assets, shares));
    }

https://github.com/yearn/yearn-vaults/blob/master/contracts/Vault.vy#L1028-L1167

@external
@nonreentrant("withdraw")
def withdraw(
    maxShares: uint256 = MAX_UINT256,
    recipient: address = msg.sender,
    maxLoss: uint256 = 1,  # 0.01% [BPS]
) -> uint256:
    """
    ...
    @param maxLoss
        The maximum acceptable loss to sustain on withdrawal. Defaults to 0.01%.
        If a loss is specified, up to that amount of shares may be burnt to cover losses on withdrawal.
    @return The quantity of tokens redeemed for `_shares`.
    """
    shares: uint256 = maxShares  # May reduce this number below

    # Max Loss is <=100%, revert otherwise
    assert maxLoss <= MAX_BPS

    # If _shares not specified, transfer full share balance
    if shares == MAX_UINT256:
        shares = self.balanceOf[msg.sender]

    # Limit to only the shares they own
    assert shares <= self.balanceOf[msg.sender]

    # Ensure we are withdrawing something
    assert shares > 0

    # See @dev note, above.
    value: uint256 = self._shareValue(shares)
    vault_balance: uint256 = self.totalIdle

    if value > vault_balance:
        totalLoss: uint256 = 0
        # We need to go get some from our strategies in the withdrawal queue
        # NOTE: This performs forced withdrawals from each Strategy. During
        #       forced withdrawal, a Strategy may realize a loss. That loss
        #       is reported back to the Vault, and the will affect the amount
        #       of tokens that the withdrawer receives for their shares. They
        #       can optionally specify the maximum acceptable loss (in BPS)
        #       to prevent excessive losses on their withdrawals (which may
        #       happen in certain edge cases where Strategies realize a loss)
        for strategy in self.withdrawalQueue:
            if strategy == ZERO_ADDRESS:
                break  # We've exhausted the queue

            if value <= vault_balance:
                break  # We're done withdrawing

            amountNeeded: uint256 = value - vault_balance

            # NOTE: Don't withdraw more than the debt so that Strategy can still
            #       continue to work based on the profits it has
            # NOTE: This means that user will lose out on any profits that each
            #       Strategy in the queue would return on next harvest, benefiting others
            amountNeeded = min(amountNeeded, self.strategies[strategy].totalDebt)
            if amountNeeded == 0:
                continue  # Nothing to withdraw from this Strategy, try the next one

            # Force withdraw amount from each Strategy in the order set by governance
            preBalance: uint256 = self.token.balanceOf(self)
            loss: uint256 = Strategy(strategy).withdraw(amountNeeded)
            withdrawn: uint256 = self.token.balanceOf(self) - preBalance
            vault_balance += withdrawn

            # NOTE: Withdrawer incurs any losses from liquidation
            if loss > 0:
                value -= loss
                totalLoss += loss
                self._reportLoss(strategy, loss)

            # Reduce the Strategy's debt by the amount withdrawn ("realized returns")
            # NOTE: This doesn't add to returns as it's not earned by "normal means"
            self.strategies[strategy].totalDebt -= withdrawn
            self.totalDebt -= withdrawn
            log WithdrawFromStrategy(strategy, self.strategies[strategy].totalDebt, loss)

        self.totalIdle = vault_balance
        # NOTE: We have withdrawn everything possible out of the withdrawal queue
        #       but we still don't have enough to fully pay them back, so adjust
        #       to the total amount we've freed up through forced withdrawals
        if value > vault_balance:
            value = vault_balance
            # NOTE: Burn # of shares that corresponds to what Vault has on-hand,
            #       including the losses that were incurred above during withdrawals
            shares = self._sharesForAmount(value + totalLoss)

        # NOTE: This loss protection is put in place to revert if losses from
        #       withdrawing are more than what is considered acceptable.
        assert totalLoss <= maxLoss * (value + totalLoss) / MAX_BPS

    # Burn shares (full value of what is being withdrawn)
    self.totalSupply -= shares
    self.balanceOf[msg.sender] -= shares
    log Transfer(msg.sender, ZERO_ADDRESS, shares)
    
    self.totalIdle -= value
    # Withdraw remaining balance to _recipient (may be different to msg.sender) (minus fee)
    self.erc20_safe_transfer(self.token.address, recipient, value)
    log Withdraw(recipient, shares, value)
    
    return value

Proof of Concept

The following steps can occur for the described scenario.

1. A vault that uses the YearnAdapter contract as its adapter exists.
2. A user calls the Vault.deposit function to deposit some asset tokens in the corresponding Yearn vault.
3. A bear market starts so the Yearn vault's strategies do not perform well, and the total loss is more than 0.01% consistently.
4. Calling the Vault.withdraw or Vault.redeem function always reverts because the user cannot specify the maxLoss input for calling the Yearn vault's withdraw function. As a result, the user loses the deposited asset tokens.

Tools Used

VSCode

Recommended Mitigation Steps

The YearnAdapter._protocolWithdraw function can be updated to add an additional input that would be used as the maxLoss input for calling the Yearn vault's withdraw function. The other functions that call the YearnAdapter._protocolWithdraw function need to add this input as well.

Lines of code

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

Vulnerability details

Impact

Calling the following AdapterBase.harvest function can execute the strategy's harvest function only when (lastHarvest + harvestCooldown) < block.timestamp is true. As shown below, lastHarvest is only updated in the __AdapterBase_init function, and is not updated after the strategy's harvest function is called. This means that after (lastHarvest + harvestCooldown) < block.timestamp becomes true for the first time, (lastHarvest + harvestCooldown) < block.timestamp will always stay true and harvestCooldown has no effect afterwards. As a result, the strategy's harvest function will always be executed whenever the AdapterBase.harvest function is called after (lastHarvest + harvestCooldown) < block.timestamp becomes true for the first time, which defies the purpose of having a harvest cooldown.

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

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

        emit Harvested();
    }

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

    function __AdapterBase_init(bytes memory popERC4626InitData)
        internal
        onlyInitializing
    {
        (
            address asset,
            address _owner,
            address _strategy,
            uint256 _harvestCooldown,
            bytes4[8] memory _requiredSigs,
            bytes memory _strategyConfig
        ) = abi.decode(
                popERC4626InitData,
                (address, address, address, uint256, bytes4[8], bytes)
            );
        ...
        lastHarvest = block.timestamp;
    }

Proof of Concept

The following steps can occur for the described scenario.

1. (lastHarvest + harvestCooldown) < block.timestamp becomes true for the first time, and the AdapterBase.harvest function can be called to execute the strategy's harvest function.
2. After one second, the AdapterBase.harvest function can be called again to execute the strategy's harvest function though the harvest cooldown is 20 hours.
3. One minute is passed, and the AdapterBase.harvest function can still be called to execute the strategy's harvest function.
4. The strategy's harvest function will always be executed whenever the AdapterBase.harvest function is called.

Tools Used

VSCode

Recommended Mitigation Steps

The AdapterBase.harvest function can be updated to update lastHarvest using block.timestamp after the strategy's harvest function is executed.

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L253-L278 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L211-L240 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L496-L499 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L473-L477 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L480-L494 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L447-L460

Vulnerability details

Impact

The following Vault.redeem function does not use the syncFeeCheckpoint modifier, which is unlike the Vault.withdraw function below. Because of this, after calling the Vault.redeem function, highWaterMark is not sync'ed. In this case, calling functions like Vault.takeManagementAndPerformanceFees after the Vault.redeem function is called and before the syncFeeCheckpoint modifier is triggered will eventually use a stale highWaterMark to call the Vault.accruedPerformanceFee function. This will cause the performance fee to be calculated inaccurately in which the feeRecipient can receive more performance fee than it should receive or receive no performance fee when it should.

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

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

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

    function withdraw(
        uint256 assets,
        address receiver,
        address owner
    ) public nonReentrant syncFeeCheckpoint returns (uint256 shares) {
        ...
    }

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

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

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

    function takeManagementAndPerformanceFees()
        external
        nonReentrant
        takeFees
    {}

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

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

        _;
    }

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

    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

The following steps can occur for the described scenario.

1. A user calls the Vault.redeem function, which does not sync highWaterMark.
2. The vault owner calls the Vault.takeManagementAndPerformanceFees function, which eventually calls the accruedPerformanceFee function.
3. When calling the Vault.accruedPerformanceFee function, because convertToAssets(1e18) is less than the stale highWaterMark, no performance fee is accrued. If calling the Vault.redeem function can sync highWaterMark, some performance fee would be accrued through using such updated highWaterMark but that is not the case here.
4. feeRecipient receives no performance fee when it should.

Tools Used

VSCode

Recommended Mitigation Steps

The Vault.redeem function can be updated to use the syncFeeCheckpoint modifier.

Judge has assessed an item in Issue #795 as 2 risk. The relevant finding follows:

[01] FEE-ON-TRANSFER TOKENS ARE NOT SUPPORTED This protocol currently does not support fee-on-transfer tokens. For example, for a fee-on-transfer token, calling the following Vault.deposit function with the assets input being 1e18 can transfer less than 1e18 of such token to the Vault contract since the transfer fee is deducted during the transfer; then, when this function futher calls the AdapterBase.deposit function with the assets input still being 1e18, IERC20(asset()).safeTransferFrom(caller, address(this), assets) is executed but this execution reverts because the Vault contract does not have sufficient balance of such token for transferring 1e18 again to the AdapterBase contract. To support fee-on-transfer tokens, functions like Vault.deposit function, which needs to transfer the received assets out again, can be updated to calculate the actual token amount that is received and then transfer out that received amount instead of the assets input value.

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

function deposit(uint256 assets, address receiver)
    public
    nonReentrant
    whenNotPaused
    syncFeeCheckpoint
    returns (uint256 shares)
{
    ...
    asset.safeTransferFrom(msg.sender, address(this), assets);

    adapter.deposit(assets, address(this));
    ...
}

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

function deposit(uint256 assets, address receiver)
    public
    virtual
    override
    returns (uint256)
{
    ...
    _deposit(_msgSender(), receiver, assets, shares);
    ...
}

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

function _deposit(
    address caller,
    address receiver,
    uint256 assets,
    uint256 shares
) internal nonReentrant virtual override {
    IERC20(asset()).safeTransferFrom(caller, address(this), assets);
    ...
}

Judge has assessed an item in Issue #795 as 2 risk. The relevant finding follows:

[04] VALUES OF fees_ ARE NOT CHECKED IN Vault.initialize FUNCTION When calling the following Vault.initialize function, the values of fees_ are not checked. It is possible that these fees are set to be above 1e18 when calling the Vault.initialize function; if this happens, calling functions like Vault.deposit can revert because the fee amount would exceed the asset amount. To avoid reverting these function calls, please consider checking the values of fees_ in the Vault.initialize function to ensure that these values are capped by some reasonable limits.

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

function initialize(
    IERC20 asset_,
    IERC4626 adapter_,
    VaultFees calldata fees_,
    address feeRecipient_,
    address owner
) external initializer {
    ...
    fees = fees_;
    ...
}

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L559-L567 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L529-L542 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L480-L494 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L447-L460

Vulnerability details

Impact

In the following AdapterBase.takeFees modifier, if (shareValue > highWaterMark) highWaterMark = shareValue is executed after executing if (fee > 0) _mint(FEE_RECIPIENT, convertToShares(fee)). After this modifier executes _mint(FEE_RECIPIENT, convertToShares(fee)), totalSupply() increases. Then, executing uint256 shareValue = convertToAssets(1e18) can set shareValue to a value that is lower than the shareValue that is used when calling the AdapterBase.accruedPerformanceFee function. When executing if (shareValue > highWaterMark) highWaterMark = shareValue, highWaterMark is set to this lower shareValue.

This is different than the Vault.takeFees modifier below, which executes if (totalFee > 0 && currentAssets > 0) _mint(feeRecipient, convertToShares(totalFee)) after executing if (shareValue > highWaterMark) highWaterMark = shareValue. In this modifier, the shareValue that is used to set highWaterMark is the same shareValue that is used when calling the Vault.accruedPerformanceFee function.

Hence, in the AdapterBase.takeFees modifier, highWaterMark can be set to the shareValue that is lower than that shareValue used when calling the AdapterBase.accruedPerformanceFee function. This will make performance fee be more likely to be accrued for the adapter when the AdapterBase.takeFees modifier is trigged again because highWaterMark is essentially lower than it should be. As a result, the performance fee for the adapter paid to the FEE_RECIPIENT becomes more than it should be.

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

    modifier takeFees() {
        _;

        uint256 fee = accruedPerformanceFee();
        if (fee > 0) _mint(FEE_RECIPIENT, convertToShares(fee));

        uint256 shareValue = convertToAssets(1e18);
        if (shareValue > highWaterMark) highWaterMark = shareValue;
    }

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

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

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

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

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

        _;
    }

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

    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

The following steps can occur for the described scenario.

1. Calling the AdapterBase.harvest function, which triggers the AdapterBase.takeFees modifier, updates highWaterMark using the shareValue that is lower than that shareValue used when calling the AdapterBase.accruedPerformanceFee function.
2. A short time later, the AdapterBase.harvest function is called again. This time, calling AdapterBase.accruedPerformanceFee function returns some performance fee as well because shareValue > highWaterMark_ is true; hence, the FEE_RECIPIENT receives more performance fee.
3. However, if highWaterMark was set to the higher shareValue that was used when calling the AdapterBase.accruedPerformanceFee function in Step 1, calling AdapterBase.accruedPerformanceFee function in Step 2 can possibly return no more performance fee for the FEE_RECIPIENT since shareValue > highWaterMark_ would be false in this case.

Tools Used

VSCode

Recommended Mitigation Steps

The AdapterBase.takeFees modifier can be updated to execute if (fee > 0) _mint(FEE_RECIPIENT, convertToShares(fee)) after executing if (shareValue > highWaterMark) highWaterMark = shareValue, which is similar to the Vault.takeFees modifier.

[01] FEE-ON-TRANSFER TOKENS ARE NOT SUPPORTED

This protocol currently does not support fee-on-transfer tokens. For example, for a fee-on-transfer token, calling the following Vault.deposit function with the assets input being 1e18 can transfer less than 1e18 of such token to the Vault contract since the transfer fee is deducted during the transfer; then, when this function futher calls the AdapterBase.deposit function with the assets input still being 1e18, IERC20(asset()).safeTransferFrom(caller, address(this), assets) is executed but this execution reverts because the Vault contract does not have sufficient balance of such token for transferring 1e18 again to the AdapterBase contract. To support fee-on-transfer tokens, functions like Vault.deposit function, which needs to transfer the received assets out again, can be updated to calculate the actual token amount that is received and then transfer out that received amount instead of the assets input value.

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

    function deposit(uint256 assets, address receiver)
        public
        nonReentrant
        whenNotPaused
        syncFeeCheckpoint
        returns (uint256 shares)
    {
        ...
        asset.safeTransferFrom(msg.sender, address(this), assets);

        adapter.deposit(assets, address(this));
        ...
    }

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

    function deposit(uint256 assets, address receiver)
        public
        virtual
        override
        returns (uint256)
    {
        ...
        _deposit(_msgSender(), receiver, assets, shares);
        ...
    }

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

    function _deposit(
        address caller,
        address receiver,
        uint256 assets,
        uint256 shares
    ) internal nonReentrant virtual override {
        IERC20(asset()).safeTransferFrom(caller, address(this), assets);
        ...
    }

[02] RETURN VALUE OF DELEGATECALLING STRATEGY'S harvest FUNCTION IS NOT CHECKED IN AdapterBase.harvest FUNCTION

In the following AdapterBase.harvest function, the return value of executing address(strategy).delegatecall(abi.encodeWithSignature("harvest()")) is not checked. If executing the strategy's harvest function fails, this failure will go unnoticed while emitting the Harvested event incorrectly. Moreover, if executing the strategy's harvest function fails, such execution might need to be tried again when calling the AdapterBase.harvest function next time so the protocol probably should not go into the harvest cooldown. In other words, if needing to set lastHarvest to block.timestamp after executing the strategy's harvest function, it needs to check if such execution is successful or not; if not, the lastHarvest probably should not be set to block.timestamp to delay the harvest cooldown, and the Harvested event should not be emitted.

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

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

        emit Harvested();
    }

[03] YearnAdapter.convertToUnderlyingShares DOES NOT RETURN shares WHEN totalSupply() IS 0

When calling the following YearnAdapter.convertToUnderlyingShares function, it does not return shares when totalSupply() is 0, which is different than the BeefyAdapter.convertToUnderlyingShares function below that returns shares when totalSupply() is 0. For consistency with the BeefyAdapter.convertToUnderlyingShares function, and to avoid reverting the YearnAdapter.convertToUnderlyingShares function call when totalSupply() is 0, please consider updating the YearnAdapter.convertToUnderlyingShares function to return shares when totalSupply() is 0.

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/yearn/YearnAdapter.sol#L129-L137

    function convertToUnderlyingShares(uint256, uint256 shares)
        public
        view
        override
        returns (uint256)
    {
        return
            shares.mulDiv(underlyingBalance, totalSupply(), Math.Rounding.Up);
    }

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/beefy/BeefyAdapter.sol#L122-L133

    function convertToUnderlyingShares(uint256, uint256 shares)
        public
        view
        override
        returns (uint256)
    {
        uint256 supply = totalSupply();
        return
            supply == 0
                ? shares
                : shares.mulDiv(underlyingBalance, supply, Math.Rounding.Up);
    }

[04] VALUES OF fees_ ARE NOT CHECKED IN Vault.initialize FUNCTION

When calling the following Vault.initialize function, the values of fees_ are not checked. It is possible that these fees are set to be above 1e18 when calling the Vault.initialize function; if this happens, calling functions like Vault.deposit can revert because the fee amount would exceed the asset amount. To avoid reverting these function calls, please consider checking the values of fees_ in the Vault.initialize function to ensure that these values are capped by some reasonable limits.

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

    function initialize(
        IERC20 asset_,
        IERC4626 adapter_,
        VaultFees calldata fees_,
        address feeRecipient_,
        address owner
    ) external initializer {
        ...
        fees = fees_;
        ...
    }

[05] LIMITS FOR CALLING Vault.proposeFees ARE INCONSISTENT WITH LIMIT FOR CALLING AdapterBase.setPerformanceFee FUNCTION

The limits for calling the following Vault.proposeFees function are 1e18, including the limit used for the vault's performance fee, but the limit for calling the AdapterBase.setPerformanceFee function is 2e17. Besides that it is inconsistent and can be confusing to have different limits on the fees, setting the deposit and withdrawal fees to 1e18 means that all asset amounts are considered as fees, which does not make too much sense and is not user-friendly. Hence, please consider using a consistent reasonable limit on the fees for both the Vault.proposeFees and AdapterBase.setPerformanceFee functions.

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

    function proposeFees(VaultFees calldata newFees) external onlyOwner {
        if (
            newFees.deposit >= 1e18 ||
            newFees.withdrawal >= 1e18 ||
            newFees.management >= 1e18 ||
            newFees.performance >= 1e18
        ) revert InvalidVaultFees();

        proposedFees = newFees;
        proposedFeeTime = block.timestamp;

        emit NewFeesProposed(newFees, block.timestamp);
    }

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

    function setPerformanceFee(uint256 newFee) public onlyOwner {
        // Dont take more than 20% performanceFee
        if (newFee > 2e17) revert InvalidPerformanceFee(newFee);

        emit PerformanceFeeChanged(performanceFee, newFee);

        performanceFee = newFee;
    }

[06] UNSAFE decimals() CALLS FOR TOKENS THAT DO NOT IMPLEMENT decimals()

Each of the following code calls the relevant token's decimals(). According to https://eips.ethereum.org/EIPS/eip-20, decimals() is optional so it is possible that the relevant token does not implement decimals(). When this occurs, calling such token's decimals() reverts. To support such tokens, helper functions like BoringCrypto's safeDecimals can be used instead of directly calling decimals() in the code.

utils\MultiRewardStaking.sol
  51: _decimals = IERC20Metadata(address(_stakingToken)).decimals(); 
  274: uint64 ONE = (10**IERC20Metadata(address(rewardToken)).decimals()).safeCastTo64(); 

vault\Vault.sol
  82: _decimals = IERC20Metadata(address(asset_)).decimals(); 

vault\adapter\abstracts\AdapterBase.sol
  77: _decimals = IERC20Metadata(asset).decimals(); 

vault\strategy\Pool2SingleAssetCompounder.sol
  27: uint256[] memory amountsOut = IUniswapRouterV2(router).getAmountsOut(ERC20(asset).decimals() ** 10, tradePath); 

[07] UNSAFE symbol() CALLS FOR TOKENS THAT HAVE bytes32 SYMBOLS

Each of the following code calls the relevant token's symbol() using IERC20Metadata, which expects the token to have a string symbol. However, there are legitimate tokens with bytes32 symbols, such as MKR, and calling their symbol() using IERC20Metadata will revert. To support such tokens, helper functions like BoringCrypto's safeSymbol can be used instead of directly calling symbol() using IERC20Metadata in the code below.

vault\Vault.sol
  70: string.concat("pop-", IERC20Metadata(address(asset_)).symbol())

vault\adapter\beefy\BeefyAdapter.sol
  71: _symbol = string.concat("popB-", IERC20Metadata(asset()).symbol());

vault\adapter\yearn\YearnAdapter.sol
  52: _symbol = string.concat("popY-", IERC20Metadata(asset()).symbol());

[08] UNSAFE name() CALLS FOR TOKENS THAT HAVE bytes32 NAMES

Each of the following code calls the relevant token's name() using IERC20Metadata, which expects the token to have a string name. However, there are legitimate tokens with bytes32 names, such as MKR, and calling their name() using IERC20Metadata will revert. To support such tokens, helper functions like BoringCrypto's safeName can be used instead of directly calling name() using IERC20Metadata in the following code.

vault\Vault.sol
  67: IERC20Metadata(address(asset_)).name(), 

vault\adapter\beefy\BeefyAdapter.sol
  68: IERC20Metadata(asset()).name(), 

vault\adapter\yearn\YearnAdapter.sol
  49: IERC20Metadata(asset()).name(), 

[09] MISSING address(0) CHECKS FOR CRITICAL ADDRESS INPUTS

( Please note that the following instances are not found in https://gist.github.com/Picodes/b3c3bc8df01afc2b649534da8007af88#nc-1-missing-checks-for-address0-when-assigning-values-to-address-state-variables. )

To prevent unintended behaviors, critical address inputs should be checked against address(0). address(0) checks are missing for receiver in the following functions. Please consider checking them.

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

    function deposit(uint256 assets, address receiver)
        public
        virtual
        override
        returns (uint256)
    {
        ...
    }

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

    function mint(uint256 shares, address receiver)
        public
        virtual
        override
        returns (uint256)
    {
        ...
    }

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

    function withdraw(
        uint256 assets,
        address receiver,
        address owner
    ) public virtual override returns (uint256) {
        ...
    }

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

    function redeem(
        uint256 shares,
        address receiver,
        address owner
    ) public virtual override returns (uint256) {
        ...
    }

[10] INCORRECT COMMENTS

The comments of the following functions all mention that "caller must be owner". However, these functions all use the canCreate modifier so the allowed users, who are not the owner, can also call these functions. To prevent misleading information, please consider updating the comments of these functions accordingly.

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

  modifier canCreate() {
    if (
      permissionRegistry.endorsed(address(1))
        ? !permissionRegistry.endorsed(msg.sender)
        : permissionRegistry.rejected(msg.sender)
    ) revert NotAllowed(msg.sender);
    _;
  }

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

   * @notice Deploy a new Vault. Optionally with an Adapter and Staking. Caller must be owner.
   ...
  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) {
    ...
  }

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

   * @notice Deploy a new Adapter with our without a strategy. Caller must be owner.
   ...
  function deployAdapter(
    IERC20 asset,
    DeploymentArgs memory adapterData,
    DeploymentArgs memory strategyData,
    uint256 initialDeposit
  ) external canCreate returns (address adapter) {
    ...
  }

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

   * @notice Deploy a new staking contract. Caller must be owner.
   ...
  function deployStaking(IERC20 asset) external canCreate returns (address) {
    ...
  }

[11] UNRESOLVED TODO COMMENT

Comments regarding todos indicate that there are unresolved action items for implementation, which need to be addressed before the protocol deployment. Please review the todo comment in the following code.

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

    // TODO use deterministic fee recipient proxy
    address FEE_RECIPIENT = address(0x4444);

[12] COMMENT THAT IS NOT EXACT

The following code comment mentions X seconds but the code uses 1 days. To avoid ambiguity, please consider updating the comment to reflect 1 days.

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

        // Dont wait more than X seconds
        if (newCooldown >= 1 days) revert InvalidHarvestCooldown(newCooldown);

[13] WORD TYPING TYPO

aftwards can be afterwards in the following code comment.

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

     * @dev It aftwards sets up anything required by the strategy to call `harvest()` like approvals etc.

[14] REDUNDANT NAMED RETURNS

When a function has unused named returns and used return statement, these named returns become redundant. To improve readability and maintainability, these variables for the named returns can be removed while keeping the return statement for the following execute function.

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/AdminProxy.sol#L19-L25

  function execute(address target, bytes calldata callData)
    external
    onlyOwner
    returns (bool success, bytes memory returndata)
  {
    return target.call(callData);
  }

[15] CONSTANTS CAN BE USED INSTEAD OF MAGIC NUMBERS

To improve readability and maintainability, a constant can be used instead of the magic number. Please consider replacing the magic numbers, such as 1e18, used in the following code with constants.

vault\Vault.sol
  144: assets.mulDiv(uint256(fees.deposit), 1e18, Math.Rounding.Down)  
  183: 1e18 - depositFee,  
  224: 1e18 - withdrawalFee,   
  330: assets.mulDiv(uint256(fees.deposit), 1e18, Math.Rounding.Down)  
  345: 1e18 - depositFee,  
  437: ) / 1e18    
  449: uint256 shareValue = convertToAssets(1e18); 
  484: uint256 shareValue = convertToAssets(1e18);  
  498: highWaterMark = convertToAssets(1e18);  
  527: newFees.deposit >= 1e18 ||  
  528: newFees.withdrawal >= 1e18 ||  
  529: newFees.management >= 1e18 ||  
  530: newFees.performance >= 1e18  

vault\adapter\abstracts\AdapterBase.sol
  85: highWaterMark = 1e18;   
  531: uint256 shareValue = convertToAssets(1e18); 
  538: 1e36,   
  551: if (newFee > 2e17) revert InvalidPerformanceFee(newFee);    
  565: uint256 shareValue = convertToAssets(1e18); 

[16] FLOATING PRAGMAS

It is a best practice to lock pragmas instead of using floating pragmas to ensure that contracts are tested and deployed with the intended compiler version. Accidentally deploying contracts with different compiler versions can lead to unexpected risks and undiscovered bugs. Please consider locking pragmas for the following files.

utils\MultiRewardEscrow.sol
  4: pragma solidity ^0.8.15;

utils\MultiRewardStaking.sol
  4: pragma solidity ^0.8.15;

vault\AdminProxy.sol
  4: pragma solidity ^0.8.15;

vault\CloneFactory.sol
  4: pragma solidity ^0.8.15;

vault\CloneRegistry.sol
  4: pragma solidity ^0.8.15;

vault\DeploymentController.sol
  4: pragma solidity ^0.8.15;

vault\PermissionRegistry.sol
  4: pragma solidity ^0.8.15;

vault\TemplateRegistry.sol
  4: pragma solidity ^0.8.15;

vault\Vault.sol
  4: pragma solidity ^0.8.15;

vault\VaultController.sol
  3: pragma solidity ^0.8.15;

vault\VaultRegistry.sol
  4: pragma solidity ^0.8.15;

vault\adapter\abstracts\AdapterBase.sol
  4: pragma solidity ^0.8.15;

vault\adapter\abstracts\OnlyStrategy.sol
  4: pragma solidity ^0.8.15;

vault\adapter\abstracts\WithRewards.sol
  4: pragma solidity ^0.8.15;

vault\adapter\beefy\BeefyAdapter.sol
  4: pragma solidity ^0.8.15;

vault\adapter\yearn\YearnAdapter.sol
  4: pragma solidity ^0.8.15;

[17] INCOMPLETE NATSPEC COMMENTS

NatSpec comments provide rich code documentation. The following functions miss the @param and/or @return comments. Please consider completing the NatSpec comments for these functions.

vault\adapter\abstracts\AdapterBase.sol
  110: function deposit(uint256 assets, address receiver)  
  147: function _deposit(  
  173: function withdraw(  
  294: function _previewDeposit(uint256 assets)    
  343: function _previewWithdraw(uint256 assets)   
  380: function _convertToShares(uint256 assets, Math.Rounding rounding)   
  404: function maxDeposit(address)    
  479: function _verifyAndSetupStrategy(bytes4[8] memory requiredSigs) internal {  

vault\adapter\yearn\YearnAdapter.sol
  80: function _totalAssets() internal view override returns (uint256) {  
  89: function _shareValue(uint256 yShares) internal view returns (uint256) {  
  101: function _freeFunds() internal view returns (uint256) { 
  109: function _yTotalAssets() internal view returns (uint256) { 
  114: function _calculateLockedProfit() internal view returns (uint256) { 
  129: function convertToUnderlyingShares(uint256, uint256 shares)  

[18] MISSING NATSPEC COMMENTS

NatSpec comments provide rich code documentation. The following functions miss NatSpec comments. Please consider adding NatSpec comments for these functions.

vault\PermissionRegistry.sol
  51: function endorsed(address target) external view returns (bool) {  
  55: function rejected(address target) external view returns (bool) {  

vault\Vault.sol
  100: function decimals() public view override returns (uint8) {  
  124: function deposit(uint256 assets) public returns (uint256) { 
  160: function mint(uint256 shares) external returns (uint256) {  
  200: function withdraw(uint256 assets) public returns (uint256) {    
  242: function redeem(uint256 shares) external returns (uint256) {    
  716: function computeDomainSeparator() internal view virtual returns (bytes32) { 

vault\adapter\abstracts\AdapterBase.sol
  684: function computeDomainSeparator() internal view virtual returns (bytes32) { 

vault\adapter\beefy\BeefyAdapter.sol
  108: function _totalAssets() internal view override returns (uint256) {  
  117: function _underlyingBalance() internal view override returns (uint256) {    

vault\adapter\yearn\YearnAdapter.sol
  84: function _underlyingBalance() internal view override returns (uint256) {    
  158: function _protocolDeposit(uint256 amount, uint256)  
  166: function _protocolWithdraw(uint256 assets, uint256 shares)