From a593f6aea6a1e8612f376dc58debfea2a692fbe7 Mon Sep 17 00:00:00 2001 From: Roman Date: Fri, 30 Jun 2023 06:28:39 -0400 Subject: [PATCH] refactor/fix: swap step iteration limit (#5647) * repro infinite loop in swap logic * refactor/fix: swap step iteration limit * Update x/concentrated-liquidity/swaps_test.go Co-authored-by: Adam Tucker * swap limit to 100 * clearer comments * Update x/concentrated-liquidity/swaps.go Co-authored-by: alpo <62043214+AlpinYukseloglu@users.noreply.github.com> * Update x/concentrated-liquidity/types/errors.go Co-authored-by: alpo <62043214+AlpinYukseloglu@users.noreply.github.com> --------- Co-authored-by: alpo Co-authored-by: Adam Tucker Co-authored-by: alpo <62043214+AlpinYukseloglu@users.noreply.github.com> --- x/concentrated-liquidity/swaps.go | 33 ++++++++++++++++++++++-- x/concentrated-liquidity/swaps_test.go | 12 ++++----- x/concentrated-liquidity/types/errors.go | 9 +++++++ 3 files changed, 46 insertions(+), 8 deletions(-) diff --git a/x/concentrated-liquidity/swaps.go b/x/concentrated-liquidity/swaps.go index fc2a6004fb5..82804c0511f 100644 --- a/x/concentrated-liquidity/swaps.go +++ b/x/concentrated-liquidity/swaps.go @@ -60,6 +60,14 @@ type SwapState struct { swapStrategy swapstrategy.SwapStrategy } +// swapNoProgressLimit is the maximum number of iterations that can be performed +// without progressing the swap state. If this limit is reached, the swap is +// considered to have failed. +// Note, the value is chosen arbitrarily. +// From tests, there should be no reason for a swap to make more than 2 iterations without +// progress. However, we leave a buffer of 1_000 to account for any unforeseen edge cases. +const swapNoProgressLimit = 100 + func newSwapState(specifiedAmount sdk.Int, p types.ConcentratedPoolExtension, strategy swapstrategy.SwapStrategy) SwapState { return SwapState{ amountSpecifiedRemaining: specifiedAmount.ToDec(), @@ -314,6 +322,7 @@ func (k Keeper) computeOutAmtGivenIn( defer nextInitTickIter.Close() // Iterate and update swapState until we swap all tokenIn or we reach the specific sqrtPriceLimit + swapNoProgressIterationCount := 0 // TODO: for now, we check if amountSpecifiedRemaining is GT 0.0000001. This is because there are times when the remaining // amount may be extremely small, and that small amount cannot generate and amountIn/amountOut and we are therefore left // in an infinite loop. @@ -383,6 +392,15 @@ func (k Keeper) computeOutAmtGivenIn( } swapState.tick = newTick } + + // If nothing was consumed from swapState.amountSpecifiedRemaining, we increment the swapNoProgressIterationCount. + // See definition of swapNoProgressLimit for more details. + if amountIn.IsZero() { + if swapNoProgressIterationCount >= swapNoProgressLimit { + return sdk.Coin{}, sdk.Coin{}, PoolUpdates{}, sdk.Dec{}, types.SwapNoProgressError{PoolId: poolId, UserProvidedCoin: tokenInMin} + } + swapNoProgressIterationCount++ + } } // Add spread reward growth per share to the pool-global spread reward accumulator. @@ -442,8 +460,10 @@ func (k Keeper) computeInAmtGivenOut( return sdk.Coin{}, sdk.Coin{}, PoolUpdates{}, sdk.Dec{}, err } - // TODO: This should be GT 0 but some instances have very small remainder - // need to look into fixing this + swapNoProgressIterationCount := 0 + // TODO: for now, we check if amountSpecifiedRemaining is GT 10^-18. This is because there are times when the remaining + // amount may be extremely small, and that small amount cannot generate and amountIn/amountOut and we are therefore left + // in an infinite loop. for swapState.amountSpecifiedRemaining.GT(smallestDec) && !swapState.sqrtPrice.Equal(sqrtPriceLimit) { // log the sqrtPrice we start the iteration with sqrtPriceStart := swapState.sqrtPrice @@ -506,6 +526,15 @@ func (k Keeper) computeInAmtGivenOut( return sdk.Coin{}, sdk.Coin{}, PoolUpdates{}, sdk.Dec{}, err } } + + // If nothing was consumed from swapState.amountSpecifiedRemaining, we increment the swapNoProgressIterationCount. + // See definition of swapNoProgressLimit for more details. + if amountOut.IsZero() { + if swapNoProgressIterationCount >= swapNoProgressLimit { + return sdk.Coin{}, sdk.Coin{}, PoolUpdates{}, sdk.Dec{}, types.SwapNoProgressError{PoolId: poolId, UserProvidedCoin: desiredTokenOut} + } + swapNoProgressIterationCount++ + } } // Add spread reward growth per share to the pool-global spread reward accumulator. diff --git a/x/concentrated-liquidity/swaps_test.go b/x/concentrated-liquidity/swaps_test.go index ff63dc6bd98..6abcd6efd85 100644 --- a/x/concentrated-liquidity/swaps_test.go +++ b/x/concentrated-liquidity/swaps_test.go @@ -3058,8 +3058,9 @@ func (s *KeeperTestSuite) TestFunctionalSwaps() { s.Require().Equal(0, multiplicativeTolerance.Compare(expectedTokenOut, totalTokenOut.Amount)) } -// TestInfiniteSwapLoop demonstrates a case where an infinite loop can be triggered in swap logic. -func (s *KeeperTestSuite) TestInfiniteSwapLoop() { +// TestInfiniteSwapLoop_OutGivenIn demonstrates a case where an infinite loop can be triggered in swap logic if no +// swap limit and other constraints are applied. +func (s *KeeperTestSuite) TestInfiniteSwapLoop_OutGivenIn() { s.SetupTest() pool := s.PrepareConcentratedPool() @@ -3077,10 +3078,9 @@ func (s *KeeperTestSuite) TestInfiniteSwapLoop() { swapUSDCFunded := sdk.NewCoin(USDC, sdk.Int(sdk.MustNewDecFromStr("10000"))) s.FundAcc(swapAddress, sdk.NewCoins(swapEthFunded, swapUSDCFunded)) _, tokenOut, _, err := s.clk.SwapInAmtGivenOut(s.Ctx, swapAddress, pool, sdk.NewCoin(USDC, sdk.NewInt(10000)), ETH, pool.GetSpreadFactor(s.Ctx), sdk.ZeroDec()) - fmt.Println("Token swapped out: ", tokenOut) // Swap back in the amount that was swapped out to test the inverse relationship - // This line is commented out as it triggers an infinite loop. - // _, _, _, err = s.clk.SwapOutAmtGivenIn(s.Ctx, swapAddress, pool, tokenOut, ETH, pool.GetSpreadFactor(s.Ctx), sdk.ZeroDec()) - s.Require().NoError(err) + _, _, _, err = s.clk.SwapOutAmtGivenIn(s.Ctx, swapAddress, pool, tokenOut, ETH, pool.GetSpreadFactor(s.Ctx), sdk.ZeroDec()) + s.Require().Error(err) + s.Require().ErrorIs(err, types.SwapNoProgressError{PoolId: pool.GetId(), UserProvidedCoin: tokenOut}) } diff --git a/x/concentrated-liquidity/types/errors.go b/x/concentrated-liquidity/types/errors.go index 78c0e1ac094..84be8200ced 100644 --- a/x/concentrated-liquidity/types/errors.go +++ b/x/concentrated-liquidity/types/errors.go @@ -840,3 +840,12 @@ type TickToSqrtPriceConversionError struct { func (e TickToSqrtPriceConversionError) Error() string { return fmt.Sprintf("could not convert next tick to nextSqrtPrice (%v)", e.NextTick) } + +type SwapNoProgressError struct { + PoolId uint64 + UserProvidedCoin sdk.Coin +} + +func (e SwapNoProgressError) Error() string { + return fmt.Sprintf("ran out of iterations during swap. Possibly entered an infinite loop. Pool id (%d), user provided coin (%s)", e.PoolId, e.UserProvidedCoin) +}