Open sherlock-admin3 opened 4 months ago
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.
Escalate
In order to overflow we need
sqrtPriceX96 * sqrtPriceX96
to be larger than uint256.max. This means thattokenPrice * 2^192
must exceed 2^256, or we needtokenPrice >= 2^64
ortokenPrice >= ~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.
@spacegliderrrr it also takes the decimal difference between tokens into account, that is why it is likely to happen for some pools.
@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.
@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.
Result: Medium Has Duplicates
ok i will fix this -- the fix is just to use that math lib call correct?
The protocol team fixed this issue in the following PRs/commits: https://github.com/teller-protocol/teller-protocol-v2-audit-2024/pull/46
The Lead Senior Watson signed off on the fix.
0xadrii
medium
Performing a direct multiplication in
_getPriceFromSqrtX96
will overflow for some uniswap poolsSummary
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:This function will perform two actions:
Get the
sqrtPriceX96
from the uniswap pool by querying the pool’s TWAP or the pool’sslot0
function. It is important to note that the returnedsqrtPriceX96
is auint160
value that can store numbers bigger than 128 bits:After obtaining the
sqrtPriceX96
, thepriceX96
will be obtained by multiplyingsqrtPriceX96
by itself and dividing it by(2**96)
: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, theslot0
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: