returnExpectedBalanceWithoutFees can process using 0 calculated prices for certain uniswap v3 pair

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/TokenisableRange.sol#L333-L339

Vulnerability details

Impact

returnExpectedBalanceWithoutFees is a crucial function that will return the amount of token0 and token1 from the given price, ticks price and liquidity. However, the calculation of sqrt price using oracle price has very minimal underflow protection, could go underflow for certain pairs.

Proof of Concept

This is the calculation inside returnExpectedBalanceWithoutFees :

https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/TokenisableRange.sol#L338

  function returnExpectedBalanceWithoutFees(uint TOKEN0_PRICE, uint TOKEN1_PRICE) internal view returns (uint256 amt0, uint256 amt1) {
    // if 0 get price from oracle
    if (TOKEN0_PRICE == 0) TOKEN0_PRICE = ORACLE.getAssetPrice(address(TOKEN0.token));
    if (TOKEN1_PRICE == 0) TOKEN1_PRICE = ORACLE.getAssetPrice(address(TOKEN1.token));

    (amt0, amt1) = LiquidityAmounts.getAmountsForLiquidity( uint160( sqrt( (2 ** 192 * ((TOKEN0_PRICE * 10 ** TOKEN1.decimals) / TOKEN1_PRICE)) / ( 10 ** TOKEN0.decimals ) ) ), TickMath.getSqrtRatioAtTick(lowerTick), TickMath.getSqrtRatioAtTick(upperTick),  liquidity);
  }

It can be observed that it has inner calculation :

(TOKEN0_PRICE * 10 ** TOKEN1.decimals) / TOKEN1_PRICE)

This calculation have very minimal underflow protection ((TOKEN0_PRICE * 10 ** TOKEN1.decimals) must be greater than TOKEN1_PRICE), especially when TOKEN1.decimals is low and TOKEN0_PRICE value is very low compared to TOKEN1_PRICE.

When this happened (price underflow to 0) and passed to getAmountsForLiquidity, the function will always process and return only amt0. This will result wrong amount is calculated and used when deciding amount to process inside claimFees.

Tools Used

Manual review

Recommended Mitigation Steps

Change the operation when calculating price using this calculation to improve overflow/underflow protection :

sqrt((TOKEN0_PRICE * 10 ** TOKEN1.decimals * 2 ** 96) / (TOKEN1_PRICE * 10 ** TOKEN0.decimals)) * 2 **48

reference : https://github.com/makerdao/univ3-lp-oracle

Assessed type

Math

[141345]

low decimal, cheap token cause rounding to 0

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-pre-sort]

141345 marked the issue as duplicate of #316

[c4-pre-sort]

141345 marked the issue as not a duplicate

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-sponsor]

Keref marked the issue as sponsor confirmed

[c4-sponsor]

Keref marked the issue as disagree with severity

[Keref]

((TOKEN0_PRICE * 10 ** TOKEN1.decimals) must be greater than TOKEN1_PRICE)

Both prices are given with 8 decimals as per Chainlink. Considering lowest possible case of USDC with only 6 decimals, that means the price of the token must be lower than 1e-6 USDC. Low impact in practice as there is no such token listed on Chainlink.

It makes sense anyway to update the code according to the recommendation

[Keref]

See PR#5

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[c4-judge]

gzeon-c4 changed the severity to QA (Quality Assurance)

[c4-judge]

gzeon-c4 marked the issue as grade-b