[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

@@ -10,6 +10,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"
@@ -1554,21 +1555,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(),
@@ -1612,6 +1622,12 @@ func (s *KeeperTestSuite) TestInitializeInitialPositionForPool() {
 
 				s.Require().Equal(tc.expectedCurrSqrtPrice.String(), pool.GetCurrentSqrtPrice().String())
 				s.Require().Equal(tc.expectedTick, pool.GetCurrentTick())
+
+				// Assert current sqrt price is in correct tick range
+				_, curTickSqrtPrice, err := math.TickToSqrtPrice(pool.GetCurrentTick())
+				_, nextTickSqrtPrice, err := math.TickToSqrtPrice(pool.GetCurrentTick() + 1)
+				s.Require().True(curTickSqrtPrice.LTE(pool.GetCurrentSqrtPrice()))
+				s.Require().True(pool.GetCurrentSqrtPrice().LT(nextTickSqrtPrice))
 			}
 		})
 	}

x/concentrated-liquidity/math/tick.go

@@ -108,17 +108,20 @@ 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) {
+func PriceToTickRoundDownSpacing(price sdk.Dec, tickSpacing uint64) (int64, error) {
 	tickIndex, err := PriceToTick(price)
 	if err != nil {
 		return 0, err
@@ -168,11 +171,12 @@ 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.
+// CONTRACT: `price` is between MinSpotPrice and MaxSpotPrice, inclusive.
 // NOTE: This is really returning a "Bucket index". Bucket index `b` corresponds to
 // all prices in range [TickToPrice(b), TickToPrice(b+1)).
-func CalculatePriceToTick(price sdk.Dec) (tickIndex int64) {
+func CalculatePriceToTick(price sdk.Dec) (tickIndex int64, err error) {
 	if price.Equal(sdkOneDec) {
-		return 0
+		return 0, nil
 	}
 
 	// The approach here is to try determine which "geometric spacing" are we in.
@@ -197,7 +201,7 @@ func CalculatePriceToTick(price sdk.Dec) (tickIndex int64) {
 		}
 	}
 
-	// We know were between (geoSpacing.initialPrice, geoSpacing.endPrice)
+	// We know we're between (geoSpacing.initialPrice, geoSpacing.endPrice)
 	// The number of ticks that need to be filled by our current spacing is
 	// (price - geoSpacing.initialPrice) / geoSpacing.additiveIncrementPerTick
 	priceInThisExponent := osmomath.BigDecFromSDKDec(price.Sub(geoSpacing.initialPrice))
@@ -206,7 +210,64 @@ func CalculatePriceToTick(price sdk.Dec) (tickIndex int64) {
 	// * taking the bucket index of the smallest price in this tick
 	// * adding to it the number of ticks "completely" filled by the current spacing
 	// the latter is the truncation of the division above
-	// TODO: This should be rounding down?
-	tickIndex = geoSpacing.initialTick + ticksFilledByCurrentSpacing.SDKDec().RoundInt64()
-	return tickIndex
+	tickIndex = geoSpacing.initialTick + ticksFilledByCurrentSpacing.SDKDec().TruncateInt64()
+
+	// We get the price corresponding to our calculated bucket index to compare against the input price.
+	curPrice, err := TickToPrice(tickIndex)
+	if err != nil {
+		return 0, err
+	}
+
+	// We first handle cases related to min and max tick independently here, as in either case checking both the tick above and below is not possible.
+	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) {
+		// We assume that the error to the upside is caught by input price bound checks, so we only
+		// check the price in the tick below.
+		tickBelowPrice, err := TickToPrice(tickIndex - 1)
+		if err != nil {
+			return 0, err
+		}
+
+		// If price is lower than the price in the tick below, our error assumption is violated so we panic.
+		if price.LT(tickBelowPrice) {
+			panic(fmt.Sprintf("price %s is outside of error bounds for tick %d", price, tickIndex))
+		}
+
+		// If price falls in the bucket below `tickIndex`, return that bucket index.
+		// Otherwise, return the current tick index.
+		if price.GTE(tickBelowPrice) && price.LT(curPrice) {
+			return tickIndex - 1, nil
+		}
+
+		return types.MaxTick, nil
+	}
+
+	// We get the prices corresponding to the ticks above and below our calculated tick index to compare against the input price.
+	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) {
+		return tickIndex - 1, nil
+	}
+
+	return tickIndex, nil
 }

x/concentrated-liquidity/math/tick_test.go

@@ -285,6 +285,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
@@ -379,7 +384,7 @@ func (suite *ConcentratedMathTestSuite) TestPriceToTick() {
 			tick, _ := math.PriceToTick(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())
@@ -444,16 +449,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)
@@ -464,6 +472,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
@@ -571,9 +581,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"),
@@ -591,7 +602,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)
 
@@ -607,14 +618,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)
@@ -624,7 +635,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)
@@ -699,8 +710,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),
@@ -709,7 +723,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)
 		})
 	}

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() {
+	s.SetupTest()
+	pool := s.PrepareConcentratedPool()
+
+	testAccs := apptesting.CreateRandomAccounts(3)
+	firstPositionAddr := testAccs[0]
+	secondPositionAddr := testAccs[1]
+
+	// Create two identical positions with the initial assets set such that both positions are fully in one asset
+	firstPositionAssets := sdk.NewCoins(sdk.NewCoin(ETH, sdk.NewInt(9823358512)), sdk.NewCoin(USDC, sdk.NewInt(8985893232)))
+	firstPosLiq, firstPosId := s.SetupPosition(pool.GetId(), firstPositionAddr, firstPositionAssets, -68720000, -68710000, true)
+	secondPositionAssets := sdk.NewCoins(sdk.NewCoin(ETH, sdk.NewInt(9823358512)), sdk.NewCoin(USDC, sdk.NewInt(8985893232)))
+	secondPosLiq, secondPosId := s.SetupPosition(pool.GetId(), secondPositionAddr, secondPositionAssets, -68720000, -68710000, true)
+
+	// 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)
+}

