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 #13

Closed
howlbot-integration bot opened this issue Nov 4, 2024 · 1 comment
Closed

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

howlbot-integration bot opened this issue Nov 4, 2024 · 1 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 satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality

Comments

@howlbot-integration
Copy link

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)?,
-           )?;
-       }
@howlbot-integration howlbot-integration bot added 3 (High Risk) Assets can be stolen/lost/compromised directly 🤖_02_group AI based duplicate group recommendation bug Something isn't working duplicate-5 sufficient quality report This report is of sufficient quality labels Nov 4, 2024
howlbot-integration bot added a commit that referenced this issue Nov 4, 2024
@c4-judge
Copy link

alex-ppg marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Nov 13, 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 satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

1 participant