IchiSpell#_withdraw attempts to limit slippage but applies slippage limit to user supplied data making it ineffective

Summary

IchiSpell#_withdraw attempts to limit slippage but applies slippage limit to user supplied data making it completely ineffective. Slippage limits protect the protocol in the event that a malicious user wants to extract value via swaps, this is an important protection in the event that a user finds a way to trick collateral requirements.

Vulnerability Detail

IchiSpell.sol#L210-L222

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

We can see above that the slippage values are applied on top of user supplied slippage data which means that it is completely ineffective. Assume the protocol wishes to limit slippage to 3%. The user can specify a slippage value of 3% and this would add another 3% allowing the user 6% slippage. The user can specify any slippage value they wish and the 3% will be added on top making it completely ineffective.

Impact

Slippage limits can be bypassed

Code Snippet

IchiSpell.sol#L181-L236

Tool used

Manual Review

Recommendation

Apply slippage limits based on oracle pricing

sherlock-audit / 2023-04-blueberry-judging

0x52 - IchiSpell#_withdraw attempts to limit slippage but applies slippage limit to user supplied data making it ineffective #130