x/concentrated-liquidity/spread_rewards_test.go

@@ -489,7 +489,7 @@ func (s *KeeperTestSuite) TestGetInitialSpreadRewardGrowthOppositeDirectionOfLas
 		validPoolId = 1
 	)
 
-	initialPoolTick, err := math.PriceToTickRoundDown(DefaultAmt1.ToDec().Quo(DefaultAmt0.ToDec()), DefaultTickSpacing)
+	initialPoolTick, err := math.PriceToTickRoundDownSpacing(DefaultAmt1.ToDec().Quo(DefaultAmt0.ToDec()), DefaultTickSpacing)
 	s.Require().NoError(err)
 
 	tests := map[string]struct {

x/concentrated-liquidity/swaps.go

@@ -366,7 +366,7 @@ func (k Keeper) computeOutAmtGivenIn(
 			// Otherwise if the sqrtPrice calculated from computeSwapStep does not equal the sqrtPrice we started with at the
 			// beginning of this iteration, we set the swapState tick to the corresponding tick of the sqrtPrice calculated from computeSwapStep
 			price := sqrtPrice.Mul(sqrtPrice)
-			swapState.tick, err = math.PriceToTickRoundDown(price, p.GetTickSpacing())
+			swapState.tick, err = math.PriceToTickRoundDownSpacing(price, p.GetTickSpacing())
 			if err != nil {
 				return sdk.Coin{}, sdk.Coin{}, 0, sdk.Dec{}, sdk.Dec{}, sdk.Dec{}, err
 			}
@@ -496,7 +496,7 @@ func (k Keeper) computeInAmtGivenOut(
 			// otherwise if the sqrtPrice calculated from computeSwapStep does not equal the sqrtPrice we started with at the
 			// beginning of this iteration, we set the swapState tick to the corresponding tick of the sqrtPrice calculated from computeSwapStep
 			price := sqrtPrice.Mul(sqrtPrice)
-			swapState.tick, err = math.PriceToTickRoundDown(price, p.GetTickSpacing())
+			swapState.tick, err = math.PriceToTickRoundDownSpacing(price, p.GetTickSpacing())
 			if err != nil {
 				return sdk.Coin{}, sdk.Coin{}, 0, sdk.Dec{}, sdk.Dec{}, sdk.Dec{}, err
 			}