From db2f738134de83ea68671af3e09ea4ee6bbaaa6f Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Sun, 2 Jul 2023 18:27:55 -0400 Subject: [PATCH] [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 <45252226+mattverse@users.noreply.github.com> --------- Co-authored-by: Adam Tucker Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com> (cherry picked from commit a923e6c6f898c5401c35727fbf3990790fa1c7ba) Co-authored-by: Sishir Giri --- x/concentrated-liquidity/position.go | 4 +-- x/concentrated-liquidity/position_test.go | 13 +++++---- x/superfluid/keeper/export_test.go | 4 +-- x/superfluid/keeper/migrate.go | 33 ++++++++++++++++------- x/superfluid/keeper/migrate_test.go | 3 ++- 5 files changed, 38 insertions(+), 19 deletions(-) diff --git a/x/concentrated-liquidity/position.go b/x/concentrated-liquidity/position.go index 7e392a4e6eb..41ae80f6688 100644 --- 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 index 874cb658724..edfa8e72296 100644 --- 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 index 23861856c6b..12c5d26ae97 100644 --- 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 index f7c58721403..069206090bd 100644 --- 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 index 8b882b2eefb..94e5cd31492 100644 --- 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())