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

Incorrect Decoding of Swap Results Leads to Ineffective Slippage Protection #53

Closed
howlbot-integration bot opened this issue Sep 16, 2024 · 5 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-35 🤖_primary AI based primary recommendation 🤖_14_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards 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 upgraded by judge Original issue severity upgraded from QA/Gas by judge

Comments

@howlbot-integration
Copy link

Lines of code

https://github.com/code-423n4/2024-08-superposition/blob/4528c9d2dbe1550d2660dac903a8246076044905/pkg/sol/SeawaterAMM.sol#L316
https://github.com/code-423n4/2024-08-superposition/blob/4528c9d2dbe1550d2660dac903a8246076044905/pkg/sol/SeawaterAMM.sol#L316
https://github.com/code-423n4/2024-08-superposition/blob/4528c9d2dbe1550d2660dac903a8246076044905/pkg/seawater/src/lib.rs#L194

Vulnerability details

The swapOut5E08A399() and swapOutPermit23273373B() functions in the SeawaterAMM contract incorrectly decode the results of a swap operation, leading to ineffective slippage protection. This issue arises from a mismatch between the swap direction and the order of decoded values.

In both functions, a swap is executed with zeroForOne set to false, indicating a swap from token1 to token0. However, when decoding the swap results, the order of swapAmountIn and swapAmountOut is reversed, causing the slippage check to be performed on the wrong value. This is because the function swap904369BE returns amount0, amount1 so is the caller which should interpret which corresponds to token in and token out. This incorrect decoding leads to a situation where the slippage protection is not applied as intended, potentially exposing users to unexpected losses.

Impact

Users may receive fewer tokens than expected without the transaction reverting, leading to financial losses. This issue could be exploited by malicious actors to manipulate swap outcomes to their advantage.

Proof of Concept

  1. A user calls swapOut5E08A399() or swapOutPermit23273373B() to swap token1 for token0.
  2. The function executes the swap with zeroForOne set to false.
  3. The swap is successful, returning the amounts swapped.
  4. The slippage check is performed on swapAmountOut, which actually contains the input amount.
  5. The user receives fewer tokens than expected without the transaction reverting.

Tools Used

Manual review

Recommended Mitigation Steps

Correct the decoding of swap results in both swapOut5E08A399() and swapOutPermit23273373B() functions.

Assessed type

Other

@howlbot-integration howlbot-integration bot added 3 (High Risk) Assets can be stolen/lost/compromised directly 🤖_14_group AI based duplicate group recommendation 🤖_primary AI based primary recommendation bug Something isn't working primary issue Highest quality submission among a set of duplicates sufficient quality report This report is of sufficient quality labels Sep 16, 2024
howlbot-integration bot added a commit that referenced this issue Sep 16, 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
@c4-judge c4-judge added duplicate-35 and removed primary issue Highest quality submission among a set of duplicates labels Sep 23, 2024
@c4-judge
Copy link
Contributor

alex-ppg marked issue #35 as primary and marked this issue as a duplicate of 35

@c4-judge
Copy link
Contributor

alex-ppg changed the severity to 2 (Med Risk)

@c4-judge c4-judge added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value downgraded by judge Judge downgraded the risk level of this issue and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Sep 23, 2024
@c4-judge
Copy link
Contributor

alex-ppg marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Sep 23, 2024
@0xTester
Copy link

I think this issue should be classified as high severity due to its direct impact on user funds. The vulnerability exposes users to significant financial losses, particularly through sandwich attacks, which have been estimated to cause approximately $190M in losses as of 2021 (see reference). It's important to emphasize that this report is not highlighting the absence of such protection (normal situation for core functions, use a periphery function that adds protection), but rather its incorrect implementation. Therefore, the functions in question are specifically designed to be entry points for executing secure swaps.

Moreover, the nature of this issue aligns with a high-severity classification as it directly exposes users to financial losses without requiring external conditions for the attacker to carry on the attack nor the need for users to make mistakes. It presents an immediate and direct risk to any user interacting with these functions.

Reference: https://pub.tik.ee.ethz.ch/students/2021-FS/BA-2021-07.pdf

@c4-judge c4-judge added 3 (High Risk) Assets can be stolen/lost/compromised directly upgraded by judge Original issue severity upgraded from QA/Gas by judge and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value downgraded by judge Judge downgraded the risk level of this issue labels Oct 7, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Oct 7, 2024

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

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-35 🤖_primary AI based primary recommendation 🤖_14_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards 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 upgraded by judge Original issue severity upgraded from QA/Gas by judge
Projects
None yet
Development

No branches or pull requests

3 participants