[CL] Changes from audit results(1/2) (#5568)

* Some optimizations

* Update x/concentrated-liquidity/math/math.go

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>

* [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 <ValarDragon@users.noreply.github.com>
Co-authored-by: alpo <62043214+AlpinYukseloglu@users.noreply.github.com>
Co-authored-by: Roman <roman@osmosis.team>
Co-authored-by: Adam Tucker <adam@osmosis.team>

osmomath/math_test.go

@@ -90,7 +90,7 @@ func TestPowApprox(t *testing.T) {
 
 	for i, tc := range testCases {
 		var actualResult sdk.Dec
-		ConditionalPanic(t, tc.base.Equal(sdk.ZeroDec()), func() {
+		ConditionalPanic(t, tc.base.IsZero(), func() {
 			fmt.Println(tc.base)
 			actualResult = PowApprox(tc.base, tc.exp, tc.powPrecision)
 			require.True(
@@ -154,7 +154,7 @@ func TestPow(t *testing.T) {
 
 	for i, tc := range testCases {
 		var actualResult sdk.Dec
-		ConditionalPanic(t, tc.base.Equal(sdk.ZeroDec()), func() {
+		ConditionalPanic(t, tc.base.IsZero(), func() {
 			actualResult = Pow(tc.base, tc.exp)
 			require.True(
 				t,

osmoutils/accum/accum.go

@@ -294,7 +294,7 @@ func (accum *AccumulatorObject) UpdatePosition(name string, numShares sdk.Dec) e
 // All intervalAccumulationPerShare DecCoin value must be non-negative. They must also be a superset of the
 // old accumulator value associated with the position.
 func (accum *AccumulatorObject) UpdatePositionIntervalAccumulation(name string, numShares sdk.Dec, intervalAccumulationPerShare sdk.DecCoins) error {
-	if numShares.Equal(sdk.ZeroDec()) {
+	if numShares.IsZero() {
 		return ZeroSharesError
 	}
 
@@ -422,7 +422,7 @@ func (accum AccumulatorObject) ClaimRewards(positionName string) (sdk.Coins, sdk
 	// This is acceptable because we round in favor of the protocol.
 	truncatedRewardsTotal, dust := totalRewards.TruncateDecimal()
 
-	if position.NumShares.Equal(sdk.ZeroDec()) {
+	if position.NumShares.IsZero() {
 		// remove the position from state entirely if numShares = zero
 		accum.deletePosition(positionName)
 	} else {

osmoutils/accum/accum_test.go

@@ -972,7 +972,7 @@ func (suite *AccumTestSuite) TestRemoveFromPosition() {
 				suite.Require().Equal(tc.startingUnclaimedRewards.Add(tc.expAccumDelta.MulDec(tc.startingNumShares)...), newPosition.UnclaimedRewardsTotal)
 
 				// Ensure address's position properly reflects new number of shares
-				if (tc.startingNumShares.Sub(tc.removedShares)).Equal(sdk.ZeroDec()) {
+				if (tc.startingNumShares.Sub(tc.removedShares)).IsZero() {
 					suite.Require().Equal(emptyDec, newPosition.NumShares)
 				} else {
 					suite.Require().Equal(tc.startingNumShares.Sub(tc.removedShares), newPosition.NumShares)
@@ -1163,7 +1163,7 @@ func (suite *AccumTestSuite) TestGetPositionSize() {
 			// Get position size from valid address (or from nonexistant if address does not exist)
 			positionSize, err := curAccum.GetPositionSize(positionName)
 
-			if tc.changedShares.GT(sdk.ZeroDec()) {
+			if tc.changedShares.IsPositive() {
 				accumPackage.InitOrUpdatePosition(curAccum, curAccum.GetValue(), positionName, tc.numShares.Add(tc.changedShares), sdk.NewDecCoins(), nil)
 			}
 
@@ -1613,14 +1613,14 @@ func (suite *AccumTestSuite) TestGetTotalShares() {
 
 		// Half the time, we add to the new position
 		addAmt := baseAmt.Mul(addShares)
-		if addAmt.GT(sdk.ZeroDec()) {
+		if addAmt.IsPositive() {
 			err = curAccum.AddToPosition(curAddr, addAmt)
 			suite.Require().NoError(err)
 		}
 
 		// Half the time, we remove one share from the position
 		amtToRemove := sdk.OneDec().Mul(removeShares)
-		if amtToRemove.GT(sdk.ZeroDec()) {
+		if amtToRemove.IsPositive() {
 			err = curAccum.RemoveFromPosition(curAddr, amtToRemove)
 			suite.Require().NoError(err)
 		}

x/concentrated-liquidity/export_test.go

@@ -118,10 +118,6 @@ func (k Keeper) CollectSpreadRewards(ctx sdk.Context, owner sdk.AccAddress, posi
 	return k.collectSpreadRewards(ctx, owner, positionId)
 }
 
-func (k Keeper) EnsurePositionOwner(ctx sdk.Context, sender sdk.AccAddress, poolId uint64, positionId uint64) error {
-	return k.ensurePositionOwner(ctx, sender, poolId, positionId)
-}
-
 func (k Keeper) PrepareClaimableSpreadRewards(ctx sdk.Context, positionId uint64) (sdk.Coins, error) {
 	return k.prepareClaimableSpreadRewards(ctx, positionId)
 }

x/concentrated-liquidity/incentives.go

@@ -403,27 +403,24 @@ func (k Keeper) updateGivenPoolUptimeAccumulatorsToNow(ctx sdk.Context, pool typ
 
 	// We optimistically assume that all liquidity on the active tick qualifies and handle
 	// uptime-related checks in forfeiting logic.
+
+	// If there is no share to be incentivized for the current uptime accumulator, we leave it unchanged
 	qualifyingLiquidity := pool.GetLiquidity().Add(qualifyingBalancerShares)
+	if !qualifyingLiquidity.LT(sdk.OneDec()) {
+		for uptimeIndex := range uptimeAccums {
+			// Get relevant uptime-level values
+			curUptimeDuration := types.SupportedUptimes[uptimeIndex]
+			incentivesToAddToCurAccum, updatedPoolRecords, err := calcAccruedIncentivesForAccum(ctx, curUptimeDuration, qualifyingLiquidity, timeElapsedSec, poolIncentiveRecords)
+			if err != nil {
+				return err
+			}
 
-	for uptimeIndex := range uptimeAccums {
-		// Get relevant uptime-level values
-		curUptimeDuration := types.SupportedUptimes[uptimeIndex]
+			// Emit incentives to current uptime accumulator
+			uptimeAccums[uptimeIndex].AddToAccumulator(incentivesToAddToCurAccum)
 
-		// If there is no share to be incentivized for the current uptime accumulator, we leave it unchanged
-		if qualifyingLiquidity.LT(sdk.OneDec()) {
-			continue
+			// Update pool records (stored in state after loop)
+			poolIncentiveRecords = updatedPoolRecords
 		}
-
-		incentivesToAddToCurAccum, updatedPoolRecords, err := calcAccruedIncentivesForAccum(ctx, curUptimeDuration, qualifyingLiquidity, timeElapsedSec, poolIncentiveRecords)
-		if err != nil {
-			return err
-		}
-
-		// Emit incentives to current uptime accumulator
-		uptimeAccums[uptimeIndex].AddToAccumulator(incentivesToAddToCurAccum)
-
-		// Update pool records (stored in state after loop)
-		poolIncentiveRecords = updatedPoolRecords
 	}
 
 	// Update pool incentive records and LastLiquidityUpdate time in state to reflect emitted incentives
@@ -538,7 +535,7 @@ func (k Keeper) setIncentiveRecord(ctx sdk.Context, incentiveRecord types.Incent
 	// In all other cases, we update the record in state
 	if store.Has(key) && incentiveRecordBody.RemainingCoin.IsZero() {
 		store.Delete(key)
-	} else if incentiveRecordBody.RemainingCoin.Amount.GT(sdk.ZeroDec()) {
+	} else if incentiveRecordBody.RemainingCoin.Amount.IsPositive() {
 		osmoutils.MustSet(store, key, &incentiveRecordBody)
 	}
 
@@ -972,9 +969,11 @@ func (k Keeper) collectIncentives(ctx sdk.Context, sender sdk.AccAddress, positi
 		return sdk.Coins{}, sdk.Coins{}, err
 	}
 
-	err = k.ensurePositionOwner(ctx, sender, position.PoolId, positionId)
-	if err != nil {
-		return sdk.Coins{}, sdk.Coins{}, err
+	if sender.String() != position.Address {
+		return sdk.Coins{}, sdk.Coins{}, types.NotPositionOwnerError{
+			PositionId: positionId,
+			Address:    sender.String(),
+		}
 	}
 
 	// Claim all incentives for the position.

x/concentrated-liquidity/lp_test.go

@@ -663,9 +663,6 @@ func (s *KeeperTestSuite) TestWithdrawPosition() {
 				s.Require().ErrorAs(err, &types.PositionIdNotFoundError{PositionId: config.positionId})
 				s.Require().Equal(clmodel.Position{}, position)
 
-				err = concentratedLiquidityKeeper.EnsurePositionOwner(s.Ctx, owner, config.poolId, config.positionId)
-				s.Require().Error(err, types.NotPositionOwnerError{PositionId: config.positionId, Address: owner.String()})
-
 				// Since the positionLiquidity is deleted, retrieving it should return an error.
 				positionLiquidity, err := s.App.ConcentratedLiquidityKeeper.GetPositionLiquidity(s.Ctx, config.positionId)
 				s.Require().Error(err)

x/concentrated-liquidity/math/math.go

@@ -26,11 +26,11 @@ func Liquidity0(amount sdk.Int, sqrtPriceA, sqrtPriceB sdk.Dec) sdk.Dec {
 
 	product := sqrtPriceABigDec.Mul(sqrtPriceBBigDec)
 	diff := sqrtPriceBBigDec.Sub(sqrtPriceABigDec)
-	if diff.Equal(osmomath.ZeroDec()) {
+	if diff.IsZero() {
 		panic(fmt.Sprintf("liquidity0 diff is zero: sqrtPriceA %s sqrtPriceB %s", sqrtPriceA, sqrtPriceB))
 	}
 
-	return amountBigDec.Mul(product).Quo(diff).SDKDec()
+	return amountBigDec.MulMut(product).QuoMut(diff).SDKDec()
 }
 
 // Liquidity1 takes an amount of asset1 in the pool as well as the sqrtpCur and the nextPrice
@@ -49,11 +49,11 @@ func Liquidity1(amount sdk.Int, sqrtPriceA, sqrtPriceB sdk.Dec) sdk.Dec {
 	sqrtPriceBBigDec := osmomath.BigDecFromSDKDec(sqrtPriceB)
 
 	diff := sqrtPriceBBigDec.Sub(sqrtPriceABigDec)
-	if diff.Equal(osmomath.ZeroDec()) {
+	if diff.IsZero() {
 		panic(fmt.Sprintf("liquidity1 diff is zero: sqrtPriceA %s sqrtPriceB %s", sqrtPriceA, sqrtPriceB))
 	}
 
-	return amountBigDec.Quo(diff).SDKDec()
+	return amountBigDec.QuoMut(diff).SDKDec()
 }
 
 // CalcAmount0 takes the asset with the smaller liquidity in the pool as well as the sqrtpCur and the nextPrice and calculates the amount of asset 0
@@ -70,7 +70,7 @@ func CalcAmount0Delta(liq, sqrtPriceA, sqrtPriceB sdk.Dec, roundUp bool) sdk.Dec
 	// this is to prevent removing more from the pool than expected due to rounding
 	// example: we calculate 1000000.9999999 uusdc (~$1) amountIn and 2000000.999999 uosmo amountOut
 	// we would want the user to put in 1000001 uusdc rather than 1000000 uusdc to ensure we are charging enough for the amount they are removing
-	// additionally, without rounding, there exists cases where the swapState.amountSpecifiedRemaining.GT(sdk.ZeroDec()) for loop within
+	// additionally, without rounding, there exists cases where the swapState.amountSpecifiedRemaining.IsPositive() for loop within
 	// the CalcOut/In functions never actually reach zero due to dust that would have never gotten counted towards the amount (numbers after the 10^6 place)
 	if roundUp {
 		// Note that we do MulTruncate so that the denominator is smaller as this is
@@ -105,7 +105,7 @@ func CalcAmount1Delta(liq, sqrtPriceA, sqrtPriceB sdk.Dec, roundUp bool) sdk.Dec
 	// this is to prevent removing more from the pool than expected due to rounding
 	// example: we calculate 1000000.9999999 uusdc (~$1) amountIn and 2000000.999999 uosmo amountOut
 	// we would want the used to put in 1000001 uusdc rather than 1000000 uusdc to ensure we are charging enough for the amount they are removing
-	// additionally, without rounding, there exists cases where the swapState.amountSpecifiedRemaining.GT(sdk.ZeroDec()) for loop within
+	// additionally, without rounding, there exists cases where the swapState.amountSpecifiedRemaining.IsPositive() for loop within
 	// the CalcOut/In functions never actually reach zero due to dust that would have never gotten counted towards the amount (numbers after the 10^6 place)
 	if roundUp {
 		// Note that we do MulRoundUp so that the end result is larger as this is
@@ -128,12 +128,14 @@ func CalcAmount1Delta(liq, sqrtPriceA, sqrtPriceB sdk.Dec, roundUp bool) sdk.Dec
 // to avoid overpaying the amount out of the pool. Therefore, we round up.
 // sqrt_next = liq * sqrt_cur / (liq + token_in * sqrt_cur)
 func GetNextSqrtPriceFromAmount0InRoundingUp(sqrtPriceCurrent, liquidity, amountZeroRemainingIn sdk.Dec) (sqrtPriceNext sdk.Dec) {
-	if amountZeroRemainingIn.Equal(sdk.ZeroDec()) {
+	if amountZeroRemainingIn.IsZero() {
 		return sqrtPriceCurrent
 	}
 
 	product := amountZeroRemainingIn.Mul(sqrtPriceCurrent)
-	denominator := product.AddMut(liquidity)
+	// denominator = product + liquidity
+	denominator := product
+	denominator.AddMut(liquidity)
 	return liquidity.Mul(sqrtPriceCurrent).QuoRoundupMut(denominator)
 }
 
@@ -143,7 +145,7 @@ func GetNextSqrtPriceFromAmount0InRoundingUp(sqrtPriceCurrent, liquidity, amount
 // so that we get the desired output amount out. Therefore, we round up.
 // sqrt_next = liq * sqrt_cur / (liq - token_out * sqrt_cur)
 func GetNextSqrtPriceFromAmount0OutRoundingUp(sqrtPriceCurrent, liquidity, amountZeroRemainingOut sdk.Dec) (sqrtPriceNext sdk.Dec) {
-	if amountZeroRemainingOut.Equal(sdk.ZeroDec()) {
+	if amountZeroRemainingOut.IsZero() {
 		return sqrtPriceCurrent
 	}
 

x/concentrated-liquidity/model/pool_test.go

@@ -695,8 +695,8 @@ func (suite *ConcentratedPoolTestSuite) TestCalcActualAmounts() {
 				amt1Diff := actualAmount1.Sub(actualAmount1Neg.Neg())
 
 				// Difference is between 0 and 1 due to positive liquidity rounding up and negative liquidity performing math normally.
-				suite.Require().True(amt0Diff.GT(sdk.ZeroDec()) && amt0Diff.LT(sdk.OneDec()))
-				suite.Require().True(amt1Diff.GT(sdk.ZeroDec()) && amt1Diff.LT(sdk.OneDec()))
+				suite.Require().True(amt0Diff.IsPositive() && amt0Diff.LT(sdk.OneDec()))
+				suite.Require().True(amt1Diff.IsPositive() && amt1Diff.LT(sdk.OneDec()))
 			}
 		})
 	}

x/concentrated-liquidity/position.go

@@ -99,22 +99,6 @@ func (k Keeper) HasAnyPositionForPool(ctx sdk.Context, poolId uint64) (bool, err
 	return osmoutils.HasAnyAtPrefix(store, poolPositionKey, parse)
 }
 
-func (k Keeper) ensurePositionOwner(ctx sdk.Context, sender sdk.AccAddress, poolId uint64, positionId uint64) error {
-	isOwner := k.isPositionOwner(ctx, sender, poolId, positionId)
-	if !isOwner {
-		return types.NotPositionOwnerError{
-			PositionId: positionId,
-			Address:    sender.String(),
-		}
-	}
-	return nil
-}
-
-// isPositionOwner returns true if the given positionId is owned by the given sender inside the given pool.
-func (k Keeper) isPositionOwner(ctx sdk.Context, sender sdk.AccAddress, poolId uint64, positionId uint64) bool {
-	return ctx.KVStore(k.storeKey).Has(types.KeyAddressPoolIdPositionId(sender, poolId, positionId))
-}
-
 // GetAllPositionsForPoolId gets all the position for a specific poolId and store prefix.
 func (k Keeper) GetAllPositionIdsForPoolId(ctx sdk.Context, prefix []byte, poolId uint64) ([]uint64, error) {
 	store := ctx.KVStore(k.storeKey)

x/concentrated-liquidity/position_test.go

@@ -313,7 +313,7 @@ func (s *KeeperTestSuite) TestInitOrUpdatePosition() {
 				if test.positionExists {
 					// Track how much the current uptime accum has grown by
 					actualUptimeAccumDelta[uptimeIndex] = newUptimeAccumValues[uptimeIndex].Sub(initUptimeAccumValues[uptimeIndex])
-					if timeElapsedSec.GT(sdk.ZeroDec()) {
+					if timeElapsedSec.IsPositive() {
 						expectedGrowthCurAccum, _, err := cl.CalcAccruedIncentivesForAccum(s.Ctx, uptime, test.param.liquidityDelta, timeElapsedSec, expectedIncentiveRecords)
 						s.Require().NoError(err)
 						expectedUptimeAccumValueGrowth[uptimeIndex] = expectedGrowthCurAccum
@@ -410,103 +410,6 @@ type positionOwnershipTest struct {
 	poolId         uint64
 }
 
-func (s *KeeperTestSuite) runIsPositionOwnerTest(test positionOwnershipTest) {
-	s.SetupTest()
-	p := s.PrepareConcentratedPool()
-	for i, owner := range test.setupPositions {
-		err := s.App.ConcentratedLiquidityKeeper.InitOrUpdatePosition(s.Ctx, p.GetId(), owner, DefaultLowerTick, DefaultUpperTick, DefaultLiquidityAmt, DefaultJoinTime, uint64(i))
-		s.Require().NoError(err)
-	}
-	err := s.App.ConcentratedLiquidityKeeper.EnsurePositionOwner(s.Ctx, test.queryPositionOwner, test.poolId, test.queryPositionId)
-	if test.expPass {
-		s.Require().NoError(err)
-	} else {
-		s.Require().Error(err)
-	}
-
-}
-
-func (s *KeeperTestSuite) TestIsPositionOwnerMultiPosition() {
-	tenAddrOneAddr := []sdk.AccAddress{}
-	for i := 0; i < 10; i++ {
-		tenAddrOneAddr = append(tenAddrOneAddr, s.TestAccs[0])
-	}
-	tenAddrOneAddr = append(tenAddrOneAddr, s.TestAccs[1])
-	tests := map[string]positionOwnershipTest{
-		"prefix malleability (prior bug)": {
-			queryPositionOwner: s.TestAccs[1],
-			queryPositionId:    1, expPass: false,
-			setupPositions: tenAddrOneAddr,
-		},
-		"things work": {
-			queryPositionOwner: s.TestAccs[1],
-			queryPositionId:    10, expPass: true,
-			setupPositions: tenAddrOneAddr,
-		},
-	}
-	for name, test := range tests {
-		s.Run(name, func() {
-			test.poolId = 1
-			s.runIsPositionOwnerTest(test)
-		})
-	}
-}
-
-func (s *KeeperTestSuite) TestIsPositionOwner() {
-	actualOwner := s.TestAccs[0]
-	nonOwner := s.TestAccs[1]
-
-	tests := []struct {
-		name         string
-		ownerToQuery sdk.AccAddress
-		poolId       uint64
-		positionId   uint64
-		isOwner      bool
-	}{
-		{
-			name:         "Happy path",
-			ownerToQuery: actualOwner,
-			poolId:       1,
-			positionId:   DefaultPositionId,
-			isOwner:      true,
-		},
-		{
-			name:         "query non owner",
-			ownerToQuery: nonOwner,
-			poolId:       1,
-			positionId:   DefaultPositionId,
-			isOwner:      false,
-		},
-		{
-			name:         "different pool ID, not the owner",
-			ownerToQuery: actualOwner,
-			poolId:       2,
-			positionId:   DefaultPositionId,
-			isOwner:      false,
-		},
-		{
-			name:         "different position ID, not the owner",
-			ownerToQuery: actualOwner,
-			poolId:       1,
-			positionId:   DefaultPositionId + 1,
-			isOwner:      false,
-		},
-	}
-
-	for _, test := range tests {
-		s.Run(test.name, func() {
-			s.runIsPositionOwnerTest(positionOwnershipTest{
-				queryPositionOwner: test.ownerToQuery,
-				queryPositionId:    test.positionId,
-				expPass:            test.isOwner,
-				// positions 0 and 1 are owned by actualOwner
-				setupPositions: []sdk.AccAddress{actualOwner, actualOwner},
-				poolId:         test.poolId,
-			})
-		})
-	}
-}
-
 func (s *KeeperTestSuite) TestGetUserPositions() {
 	s.Setup()
 	defaultAddress := s.TestAccs[0]

x/concentrated-liquidity/simulation/sim_msgs.go

@@ -114,7 +114,7 @@ func RandMsgWithdrawPosition(k clkeeper.Keeper, sim *osmosimtypes.SimCtx, ctx sd
 	}
 
 	withdrawAmount := sim.RandomDecAmount(position.Liquidity)
-	if withdrawAmount.TruncateDec().LT(sdk.ZeroDec()) {
+	if withdrawAmount.TruncateDec().IsNegative() {
 		return nil, fmt.Errorf("Invalid withdraw amount")
 	}