swap_2 implementation will randomly revert due to improper check, root cause for failed test ethers_suite_uniswap_orchestrated_uniswap_two

[c4-bot-8]

Lines of code

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

Vulnerability details

Impact

swap_2 implementation will randomly revert due to improper check, root cause for failed test ethers_suite_uniswap_orchestrated_uniswap_two.

Proof of Concept

The contest readme listed an unsolved failed test ethers_suite_uniswap_orchestrated_uniswap_two(). The failed test is due to an improper check in swap_2_internal(), which sometimes causes a valid swap_2 to revert.

We see that in swap_2_internal, interim_usdc_out is required to equal interim_usdc_in. interim_usdc_out is the fusdc swapped out in 1st swap. interim_usdc_in is the actual fusdc swapped in 2nd swap. In any swap, not all user input amount has to be used to achieve a desirable output. Especially in uniswapV3 swap logic, amount_remaining can be greater than 0, which means not all input tokens are used.

//pkg/seawater/src/lib.rs
    fn swap_2_internal(
        pools: &mut Pools,
        from: Address,
        to: Address,
        amount: U256,
        min_out: U256,
    ) -> Result<(U256, U256, U256, I256, i32, i32), Revert> {
...
        // swap in -> usdc
        let (amount_in, interim_usdc_out, final_tick_in) = pools.pools.setter(from).swap(
            true,
            amount,
            // swap with no price limit, since we use min_out instead
            tick_math::MIN_SQRT_RATIO + U256::one(),
        )?;
...
        // swap usdc -> out
        let (amount_out, interim_usdc_in, final_tick_out) = pools.pools.setter(to).swap(
            false,
            interim_usdc_out,
            tick_math::MAX_SQRT_RATIO - U256::one(),
        )?;
...
|>      assert_eq_or!(interim_usdc_out, interim_usdc_in, Error::InterimSwapNotEq);
...

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

The equality check will cause revert when 2nd swap runs out of liquidity but still satisfies the user's min_out. A valid swap can be reverted.

Test ethers_suite_uniswap_orchestrated_uniswap_two() analysis:
When running the test, the output error byte array [73, 110, 116, 101, 114, 110, 97, 108, 32, 115, 119, 97, 112, 32, 97, 109, 111, 117, 110, 116, 115, 32, 110, 111, 116, 32, 109, 97, 116, 99, 104, 101, 100] decodes(ASII) into Internal swap amounts not matched, corresponding to Error::InterimSwapNotEq. This custom error is only used in swap_2_internal.

thread 'ethers_suite_orchestrated_uniswap_two' panicked at seawater/tests/lib.rs:783:18:
called `Result::unwrap()` on an `Err` value: [73, 110, 116, 101, 114, 110, 97, 108, 32, 115, 119, 97, 112, 32, 97, 109, 111, 117, 110, 116, 115, 32, 110, 111, 116, 32, 109, 97, 116, 99, 104, 101, 100]

I added a unit test to break down the failed reason ethers_suite_orchestrated_uniswap_two_breakdown.

//pkg/seawater/tests/lib.rs
#[test]
fn ethers_suite_orchestrated_uniswap_two_breakdown() {
    test_utils::with_storage::<_, Pools, _>(
        Some(address!("3f1Eae7D46d88F08fc2F8ed27FCb2AB183EB2d0E").into_array()), // sender
        None,
        None,
        None,
        |contract| {
            let token0 = address!("9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0");
            let token1 = address!("9fE46736679d2D9a65F0992F2272dE9f3c7fa6e1");
            contract
                .ctor(msg::sender(), Address::ZERO, Address::ZERO)
                .unwrap();
            contract
                .create_pool_D650_E2_D0(
                    token0,
                    U256::from_limbs([0, 42949672960, 0, 0]), //792281625142643375935439503360
                    500,                                      // fee
                    10,                                       // tick spacing
                    u128::MAX,
                )
                .unwrap();
            contract
                .create_pool_D650_E2_D0(
                    token1,
                    U256::from_limbs([0, 42949672960, 0, 0]), //792281625142643375935439503360
                    500,                                      // fee
                    10,                                       // tick spacing
                    u128::MAX,
                )
                .unwrap();
            contract.enable_pool_579_D_A658(token0, true).unwrap();
            contract.enable_pool_579_D_A658(token1, true).unwrap();
            contract
                .mint_position_B_C5_B086_D(token0, 39120, 50100)
                .unwrap();
            contract
                .mint_position_B_C5_B086_D(token1, 39120, 50100)
                .unwrap();
            let id = U256::ZERO;
            let (amount_0_in, fusdc_0_in) = contract
                .update_position_C_7_F_1_F_740(token0, id, 20000)
                .unwrap();
            println!("amount_0_in: {amount_0_in}, fusdc_0_in: {fusdc_0_in}");
            let (amount_1_in, fusdc_1_in) = contract
                .update_position_C_7_F_1_F_740(token1, U256::one(), 20000)
                .unwrap();
            println!("amount_1_in: {amount_1_in}, fusdc_1_in: {fusdc_1_in}");
            //breakdown: step1 token0 -> fusdc
            let (amount_out_0, fusdc_out_0) = contract
                .swap_904369_B_E(token0, true, I256::try_from(1000_i32).unwrap(), U256::MAX)
                .unwrap();
            println!("amount_out_0: {amount_out_0}, fusdc_out_0: {fusdc_out_0}");
            //breakdown: step2 fusdc -> token1
            let (amount_out_1, fusdc_out_1) = contract
                .swap_904369_B_E(token1, false, -fusdc_out_0, U256::MAX)
                .unwrap();
            println!("amount_out_1: {amount_out_1}, fusdc_out_1: {fusdc_out_1}");
            assert_ne!(-fusdc_out_0, fusdc_out_1);
        },
    );
}

Test result:

     Running tests/lib.rs (target/debug/deps/lib-b27e97df8f1d4fcd)

running 1 test
amount_0_in: 367, fusdc_0_in: 58595
amount_1_in: 367, fusdc_1_in: 58595
amount_out_0: 833, fusdc_out_0: -58592
amount_out_1: -365, fusdc_out_1: 44866
test ethers_suite_orchestrated_uniswap_two_breakdown ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 16 filtered out; finished in 0.04s

As seen above, due to in the 2nd swap (fusdc → token1) , the token1 pool ran out of liquidity, only a portion of fusdc_out_0 is used to perform the swap.
fusdc_out_0 ≠ fusdc_out_1 reverted the original ethers_suite_uniswap_orchestrated_uniswap_two test.

Tools Used

Manual, vscode

Recommended Mitigation Steps

Consider removing/relaxing the strict equality check.

Assessed type

Other

[alex-ppg]

The submission and its duplicates have correctly identified that a restriction imposed in lib::swap_2_internal will cause proper swaps to fail due to mandating strict equality for the funds consumed during a swap.

I believe a medium risk rating is appropriate given that it will cause a subset of transactions to fail with no harm beyond a Denial-of-Service.

[c4-judge]

alex-ppg marked the issue as selected for report

[c4-judge]

alex-ppg marked the issue as satisfactory