Incorrect token refunded in swap_2_internal_erc20()

[howlbot-integration]

Lines of code

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

Vulnerability details

Proof of Concept

From the previous audit, it was determined that the protocol may excessively pull tokens (original_amount) from the user during swap_2 than actually is required (amount_in) and that in such a case, a refund should be made to the user.
This is then enforced in the code as follows:

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

The swap_2_internal_erc20() function performs a token swap from from (input token) to to (output token). If the contract pulls more from tokens than required for the swap (original_amount > amount_in), it incorrectly refunds the excess in the form of to tokens, instead of returning the excess from tokens.

Impact

The current refund logic is flawed because it assumes that the excess amountX of from tokens can simply be returned as amountX of to tokens. This assumption is incorrect because the exchange rate between from and to tokens is not guaranteed to be 1:1.

---> This creates a situation where the user is refunded a different token, which may not match the original amount's value, leading to financial discrepancies for both the user and protocol.

Recommended Mitigation Steps

The refund logic should return the excess from tokens back to the user, ensuring consistency with the swap flow.

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

Assessed type

Error

[alex-ppg]

The submission has identified a flaw in the newly introduced refund logic that will cause a refund to be processed in the output token instead of the input token erroneously.

This leads to direct fund loss of the AMM pair and could theoretically be repeated multiple times to extract a non-negligible value from the protocol rendering the exhibit to be of high severity.

[c4-judge]

alex-ppg changed the severity to 3 (High Risk)

[c4-judge]

alex-ppg marked the issue as selected for report

[c4-judge]

alex-ppg marked the issue as satisfactory

[DadeKuma]

Dear Judge,

There is the following logic:

286:    // transfer tokens
287:    erc20::take(from, amount_in, permit2)?;
288:    erc20::transfer_to_sender(to, amount_out)?;
289:    
290:    if original_amount > amount_in {
291:         erc20::transfer_to_sender(
292:            to,
293:            original_amount
294:               .checked_sub(amount_in)
295:               .ok_or(Error::TransferToSenderSub)?,
296:        )?;
297:    }

• 287: we take the amount of funds that were effectively swapped by the user
• 288: we transfer the amount of funds that were effectively swapped to the user
• 290-297: we refund the user, but this logic should never happen as the swap is already complete with the previous logic (this is explained in Users are incorrectly refunded when liqudity is insufficient #5)

The Warden has submitted two issues: the current one and #14. The mitigation described in #14 does not fix #5, even though it was considered a duplicate.

However, by combining #14 with this issue, we can effectively fix #5 by performing an additional transfer (refund), although this may not be the most efficient solution. There are countless ways to address #5, and this approach is one of them; ultimately, they all address the same root cause.

I believe this issue and its duplicates should be grouped with #5, as they simply provide an alternative way to fix the same problem.

[alex-ppg]

Hey @DadeKuma, thanks for your PJQA contribution! One submission argues that the refund mechanism is incorrect whereas the other argues that the refund mechanism should not exist. Those items are effectively different vulnerabilities, however, I am inclined to invalidate this submission and its duplicate as the code itself needs to be removed for the code to behave properly.

The fix proposed will still result in the extraneous refund and thus does not really address the main concern of the vulnerability described in #5. As such, I will proceed with invalidating both this submission and its duplicate.

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid