x/gamm: Catch overflow panics on CalculateSpotPrice (#2405)

* add overflow tests * add comments for overflowing test * add non-extreme unequal weight case * add overflow tests and panic recover to spot price * add max spot price check and tests for it, and aclean up errors * reinstate error types
osmosis-labs · Aug 25, 2022 · bd683a3 · bd683a3
1 parent 16e3e4b
commit bd683a3
Show file tree

Hide file tree

Showing 3 changed files with 120 additions and 3 deletions.
diff --git a/x/gamm/pool-models/balancer/pool.go b/x/gamm/pool-models/balancer/pool.go
@@ -609,7 +609,7 @@ func (p *Pool) applySwap(ctx sdk.Context, tokensIn sdk.Coins, tokensOut sdk.Coin
 //
 // panics if the pool in state is incorrect, and has any weight that is 0.
 // TODO: Come back and improve docs for this.
-func (p Pool) SpotPrice(ctx sdk.Context, baseAsset, quoteAsset string) (sdk.Dec, error) {
+func (p Pool) SpotPrice(ctx sdk.Context, baseAsset, quoteAsset string) (spotPrice sdk.Dec, err error) {
 	quote, base, err := p.parsePoolAssetsByDenoms(quoteAsset, baseAsset)
 	if err != nil {
 		return sdk.Dec{}, err
@@ -618,14 +618,27 @@ func (p Pool) SpotPrice(ctx sdk.Context, baseAsset, quoteAsset string) (sdk.Dec,
 		return sdk.Dec{}, errors.New("pool is misconfigured, got 0 weight")
 	}
 
+	defer func() {
+		// defer function to escape the panic when spot price overflows
+		if r := recover(); r != nil {
+			spotPrice = sdk.Dec{}
+			err = types.ErrSpotPriceInternal
+		}
+	}()
+
 	// spot_price = (Base_supply / Weight_base) / (Quote_supply / Weight_quote)
 	// spot_price = (weight_quote / weight_base) * (base_supply / quote_supply)
 	invWeightRatio := quote.Weight.ToDec().Quo(base.Weight.ToDec())
 	supplyRatio := base.Token.Amount.ToDec().Quo(quote.Token.Amount.ToDec())
 	fullRatio := supplyRatio.Mul(invWeightRatio)
 	// we want to round this to `SigFigs` of precision
-	ratio := osmomath.SigFigRound(fullRatio, types.SigFigs)
-	return ratio, nil
+	spotPrice = osmomath.SigFigRound(fullRatio, types.SigFigs)
+	if spotPrice.GT(sdk.NewDec(2).Power(160)) {
+		spotPrice = sdk.Dec{}
+		err = types.ErrSpotPriceOverflow
+	}
+
+	return spotPrice, err
 }
 
 // calcPoolOutGivenSingleIn - balance pAo.

diff --git a/x/gamm/pool-models/balancer/pool_suite_test.go b/x/gamm/pool-models/balancer/pool_suite_test.go
@@ -528,6 +528,108 @@ func (suite *KeeperTestSuite) TestBalancerSpotPrice() {
 	}
 }
 
+// This test sets up 2 asset pools, and then checks the spot price on them.
+// It uses the pools spot price method, rather than the Gamm keepers spot price method.
+func (suite *KeeperTestSuite) TestBalancerSpotPriceBounds() {
+	baseDenom := "uosmo"
+	quoteDenom := "uion"
+	defaultFutureGovernor = ""
+
+	tests := []struct {
+		name                string
+		baseDenomPoolInput  sdk.Coin
+		baseDenomWeight     sdk.Int
+		quoteDenomPoolInput sdk.Coin
+		quoteDenomWeight    sdk.Int
+		expectError         bool
+		expectedOutput      sdk.Dec
+	}{
+		{
+			name: "spot price check at max bitlen supply",
+			// 2^196, as >= 2^197 trips max bitlen of 256
+			baseDenomPoolInput:  sdk.NewCoin(baseDenom, sdk.MustNewDecFromStr("100433627766186892221372630771322662657637687111424552206336").TruncateInt()),
+			baseDenomWeight:     sdk.NewInt(100),
+			quoteDenomPoolInput: sdk.NewCoin(quoteDenom, sdk.MustNewDecFromStr("100433627766186892221372630771322662657637687111424552206337").TruncateInt()),
+			quoteDenomWeight:    sdk.NewInt(100),
+			expectError:         false,
+			expectedOutput:      sdk.MustNewDecFromStr("1.000000000000000000"),
+		},
+		{
+			name:                "spot price check at min supply",
+			baseDenomPoolInput:  sdk.NewCoin(baseDenom, sdk.OneInt()),
+			baseDenomWeight:     sdk.NewInt(100),
+			quoteDenomPoolInput: sdk.NewCoin(quoteDenom, sdk.OneInt()),
+			quoteDenomWeight:    sdk.NewInt(100),
+			expectError:         false,
+			expectedOutput:      sdk.MustNewDecFromStr("1.000000000000000000"),
+		},
+		{
+			name:                "max spot price with equal weights",
+			baseDenomPoolInput:  sdk.NewCoin(baseDenom, sdk.NewDec(2).Power(160).TruncateInt()),
+			baseDenomWeight:     sdk.NewInt(100),
+			quoteDenomPoolInput: sdk.NewCoin(quoteDenom, sdk.OneInt()),
+			quoteDenomWeight:    sdk.NewInt(100),
+			expectError:         false,
+			expectedOutput:      sdk.NewDec(2).Power(160),
+		},
+		{
+			// test int overflows
+			name: "max spot price with extreme weights",
+			// 2^196, as >= 2^197 trips max bitlen of 256
+			baseDenomPoolInput:  sdk.NewCoin(baseDenom, sdk.MustNewDecFromStr("100433627766186892221372630771322662657637687111424552206336").TruncateInt()),
+			baseDenomWeight:     sdk.OneInt(),
+			quoteDenomPoolInput: sdk.NewCoin(quoteDenom, sdk.OneInt()),
+			quoteDenomWeight:    sdk.MustNewDecFromStr("100433627766186892221372630771322662657637687111424552206336").TruncateInt(),
+			expectError:         true,
+		},
+		{
+			name: "greater than max spot price with equal weights",
+			// Max spot price capped at 2^160
+			baseDenomPoolInput:  sdk.NewCoin(baseDenom, sdk.NewDec(2).Power(161).TruncateInt()),
+			baseDenomWeight:     sdk.NewInt(100),
+			quoteDenomPoolInput: sdk.NewCoin(quoteDenom, sdk.OneInt()),
+			quoteDenomWeight:    sdk.NewInt(100),
+			expectError:         true,
+		},
+	}
+
+	for _, tc := range tests {
+		suite.SetupTest()
+		suite.Run(tc.name, func() {
+			// pool assets
+			defaultBaseAsset := balancer.PoolAsset{
+				Weight: tc.baseDenomWeight,
+				Token:  tc.baseDenomPoolInput,
+			}
+			defaultQuoteAsset := balancer.PoolAsset{
+				Weight: tc.quoteDenomWeight,
+				Token:  tc.quoteDenomPoolInput,
+			}
+
+			poolAssets := []balancer.PoolAsset{defaultBaseAsset, defaultQuoteAsset}
+
+			pool, err := balancer.NewBalancerPool(defaultPoolId, defaultBalancerPoolParams, poolAssets, defaultFutureGovernor, defaultCurBlockTime)
+			suite.Require().NoError(err, "test: %s", tc.name)
+
+			sut := func() {
+				spotPrice, err := pool.SpotPrice(
+					suite.Ctx,
+					tc.baseDenomPoolInput.Denom,
+					tc.quoteDenomPoolInput.Denom)
+				if tc.expectError {
+					suite.Require().Error(err, "test: %s", tc.name)
+				} else {
+					suite.Require().NoError(err, "test: %s", tc.name)
+					suite.Require().True(spotPrice.Equal(tc.expectedOutput),
+						"test: %s\nSpot price wrong, got %s, expected %s\n", tc.name,
+						spotPrice, tc.expectedOutput)
+				}
+			}
+			assertPoolStateNotModified(suite.T(), &pool, sut)
+		})
+	}
+}
+
 func (suite *KeeperTestSuite) TestCalcJoinPoolShares() {
 	// We append shared calcSingleAssetJoinTestCases with multi-asset and edge
 	// test cases.

diff --git a/x/gamm/types/errors.go b/x/gamm/types/errors.go
@@ -28,6 +28,8 @@ var (
 	ErrNotPositiveCriteria      = sdkerrors.Register(ModuleName, 29, "min out amount or max in amount should be positive")
 	ErrNotPositiveRequireAmount = sdkerrors.Register(ModuleName, 30, "required amount should be positive")
 	ErrTooManyTokensOut         = sdkerrors.Register(ModuleName, 31, "tx is trying to get more tokens out of the pool than exist")
+	ErrSpotPriceOverflow        = sdkerrors.Register(ModuleName, 32, "invalid spot price (overflowed)")
+	ErrSpotPriceInternal        = sdkerrors.Register(ModuleName, 33, "internal spot price error")
 
 	ErrPoolParamsInvalidDenom     = sdkerrors.Register(ModuleName, 50, "pool params' LBP params has an invalid denomination")
 	ErrPoolParamsInvalidNumDenoms = sdkerrors.Register(ModuleName, 51, "pool params' LBP doesn't have same number of params as underlying pool")