cergyk - Rebalance may revert when sqrtPriceX96 > uint128

[sherlock-admin]

cergyk

medium

Rebalance may revert when sqrtPriceX96 > uint128

Summary

The squaring of the variable sqrtPriceX96 may revert if it is large due to integer overflow, and prevent rebalancing of the vault.

Vulnerability Detail

As can be seen here, FullMath library is used to make calculations on 512 bits numbers: https://github.com/sherlock-audit/2023-06-arrakis/blob/main/v2-manager-templates/contracts/SimpleManager.sol#L184

However the first argument: sqrtPriceX96 * sqrtPriceX96 is of type uint256 and can cause an overflow if sqrtPriceX96 >= 2**128

Please note that this has been reported during a previous audit: https://gist.github.com/kassandraoftroy/25f7208adb770abee9f46978326cfb3f (1st issue)

But has incorrectly been marked as fixed when it was not, so it seems that this should be counted as a valid issue in the scope of this contest, since without it, it would have stayed unnoticed.

Impact

Rebalance may be impossible to execute when sqrtPriceX96 is large (gte than 2**128)

Code Snippet

Tool used

Manual Review

Recommendation

uint256 poolPrice = 
    FullMath.mulDiv(
        FullMath.mulDiv(
            sqrtPriceX96,
            10 ** token0Decimals,
            2 ** 192
        ), 
        sqrtPriceX96, 
        1
    );

or similar

[Gevarist]

This vulnerability is not introducing user fund loss, so we are not considering this bug as a medium level issue.

[ctf-sec]

Recommend maintaining medium severity, the rebalance are not expected to revert and the case sqrtPriceX96 > uint128 should be properly handled

[ctf-sec]

Escalate

duplicates https://github.com/sherlock-audit/2023-06-arrakis-judging/issues/268

[Jeiwan]

Escalate

This is a low severity finding. The finding doesn't demonstrate a realistic scenario when sqrtPriceX96 is greater than 2**128. The overflow can happen theoretically, but there are no real tokens for which sqrtPriceX96 can overflow. Thus, the impact of the overflow is non essential.

[sherlock-admin]

Escalate

This is a low severity finding. The finding doesn't demonstrate a realistic scenario when sqrtPriceX96 is greater than 2**128. The overflow can happen theoretically, but there are no real tokens for which sqrtPriceX96 can overflow. Thus, the impact of the overflow is non essential.

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.

[SergeKireev]

Escalate

This is a low severity finding. The finding doesn't demonstrate a realistic scenario when sqrtPriceX96 is greater than 2**128. The overflow can happen theoretically, but there are no real tokens for which sqrtPriceX96 can overflow. Thus, the impact of the overflow is non essential.

If I recall correctly WBTC/USDC comes realistically close to 2**128

[Jeiwan]

Escalate This is a low severity finding. The finding doesn't demonstrate a realistic scenario when sqrtPriceX96 is greater than 2**128. The overflow can happen theoretically, but there are no real tokens for which sqrtPriceX96 can overflow. Thus, the impact of the overflow is non essential.

If I recall correctly WBTC/USDC comes realistically close to 2**128

The current value of sqrtPriceX96 of the WBTC/USDC pair is 1377830107802503820818533285179, which is 246969757 times smaller than the max value of uint128. Honestly, I don't see how it's realistically close to 2128. 1377830107802503820818533285179 2 is 60994060876105759 times smaller than the max value of uint256. Again, I don't see realistic scenarios when this value can be achieved.

[SergeKireev]

A better example still is WETH/USDC on ETH mainnet, with 1804636105417034023712047425203278. It is only ~188000 times smaller than type(uint128).max.

And this is only considering popular tokens. The contest page states in supported ERC20 section:

A wide variety. The "core" contracts fundamentally can be used by any ERC20 token pair except rebasing and fee on transfer tokens (provided Uniswap V3 pool(s) already have been created). The "core" contracts are highly flexible and can be utilized to service a number of very different use-cases.

I asked sponsor about it during the contest, and he confirmed with her team that this was a valid concern

[Jeiwan]

A better example still is WETH/USDC on ETH mainnet, with 1804636105417034023712047425203278. It is only ~188000 times smaller than type(uint128).max.

As I said. It can overflow, but there are no realistic scenarios where this could happen. There are no Uniswap V3 pairs for which it will realistically overflow.

I asked sponsor about it during the contest, and he confirmed with her team that this was a valid concern

A valid concern with low severity. Sounds correct. And the sponsor has also disagreed with the medium severity.

[syjcnss]

The impact is like a bug that makes some of the functions unfunctional.

It should be fixed.

Severity is low according to sherlock's rule on DOS.

[SergeKireev]

Found this pool: https://etherscan.io/address/0x34928666057be86c8ede5029f7d21d87f0c818de

MED34/ETH with a sqrtPriceX96 of 340276893912656726430449275405379147751 which is a bit bigger than type(uint128).max

With current code it would not be supported for rebalances, and would put user's funds at risk under volatile market conditions

Edit: Just in case that's not clear, it is a token created for the occasion. Wanted to clarify that's factually possible to have a normal token with a price causing the overflow 😄

[xiaoming9090]

I happened to see this interesting conversation. Here is some of my input for food for thought:

1) Per the Uniswap V3 bug bounty page, Uniswap V3 was developed with the assumption that the total supply of any token does not exceed 2¹²⁸ - 1, i.e. type(uint128).max. If this assumption hold, is it possible for sqrtPriceX96 to exceed type(uint128).max?

2) Are there pools (supporting tokens with sensible totalSupply and decimals) where its sqrtPriceX96 could have a reasonable chance of exceeding type(uint128).max and could lead to a material amount of funds lost for the users?

Disclaimer: I did not participate in this contest.

[c14sh]

Constructive and fair points raised by Senior. Here are research results:

1. type(uint128).max (approx. 3.4E38) for totalSupply is a pretty big number. If decimal is 18, this means totalSupply can be up to 1E20. Most tokens will fit in the range. Meanwhile, sqrtPriceX96 is more dependent on relative price, relative token order, and/or decimal difference and seems less relevant to totalSupply.

2. On Ethereum mainnet, there are high price tokens like WBTC (8), HBTC (18), XAUt (6), ETH (18) and low price tokens like SHIB (18), PEPE (18), FLOKI (9), LUNC (6). (Number in parenthesis represents decimal and all above tokens are in top 300 in CMM) Some extreme tokens exist like YAM V2 (24) and Gemini Dollar (2). To overflow, sqrt of price should be be bigger than 2^(128-96) = 2^32 (approx. 4.3E9), thus, price should be bigger than 1.9E19. One possibility is decimal difference 10 and relative price bigger than 1.9E9, for WBTC/PEPE - WBTC (8) $30,000, PEPE (18) $0.000_0016. The price is then 1.8E20 (>1.9E19) leading to uint128 overflow. This pool doesn't exist however.

WBTC/PEPES is close to the limit.

[Gevarist]

Are we agree that's a low severity issue?

[ctf-sec]

Agree with low severity based on the discussion above.

Rebalance can revert when sqrtPriceX96 > uint128 but only in extreme edge case

[hrishibhat]

Result: Low Has duplicates Considering this issue as a valid low based on the current impact as there is no pool that exists currently and unlikely that it does, the impact of revert can together be counted as low.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• Jeiwan: accepted

[Gevarist]

This issue has been fixed on v2-manager-template repository. The fix PR is https://github.com/ArrakisFinance/v2-manager-templates/pull/26.

The corresponding fix can be found in SimpleManager.sol here.

[IAm0x52]

Fix looks good. Same fix as #8