radevauditor - Swapping can be sandwiched

[sherlock-admin]

radevauditor

high

Swapping can be sandwiched

Summary

The swap() is a function designed for swapping between two tokens (token0 -> token1, or vice versa).

Vulnerability Detail

The swap() is a function designed for swapping between two tokens (token0 -> token1, or vice versa). While the function sets something like slippage protection via limitSqrtP parameter where, the user can specify a price limit that the swap can reach. The minimum and maximum price limits a user can specify is MIN_SQRT_RATIO + 1 and MAX_SQRT_RATIO - 1.

    if (willUpTick) {
      require(
        limitSqrtP > swapData.sqrtP && limitSqrtP < TickMath.MAX_SQRT_RATIO,
        'bad limitSqrtP'
      );
    } else {
      require(
        limitSqrtP < swapData.sqrtP && limitSqrtP > TickMath.MIN_SQRT_RATIO,
        'bad limitSqrtP'
      );
    }

However the price of sqrtPrice is only determined during the transaction. opening sandwich opportunity for MEV researchers as long as the slippage stays below limitSqrtP.

Let's consider the following attack scenario:

Note: for the calculations I will use the amm-calculator.vercel.app

Initial Pool State:

1. Pool: We'll start with a pool that contains 10,000 stETH and 10,695 WETH.
2. User's Intention: A user intends to stake 100 stETH.
3. Attacker's Tools: The attacker has ample liquidity (can get and flash loan) and monitors pending transactions, aiming to exploit large trades.

Attack Sequence:

Transaction 1 (TX1): Attacker's Initial Move

• Before the user's swap transaction goes through, the attacker sees it pending in the transaction pool.
• The attacker swaps 100 WETH for WETH. Given our initial pool balance, they would receive roughly 92.63 stETH.
• After this trade, the pool balance is: 9907.37 stETH and 10,795 WETH.

Transaction 2 (TX2): User's Swap

