From dba0541f08f969552ce17e8a1286ed8b09544487 Mon Sep 17 00:00:00 2001 From: alpo Date: Sat, 10 Sep 2022 14:04:22 -0700 Subject: [PATCH 1/2] add checks for invalid cfmm inputs --- x/gamm/pool-models/stableswap/amm.go | 24 +++- x/gamm/pool-models/stableswap/amm_test.go | 128 ++++++++++++++++++---- 2 files changed, 125 insertions(+), 27 deletions(-) diff --git a/x/gamm/pool-models/stableswap/amm.go b/x/gamm/pool-models/stableswap/amm.go index 21ef11b00e3..30c92620841 100644 --- a/x/gamm/pool-models/stableswap/amm.go +++ b/x/gamm/pool-models/stableswap/amm.go @@ -16,6 +16,9 @@ var ( // solidly CFMM is xy(x^2 + y^2) = k func cfmmConstant(xReserve, yReserve sdk.Dec) sdk.Dec { + if !xReserve.IsPositive() || !yReserve.IsPositive() { + panic("invalid input: reserves must be positive") + } xy := xReserve.Mul(yReserve) x2 := xReserve.Mul(xReserve) y2 := yReserve.Mul(yReserve) @@ -28,6 +31,10 @@ func cfmmConstant(xReserve, yReserve sdk.Dec) sdk.Dec { // of their squares (e.g. v = w^2 + z^2). // When u = 1 and v = 0, this is equivalent to solidly's CFMM func cfmmConstantMulti(xReserve, yReserve, uReserve, vSumSquares sdk.Dec) sdk.Dec { + if !xReserve.IsPositive() || !yReserve.IsPositive() || !uReserve.IsPositive() || vSumSquares.IsNegative() { + panic("invalid input: reserves must be positive") + } + xyu := xReserve.Mul(yReserve.Mul(uReserve)) x2 := xReserve.Mul(xReserve) y2 := yReserve.Mul(yReserve) @@ -40,8 +47,8 @@ func cfmmConstantMulti(xReserve, yReserve, uReserve, vSumSquares sdk.Dec) sdk.De // So we solve the following expression for `a` // xy(x^2 + y^2) = (x - a)(y + b)((x - a)^2 + (y + b)^2) func solveCfmm(xReserve, yReserve, yIn sdk.Dec) sdk.Dec { - if !yReserve.Add(yIn).IsPositive() { - panic("invalid yReserve, yIn combo") + if !xReserve.IsPositive() || !yReserve.IsPositive() || !yIn.IsPositive() { + panic("invalid input: reserves and input must be positive") } // use the following wolfram alpha link to solve the equation @@ -151,6 +158,11 @@ func solveCfmm(xReserve, yReserve, yIn sdk.Dec) sdk.Dec { term3 := term3Numerator.Quo(bpy) a := term1.Sub(term2).Add(term3) + + if a.GTE(xReserve) { + panic("invalid output: greater than full pool reserves") + } + return a } @@ -160,8 +172,8 @@ func solveCfmm(xReserve, yReserve, yIn sdk.Dec) sdk.Dec { // So we solve the following expression for `a` // xyz(x^2 + y^2 + w) = (x - a)(y + b)z((x - a)^2 + (y + b)^2 + w) func solveCfmmMulti(xReserve, yReserve, wSumSquares, yIn sdk.Dec) sdk.Dec { - if !yReserve.Add(yIn).IsPositive() { - panic("invalid yReserve, yIn combo") + if !xReserve.IsPositive() || !yReserve.IsPositive() || !yIn.IsPositive() { + panic("invalid input: reserves and input must be positive") } // Use the following wolfram alpha link to solve the equation @@ -256,6 +268,10 @@ func solveCfmmMulti(xReserve, yReserve, wSumSquares, yIn sdk.Dec) sdk.Dec { a := term1.Sub(term2).Add(term3) + if a.GTE(xReserve) { + panic("invalid output: greater than full pool reserves") + } + return a } diff --git a/x/gamm/pool-models/stableswap/amm_test.go b/x/gamm/pool-models/stableswap/amm_test.go index df70b1e44dd..2893ac47217 100644 --- a/x/gamm/pool-models/stableswap/amm_test.go +++ b/x/gamm/pool-models/stableswap/amm_test.go @@ -19,42 +19,70 @@ func TestCFMMInvariantTwoAssets(t *testing.T) { kErrTolerance := sdk.OneDec() tests := []struct { - xReserve sdk.Dec - yReserve sdk.Dec - yIn sdk.Dec + xReserve sdk.Dec + yReserve sdk.Dec + yIn sdk.Dec + expectPanic bool }{ { sdk.NewDec(100), sdk.NewDec(100), sdk.NewDec(1), + false, }, { sdk.NewDec(100), sdk.NewDec(100), sdk.NewDec(1000), + false, }, // { // sdk.NewDec(100000), // sdk.NewDec(100000), // sdk.NewDec(10000), // }, + + // panic catching + { // xReserve negative + sdk.NewDec(-100), + sdk.NewDec(100), + sdk.NewDec(1), + true, + }, + { // yReserve negative + sdk.NewDec(100), + sdk.NewDec(-100), + sdk.NewDec(1), + true, + }, + { // yIn negative + sdk.NewDec(100), + sdk.NewDec(100), + sdk.NewDec(-1), + true, + }, } for _, test := range tests { - // using two-asset cfmm - k0 := cfmmConstant(test.xReserve, test.yReserve) - xOut := solveCfmm(test.xReserve, test.yReserve, test.yIn) - - k1 := cfmmConstant(test.xReserve.Sub(xOut), test.yReserve.Add(test.yIn)) - osmoassert.DecApproxEq(t, k0, k1, kErrTolerance) - - // using multi-asset cfmm (should be equivalent with u = 1, w = 0) - k2 := cfmmConstantMulti(test.xReserve, test.yReserve, sdk.OneDec(), sdk.ZeroDec()) - osmoassert.DecApproxEq(t, k2, k0, kErrTolerance) - xOut2 := solveCfmmMulti(test.xReserve, test.yReserve, sdk.ZeroDec(), test.yIn) - fmt.Println(xOut2) - k3 := cfmmConstantMulti(test.xReserve.Sub(xOut2), test.yReserve.Add(test.yIn), sdk.OneDec(), sdk.ZeroDec()) - osmoassert.DecApproxEq(t, k2, k3, kErrTolerance) + // system under test + sut := func() { + // using two-asset cfmm + k0 := cfmmConstant(test.xReserve, test.yReserve) + xOut := solveCfmm(test.xReserve, test.yReserve, test.yIn) + + k1 := cfmmConstant(test.xReserve.Sub(xOut), test.yReserve.Add(test.yIn)) + osmoassert.DecApproxEq(t, k0, k1, kErrTolerance) + + // using multi-asset cfmm (should be equivalent with u = 1, w = 0) + k2 := cfmmConstantMulti(test.xReserve, test.yReserve, sdk.OneDec(), sdk.ZeroDec()) + osmoassert.DecApproxEq(t, k2, k0, kErrTolerance) + xOut2 := solveCfmmMulti(test.xReserve, test.yReserve, sdk.ZeroDec(), test.yIn) + fmt.Println(xOut2) + k3 := cfmmConstantMulti(test.xReserve.Sub(xOut2), test.yReserve.Add(test.yIn), sdk.OneDec(), sdk.ZeroDec()) + osmoassert.DecApproxEq(t, k2, k3, kErrTolerance) + } + + osmoassert.ConditionalPanic(t, test.expectPanic, sut) } } @@ -67,6 +95,7 @@ func TestCFMMInvariantMultiAssets(t *testing.T) { uReserve sdk.Dec wSumSquares sdk.Dec yIn sdk.Dec + expectPanic bool }{ { sdk.NewDec(100), @@ -75,6 +104,7 @@ func TestCFMMInvariantMultiAssets(t *testing.T) { sdk.NewDec(200), sdk.NewDec(20000), sdk.NewDec(1), + false, }, { sdk.NewDec(100), @@ -82,21 +112,73 @@ func TestCFMMInvariantMultiAssets(t *testing.T) { sdk.NewDec(200), sdk.NewDec(20000), sdk.NewDec(1000), + false, }, // { // sdk.NewDec(100000), // sdk.NewDec(100000), // sdk.NewDec(10000), // }, + + // panic catching + { // negative xReserve + sdk.NewDec(-100), + sdk.NewDec(100), + // represents a 4-asset pool with 100 in each reserve + sdk.NewDec(200), + sdk.NewDec(20000), + sdk.NewDec(1), + true, + }, + { // negative yReserve + sdk.NewDec(100), + sdk.NewDec(-100), + // represents a 4-asset pool with 100 in each reserve + sdk.NewDec(200), + sdk.NewDec(20000), + sdk.NewDec(1), + true, + }, + { // negative uReserve + sdk.NewDec(100), + sdk.NewDec(100), + // represents a 4-asset pool with 100 in each reserve + sdk.NewDec(-200), + sdk.NewDec(20000), + sdk.NewDec(1), + true, + }, + { // negative sumSquares + sdk.NewDec(100), + sdk.NewDec(100), + // represents a 4-asset pool with 100 in each reserve + sdk.NewDec(200), + sdk.NewDec(-20000), + sdk.NewDec(1), + true, + }, + { // negative yIn + sdk.NewDec(100), + sdk.NewDec(100), + // represents a 4-asset pool with 100 in each reserve + sdk.NewDec(200), + sdk.NewDec(-20000), + sdk.NewDec(1), + true, + }, } for _, test := range tests { - // using multi-asset cfmm - k2 := cfmmConstantMulti(test.xReserve, test.yReserve, test.uReserve, test.wSumSquares) - xOut2 := solveCfmmMulti(test.xReserve, test.yReserve, test.wSumSquares, test.yIn) - fmt.Println(xOut2) - k3 := cfmmConstantMulti(test.xReserve.Sub(xOut2), test.yReserve.Add(test.yIn), test.uReserve, test.wSumSquares) - osmoassert.DecApproxEq(t, k2, k3, kErrTolerance) + // system under test + sut := func() { + // using multi-asset cfmm + k2 := cfmmConstantMulti(test.xReserve, test.yReserve, test.uReserve, test.wSumSquares) + xOut2 := solveCfmmMulti(test.xReserve, test.yReserve, test.wSumSquares, test.yIn) + k3 := cfmmConstantMulti(test.xReserve.Sub(xOut2), test.yReserve.Add(test.yIn), test.uReserve, test.wSumSquares) + osmoassert.DecApproxEq(t, k2, k3, kErrTolerance) + } + + osmoassert.ConditionalPanic(t, test.expectPanic, sut) } } From 14a9ab7de8bdd351ee9406cb619d9b1eca17172a Mon Sep 17 00:00:00 2001 From: alpo Date: Wed, 14 Sep 2022 13:02:19 -0700 Subject: [PATCH 2/2] refactor tests --- x/gamm/pool-models/stableswap/amm_test.go | 97 ++++++++++++----------- 1 file changed, 51 insertions(+), 46 deletions(-) diff --git a/x/gamm/pool-models/stableswap/amm_test.go b/x/gamm/pool-models/stableswap/amm_test.go index 2893ac47217..afbf5b5a739 100644 --- a/x/gamm/pool-models/stableswap/amm_test.go +++ b/x/gamm/pool-models/stableswap/amm_test.go @@ -18,24 +18,25 @@ type StableSwapTestSuite struct { func TestCFMMInvariantTwoAssets(t *testing.T) { kErrTolerance := sdk.OneDec() - tests := []struct { + tests := map[string]struct { xReserve sdk.Dec yReserve sdk.Dec yIn sdk.Dec expectPanic bool }{ - { + "small pool small input": { sdk.NewDec(100), sdk.NewDec(100), sdk.NewDec(1), false, }, - { + "small pool large input": { sdk.NewDec(100), sdk.NewDec(100), sdk.NewDec(1000), false, }, + // This test fails due to a bug in our original solver // { // sdk.NewDec(100000), // sdk.NewDec(100000), @@ -43,19 +44,19 @@ func TestCFMMInvariantTwoAssets(t *testing.T) { // }, // panic catching - { // xReserve negative + "xReserve negative": { sdk.NewDec(-100), sdk.NewDec(100), sdk.NewDec(1), true, }, - { // yReserve negative + "yReserve negative": { sdk.NewDec(100), sdk.NewDec(-100), sdk.NewDec(1), true, }, - { // yIn negative + "yIn negative": { sdk.NewDec(100), sdk.NewDec(100), sdk.NewDec(-1), @@ -63,33 +64,34 @@ func TestCFMMInvariantTwoAssets(t *testing.T) { }, } - for _, test := range tests { - // system under test - sut := func() { - // using two-asset cfmm - k0 := cfmmConstant(test.xReserve, test.yReserve) - xOut := solveCfmm(test.xReserve, test.yReserve, test.yIn) - - k1 := cfmmConstant(test.xReserve.Sub(xOut), test.yReserve.Add(test.yIn)) - osmoassert.DecApproxEq(t, k0, k1, kErrTolerance) - - // using multi-asset cfmm (should be equivalent with u = 1, w = 0) - k2 := cfmmConstantMulti(test.xReserve, test.yReserve, sdk.OneDec(), sdk.ZeroDec()) - osmoassert.DecApproxEq(t, k2, k0, kErrTolerance) - xOut2 := solveCfmmMulti(test.xReserve, test.yReserve, sdk.ZeroDec(), test.yIn) - fmt.Println(xOut2) - k3 := cfmmConstantMulti(test.xReserve.Sub(xOut2), test.yReserve.Add(test.yIn), sdk.OneDec(), sdk.ZeroDec()) - osmoassert.DecApproxEq(t, k2, k3, kErrTolerance) - } - - osmoassert.ConditionalPanic(t, test.expectPanic, sut) + for name, test := range tests { + t.Run(name, func(t *testing.T) { + // system under test + sut := func() { + // using two-asset cfmm + k0 := cfmmConstant(test.xReserve, test.yReserve) + xOut := solveCfmm(test.xReserve, test.yReserve, test.yIn) + + k1 := cfmmConstant(test.xReserve.Sub(xOut), test.yReserve.Add(test.yIn)) + osmoassert.DecApproxEq(t, k0, k1, kErrTolerance) + + // using multi-asset cfmm (should be equivalent with u = 1, w = 0) + k2 := cfmmConstantMulti(test.xReserve, test.yReserve, sdk.OneDec(), sdk.ZeroDec()) + osmoassert.DecApproxEq(t, k2, k0, kErrTolerance) + xOut2 := solveCfmmMulti(test.xReserve, test.yReserve, sdk.ZeroDec(), test.yIn) + k3 := cfmmConstantMulti(test.xReserve.Sub(xOut2), test.yReserve.Add(test.yIn), sdk.OneDec(), sdk.ZeroDec()) + osmoassert.DecApproxEq(t, k2, k3, kErrTolerance) + } + + osmoassert.ConditionalPanic(t, test.expectPanic, sut) + }) } } func TestCFMMInvariantMultiAssets(t *testing.T) { kErrTolerance := sdk.OneDec() - tests := []struct { + tests := map[string]struct { xReserve sdk.Dec yReserve sdk.Dec uReserve sdk.Dec @@ -97,7 +99,7 @@ func TestCFMMInvariantMultiAssets(t *testing.T) { yIn sdk.Dec expectPanic bool }{ - { + "4-asset pool, small input": { sdk.NewDec(100), sdk.NewDec(100), // represents a 4-asset pool with 100 in each reserve @@ -106,7 +108,7 @@ func TestCFMMInvariantMultiAssets(t *testing.T) { sdk.NewDec(1), false, }, - { + "4-asset pool, large input": { sdk.NewDec(100), sdk.NewDec(100), sdk.NewDec(200), @@ -114,14 +116,15 @@ func TestCFMMInvariantMultiAssets(t *testing.T) { sdk.NewDec(1000), false, }, - // { + // This test fails due to a bug in our original solver + // "large pool, large input": { // sdk.NewDec(100000), // sdk.NewDec(100000), // sdk.NewDec(10000), // }, // panic catching - { // negative xReserve + "negative xReserve": { sdk.NewDec(-100), sdk.NewDec(100), // represents a 4-asset pool with 100 in each reserve @@ -130,7 +133,7 @@ func TestCFMMInvariantMultiAssets(t *testing.T) { sdk.NewDec(1), true, }, - { // negative yReserve + "negative yReserve": { sdk.NewDec(100), sdk.NewDec(-100), // represents a 4-asset pool with 100 in each reserve @@ -139,7 +142,7 @@ func TestCFMMInvariantMultiAssets(t *testing.T) { sdk.NewDec(1), true, }, - { // negative uReserve + "negative uReserve": { sdk.NewDec(100), sdk.NewDec(100), // represents a 4-asset pool with 100 in each reserve @@ -148,7 +151,7 @@ func TestCFMMInvariantMultiAssets(t *testing.T) { sdk.NewDec(1), true, }, - { // negative sumSquares + "negative sumSquares": { sdk.NewDec(100), sdk.NewDec(100), // represents a 4-asset pool with 100 in each reserve @@ -157,7 +160,7 @@ func TestCFMMInvariantMultiAssets(t *testing.T) { sdk.NewDec(1), true, }, - { // negative yIn + "negative yIn": { sdk.NewDec(100), sdk.NewDec(100), // represents a 4-asset pool with 100 in each reserve @@ -168,17 +171,19 @@ func TestCFMMInvariantMultiAssets(t *testing.T) { }, } - for _, test := range tests { - // system under test - sut := func() { - // using multi-asset cfmm - k2 := cfmmConstantMulti(test.xReserve, test.yReserve, test.uReserve, test.wSumSquares) - xOut2 := solveCfmmMulti(test.xReserve, test.yReserve, test.wSumSquares, test.yIn) - k3 := cfmmConstantMulti(test.xReserve.Sub(xOut2), test.yReserve.Add(test.yIn), test.uReserve, test.wSumSquares) - osmoassert.DecApproxEq(t, k2, k3, kErrTolerance) - } - - osmoassert.ConditionalPanic(t, test.expectPanic, sut) + for name, test := range tests { + t.Run(name, func(t *testing.T) { + // system under test + sut := func() { + // using multi-asset cfmm + k2 := cfmmConstantMulti(test.xReserve, test.yReserve, test.uReserve, test.wSumSquares) + xOut2 := solveCfmmMulti(test.xReserve, test.yReserve, test.wSumSquares, test.yIn) + k3 := cfmmConstantMulti(test.xReserve.Sub(xOut2), test.yReserve.Add(test.yIn), test.uReserve, test.wSumSquares) + osmoassert.DecApproxEq(t, k2, k3, kErrTolerance) + } + + osmoassert.ConditionalPanic(t, test.expectPanic, sut) + }) } }