radevauditor - `Pool` contract applies slippage to sqrtPrice which is wrong and leads to unpredictable slippage

[sherlock-admin2]

radevauditor

high

Pool contract applies slippage to sqrtPrice which is wrong and leads to unpredictable slippage

Summary

Swapping in Pool.sol contract leverages the square root of the price (sqrtPrice) instead of the actual price. In swap() function slippage is directly applied to this sqrtPrice, resulting in unpredictable and often greater-than-intended slippage rates.

Vulnerability Detail

The code in swap()#Pool.sol applies the limitSqrtP directly to the sqrtPrice. As a consequence, the resultant slippage limit is inadvertently magnified.

    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'
      );
    }

Assuming a user set slippage to 5%, and the sqrtPrice is 10 (resulting in an actual price of 100). Applying a 5% slippage directly to the sqrtPrice brings it to 4. However, the squared value of this gives 81, representing a real slippage of 19%, not the intended 5% from the user.

Impact

The unintentional increase in slippage limits.

Code Snippet

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

Tool used

Manual Review

Recommendation

Rather than applying slippage directly to the sqrtPrice, it should be applied to the actual price to maintain the desired limit.

[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" doesn't matter.

I'd like to provide further insights to explain my stance:

1. The Mathematics Behind Slippage on sqrtPrice: Using the sqrtPrice to determine slippage can introduce unintended mathematical inaccuracies. As demonstrated, a 5% slippage on a sqrtPrice of 10, gives a value of 4. Squaring this results in 81, implying a 19% actual slippage instead of the user-intended 5%.

2. User Expectations & Real-world Implications: Regardless of the protocol's intentions, users typically anticipate a consistent experience. If they set a 5% slippage, they would not expect the outcome to be as high as 19%. Such unpredictable behavior can be misleading and may result in unintended financial losses.

3. Redundancy in DeFi Systems: While the Router have slippage protections, the essence of robust DeFi systems often lies in redundancy. Ensuring that all critical components of a protocol have inherent security and functionality checks ensures that users are protected from all angles. Depending solely on the Router for slippage protection could be risky.

---

Given these considerations, I think that this is a valid medium severity issue.

@Trumpero @sherlock-admin2

[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" doesn't matter.

I'd like to provide further insights to explain my stance:

1. The Mathematics Behind Slippage on sqrtPrice: Using the sqrtPrice to determine slippage can introduce unintended mathematical inaccuracies. As demonstrated, a 5% slippage on a sqrtPrice of 10, gives a value of 4. Squaring this results in 81, implying a 19% actual slippage instead of the user-intended 5%.

2. User Expectations & Real-world Implications: Regardless of the protocol's intentions, users typically anticipate a consistent experience. If they set a 5% slippage, they would not expect the outcome to be as high as 19%. Such unpredictable behavior can be misleading and may result in unintended financial losses.

3. Redundancy in DeFi Systems: While the Router have slippage protections, the essence of robust DeFi systems often lies in redundancy. Ensuring that all critical components of a protocol have inherent security and functionality checks ensures that users are protected from all angles. Depending solely on the Router for slippage protection could be risky.

---

Given these considerations, I think that this is a valid medium severity issue.

@Trumpero @sherlock-admin2

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]

Disagree with the escalation. The slippage of the swap is primarily controlled by the minAmountOut config in the Router contract. For example, if a user's expected amount is 100 with a slippage of 5%, the minAmountOut param needs to be set to 95 (95% of the expected amount) from the input. Users should 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 to the Pool contract.

The statement "5% slippage on a sqrtPrice of 10 gives a value of 4" found in both the report and the comment is fabricated and incorrect. No line of code uses the sqrtPrice as a slippage check.

The scenario where "a user sets a 5% slippage but receives 19% in the results" is also incorrect. In this case, the minAmountOut in the Router contract is set to 95% of the expected amount from the input, and the transaction will revert if the returned amount is insufficient.

The limitSqrtP is another configuration used to restrict price movement in the pool during a swap, and the redundancy of this configuration does not cause any issues. Therefore, this issue should be invalid.

[hrishibhat]

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

[hrishibhat]

Result: Low Unique I agree with Lead Judge's comments above as the router contract has the necessary slippage protections. Considering this issue a low

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• 0xdeth: rejected