[CL]: Fix tick rounding bug and implement direct conversion from tick <> sqrt price #5541

AlpinYukseloglu · 2023-06-15T18:19:51Z

Closes: #5516

Note to reviewer: the only real state machine change is in tick.go and heavily mirrors @ValarDragon's PR here: #5522
The rest of the changes are related to function renames/test refactors.

What is the purpose of the change

This PR expands on #5522 and updates all price to tick conversions to use SqrtPriceToTick instead.

Testing and Verifying

The new function is tested in math/tick_test.go

The original attack vector is also converted into an edge case test in position_test.go

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes?
Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

Specification (x/{module}/README.md)
Osmosis documentation site
Code comments?
N/A

AlpinYukseloglu · 2023-06-16T01:15:34Z

x/concentrated-liquidity/position_test.go

@@ -2509,3 +2509,32 @@ func (s *KeeperTestSuite) TestCreateFullRangePositionLocked() {
 		})
 	}
 }
+
+// TestTickRoundingEdgeCase tests an edge case where incorrect tick rounding would cause LP funds to be drained.
+func (s *KeeperTestSuite) TestTickRoundingEdgeCase() {


Note to reviewer: this is a test case derived from the original attack vector, demonstrating that the change fixes the issue

Lets link the PR number that showcased it

AlpinYukseloglu · 2023-06-16T01:17:42Z

x/concentrated-liquidity/keeper_test.go

+	DefaultExponentConsecutivePositionLowerTick, _       = math.PriceToTickRoundDownSpacing(sdk.NewDec(5500), DefaultTickSpacing)
+	DefaultExponentConsecutivePositionUpperTick, _       = math.PriceToTickRoundDownSpacing(sdk.NewDec(6250), DefaultTickSpacing)
+	DefaultExponentOverlappingPositionLowerTick, _       = math.PriceToTickRoundDownSpacing(sdk.NewDec(4000), DefaultTickSpacing)
+	DefaultExponentOverlappingPositionUpperTick, _       = math.PriceToTickRoundDownSpacing(sdk.NewDec(4999), DefaultTickSpacing)


Driveby rename as the original name could be misinterpreted to refer to rounding behavior

AlpinYukseloglu · 2023-06-16T01:19:03Z

x/concentrated-liquidity/math/tick.go

 	}

