Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

If liquidity is insufficient, users may need to pay more tokens in swap2 #12

Open
c4-bot-10 opened this issue Sep 8, 2024 · 4 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working M-10 primary issue Highest quality submission among a set of duplicates 🤖_07_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") sufficient quality report This report is of sufficient quality

Comments

@c4-bot-10
Copy link
Contributor

Lines of code

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

Vulnerability details

Impact

In the swap_2_internal function, if the first pool experiences a liquidity depletion, it could result in amount_in being less than original_amount. However, the contract might still require the user to transfer original_amount of tokens, causing the user to pay more tokens.

Proof of Concept

When calling swap2 to convert token1 to token2 through fusdc, if the token1-fusdc pool experiences a liquidity depletion, it will lead to the issues that the calculated amount_in of token1 will be less than the expected input original_amount.

    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;

        let amount = I256::try_from(amount).map_err(|_| Error::SwapResultTooHigh)?;

        // 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(),
        )?;
        ...
    }

After the swap_2_internal function completes, if the user is required to pay the protocol original_amount of token1, they end up paying original_amount - amount_in more tokens than they actually receive.

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

        // transfer tokens
        erc20::take(from, original_amount, permit2)?;
        erc20::transfer_to_sender(to, amount_out)?;

        Ok((amount_in, amount_out))
    }
}

Tools Used

Manual Review

Recommended Mitigation Steps

Since compatibility with Permit2 is required, it is recommended to refund the excess tokens back to the user.

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

        // transfer tokens
        erc20::take(from, original_amount, permit2)?;
        erc20::transfer_to_sender(to, amount_out)?;

+       if(original_amount > amount_in){
+           erc20::transfer_to_sender(to, original_amount - amount_in)?;
+       }

        Ok((amount_in, amount_out))
    }
}

Assessed type

Other

@c4-bot-10 c4-bot-10 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Sep 8, 2024
c4-bot-7 added a commit that referenced this issue Sep 8, 2024
@c4-bot-11 c4-bot-11 added the 🤖_07_group AI based duplicate group recommendation label Sep 13, 2024
@howlbot-integration howlbot-integration bot added primary issue Highest quality submission among a set of duplicates sufficient quality report This report is of sufficient quality labels Sep 16, 2024
@af-afk af-afk added sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") and removed sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") labels Sep 18, 2024
@af-afk af-afk added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Sep 18, 2024
@af-afk
Copy link

af-afk commented Sep 23, 2024

@alex-ppg
Copy link

The submission and its duplicates have correctly identified that a swap_2_internal_erc20 operation will result in an overpayment of funds if the input amount is not consumed in full.

I believe a medium risk severity rating to be appropriate for this submission as it can lead to a minor fund loss under certain circumstances for the user.

@c4-judge
Copy link
Contributor

alex-ppg marked the issue as selected for report

@c4-judge c4-judge added selected for report This submission will be included/highlighted in the audit report satisfactory satisfies C4 submission criteria; eligible for awards labels Sep 23, 2024
@c4-judge
Copy link
Contributor

alex-ppg marked the issue as satisfactory

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working M-10 primary issue Highest quality submission among a set of duplicates 🤖_07_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

6 participants