[CL: Audit] Superfluid Migration audit / cleanup (#5615) (#5717)

* issue no lock send * added verbose test * audit progress * lock handling case * investigating calculation * cleanup * locked LP shares issue * added issue * issue fixed * cleanup * more cleanup * Update x/superfluid/keeper/migrate.go * Update x/superfluid/keeper/migrate.go Co-authored-by: Matt, Park <[email protected]> --------- Co-authored-by: Adam Tucker <[email protected]> Co-authored-by: Matt, Park <[email protected]> (cherry picked from commit a923e6c) Co-authored-by: Sishir Giri <[email protected]>
osmosis-labs · Jul 2, 2023 · db2f738 · db2f738
1 parent 8b199a7
commit db2f738
Show file tree

Hide file tree

Showing 5 changed files with 38 additions and 19 deletions.
diff --git a/x/concentrated-liquidity/position.go b/x/concentrated-liquidity/position.go
@@ -431,7 +431,7 @@ func (k Keeper) CreateFullRangePositionLocked(ctx sdk.Context, clPoolId uint64,
 		return 0, sdk.Int{}, sdk.Int{}, sdk.Dec{}, 0, err
 	}
 
-	// Mint cl shares for the position and lock them for the remaining lock duration.
+	// Mint CL shares (similar to GAMM shares) for the position and lock them for the remaining lock duration.
 	// Also sets the position ID to underlying lock ID mapping.
 	concentratedLockId, _, err := k.mintSharesAndLock(ctx, clPoolId, positionId, owner, remainingLockDuration)
 	if err != nil {
@@ -484,7 +484,7 @@ func (k Keeper) mintSharesAndLock(ctx sdk.Context, concentratedPoolId, positionI
 	// Create a coin object to represent the underlying liquidity for the cl position.
 	underlyingLiquidityTokenized = sdk.NewCoins(sdk.NewCoin(types.GetConcentratedLockupDenomFromPoolId(concentratedPoolId), position.Liquidity.TruncateInt()))
 
-	// Mint the underlying liquidity as a token and send to the owner.
+	// Mint the underlying liquidity as a token
 	err = k.bankKeeper.MintCoins(ctx, lockuptypes.ModuleName, underlyingLiquidityTokenized)
 	if err != nil {
 		return 0, sdk.Coins{}, err

diff --git a/x/concentrated-liquidity/position_test.go b/x/concentrated-liquidity/position_test.go
@@ -2327,6 +2327,8 @@ func (s *KeeperTestSuite) TestCreateFullRangePositionLocked() {
 	invalidCoin1Denom := sdk.NewCoins(DefaultCoin0, sdk.NewCoin("invalidDenom", sdk.NewInt(1000000000000000000)))
 	zeroCoins := sdk.NewCoins()
 
+	defaultRemainingLockDuration := s.App.StakingKeeper.GetParams(s.Ctx).UnbondingTime
+
 	tests := []struct {
 		name                  string
 		remainingLockDuration time.Duration
@@ -2335,30 +2337,30 @@ func (s *KeeperTestSuite) TestCreateFullRangePositionLocked() {
 	}{
 		{
 			name:                  "valid test",
-			remainingLockDuration: 24 * time.Hour * 14,
+			remainingLockDuration: defaultRemainingLockDuration,
 			coinsForPosition:      DefaultCoins,
 		},
 		{
 			name:                  "invalid coin0 denom",
-			remainingLockDuration: 24 * time.Hour * 14,
+			remainingLockDuration: defaultRemainingLockDuration,
 			coinsForPosition:      invalidCoin0Denom,
 			expectedErr:           types.Amount0IsNegativeError{Amount0: sdk.ZeroInt()},
 		},
 		{
 			name:                  "invalid coin1 denom",
-			remainingLockDuration: 24 * time.Hour * 14,
+			remainingLockDuration: defaultRemainingLockDuration,
 			coinsForPosition:      invalidCoin1Denom,
 			expectedErr:           types.Amount1IsNegativeError{Amount1: sdk.ZeroInt()},
 		},
 		{
 			name:                  "invalid coins amount",
-			remainingLockDuration: 24 * time.Hour * 14,
+			remainingLockDuration: defaultRemainingLockDuration,
 			coinsForPosition:      invalidCoinsAmount,
 			expectedErr:           types.NumCoinsError{NumCoins: len(invalidCoinsAmount)},
 		},
 		{
 			name:                  "edge: both coins amounts' are zero",
-			remainingLockDuration: 24 * time.Hour * 14,
+			remainingLockDuration: defaultRemainingLockDuration,
 			coinsForPosition:      zeroCoins,
 			expectedErr:           types.NumCoinsError{NumCoins: 0},
 		},
@@ -2398,6 +2400,7 @@ func (s *KeeperTestSuite) TestCreateFullRangePositionLocked() {
 			// Check locked
 			concentratedLock, err := s.App.LockupKeeper.GetLockByID(s.Ctx, concentratedLockId)
 			s.Require().NoError(err)
+
 			s.Require().Equal(concentratedLock.Coins[0].Amount.String(), liquidity.TruncateInt().String())
 			s.Require().False(concentratedLock.IsUnlocking())
 		})

diff --git a/x/superfluid/keeper/export_test.go b/x/superfluid/keeper/export_test.go
@@ -34,8 +34,8 @@ func (k Keeper) MigrateNonSuperfluidLockBalancerToConcentrated(ctx sdk.Context,
 	return k.migrateNonSuperfluidLockBalancerToConcentrated(ctx, sender, lockId, sharesToMigrate, tokenOutMins)
 }
 
-func (k Keeper) ValidateSharesToMigrateUnlockAndExitBalancerPool(ctx sdk.Context, sender sdk.AccAddress, poolIdLeaving uint64, lock *lockuptypes.PeriodLock, sharesToMigrate sdk.Coin, tokenOutMins sdk.Coins, remainingLockTime time.Duration) (exitCoins sdk.Coins, err error) {
-	return k.validateSharesToMigrateUnlockAndExitBalancerPool(ctx, sender, poolIdLeaving, lock, sharesToMigrate, tokenOutMins, remainingLockTime)
+func (k Keeper) ValidateSharesToMigrateUnlockAndExitBalancerPool(ctx sdk.Context, sender sdk.AccAddress, poolIdLeaving uint64, lock *lockuptypes.PeriodLock, sharesToMigrate sdk.Coin, tokenOutMins sdk.Coins) (exitCoins sdk.Coins, err error) {
+	return k.validateSharesToMigrateUnlockAndExitBalancerPool(ctx, sender, poolIdLeaving, lock, sharesToMigrate, tokenOutMins)
 }
 
 func (k Keeper) RouteMigration(ctx sdk.Context, sender sdk.AccAddress, lockId uint64, sharesToMigrate sdk.Coin) (synthLockBeforeMigration lockuptypes.SyntheticLock, migrationType MigrationType, err error) {

diff --git a/x/superfluid/keeper/migrate.go b/x/superfluid/keeper/migrate.go
@@ -24,9 +24,23 @@ const (
 )
 
 // RouteLockedBalancerToConcentratedMigration routes the provided lock to the proper migration function based on the lock status.
-// If the lock is superfluid delegated, it will instantly undelegate the superfluid position and redelegate it as a concentrated liquidity position.
-// If the lock is superfluid undelegating, it will instantly undelegate the superfluid position and redelegate it as a concentrated liquidity position, but continue to unlock where it left off.
-// If the lock is locked or unlocking but not superfluid delegated/undelegating, it will migrate the position and either start unlocking or continue unlocking where it left off.
+// The testing conditions and scope for the different lock status are as follows: 
+// Lock Status = Superfluid delegated
+// - Instantly undelegate which will bypass unbonding time.
+// - Create new CL Lock and Re-delegate it as a concentrated liquidity position.
+//
+// Lock Status = Superfluid undelegating
+// - Continue undelegating as superfluid unbonding CL Position.
+// - Lock the tokens and create an unlocking syntheticLock (to handle cases of slashing)
+//
+// Lock Status = Locked or unlocking (no superfluid delegation/undelegation)
+// - Force unlock tokens from gamm shares.
+// - Create new CL lock and starts unlocking or unlocking where it left off.
+//
+// Lock Status = Unlocked
+// - For ex: LP shares
+// - Create new CL lock and starts unlocking or unlocking where it left off.
+//
 // Errors if the lock is not found, if the lock is not a balancer pool lock, or if the lock is not owned by the sender.
 func (k Keeper) RouteLockedBalancerToConcentratedMigration(ctx sdk.Context, sender sdk.AccAddress, lockId uint64, sharesToMigrate sdk.Coin, tokenOutMins sdk.Coins) (positionId uint64, amount0, amount1 sdk.Int, liquidity sdk.Dec, poolIdLeaving, poolIdEntering, concentratedLockId uint64, err error) {
 	synthLockBeforeMigration, migrationType, err := k.routeMigration(ctx, sender, lockId, sharesToMigrate)
@@ -81,7 +95,7 @@ func (k Keeper) migrateSuperfluidBondedBalancerToConcentrated(ctx sdk.Context,
 	intermediateAccount := types.SuperfluidIntermediaryAccount{}
 	var gammLockToMigrate *lockuptypes.PeriodLock
 	if isPartialMigration {
-		// Note that lock's id is different from the originalLockId sinec it was split.
+		// Note that lock's id is different from the originalLockId since it was split.
 		// The original lock id stays in gamm.
 		intermediateAccount, gammLockToMigrate, err = k.partialSuperfluidUndelegateToConcentratedPosition(ctx, sender.String(), originalLockId, sharesToMigrate)
 		if err != nil {
@@ -100,7 +114,7 @@ func (k Keeper) migrateSuperfluidBondedBalancerToConcentrated(ctx sdk.Context,
 	// Force unlock, validate the provided sharesToMigrate, and exit the balancer pool.
 	// This will return the coins that will be used to create the concentrated liquidity position.
 	// It also returns the lock object that contains the remaining shares that were not used in this migration.
-	exitCoins, err := k.validateSharesToMigrateUnlockAndExitBalancerPool(ctx, sender, poolIdLeaving, gammLockToMigrate, sharesToMigrate, tokenOutMins, remainingLockTime)
+	exitCoins, err := k.validateSharesToMigrateUnlockAndExitBalancerPool(ctx, sender, poolIdLeaving, gammLockToMigrate, sharesToMigrate, tokenOutMins)
 	if err != nil {
 		return 0, sdk.Int{}, sdk.Int{}, sdk.Dec{}, 0, 0, 0, err
 	}
@@ -110,6 +124,7 @@ func (k Keeper) migrateSuperfluidBondedBalancerToConcentrated(ctx sdk.Context,
 	if err != nil {
 		return 0, sdk.Int{}, sdk.Int{}, sdk.Dec{}, 0, 0, 0, err
 	}
+
 	err = k.SuperfluidDelegate(ctx, sender.String(), concentratedLockId, intermediateAccount.ValAddr)
 	if err != nil {
 		return 0, sdk.Int{}, sdk.Int{}, sdk.Dec{}, 0, 0, 0, err
@@ -145,13 +160,12 @@ func (k Keeper) migrateSuperfluidUnbondingBalancerToConcentrated(ctx sdk.Context
 	// Force unlock, validate the provided sharesToMigrate, and exit the balancer pool.
 	// This will return the coins that will be used to create the concentrated liquidity position.
 	// It also returns the lock object that contains the remaining shares that were not used in this migration.
-	exitCoins, err := k.validateSharesToMigrateUnlockAndExitBalancerPool(ctx, sender, poolIdLeaving, preMigrationLock, sharesToMigrate, tokenOutMins, remainingLockTime)
+	exitCoins, err := k.validateSharesToMigrateUnlockAndExitBalancerPool(ctx, sender, poolIdLeaving, preMigrationLock, sharesToMigrate, tokenOutMins)
 	if err != nil {
 		return 0, sdk.Int{}, sdk.Int{}, sdk.Dec{}, 0, 0, 0, err
 	}
 
 	// Create a full range (min to max tick) concentrated liquidity position.
-	// If the lock was unlocking, we create a new lock that is unlocking for the remaining time of the old lock.
 	positionId, amount0, amount1, liquidity, concentratedLockId, err = k.clk.CreateFullRangePositionUnlocking(ctx, poolIdEntering, sender, exitCoins, remainingLockTime)
 	if err != nil {
 		return 0, sdk.Int{}, sdk.Int{}, sdk.Dec{}, 0, 0, 0, err
@@ -166,6 +180,7 @@ func (k Keeper) migrateSuperfluidUnbondingBalancerToConcentrated(ctx sdk.Context
 		return 0, sdk.Int{}, sdk.Int{}, sdk.Dec{}, 0, 0, 0, err
 	}
 
+	// Synthetic lock is created to indicate unbonding position. The synthetic lock will be in unbonding period for remainingLockTime.
 	// Create a new synthetic lockup for the new intermediary account in an unlocking status for the remaining duration.
 	err = k.createSyntheticLockupWithDuration(ctx, concentratedLockId, clIntermediateAccount, remainingLockTime, unlockingStatus)
 	if err != nil {
@@ -193,7 +208,7 @@ func (k Keeper) migrateNonSuperfluidLockBalancerToConcentrated(ctx sdk.Context,
 	// Force unlock, validate the provided sharesToMigrate, and exit the balancer pool.
 	// This will return the coins that will be used to create the concentrated liquidity position.
 	// It also returns the lock object that contains the remaining shares that were not used in this migration.
-	exitCoins, err := k.validateSharesToMigrateUnlockAndExitBalancerPool(ctx, sender, poolIdLeaving, preMigrationLock, sharesToMigrate, tokenOutMins, remainingLockTime)
+	exitCoins, err := k.validateSharesToMigrateUnlockAndExitBalancerPool(ctx, sender, poolIdLeaving, preMigrationLock, sharesToMigrate, tokenOutMins)
 	if err != nil {
 		return 0, sdk.Int{}, sdk.Int{}, sdk.Dec{}, 0, 0, 0, err
 	}
@@ -289,7 +304,7 @@ func (k Keeper) validateMigration(ctx sdk.Context, sender sdk.AccAddress, lockId
 // 4. Exits the position in the Balancer pool.
 // 5. Ensures that exactly two coins are returned.
 // 6. Any remaining shares that were not migrated are re-locked as a new lock for the remaining time on the lock.
-func (k Keeper) validateSharesToMigrateUnlockAndExitBalancerPool(ctx sdk.Context, sender sdk.AccAddress, poolIdLeaving uint64, lock *lockuptypes.PeriodLock, sharesToMigrate sdk.Coin, tokenOutMins sdk.Coins, remainingLockTime time.Duration) (exitCoins sdk.Coins, err error) {
+func (k Keeper) validateSharesToMigrateUnlockAndExitBalancerPool(ctx sdk.Context, sender sdk.AccAddress, poolIdLeaving uint64, lock *lockuptypes.PeriodLock, sharesToMigrate sdk.Coin, tokenOutMins sdk.Coins) (exitCoins sdk.Coins, err error) {
 	// validateMigration ensures that the preMigrationLock contains coins of length 1.
 	gammSharesInLock := lock.Coins[0]
 

diff --git a/x/superfluid/keeper/migrate_test.go b/x/superfluid/keeper/migrate_test.go
@@ -420,6 +420,7 @@ func (s *KeeperTestSuite) TestMigrateSuperfluidBondedBalancerToConcentrated() {
 			// Check newly created concentrated lock.
 			concentratedLock, err := lockupKeeper.GetLockByID(s.Ctx, concentratedLockId)
 			s.Require().NoError(err)
+
 			s.Require().Equal(liquidityMigrated.TruncateInt().String(), concentratedLock.Coins[0].Amount.String(), "expected %s shares, found %s shares", coinsToMigrate.Amount.String(), concentratedLock.Coins[0].Amount.String())
 			s.Require().Equal(balancerLock.Duration, concentratedLock.Duration)
 			s.Require().Equal(balancerLock.EndTime, concentratedLock.EndTime)
@@ -976,7 +977,7 @@ func (s *KeeperTestSuite) TestValidateSharesToMigrateUnlockAndExitBalancerPool()
 			}
 
 			// System under test
-			exitCoins, err := superfluidKeeper.ValidateSharesToMigrateUnlockAndExitBalancerPool(ctx, poolJoinAcc, balancerPooId, lock, coinsToMigrate, tc.tokenOutMins, lock.Duration)
+			exitCoins, err := superfluidKeeper.ValidateSharesToMigrateUnlockAndExitBalancerPool(ctx, poolJoinAcc, balancerPooId, lock, coinsToMigrate, tc.tokenOutMins)
 			if tc.expectedError != nil {
 				s.Require().Error(err)
 				s.Require().ErrorContains(err, tc.expectedError.Error())