Users are incorrectly refunded when liqudity is insufficient

[howlbot-integration]

Lines of code

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

Vulnerability details

Proof of Concept

1. Context

In swap_2_internal, if the first pool doesn't have enough liquidity, amount_incould be less than original_amount, and as expected, amount_in is taken from swapper. But the function still refunds original_amount - amount_in to the user if original_amount is more than amount_in.

2. Bug Location

From the function, we can see than amount_in is taken from swapper. Then the function checks if original_amount is more than amount_in, before which the difference is transferred back to the sender.

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

3. Final Effect

An unnecessary refund is processed leading to loss of funds for the protocol. Malicious users can take advantage of this to "rob" the protocol of funds through the refunds.

Recommended Mitigation Steps

No need to process refunds since amount_in is already taken.

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

Assessed type

Context

[alex-ppg]

The submission and its duplicates have correctly identified that the refund process in the swap_2_internal_erc20 function is extraneous and thus results in excess funds being sent to the user.

I believe a high-risk severity rating is appropriate as the issue manifests itself in all cases and would result in direct fund loss for the AMM pair.

[c4-judge]

alex-ppg marked the issue as selected for report

[c4-judge]

alex-ppg marked the issue as satisfactory

[af-afk]

code-423n4/2024-08-superposition-findings#12 @alex-ppg how does this compare to your findings here?

[alex-ppg]

Hey @af-afk, thanks for your contribution! PJQA has concluded this contest, and I am unsure what comparison is to be drawn here. None of the findings are mine as I am a judge, and I do not believe that the finding referenced has any relation to this one when it comes to impact.

[af-afk]

Sorry, I should clarify, I mean your assessment that both are valid. It's not possible for both of these to be correct right? I'm of the opinion that this refund should not be implemented after consideration (and this submission) since the contract's quoting functionality should indicate that this is taking place.

[alex-ppg]

Hey @af-afk, thanks for following up! The original submission shared was submitted in a contest that relies on a different commit hash from this one. As we can observe in the highlighted code segment, the code originally transferred the original_amount from the from address.

In the remediated code that was part of this contest, the code was updated to simultaneously extract the amount_in from the user and perform a refund. The incorrect aspect is that two different solutions for the same problem were incorporated, rendering the refund to be extraneous. I hope this clears things up!

[af-afk]

Fix: fluidity-money/long.so@9c7657e