• The user goes ahead with their swap. Given the manipulated pool balance, the exchange rate for WETH to stETH has shifted unfavorably for the user.
• Using the contract's logic, let's say that the minimum WETH the user expects for their 33.3333 WETH is 30.284 stETH.
• The user's swap transaction goes through, and the user ends up receiving slightly more than the minimum due to the pool's mechanics, say 30.498 stETH (still much less than they would have gotten without the attacker's interference).
• After this trade, the pool balance is: 9876.86 stETH and 10,828.33 WETH.

Transaction 3 (TX3): Attacker's Reversal

• The attacker now sells back their 92.63 stETH for WETH.
• Given the new pool balance, the attacker would receive roughly 100.61 WETH for their 92.63 stETH.
• After this trade, the pool has around 9969.49 stETH and 10,727.72 WETH.

Outcome:

1. Attacker's Profit: The attacker started with 100 WETH and now has 100.61 WETH, netting a profit of 0.61 WETH.
2. User's Loss: The user swapped 10 WETH but got a worse rate (7.9 stETH instead of a potentially better rate) because of the attacker's manipulation.

So, the user's action in the "sandwiched" transaction (TX2) is about the attacker manipulating the price in an AMM pool right before a large user trade, causing the user to get a less favorable rate, then the attacker trades back after the user's transaction, reverting the price and profiting from the artificially created price difference.

Impact

A user can gain significant profit multiple times by executing a sandwiching attack.

Code Snippet

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/main/ks-elastic-sc/contracts/Pool.sol#L327-L521

Tool used

Manual Review

Recommendation

Consider to determine sqrtPrice using the TWAP price.

[Trumpero]

invalid, pool is not intended to have slippage protection. Router has slippage protections.

[radevauditor]

Escalate

Judge: "invalid, pool is not intended to have slippage protection. Router has slippage protections."

---

I think this is wrongly classified as invalid issue.

The fact that "pool is not intended to have slippage protection. Router has slippage protections" is irrelevant to this vulnerability and the issue is still valid.

I believe the Swapping can be sandwiched issue in the Pool contract is a valid high severity issue.

---

@Trumpero @sherlock-admin

[sherlock-admin2]

Escalate

Judge: "invalid, pool is not intended to have slippage protection. Router has slippage protections."

---

I think this is wrongly classified as invalid issue.

The fact that "pool is not intended to have slippage protection. Router has slippage protections" is irrelevant to this vulnerability and the issue is still valid.

I believe the Swapping can be sandwiched issue in the Pool contract is a valid high severity issue.

---

@Trumpero @sherlock-admin

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.

[0xdeth]

Escalate

[sherlock-admin2]

Escalate

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[radevauditor]

Also, this issue should be discussed with the contest sponsors.

[Trumpero]

1. Users should only use the Router contract to swap because the Pool.swap() function requires a swapCallBack function from the sender so they can't call swap directly on the Pool contract.
2. limitSqrtP is not the slippage check of a swap. It's a config used to restrict the price movement in the pool during swap. The slippage of the swap is primarily controlled by the minAmountOut config in the Router contract (regarding this comment).
3. Swapping cannot be sandwiched if users set the slippage correctly in the Router contract. For example, when users want to limit slippage to 1%, they should set minReturnAmount to 1% of their expected amount from the input. Then the swap transaction will revert if the returned amount is insufficient due to any reasons, such as a sandwich attack. Therefore, I believe the above escalation and issue are invalid.

[hrishibhat]

@radevauditor Agree with the Lead Judge's points raised above. Do you have any additional response for the above?

[radevauditor]

@radevauditor Agree with the Lead Judge's points raised above. Do you have any additional response for the above?

• Yes.

---

Judge Comments:

• "Invalid, the pool is not intended to have slippage protection. The router has slippage protections."
• "Users should only use the Router contract to swap because the Pool.swap() function requires a swapCallBack function from the sender so they can't call swap directly on the Pool contract."
• "Swapping cannot be sandwiched if users set the slippage correctly in the Router contract..."

My Comment: The root cause of my issue does not stem from the lack of slippage protection in the Pool.

The crux of my concern revolves around price manipulation within the pool, enabling attackers to reap significant profits multiple times by executing a sandwiching attack.

Moreover, an attacker can easily call swap directly on the Pool contract by merely implementing a contract with the swapCallBack() function. Thus, attackers or malicious users can interact directly with the Pool contract without any hindrances, accruing significant profits multiple times by executing a sandwiching attack.

---

Now, let's consider the following attack scenario again:

Initial Pool State:

• Pool: We'll start with a pool that contains 10,000 stETH and 10,695 WETH.
• User's Intention: A user intends to stake 100 stETH.
• Attacker's Tools: The attacker has ample liquidity (can get and flash loan) and monitors pending transactions, aiming to exploit large trades.

Attack Sequence:

Transaction 1 (TX1): Attacker's Initial Move

• Before the user's swap transaction goes through, the attacker sees it pending in the transaction pool.
• The attacker swaps 100 WETH for WETH. Given our initial pool balance, they would receive roughly 92.63 stETH.
• After this trade, the pool balance is: 9907.37 stETH and 10,795 WETH.

Transaction 2 (TX2): User's Swap

• The user goes ahead with their swap. Given the manipulated pool balance, the exchange rate for WETH to stETH has shifted unfavorably for the user.
• Using the contract's logic, let's say that the minimum WETH the user expects for their 33.3333 WETH is 30.284 stETH.
• The user's swap transaction goes through, and the user ends up receiving slightly more than the minimum due to the pool's mechanics, say 30.498 stETH (still much less than they would have gotten without the attacker's interference).
• After this trade, the pool balance is: 9876.86 stETH and 10,828.33 WETH.

Transaction 3 (TX3): Attacker's Reversal

• The attacker now sells back their 92.63 stETH for WETH.
• Given the new pool balance, the attacker would receive roughly 100.61 WETH for their 92.63 stETH.
• After this trade, the pool has around 9969.49 stETH and 10,727.72 WETH.

Outcome:

1. Attacker's Profit: The attacker started with 100 WETH and now has 100.61 WETH, netting a profit of 0.61 WETH.
2. User's Loss: The user swapped 10 WETH but got a worse rate (7.9 stETH instead of a potentially better rate) because of the attacker's manipulation.

So, the user's action in the "sandwiched" transaction (TX2) is about the attacker manipulating the price in an AMM pool right before a large user trade, causing the user to get a less favorable rate, then the attacker trades back after the user's transaction, reverting the price and profiting from the artificially created price difference.

So, a user can gain significant profit multiple times by executing a sandwiching attack.

---

Therefore, I believe that the attack scenario outlined above is highly likely to occur when the attacker interacts directly with the Pool contract.

My entire concern revolves around price manipulation within the pool, not the lack of slippage protection.

[Trumpero]

@radevauditor I meant that limitSqrtP is not the slippage protection of swap, as you mentioned in the report. Slippage protection is controlled by the minAmountOut config in the Router contract. This protection is used to prevent attacks that can affect the results of swaps, such as price manipulation and sandwiched attacks. It ensures that the transaction will revert if a user incurs any loss. Therefore, when users correctly set up the slippage protection using the minAmountOut config in the Router contract, they can prevent these attacks and avoid incurring any losses.

[radeveth]

Yes, the normal users will use the Router contract for swapping.

But my point is that the attacker can bypass the slippage protection in the Router by interacting directly with the Pool and cause price manipulation, after that and sandwich the swaps. (to do the attack scenario described above by interacting directly with the Pool)

[Trumpero]

@radevauditor I mentioned that:

Users should only use the Router contract to swap because the Pool.swap() function requires a swapCallBack function from the sender so they can't call swap directly on the Pool contract.

Using a router is a popular design in many Dex protocols. If users directly interact with the pool, they can easily become targets for sandwich attacks or price manipulation attacks, even in pools of Uniswap/UniswapV3, or other dexes. So, I believe it's a non-issue because this case should be considered as user's mistake.

[radeveth]

I say that the normal users will use the Router contract for swapping.

Only attacker will use the Pool contract for direct swapping, to cause price manipulation.

My issue has nothing to do with user mistake, because only the attacker will swap directly from the Pool contract, to cause price manipulation, after that and sandwich the normal user swaps (which swaps will be done by the Router contract).

[radeveth]

@radevauditor I mentioned that:

Users should only use the Router contract to swap because the Pool.swap() function requires a swapCallBack function from the sender so they can't call swap directly on the Pool contract.

Using a router is a popular design in many Dex protocols. If users directly interact with the pool, they can easily become targets for sandwich attacks or price manipulation attacks, even in pools of Uniswap/UniswapV3, or other dexes. So, I believe it's a non-issue because this case should be considered as user's mistake.

—-

Judge Comment: “Users should only use the Router contract to swap because the Pool.swap() function requires a swapCallBack function from the sender so they can't call swap directly on the Pool contract.”

My comment: An attacker can easily call swap directly on the Pool contract by merely implementing a contract with the swapCallBack() function. Thus, attackers can interact directly with the Pool contract.

—-

I described all of this above.

@Trumpero @hrishibhat

[Trumpero]

@radevauditor Please check my mentions carefully:

Slippage protection is controlled by the minAmountOut config in the Router contract. This protection is used to prevent attacks that can affect the results of swaps, such as price manipulation and sandwiched attacks. It ensures that the transaction will revert if a user incurs any loss. Therefore, when users correctly set up the slippage protection using the minAmountOut config in the Router contract, they can prevent these attacks and avoid incurring any losses.

The slippage protection in Router (controlled by minAmountOut config) will protect users from incurring any losses. It prevent the attacks for user's swap, such as price manipulation and sandwiched attacks. So when users use the swap function from Router with the correct protection, they can't incur any loss from any attacks. For example, Bob attemps to swap 10 WETH to 9.9 wstETH, he set minAmountOut to be 9.9e18, so his swap will revert if he receives insufficient this amount. Therefore, attacker can't do any attack to make him receive less.

[radeveth]

But that doesn't change the fact that an attacker/malicious user can manipulate the price and make the protocol unsatisfactory for users at some point in time.

So, I believe that this should classified as valid medium severity issue.

[Trumpero]

@radevauditor I don't agree that attackers can make the protocol unsatisfactory for users at some point in time. There are already protections and safety checks in the Router contract that protect users from on-chain attacks (specifically, the minAmountOut configurations). This design is similar to UniswapV3 and is very popular in the router contracts of other DEX protocols. No attack can result in losses for users due to these validations.

References for UniswapV3: https://github.com/Uniswap/v3-periphery/blob/main/contracts/SwapRouter.sol#L115-L129 https://github.com/Uniswap/v3-periphery/blob/main/contracts/SwapRouter.sol#L87-L112

[hrishibhat]

Result: Low Unique Agree with the Lead judge that There are necessary slippage protections in the router. There user will get the minimum amount mentioned.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• 0xdeth: rejected