0xRobocop - Then getAmountsForDelta function at Underlying.sol is implemented incorrectly

[sherlock-admin]

0xRobocop

medium

Then getAmountsForDelta function at Underlying.sol is implemented incorrectly

Summary

The function getAmountsForDelta() at the Underlying.sol contract is used to compute the quantity of token0 and token1 to add to the position given a delta of liquidity. These quantities depend on the delta of liquidity, the current tick and the ticks of the range boundaries. Actually, getAmountsForDelta() uses the sqrt prices instead of the ticks, but they are equivalent since each tick represents a sqrt price.

There exists 3 cases:

• The current tick is outside the range from the left, this means only token0 should be added.
• The current tick is within the range, this means both token0 and token1 should be added.
• The current tick is outside the range from the right, this means only token1 should be added.

Vulnerability Detail

The issue on the implementation is on the first case, which is coded as follows:

if (sqrtRatioX96 <= sqrtRatioAX96) {
      amount0 = SafeCast.toUint256(
          SqrtPriceMath.getAmount0Delta(
               sqrtRatioAX96,
               sqrtRatioBX96,
               liquidity
          )
      );
} 

The implementation says that if the current price is equal to the price of the lower tick, it means that it is outside of the range and hence only token0 should be added to the position.

But for the UniswapV3 implementation, the current price must be lower in order to consider it outside:

if (_slot0.tick < params.tickLower) {
   // current tick is below the passed range; liquidity can only become in range by crossing from left to
   // right, when we'll need _more_ token0 (it's becoming more valuable) so user must provide it
   amount0 = SqrtPriceMath.getAmount0Delta(
          TickMath.getSqrtRatioAtTick(params.tickLower),
          TickMath.getSqrtRatioAtTick(params.tickUpper),
          params.liquidityDelta
    );
}

Reference

Impact

When the current price is equal to the left boundary of the range, the uniswap pool will request both token0 and token1, but arrakis will only request from the user token0 so the pool will lose some token1 if it has enough to cover it.

Code Snippet

https://github.com/sherlock-audit/2023-06-arrakis/blob/main/v2-core/contracts/libraries/Underlying.sol#LL311-L318

Tool used

Manual Review

Recommendation

Change from:

// @audit-issue Change <= to <.
if (sqrtRatioX96 <= sqrtRatioAX96) {
     amount0 = SafeCast.toUint256(
        SqrtPriceMath.getAmount0Delta(
           sqrtRatioAX96,
           sqrtRatioBX96,
           liquidity
         )
     );
}

to:

if (sqrtRatioX96 < sqrtRatioAX96) {
     amount0 = SafeCast.toUint256(
        SqrtPriceMath.getAmount0Delta(
           sqrtRatioAX96,
           sqrtRatioBX96,
           liquidity
         )
     );
}

[Gevarist]

We consider this issue as a medium severity issue, because the cost of an attacker to benefit from this vulnerability and steal some token1 as expensive. The attacker needs to provide the equivalent amount of token0.

[ctf-sec]

The calculated amount to supply the token mismatched the actually supplied amount depends on the ticker range and the over-charged part from user fund is lost, recommend maintaining the high severity.

[0xRobocop]

Escalate

Apart from #142

The issues #65 #118 #149 and #269 are not duplicates of this issue.

Regardless on their own validity, the mitigation is different, pointing to different root causes.

[ctf-sec]

Emm You may need to edit from "Escalate for 10 USDC" to "Escalate" in the comments.

[sherlock-admin]

Escalate

Apart from #142

The issues #65 #118 #149 and #269 are not duplicates of this issue.

Regardless on their own validity, the mitigation is different, pointing to different root causes.

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.

[ghost]

The calculated amount to supply the token mismatched the actually supplied amount depends on the ticker range and the over-charged part from user fund is lost, recommend maintaining the high severity.

Should be medium considering high is defined as "This vulnerability would result in a material loss of funds, and the cost of the attack is low (relative to the amount of funds lost)."

[Jeiwan]

Escalate

This is a medium severity issue. The miscalculation won't cause a loss of funds since the wrong amount will be rejected by the underlying Uniswap pool. Also, the protocol has robust slippage protections, which won't allow users to deposit more tokens than required.

All in all, the issue doesn't demonstrate an attack scenario, or a scenario when users lose significant funds, thus it cannot have a high severity.

[sherlock-admin]

Escalate

This is a medium severity issue. The miscalculation won't cause a loss of funds since the wrong amount will be rejected by the underlying Uniswap pool. Also, the protocol has robust slippage protections, which won't allow users to deposit more tokens than required.

All in all, the issue doesn't demonstrate an attack scenario, or a scenario when users lose significant funds, thus it cannot have a high severity.

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.

[JeffCX]

After reading the escalation, seems like the severity is medium

When the current price is equal to the left boundary of the range, the uniswap pool will request both token0 and token1, but arrakis will only request from the user token0 so the pool will lose some token1 if it has enough to cover it.

this does cause protocol to lose fund because the protocol's fund instead of user's fund is used to add liquidity.

but mainly because the attacker not able to leverage this bug to steal fund and the pre-condition:

current price is equal to the left boundary of the range

Recommend changing severity from high to medium

I will adress 0xRobocoop's escalation seperately

[Gevarist]

we agree that this issue is medium.

[hrishibhat]

Result:
Medium
Has duplicates
Accepting both escalations. This is a valid medium issue. and only #142 is the duplicate

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• 0xRobocop: accepted
• Jeiwan: accepted

[Gevarist]

We replace it with strict inequality here.
PR: ArrakisFinance/v2-core#159

[IAm0x52]

Fix looks good. Now uses < instead of <=