-	if tickIndexModulus != 0 {
-		tickIndex = tickIndex - tickIndexModulus


Seems like diff got cluttered when rebasing to main. This section might be easier to review in split view

x/concentrated-liquidity/swaps.go

czarcas7ic

Well done on this, will approve after some discourse in some of these comments

x/concentrated-liquidity/invariant_test.go

x/concentrated-liquidity/math/tick.go

czarcas7ic · 2023-06-16T20:52:37Z

x/concentrated-liquidity/math/tick.go

+	if (!outOfBounds && sqrtPrice.GTE(sqrtPriceTplus2)) || sqrtPrice.LT(sqrtPriceTmin1) || sqrtPrice.GT(sqrtPriceTplus2) {
+		return 0, fmt.Errorf("sqrt price to tick could not find a satisfying tick index. Hit bounds: %v", outOfBounds)
+	}


Not doubting this is correct, but its a bit hard to reason about. Consider breaking each of the three cases further in the comments, but not blocking (especially if its just me who is having trouble groking it)

I am confused about the !outOfBounds conditional as tied to this

(I'd expect it to be !outofbounds && (many or clauses)

This is unfortunately a slightly hacky way of special casing bound checks for MinTick/MaxTick. The first conditional is intended to cover all ticks/sqrt prices that are not boundary ticks. Please see my recent commit for a hopefully more readable bound check: 7c6f256

x/concentrated-liquidity/position_test.go

x/concentrated-liquidity/range_test.go

x/concentrated-liquidity/swaps.go

…verrounding in pool's favor

czarcas7ic

Spoke offline IRT that unanswered questions. Approving but looking for a second approval before merge. Nice work again :)

p0mvn

Great work! I would like to do another pass tomorrow and run tests more. Leaving nits for now

p0mvn · 2023-06-18T01:09:08Z

x/concentrated-liquidity/position_test.go

+	// Execute a swap that brings the price close enough to the edge of a tick to trigger bankers rounding
+	swapAddr := testAccs[2]
+	desiredTokenOut := sdk.NewCoin(USDC, sdk.NewInt(10000))
+	s.FundAcc(swapAddr, sdk.NewCoins(sdk.NewCoin(ETH, sdk.NewInt(1000000000000000000))))
+	_, _, _, _, _, err := s.clk.SwapInAmtGivenOut(s.Ctx, swapAddr, pool, desiredTokenOut, ETH, sdk.ZeroDec(), sdk.ZeroDec())
+	s.Require().NoError(err)
+
+	// Both positions should be able to withdraw successfully
+	_, _, err = s.clk.WithdrawPosition(s.Ctx, firstPositionAddr, firstPosId, firstPosLiq)
+	s.Require().NoError(err)
+	_, _, err = s.clk.WithdrawPosition(s.Ctx, secondPositionAddr, secondPosId, secondPosLiq)
+	s.Require().NoError(err)


Commenting for visibility, I expect this to solve this issue in #5526 :
https://github.com/osmosis-labs/osmosis/pull/5526/files#r1233117388

x/concentrated-liquidity/math/tick.go

AlpinYukseloglu · 2023-06-18T23:17:18Z

app/apptesting/concentrated_liquidity.go

+// CONTRACT: tickSpacing must be smaller or equal to the max of 1 << 63 - 1.
+// This is not a concern because we have authorized tick spacings that are smaller than this max,
+// and we don't expect to ever require it to be this large.
+func (s *KeeperTestHelper) PriceToTickRoundDownSpacing(price sdk.Dec, tickSpacing uint64) (int64, error) {


Note to reviewer: these functions were moved to be high level test helpers to get them out of the state machine without breaking the many tests that rely on them to (correctly) generate expected values

…ticks

AlpinYukseloglu · 2023-06-18T23:22:06Z

x/concentrated-liquidity/range_test.go

@@ -47,7 +47,7 @@ type RangeTestParams struct {
 var (
 	DefaultRangeTestParams = RangeTestParams{
 		// Base amounts
-		baseNumPositions:     1000,
+		baseNumPositions:     10,


Note to reviewer: the changes in this file are mainly cleanup/test speed related changes to ensure tests run in a reasonable amount of time

x/concentrated-liquidity/position_test.go

ValarDragon · 2023-06-19T15:10:20Z

x/concentrated-liquidity/math/tick.go

+	outOfBounds := false
+	if truncatedTick <= types.MinTick {
+		truncatedTick = types.MinTick + 1
+		outOfBounds = true
+	} else if truncatedTick >= types.MaxTick-1 {
+		truncatedTick = types.MaxTick - 2
+		outOfBounds = true


I wonder if its simpler to just deal with these by checking the boundary cases in a separate function, on the sqrt price directly. (pre-squaring)

Possibly, although my previous attempts at this led to constraining the "real" min tick and max tick by 1-2 ticks (which might be fine but leads to more cases that need to be checked/handled)

If you see a way to abstract this logic without triggering this, would be great for us to do a follow up PR to clean this up!

ValarDragon

This LGTM.

I think I'd like to revisit the edge case detections, and add some more randomized TickToPrice / boundary checks, but those can all be done in followup!

p0mvn

LGTM. Great work

p0mvn · 2023-06-19T15:14:58Z

x/concentrated-liquidity/math/tick_test.go

+		"sqrt price exactly equal to min sqrt price": {
+			sqrtPrice:    types.MinSqrtPrice,
+			tickSpacing:  defaultTickSpacing,
+			tickExpected: types.MinTick,
+		},
+		"sqrt price equal to max sqrt price minus one ULP": {
+			sqrtPrice:    types.MaxSqrtPrice.Sub(sdk.SmallestDec()),
+			tickSpacing:  defaultTickSpacing,
+			tickExpected: types.MaxTick - defaultTickSpacing,
+		},
+		"sqrt price corresponds exactly to max tick - 1 (tick spacing 1)": {
+			// Calculated using TickToSqrtPrice(types.MaxTick - 1)
+			sqrtPrice:    sdk.MustNewDecFromStr("9999999499999987499.999374999960937497"),
+			tickSpacing:  1,
+			tickExpected: types.MaxTick - 1,
+		},
+		"sqrt price one ULP below max tick - 1 (tick spacing 1)": {
+			// Calculated using TickToSqrtPrice(types.MaxTick - 1) - 1 ULP
+			sqrtPrice:    sdk.MustNewDecFromStr("9999999499999987499.999374999960937496"),
+			tickSpacing:  1,
+			tickExpected: types.MaxTick - 2,
+		},
+		"sqrt price one ULP below max tick - 1 (tick spacing 100)": {
+			// Calculated using TickToSqrtPrice(types.MaxTick - 1) - 1 ULP
+			sqrtPrice:    sdk.MustNewDecFromStr("9999999499999987499.999374999960937496"),
+			tickSpacing:  defaultTickSpacing,
+			tickExpected: types.MaxTick - defaultTickSpacing,
+		},


Should we also be having test cases around min tick bounds similar to how this is done for max tick?

Will add these cases in the min tick change PR as the expected values are thrown off due to canonical tick rounding

@ValarDragon

… <> sqrt price (#5541) Closes: #5516 > **Note to reviewer:** the only real state machine change is in `tick.go` and heavily mirrors @ValarDragon's PR here: #5522 > The rest of the changes are related to function renames/test refactors. ## What is the purpose of the change This PR expands on #5522 and updates all price to tick conversions to use SqrtPriceToTick instead. ## Testing and Verifying The new function is tested in `math/tick_test.go` The original attack vector is also converted into an edge case test in `position_test.go` ## Documentation and Release Note - [ ] Does this pull request introduce a new feature or user-facing behavior changes? - [ ] Changelog entry added to `Unreleased` section of `CHANGELOG.md`? Where is the change documented? - [ ] Specification (`x/{module}/README.md`) - [ ] Osmosis documentation site - [ ] Code comments? - [ ] N/A

@ValarDragon

* Some optimizations * Update x/concentrated-liquidity/math/math.go Co-authored-by: Dev Ojha <[email protected]> * [CL]: Fix incorrect bound check/chain halt vector (#5557) * repro panic trigger and fix bound check * fix comments * docs: precision issues around price; short and long term solution (#5552) Closes: #XXX ## What is the purpose of the change Related to: #5550 Documenting latest decisions around tick and price conversions ## Testing and Verifying This change is a trivial rework / code cleanup without any test coverage. ## Documentation and Release Note - [ ] Does this pull request introduce a new feature or user-facing behavior changes? - [ ] Changelog entry added to `Unreleased` section of `CHANGELOG.md`? Where is the change documented? - [ ] Specification (`x/{module}/README.md`) - [ ] Osmosis documentation site - [ ] Code comments? - [ ] N/A * Make go tests only run if relevant (#5569) * CL: migration functional test (#5560) Closes: #XXX ## What is the purpose of the change The following PR introduces a functional test that: - Creates every type of balancer position that can be created - Bonded superfluid - Unbonded superfluid (locked) - Unbonded superfluid (unlocking) - Vanilla lock (locked) - Vanilla lock (unlocking) - No lock - It then migrates each position, asserting invariants after each position is migrated - Finally, an overall invariant is run after all positions have been migrated, asserting that all funds are accounted for in some way The next PR subsequent to this will be adding randomization to the inputs in order to make the test non deterministic. ## Testing and Verifying Functional test above added ## Documentation and Release Note - [ ] Does this pull request introduce a new feature or user-facing behavior changes? - [ ] Changelog entry added to `Unreleased` section of `CHANGELOG.md`? Where is the change documented? - [ ] Specification (`x/{module}/README.md`) - [ ] Osmosis documentation site - [ ] Code comments? - [ ] N/A * [CL]: Fix tick rounding bug and implement direct conversion from tick <> sqrt price (#5541) Closes: #5516 > **Note to reviewer:** the only real state machine change is in `tick.go` and heavily mirrors @ValarDragon's PR here: #5522 > The rest of the changes are related to function renames/test refactors. ## What is the purpose of the change This PR expands on #5522 and updates all price to tick conversions to use SqrtPriceToTick instead. ## Testing and Verifying The new function is tested in `math/tick_test.go` The original attack vector is also converted into an edge case test in `position_test.go` ## Documentation and Release Note - [ ] Does this pull request introduce a new feature or user-facing behavior changes? - [ ] Changelog entry added to `Unreleased` section of `CHANGELOG.md`? Where is the change documented? - [ ] Specification (`x/{module}/README.md`) - [ ] Osmosis documentation site - [ ] Code comments? - [ ] N/A * Still fixing merge conlfict * Revert lp.go change as commented --------- Co-authored-by: Dev Ojha <[email protected]> Co-authored-by: alpo <[email protected]> Co-authored-by: Roman <[email protected]> Co-authored-by: Adam Tucker <[email protected]>

ValarDragon and others added 2 commits June 14, 2023 15:09

Add sqrt price to tick

a33f23c

implement and test sqrt price to tick

f7447b7

AlpinYukseloglu added V:state/breaking State machine breaking PR A:no-changelog labels Jun 15, 2023

github-actions bot added C:docs Improvements or additions to documentation C:x/concentrated-liquidity labels Jun 15, 2023

AlpinYukseloglu changed the title ~~[CL]: Implement and test SqrtPriceToTickRoundDownSpacing and replace all uses of price to tick conversion with it~~ [CL]: Implement and test SqrtPriceToTickRoundDownSpacing and use in place of all tick conversions Jun 15, 2023

AlpinYukseloglu changed the base branch from main to dev/sqrt_price_to_tick June 15, 2023 18:20

add original attack vector as test case

a1baa14

github-actions bot removed the C:docs Improvements or additions to documentation label Jun 15, 2023

AlpinYukseloglu added 2 commits June 15, 2023 11:38

fix tick tests broken from renaming

f6583ea

fix max tick handling and add tests at limits

7baf575

AlpinYukseloglu marked this pull request as ready for review June 16, 2023 01:13

AlpinYukseloglu requested review from czarcas7ic and p0mvn as code owners June 16, 2023 01:13

AlpinYukseloglu requested a review from ValarDragon June 16, 2023 01:13

AlpinYukseloglu commented Jun 16, 2023

View reviewed changes

AlpinYukseloglu changed the base branch from dev/sqrt_price_to_tick to main June 16, 2023 01:17

AlpinYukseloglu commented Jun 16, 2023

View reviewed changes

AlpinYukseloglu changed the title ~~[CL]: Implement and test SqrtPriceToTickRoundDownSpacing and use in place of all tick conversions~~ [CL]: Fix tick rounding bug and implement direct conversion from tick <> sqrt price Jun 16, 2023

AlpinYukseloglu added 2 commits June 15, 2023 18:21

clean up prints

ac92ddb

Merge branch 'main' into alpo/sqrt_price_to_tick

1c737a3

github-actions bot added the C:docs Improvements or additions to documentation label Jun 16, 2023

AlpinYukseloglu mentioned this pull request Jun 16, 2023

[CL]: Fix low level tick rounding to mitigate fund drain bug #5510

Closed

6 tasks

enable relevant randomized test cases

d5b3821

p0mvn reviewed Jun 16, 2023

View reviewed changes

x/concentrated-liquidity/swaps.go Outdated Show resolved Hide resolved

czarcas7ic reviewed Jun 16, 2023

View reviewed changes

relax exact rounding error checks in global invariants to allow for o…

1fa59a8

…verrounding in pool's favor

czarcas7ic approved these changes Jun 17, 2023

View reviewed changes

p0mvn mentioned this pull request Jun 17, 2023

fix(CL): tick iterator bugs and tests #5526

Merged

9 tasks

p0mvn reviewed Jun 18, 2023

View reviewed changes

AlpinYukseloglu added 2 commits June 18, 2023 10:37

clean up comments

2a4f1c4

move unused functions outside the state machine

08bd83f

github-actions bot added the C:app-wiring Changes to the app folder label Jun 18, 2023

AlpinYukseloglu and others added 3 commits June 18, 2023 15:19

Merge branch 'main' into alpo/sqrt_price_to_tick

939afad

fix conflicts

83fcbcf

clean up tests

13b213b

AlpinYukseloglu commented Jun 18, 2023

View reviewed changes

remove empty tick handling file in favor of future readme section on …

7e0c38c

…ticks

github-actions bot removed the C:docs Improvements or additions to documentation label Jun 18, 2023

AlpinYukseloglu commented Jun 18, 2023

View reviewed changes

AlpinYukseloglu mentioned this pull request Jun 18, 2023

[CL]: Extract invariant and range test changes from bugfix PR #5561

Merged

6 tasks

AlpinYukseloglu added 2 commits June 18, 2023 16:35

clean up diff

5e3b291

Merge branch 'main' into alpo/sqrt_price_to_tick

3069a23

ValarDragon reviewed Jun 19, 2023

View reviewed changes

x/concentrated-liquidity/position_test.go Outdated Show resolved Hide resolved

ValarDragon reviewed Jun 19, 2023

View reviewed changes

clean up boundary check

7c6f256

ValarDragon approved these changes Jun 19, 2023

View reviewed changes

p0mvn approved these changes Jun 19, 2023

View reviewed changes

enable remaining fuzz tests

d621b42

AlpinYukseloglu added the A:automerge label Jun 19, 2023

Merge branch 'main' into alpo/sqrt_price_to_tick

cfebc3d

mergify bot merged commit 2853245 into main Jun 19, 2023

mergify bot deleted the alpo/sqrt_price_to_tick branch June 19, 2023 15:37

This was referenced Jun 19, 2023

CL: investigate removing a hack in tick updates when stopping swap within a bucket #5571

Closed

investigate(CL): potential off-by-one error when stopping close to lower tick of a bucket #5574

Closed

AlpinYukseloglu mentioned this pull request Jun 21, 2023

[CL]: Attack vector repro to drain LP funds from any CL pool #5493

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CL]: Fix tick rounding bug and implement direct conversion from tick <> sqrt price #5541

[CL]: Fix tick rounding bug and implement direct conversion from tick <> sqrt price #5541

AlpinYukseloglu commented Jun 15, 2023 •

edited

Loading

AlpinYukseloglu Jun 16, 2023

ValarDragon Jun 19, 2023

AlpinYukseloglu Jun 16, 2023

AlpinYukseloglu Jun 16, 2023

czarcas7ic left a comment

czarcas7ic Jun 16, 2023

ValarDragon Jun 19, 2023

ValarDragon Jun 19, 2023

AlpinYukseloglu Jun 19, 2023

czarcas7ic left a comment

p0mvn left a comment

p0mvn Jun 18, 2023

AlpinYukseloglu Jun 18, 2023

AlpinYukseloglu Jun 18, 2023

ValarDragon Jun 19, 2023

AlpinYukseloglu Jun 19, 2023

ValarDragon left a comment

p0mvn left a comment

p0mvn Jun 19, 2023

AlpinYukseloglu Jun 19, 2023

[CL]: Fix tick rounding bug and implement direct conversion from tick <> sqrt price #5541

[CL]: Fix tick rounding bug and implement direct conversion from tick <> sqrt price #5541

Conversation

AlpinYukseloglu commented Jun 15, 2023 • edited Loading

What is the purpose of the change

Testing and Verifying

Documentation and Release Note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czarcas7ic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czarcas7ic left a comment

Choose a reason for hiding this comment

p0mvn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon left a comment

Choose a reason for hiding this comment

p0mvn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlpinYukseloglu commented Jun 15, 2023 •

edited

Loading