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

Protocol is wrongly refunding too many funds in a swap #43

Closed
c4-bot-10 opened this issue Nov 1, 2024 · 0 comments
Closed

Protocol is wrongly refunding too many funds in a swap #43

c4-bot-10 opened this issue Nov 1, 2024 · 0 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-5 🤖_02_group AI based duplicate group recommendation 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-10-superposition/blob/7ad51104a8514d46e5c3d756264564426f2927fe/pkg/seawater/src/lib.rs#L290-L297

Vulnerability details

Impact

AMM is providing excessive refunds for each swap, which may result in the funds being drained.

Proof of Concept

This previous issue wasn't fixed properly:
code-423n4/2024-08-superposition-findings#12

With the current new logic, users are refunded with an excessive amount of tokens:

        erc20::take(from, amount_in, permit2)?;
        erc20::transfer_to_sender(to, amount_out)?;

        if original_amount > amount_in {
            erc20::transfer_to_sender(
                to,
                original_amount
                    .checked_sub(amount_in)
                    .ok_or(Error::TransferToSenderSub)?,
            )?;
        }

https://github.com/code-423n4/2024-10-superposition/blob/7ad51104a8514d46e5c3d756264564426f2927fe/pkg/seawater/src/lib.rs#L290-L297

The issue was that an user was paying original_amount even if the actual swap was only amount_in, but now:

  1. User pays amount_in (the actual swap amount)
  2. User receives amount_out (the actual swap amount)
  3. If the original intended payment is higher than the actual swap amount, users are 'refunded' with the difference between these values

This can be abused to drain all the funds from the AMM.

Recommended mitigation steps

Consider removing the last transfer:

        erc20::take(from, amount_in, permit2)?;
        erc20::transfer_to_sender(to, amount_out)?;

-       if original_amount > amount_in {
-           erc20::transfer_to_sender(
-               to,
-               original_amount
-                   .checked_sub(amount_in)
-                   .ok_or(Error::TransferToSenderSub)?,
-           )?;
-       }
@c4-bot-10 c4-bot-10 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Nov 1, 2024
c4-bot-9 added a commit that referenced this issue Nov 1, 2024
@c4-bot-12 c4-bot-12 added the 🤖_02_group AI based duplicate group recommendation label Nov 1, 2024
howlbot-integration bot added a commit that referenced this issue Nov 4, 2024
@howlbot-integration howlbot-integration bot added sufficient quality report This report is of sufficient quality duplicate-5 labels Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-5 🤖_02_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

2 participants