0xadrii - Performing a direct multiplication in _getPriceFromSqrtX96 will overflow for some uniswap pools

[sherlock-admin3]

0xadrii

medium

Performing a direct multiplication in _getPriceFromSqrtX96 will overflow for some uniswap pools

Summary

The _getPriceFromSqrtX96 will revert for pools that return a _sqrtPriceX96 bigger than type(uint128).max.

Vulnerability Detail

The LenderCommitmentGroup_Smart uses the price obtained from the uniswap v3 pool in order to compute the amount of collateral that must be deposited in order to perform a borrow. Currently, the prices are fetched via the _getUniswapV3TokenPairPrice function:

// LenderCommitmentGroup_Smart.sol
function _getUniswapV3TokenPairPrice(uint32 _twapInterval)
        internal
        view
        returns (uint256)
    {
        // represents the square root of the price of token1 in terms of token0

        uint160 sqrtPriceX96 = getSqrtTwapX96(_twapInterval);

        //this output is the price ratio expanded by 1e18
        return _getPriceFromSqrtX96(sqrtPriceX96);
    }

This function will perform two actions:

1. Get the sqrtPriceX96 from the uniswap pool by querying the pool’s TWAP or the pool’s slot0 function. It is important to note that the returned sqrtPriceX96 is a uint160 value that can store numbers bigger than 128 bits:

   // LenderCommitmentGroup_Smart.sol
   
   function getSqrtTwapX96(uint32 twapInterval)
           public
           view
           returns (uint160 sqrtPriceX96)
       {
           if (twapInterval == 0) {
               // return the current price if twapInterval == 0
               (sqrtPriceX96, , , , , , ) = IUniswapV3Pool(UNISWAP_V3_POOL)
                   .slot0();
           } else {
               uint32[] memory secondsAgos = new uint32[](2);
               secondsAgos[0] = twapInterval; // from (before)
               secondsAgos[1] = 0; // to (now)
   
               (int56[] memory tickCumulatives, ) = IUniswapV3Pool(UNISWAP_V3_POOL)
                   .observe(secondsAgos);
   
               // tick(imprecise as it's an integer) to price
               sqrtPriceX96 = TickMath.getSqrtRatioAtTick(
                   int24(
                       (tickCumulatives[1] - tickCumulatives[0]) /
                           int32(twapInterval)
                   )
               );
           }
       }

2. After obtaining the sqrtPriceX96 , the priceX96 will be obtained by multiplying sqrtPriceX96 by itself and dividing it by (2**96) :

   // LenderCommitmentGroup_Smart.sol
   function _getPriceFromSqrtX96(uint160 _sqrtPriceX96)
           internal
           pure
           returns (uint256 price_)
       {
          
           uint256 priceX96 = (uint256(_sqrtPriceX96) * uint256(_sqrtPriceX96)) /
               (2**96);
   
           // sqrtPrice is in X96 format so we scale it down to get the price
           // Also note that this price is a relative price between the two tokens in the pool
           // It's not a USD price
           price_ = priceX96;
       }

The problem is that the _getPriceFromSqrtX96 performs a direct multiplication between _sqrtPriceX96 by itself. As mentioned in step 1, this multiplication can lead to overflow because the _sqrtPriceX96 value returned by the Uniswap pool can be a numer larger than 128 bits.

As an example, take Uniswap’s WBTC/SHIBA pool and query slot0. At timestamp 1714392894, the slot0 value returned is  380146371870332863053439965317548561928, which is a 129 bit value. When the _getPriceFromSqrtX96 gets executed for the WBTC/SHIBA pool, an overflow will always occur because a multiplication of two 129-bit integers surpasses 256 bits.

Note how this is also handled in Uniswap’s official Oracle Library] contract, where a check is performed to ensure that no overflows can occur.

Impact

Medium. Some uniswap pool’s will be unusable and will DoS in LenderCommitmentGroup_Smart because the uniswap price computations will always overflow.

Code Snippet

https://github.com/sherlock-audit/2024-04-teller-finance/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L559

Tool used

Manual Review

Recommendation

Use Uniswap’s Fullmath library to perform the multiplication in _getPriceFromSqrtX96, which already handles this situation:

// LenderCommitmentGroup_Smart.sol
function _getPriceFromSqrtX96(uint160 _sqrtPriceX96)
        internal
        pure
        returns (uint256 price_)
    {
       
-        uint256 priceX96 = (uint256(_sqrtPriceX96) * uint256(_sqrtPriceX96)) /
-            (2**96);
+        uint256 priceX96 = FullMath.mulDiv(uint256(_sqrtPriceX96), uint256(_sqrtPriceX96), (2**96);

        // sqrtPrice is in X96 format so we scale it down to get the price
        // Also note that this price is a relative price between the two tokens in the pool
        // It's not a USD price
        price_ = priceX96;
    }

[spacegliderrrr]

Escalate

In order to overflow we need sqrtPriceX96 * sqrtPriceX96 to be larger than uint256.max. This means that tokenPrice * 2^192 must exceed 2^256, or we need tokenPrice >= 2^64 or tokenPrice >= ~1e18. This requires 1 wei of token0 to be worth >1e18 wei of token1, which is absolutely unrealistic edge case scenario. Issue should be low.

[sherlock-admin3]

Escalate

In order to overflow we need sqrtPriceX96 * sqrtPriceX96 to be larger than uint256.max. This means that tokenPrice * 2^192 must exceed 2^256, or we need tokenPrice >= 2^64 or tokenPrice >= ~1e18. This requires 1 wei of token0 to be worth >1e18 wei of token1, which is absolutely unrealistic edge case scenario. Issue should be low.

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.

[0x73696d616f]

@spacegliderrrr it also takes the decimal difference between tokens into account, that is why it is likely to happen for some pools.

[nevillehuang]

@spacegliderrrr Any direct arguments for the example provided in the issue above? If not I believe this issue should remain valid

As an example, take Uniswap’s WBTC/SHIBA pool and query slot0. At timestamp 1714392894, the slot0 value returned is 380146371870332863053439965317548561928, which is a 129 bit value. When the _getPriceFromSqrtX96 gets executed for the WBTC/SHIBA pool, an overflow will always occur because a multiplication of two 129-bit integers surpasses 256 bits.

[cvetanovv]

@0xadrii has given a good example of how some Uniswap pools may not be usable by the protocol due to overflow. We can also see how Uniswap has handled this.

Planning to reject the escalation and leave the issue as is.

[Evert0x]

Result:
Medium
Has Duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• spacegliderrrr: rejected

[ethereumdegen]

ok i will fix this -- the fix is just to use that math lib call correct?

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
teller-protocol/teller-protocol-v2-audit-2024#46

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.