Lack of slippage control and deadline check when depositing into / withdraw from the IChiVaultSpell and IChiFarm integration

Summary

Lack of slippage control and deadline check when depositing into / withdraw from the vault and when performing cToken mint / redeeming

Vulnerability Detail

In IChiVaultSpell.sol, when depositInternal is called, the function below executes

    function depositInternal(
        uint256 strategyId,
        address collToken,
        address borrowToken,
        uint256 collAmount,
        uint256 borrowAmount
    ) internal {
        Strategy memory strategy = strategies[strategyId];

        // 1. Lend isolated collaterals on compound
        doLend(collToken, collAmount);

        // 2. Borrow specific amounts
        doBorrow(borrowToken, borrowAmount);

        // 3. Add liquidity - Deposit on ICHI Vault
        IICHIVault vault = IICHIVault(strategy.vault);
        bool isTokenA = vault.token0() == borrowToken;
        uint256 balance = IERC20(borrowToken).balanceOf(address(this));
        ensureApprove(borrowToken, address(vault));
        if (isTokenA) {
            vault.deposit(balance, 0, address(this));
        } else {
            vault.deposit(0, balance, address(this));
        }

note the function calls:

if (isTokenA) {
    vault.deposit(balance, 0, address(this));
} else {
    vault.deposit(0, balance, address(this));
}

Vault.deposit extends to mind share, but because the function lack of slippage control and deadline check, a very sub-optimal amount of share can be minted when transaction executes.

If we use the MockVault as a reference:

https://github.com/sherlock-audit/2023-02-blueberry/blob/main/contracts/mock/MockIchiVault.sol#L270-L280

The minted share is heavily depend on the token price and the totalSupply

shares = deposit1 + deposit0PricedInToken1;

if (totalSupply() != 0) {
    uint256 pool0PricedInToken1 = (pool0 *
        ((price > twap) ? price : twap)) / PRECISION;
    shares = (shares * totalSupply()) / (pool0PricedInToken1 + pool1);
}
_mint(to, shares);
emit Deposit(msg.sender, to, shares, deposit0, deposit1);

If the user fires a transaction but there is gas spike, the user's transaction is pending in the mempool for a long time until later the gas price drops, the user's transaction is executed when the token price increases, which result in less shares.

Same issue in the IChiFarm.deposit

      ichiFarm.deposit(pid, amount, address(this));
        (uint256 ichiPerShare, , ) = ichiFarm.poolInfo(pid);
        uint256 id = encodeId(pid, ichiPerShare);
        _mint(msg.sender, id, amount, "");
        return id;

which calls:

https://github.com/sherlock-audit/2023-02-blueberry/blob/main/contracts/mock/MockIchiFarm.sol#L246

`    /// @param to The receiver of `amount` deposit benefit.
    function deposit(
        uint256 pid,
        uint256 amount,
        address to
    ) external {
        require(!nonReentrant, 'ichiFarmV2::nonReentrant - try again');
        nonReentrant = true;

        PoolInfo memory pool = updatePool(pid);
        UserInfo storage user = userInfo[pid][to];

        // Effects
        user.amount += amount;
        user.rewardDebt += int256(
            (amount * pool.accIchiPerShare) / ACC_ICHI_PRECISION
        );

        // Interactions
        lpToken[pid].safeTransferFrom(msg.sender, address(this), amount);

        emit Deposit(msg.sender, pid, amount, to);
        nonReentrant = false;
    }

If the transaction is pending for a long time, the ichiPerShare could drops a lot when the transaction lended.

Same issue exists for vault.withdraw iin IChiVaultSpell.sol

      // 2. Calculate actual amount to remove
        uint256 amtLPToRemove = vault.balanceOf(address(this)) -
            amountLpWithdraw;

        // 3. Withdraw liquidity from ICHI vault
        vault.withdraw(amtLPToRemove, address(this));

which calls:

https://github.com/sherlock-audit/2023-02-blueberry/blob/main/contracts/mock/MockIchiVault.sol#L320

        // Push tokens proportional to unused balances
        uint256 _totalSupply = totalSupply();
        uint256 unusedAmount0 = (IERC20(token0).balanceOf(address(this)) *
            shares) / _totalSupply;
        uint256 unusedAmount1 = (IERC20(token1).balanceOf(address(this)) *
            shares) / _totalSupply;
        if (unusedAmount0 > 0) IERC20(token0).safeTransfer(to, unusedAmount0);
        if (unusedAmount1 > 0) IERC20(token1).safeTransfer(to, unusedAmount1);

        amount0 = base0 + limit0 + unusedAmount0;
        amount1 = base1 + limit1 + unusedAmount1;

        _burn(msg.sender, shares);

        emit Withdraw(msg.sender, to, shares, amount0, amount1);

the amount of token withdraw is derived from totalSupply() and token1 balance and token0,

suppose a user want to withdraw from the IChiVaultSpell, the user expected to withdraw 50 amount of token, but the transaction is pending for a long in the mempool

There are transaction landed before the withdraw transaction which change the value of totalSupply in the vault.

and the totalSupply() increases a lot, the amount of token withdraws is less than 50 token and the user only get 30 tokens because the math:

uint256 unusedAmount0 = (IERC20(token0).balanceOf(address(this)) *
    shares) / _totalSupply;
uint256 unusedAmount1 = (IERC20(token1).balanceOf(address(this)) *
    shares) / _totalSupply;

Same issue happens in IChiFarm withdraw:

    function burn(uint256 id, uint256 amount)
        external
        nonReentrant
        returns (uint256)
    {
        if (amount == type(uint256).max) {
            amount = balanceOf(msg.sender, id);
        }
        (uint256 pid, uint256 stIchiPerShare) = decodeId(id);
        _burn(msg.sender, id, amount);

        uint256 ichiRewards = ichiFarm.pendingIchi(pid, address(this));
        ichiFarm.harvest(pid, address(this));
        ichiFarm.withdraw(pid, amount, address(this));

Impact

Without deadline check for withdraw / deposit, the pending transaction can be executed when the withdraw amount / deposit share minted amount is against user.

Without slippage check, when the withdraw amount / deposit share minted amount is against user, transaction does not revert.

Code Snippet

https://github.com/sherlock-audit/2023-02-blueberry/blob/main/contracts/spell/IchiVaultSpell.sol#L292-L299

Tool used

Manual Review

Recommendation

We recommend add deadline check and slippage control when deposit / withdraw in vault and farm.

Duplicate of #130

sherlock-audit / 2023-02-blueberry-judging

ctf_sec - Lack of slippage control and deadline check when depositing into / withdraw from the IChiVaultSpell and IChiFarm integration #356