Incorrect amount taken users may lead to paying excess during erc20 swaps

[howlbot-integration]

Lines of code

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

Vulnerability details

Impact

Users are charged more than the calculated amount_in and excess tokens taken from the user is not refunded leading to loss of funds for the users.

Proof of Concept

1. Context

swap_2_internal_erc20 allows users to swap tokens between two pools. The function queries the swap_2_internal function which performs the two step swap, first pool to usdc tokens, then usdc tokens to second pool. The thing about this function is that it acts as a two way step involving the amalgamation of a pseudo "swapPoolATokensForExactUSDCTokens" and "swapExactUSDCTokensForPoolBTokens". As can be seen below, this amount specified by the user is passed in to the swap_2_internal function and eventually returned as the original_amount parameter.

    pub fn swap_2_internal_erc20(
        pools: &mut Pools,
        from: Address,
        to: Address,
        amount: U256,
        min_out: U256,
        permit2: Option<Permit2Args>,
    ) -> Result<(U256, U256), Revert> {
//...
        let (
            original_amount,
            amount_in,
            amount_out,
            _interim_usdc_out,
            _final_tick_in,
            _final_tick_out,
        ) = Self::swap_2_internal(pools, from, to, amount, min_out)?;
//...

The function first attempts to swap tokens in for usdc, passing in the various params includiding the amount passed in from swap_2_internal_erc20 which is the amount specified by the user. After this, the amount_in parameter is returned from the swap function. Further down the line, we can see that amount_in is checked to ensure its psitive before its finally returned as the result of the function.

    fn swap_2_internal(
        pools: &mut Pools,
        from: Address,
        to: Address,
        amount: U256,
        min_out: U256,
    ) -> Result<(U256, U256, U256, I256, i32, i32), Revert> {
        let original_amount = amount;
//...
        // 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(),
        )?;
//...
>>        let amount_in = amount_in.abs_pos()?;
//...
        Ok((
>>            original_amount,
>>            amount_in,
            amount_out,
            interim_usdc_out,
            final_tick_in,
            final_tick_out,
        ))
    }

Now, during the first swap, it can be observed that the original amount of tokens specified by the user is not necessarily be the amount_in that is returned during the swap. For the most part, its either the calculated amount or the difference between the amount specified by the user, and the calculated remaining amount. As such, in certain cases, due to what ever conditions, the amount_0 returned during swap (which is now returned as amount_in in swap_2_internal), may sometimes differ from the amount specified by the user which is now returned as original_amount

    pub fn swap(
        &mut self,
        zero_for_one: bool,
        amount: I256,
        mut price_limit: U256,
    ) -> Result<(I256, I256, i32), Revert> {
//...
        let token0_is_input = (zero_for_one && exact_in) || (!zero_for_one && !exact_in);
        let (amount_0, amount_1) = match token0_is_input {
            true => (amount - state.amount_remaining, state.amount_calculated),
            false => (state.amount_calculated, amount - state.amount_remaining),
        };

        Ok((amount_0, amount_1, state.tick))
    }

2. Bug Location

Establishing this, we can now go through swap_2_internal_erc20, specfically, after swap_2_internal has been queried. We can see that, as established above, the original_amount which is the amount specified by the user and amount_in which is the actual amount of tokens swapped is returned. As such, we'd expect that when token is being taken from the caller, it will be the amount swapped, which in this case is our amount_in parameter. However, this is not so (see @Audit tag), the original amount specified by the user is taken.

    pub fn swap_2_internal_erc20(
        pools: &mut Pools,
        from: Address,
        to: Address,
        amount: U256,
        min_out: U256,
        permit2: Option<Permit2Args>,
    ) -> Result<(U256, U256), Revert> {
//...
        let (
>>           original_amount,
>>            amount_in,
            amount_out,
            _interim_usdc_out,
            _final_tick_in,
            _final_tick_out,
        ) = Self::swap_2_internal(pools, from, to, amount, min_out)?;
//...
        erc20::take(from, original_amount, permit2)?; //@audit
        erc20::transfer_to_sender(to, amount_out)?;
//...
        // return amount - amount_in to the user
        // send amount_out to the user
        Ok((amount_in, amount_out))
    }
}

3. Final Effect

Due to this error, uses are sometimes made to pay way more for the swap than is actually intended as rather than take the exact amount that was used during the swap, the amount the user specified is taken instead, leading to a potential loss of funds.

For extra context, we can observe the swap_internal function in which the take_amount is set as amount_0 returned from swap, and not amount sent in by the user. This handles cases in which these amounts are not the same.

    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> {
//...
        // if zero_for_one, send them token1 and take token0
        let (take_token, take_amount, give_token, give_amount) = match zero_for_one {
            true => (pool, amount_0, FUSDC_ADDR, amount_1),
            false => (FUSDC_ADDR, amount_1, pool, amount_0),
        };

        erc20::take(take_token, take_amount.abs_pos()?, permit2)?;
        erc20::transfer_to_sender(give_token, give_amount.abs_neg()?)?;
//...
    }

Also, the comment on the Ok((amount_in, amount_out)) points out that amount - amount_in need to be returned to the user, but this is not done.

Tools Used

Manual code review

Recommended Mitigation Steps

Update the code to take amount_in instead from the user.

    pub fn swap_2_internal_erc20
//...
-        erc20::take(from, original_amount, permit2)?;
+        erc20::take(from, amount_in, permit2)?;
//...
    }
}

Assessed type

Context

[c4-judge]

alex-ppg changed the severity to 2 (Med Risk)

[c4-judge]

alex-ppg marked the issue as satisfactory