code-423n4 / 2024-05-bakerfi-findings

4 stars 4 forks source link

Multiple swap lack slippage protection. #32

Open c4-bot-7 opened 6 months ago

c4-bot-7 commented 6 months ago

Lines of code

https://github.com/code-423n4/2024-05-bakerfi/blob/59b1f70cbf170871f9604e73e7fe70b70981ab43/contracts/core/hooks/UseSwapper.sol#L72

Vulnerability details

Vulnerability details

The current protocol requires swapping tokens in multiple places, such as weth -> ierc20A or ierc20A -> weth. Primarily, these swaps are executed using the _swap() method.

    function _swap(
        ISwapHandler.SwapParams memory params
    ) internal override returns (uint256 amountOut) {
        if (params.underlyingIn == address(0)) revert InvalidInputToken();
        if (params.underlyingOut == address(0)) revert InvalidOutputToken();
        uint24 fee = params.feeTier;
        if (fee == 0) revert InvalidFeeTier();

        // Exact Input
        if (params.mode == ISwapHandler.SwapType.EXACT_INPUT) {
            amountOut = _uniRouter.exactInputSingle(
                IV3SwapRouter.ExactInputSingleParams({
                    tokenIn: params.underlyingIn,
                    tokenOut: params.underlyingOut,
                    amountIn: params.amountIn,
@>                  amountOutMinimum: 0,   //@audit miss set params.amountOut
                    fee: fee,
                    recipient: address(this),
                    sqrtPriceLimitX96: 0
                })
            );
            if (amountOut == 0) {
                revert SwapFailed();
            }
            emit Swap(params.underlyingIn, params.underlyingOut, params.amountIn, amountOut);
            // Exact Output
        } else if (params.mode == ISwapHandler.SwapType.EXACT_OUTPUT) {
            uint256 amountIn = _uniRouter.exactOutputSingle(
                IV3SwapRouter.ExactOutputSingleParams({
                    tokenIn: params.underlyingIn,
                    tokenOut: params.underlyingOut,
                    fee: fee,
                    recipient: address(this),
                    amountOut: params.amountOut,
                    amountInMaximum: params.amountIn,
                    sqrtPriceLimitX96: 0
                })
            );
            if (amountIn < params.amountIn) {
                IERC20(params.underlyingIn).safeTransfer(address(this), params.amountIn - amountIn);
            }
            emit Swap(params.underlyingIn, params.underlyingOut, amountIn, params.amountOut);
            amountOut = params.amountOut;
        }
    }

This method does not set amountOutMinimum.

and when call same miss set Amount Out

abstract contract StrategyLeverage is
    function _convertFromWETH(uint256 amount) internal virtual returns (uint256) {
        // 1. Swap WETH -> cbETH/wstETH/rETH
        return
            _swap(
                ISwapHandler.SwapParams(
                    wETHA(), // Asset In
                    ierc20A(), // Asset Out
                    ISwapHandler.SwapType.EXACT_INPUT, // Swap Mode
                    amount, // Amount In
                    //@audit miss slippage protection
@>                  0, // Amount Out 
                    _swapFeeTier, // Fee Pair Tier
                    bytes("") // User Payload
                )
            );
    }

These methods do not have slippage protection.

https://docs.uniswap.org/contracts/v3/guides/swaps/single-swaps

amountOutMinimum: we are setting to zero, but this is a significant risk in production. For a real deployment, this value should be calculated using our SDK or an onchain price oracle - this helps protect against getting an unusually bad price for a trade due to a front running sandwich or another type of price manipulation

inculde:UseSwapper._swap()/ _convertFromWETH()/_convertToWETH()/_payDebt()

Impact

front running sandwich or another type of price manipulation

Recommended Mitigation

  1. _swap() need set amountOutMinimum = params.amountOut

    function _swap(
        ISwapHandler.SwapParams memory params
    ) internal override returns (uint256 amountOut) {
        if (params.underlyingIn == address(0)) revert InvalidInputToken();
        if (params.underlyingOut == address(0)) revert InvalidOutputToken();
        uint24 fee = params.feeTier;
        if (fee == 0) revert InvalidFeeTier();
    
        // Exact Input
        if (params.mode == ISwapHandler.SwapType.EXACT_INPUT) {
            amountOut = _uniRouter.exactInputSingle(
                IV3SwapRouter.ExactInputSingleParams({
                    tokenIn: params.underlyingIn,
                    tokenOut: params.underlyingOut,
                    amountIn: params.amountIn,
    -                   amountOutMinimum: 0,
    +                  amountOutMinimum: params.amountOut
                    fee: fee,
                    recipient: address(this),
                    sqrtPriceLimitX96: 0
                })
            );
            if (amountOut == 0) {
                revert SwapFailed();
            }
  2. call _swap() need set params.amountOut Calculating the allowed slippage value accurately.

Assessed type

MEV

c4-judge commented 5 months ago

0xleastwood marked the issue as primary issue

c4-judge commented 5 months ago

0xleastwood marked the issue as selected for report

ickas commented 5 months ago

Fixed → https://github.com/baker-fi/bakerfi-contracts/pull/41