swapOut functions have invalid slippage check, causing user loss of funds

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-08-superposition/blob/4528c9d2dbe1550d2660dac903a8246076044905/pkg/sol/SeawaterAMM.sol#L317 https://github.com/code-423n4/2024-08-superposition/blob/4528c9d2dbe1550d2660dac903a8246076044905/pkg/sol/SeawaterAMM.sol#L339

Vulnerability details

Impact

swapOut functions have an invalid slippage check, causing user loss of funds.

Proof of Concept

In SeawaterAMM.sol, swapOut5E08A399 and swapOutPermit23273373B are intended to allow usdc(token1) -> pool(token0) swap with slippage check. See ISeawaterAMM's doc.

However, both functions have incorrect slippage checks. (1) We see in swapOut5E08A399 swapAmountOut is used to check with minOut. But swapAmountOut is actually usdc(token1), Not the output token(token0). This uses an incorrect variable to check slippage.

    function swapOut5E08A399(
        address token,
        uint256 amountIn, //@audit-info note: this is usdc(token1)
        uint256 minOut
    ) external returns (int256, int256) {
            (bool success, bytes memory data) = _getExecutorSwap().delegatecall(
            abi.encodeCall(
                ISeawaterExecutorSwap.swap904369BE,
                (token, false, int256(amountIn), type(uint256).max)
            )
        );
        require(success, string(data));
        //@audit-info note: swapAmountIn <=> token0 , swapAmountOut <=> token1
|>      (int256 swapAmountIn, int256 swapAmountOut) = abi.decode(
            data,
            (int256, int256)
        );
       //@audit This should use token0 value, not token1
|>     require(swapAmountOut >= int256(minOut), "min out not reached!");
       return (swapAmountIn, swapAmountOut);
    }

(https://github.com/code-423n4/2024-08-superposition/blob/4528c9d2dbe1550d2660dac903a8246076044905/pkg/sol/SeawaterAMM.sol#L317)

For reference, in the swap facet, swap_internal called in the flow returns Ok((amount_0, amount_1)). This means swapAmountOut refers to token1, the input token amount.

//pkg/seawater/src/lib.rs
    pub fn swap_internal(
        pools: &mut Pools,
        pool: Address,
        zero_for_one: bool,
        amount: I256,
        price_limit_x96: U256,
        permit2: Option<Permit2Args>,
    ) -> Result<(I256, I256), Revert> {
        let (amount_0, amount_1, _ending_tick) =
            pools
                .pools
                .setter(pool)
                .swap(zero_for_one, amount, price_limit_x96)?;
...
        Ok((amount_0, amount_1))

(https://github.com/code-423n4/2024-08-superposition/blob/4528c9d2dbe1550d2660dac903a8246076044905/pkg/seawater/src/lib.rs#L194)

(2) swapOutPermit23273373B has the same erroneous slippage check.

//pkg/sol/SeawaterAMM.sol
    function swapOutPermit23273373B(address token, uint256 amountIn, uint256 minOut, uint256 nonce, uint256 deadline, uint256 maxAmount, bytes memory sig) external returns (int256, int256) {
...
|>     (int256 swapAmountIn, int256 swapAmountOut) = abi.decode(data, (int256, int256));
|>      require(swapAmountOut >= int256(minOut), "min out not reached!");
        return (swapAmountIn, swapAmountOut);
    }

(https://github.com/code-423n4/2024-08-superposition/blob/4528c9d2dbe1550d2660dac903a8246076044905/pkg/sol/SeawaterAMM.sol#L339)

Invalid slippage checks will cause users to lose funds during swaps.

Tools Used

Manual

Recommended Mitigation Steps

Consider changing into:

...
        (int256 swapAmountOut, int256 swapAmountIn) = abi.decode(
            data,
            (int256, int256)
        );
                require(uint256(-swapAmountOut) >= minOut, "min out not reached!");
                return (swapAmountOut, swapAmountIn);

Assessed type

Error

[c4-judge]

alex-ppg marked the issue as selected for report

[c4-judge]

alex-ppg marked the issue as satisfactory

[alex-ppg]

The Warden has identified that the slippage protections of the swap-out functions are incorrect and thus ineffective due to validating an incorrect amount, resulting in either incorrect successful executions or incorrect failed executions depending on the price relation between the two assets.

A medium-risk severity rating is appropriate given that a subset of the system's functionality is affected with slippage being the component affected.

[alex-ppg]

After revisiting this submission in light of the comments shared in https://github.com/code-423n4/2024-08-superposition-findings/issues/53#issuecomment-2374785547, I am inclined to upgrade it to a high-risk severity rating as user funds are directly impacted.

[c4-judge]

alex-ppg changed the severity to 3 (High Risk)