0x52 - UniswapV3 sqrtRatioLimit doesn't provide slippage protection and will result in partial swaps

[sherlock-admin]

0x52

high

UniswapV3 sqrtRatioLimit doesn't provide slippage protection and will result in partial swaps

Summary

The sqrtRatioLimit for UniV3 doesn't cause the swap to revert upon reaching that value. Instead it just cause the swap to partially fill. This is a known issue with using sqrtRatioLimit as can be seen here where the swap ends prematurely when it has been reached. This is problematic as this is meant to provide the user with slippage protection but doesn't.

Vulnerability Detail

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/IchiSpell.sol#L209-L223

    if (amountToSwap > 0) {
        SWAP_POOL = IUniswapV3Pool(vault.pool());
        uint160 deltaSqrt = (param.sqrtRatioLimit *
            uint160(param.sellSlippage)) / uint160(Constants.DENOMINATOR);
        SWAP_POOL.swap(
            address(this),
            // if withdraw token is Token0, then swap token1 -> token0 (false)
            !isTokenA,
            amountToSwap.toInt256(),
            isTokenA
                ? param.sqrtRatioLimit + deltaSqrt
                : param.sqrtRatioLimit - deltaSqrt, // slippaged price cap
            abi.encode(address(this))
        );
    }

sqrtRatioLimit is used as slippage protection for the user but is ineffective and depending on what tokens are being swapped, tokens may be left the in the contract which can be stolen by anyone.

Impact

Incorrect slippage application can result in partial swaps and loss of funds

Code Snippet

IchiSpell.sol#L181-L236

Tool used

Manual Review

Recommendation

Check the amount received from the swap and compare it against some user supplied minimum

[Gornutz]

Blueberryfi/blueberry-core@314eaf2

[IAm0x52]

Contract now uses the UniV3 router rather than interacting with the pool directly. However it still utilizes sqrtPriceLimitX96 which is potentially dangerous. Should use sqrtPriceLimitX96 = 0.