-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Min tick has wrong rounding making part of the liquidity range unaccessible #61
Comments
The Warden has identified that an extreme subset of the supported ticks in the AMM system is not available for use due to incorrect rounding behavior in the I believe that the issue outlined is a low-risk issue given that no assets are at risk, and very limited functionality of the system is inaccessible which in most cases is not useful. |
alex-ppg changed the severity to QA (Quality Assurance) |
alex-ppg marked the issue as grade-c |
Dear Judge, I respectfully ask you to reconsider classifying this issue for the following reasons:
Furthermore, as you correctly pointed out:
This limitation does indeed affect the availability of the protocol, as the full-range liquidity is not accessible. Given these points, I believe the severity should be elevated to Medium. |
Hi @DadeKuma, @alex-ppg export const getMinTick = (tickSpacing: number) => Math.ceil(-887272 / tickSpacing) * tickSpacing The comparison to the Uniswap v3 test utilities is not appropriate. Test utilities often use different implementations for simplicity or specific test scenarios, and should not be considered as the canonical implementation. What actually Uniswap uses is: @> int24 minTick = (TickMath.MIN_TICK / tickSpacing) * tickSpacing; In summary, finding is invalid due as the superposition implementation is correct and faithful to Uniswap v3 math implementation. |
@0xTenma respectfully, but your example is not related in any way to this issue. Your link is related to the function This is an entirely different concept, as my issue shows that the If you check any Uniswap test, like this one: You will see that all tests use a position minted with the ceiled If you don't think this is correct, please show an example of usage where positions should be minted with the truncated |
@DadeKuma , Thanks again! |
Hey @DadeKuma and @0xTenma, thank you for your PJQA contributions! A single tick space is omitted from the full tick range of a particular AMM, which represents a percentage lower than 1% of the available ranges. As such, the impact is very low and effectively inconsequential for most AMM pairs (feel free to demonstrate an AMM pair that actively utilizes the maximum negative tick range available). |
alex-ppg marked the issue as grade-b |
Lines of code
https://github.com/code-423n4/2024-08-superposition/blob/4528c9d2dbe1550d2660dac903a8246076044905/pkg/seawater/src/maths/tick_math.rs#L94
Vulnerability details
Impact
Min tick math has a wrong rounding direction, so it results in a min tick higher than the expected range for a pool, thus cutting the possible liquidity range.
Proof of Concept
get_min_tick
performs a truncation during the division (towards positive infinite):https://github.com/code-423n4/2024-08-superposition/blob/4528c9d2dbe1550d2660dac903a8246076044905/pkg/seawater/src/maths/tick_math.rs#L94
But the logic is wrong, it should be a ceiling to capture the full liquidity range (towards negative infinite):
https://github.com/Uniswap/v3-core/blob/d8b1c635c275d2a9450bd6a78f3fa2484fef73eb/test/shared/utilities.ts#L10
Here's an example with a 200 tick space with the current logic:
-887272 / 200 = −4436,36 = -4436
->-4436 * 200 = −887200
The correct logic should be:
-887272 / 200 = −4436,36 = -4437
->-4437 * 200 = −887400
So, 200 ticks are cut from the valid liquidity range, but they should have been accessible.
Tools Used
Manual Review
Recommended Mitigation Steps
Consider performing a ceiling op instead of truncation.
Assessed type
Uniswap
The text was updated successfully, but these errors were encountered: