From 5987d966ef3ec91253a2bdd74f61f93e9d2e237a Mon Sep 17 00:00:00 2001 From: rhuairahrighairigh Date: Mon, 11 May 2020 20:45:00 +0100 Subject: [PATCH] increase auction maths safety --- x/auction/genesis_test.go | 5 +- x/auction/keeper/auctions.go | 2 +- x/auction/keeper/math.go | 27 ++++++--- x/auction/keeper/math_test.go | 96 +++++++++++++++++++++++++++++--- x/auction/types/auctions.go | 10 +++- x/auction/types/auctions_test.go | 87 ++++++++++++++++------------- x/auction/types/msg_test.go | 2 - x/auction/types/params_test.go | 4 -- 8 files changed, 167 insertions(+), 66 deletions(-) diff --git a/x/auction/genesis_test.go b/x/auction/genesis_test.go index 78ecf1f9..28ba48e2 100644 --- a/x/auction/genesis_test.go +++ b/x/auction/genesis_test.go @@ -9,17 +9,20 @@ import ( abci "github.com/tendermint/tendermint/abci/types" + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/kava-labs/kava/app" "github.com/kava-labs/kava/x/auction" ) +var _, testAddrs = app.GeneratePrivKeyAddressPairs(2) var testTime = time.Date(1998, 1, 1, 0, 0, 0, 0, time.UTC) var testAuction = auction.NewCollateralAuction( "seller", c("lotdenom", 10), testTime, c("biddenom", 1000), - auction.WeightedAddresses{}, + auction.WeightedAddresses{Addresses: testAddrs, Weights: []sdk.Int{sdk.OneInt(), sdk.OneInt()}}, c("debt", 1000), ).WithID(3).(auction.GenesisAuction) diff --git a/x/auction/keeper/auctions.go b/x/auction/keeper/auctions.go index 9deca689..8a66f939 100644 --- a/x/auction/keeper/auctions.go +++ b/x/auction/keeper/auctions.go @@ -343,7 +343,7 @@ func (k Keeper) PlaceReverseBidCollateral(ctx sdk.Context, a types.CollateralAuc } // Decrease in lot is sent to weighted addresses (normally the CDP depositors) - // TODO: paying out rateably to cdp depositors is vulnerable to errors compounding over multiple bids - check this can't be gamed. + // Note: splitting an integer amount across weighted buckets results in small errors. lotPayouts, err := splitCoinIntoWeightedBuckets(a.Lot.Sub(lot), a.LotReturns.Weights) if err != nil { return a, err diff --git a/x/auction/keeper/math.go b/x/auction/keeper/math.go index 5759471d..2fd9198f 100644 --- a/x/auction/keeper/math.go +++ b/x/auction/keeper/math.go @@ -7,41 +7,52 @@ import ( ) // splitIntIntoWeightedBuckets divides an initial +ve integer among several buckets in proportion to the buckets' weights -// It uses the largest remainder method: -// https://en.wikipedia.org/wiki/Largest_remainder_method -// see also: https://stackoverflow.com/questions/13483430/how-to-make-rounded-percentages-add-up-to-100 +// It uses the largest remainder method: https://en.wikipedia.org/wiki/Largest_remainder_method +// See also: https://stackoverflow.com/questions/13483430/how-to-make-rounded-percentages-add-up-to-100 func splitIntIntoWeightedBuckets(amount sdk.Int, buckets []sdk.Int) []sdk.Int { - // Limit input to +ve numbers as algorithm hasn't been designed to work with -ve numbers. + // Limit input to +ve numbers as algorithm hasn't been scoped to work with -ve numbers. if amount.IsNegative() { panic("negative amount") } + if len(buckets) < 1 { + panic("no buckets") + } for _, bucket := range buckets { if bucket.IsNegative() { panic("negative bucket") } } - totalWeights := totalInts(buckets...) + // 1) Split the amount by weights, recording whole number part and remainder + + totalWeights := totalInts(buckets...) + if !totalWeights.IsPositive() { + panic("total weights must sum to > 0") + } - // split amount by weights, recording whole number part and remainder quotients := make([]quoRem, len(buckets)) for i := range buckets { + // amount * ( weight/total_weight ) q := amount.Mul(buckets[i]).Quo(totalWeights) r := amount.Mul(buckets[i]).Mod(totalWeights) quotients[i] = quoRem{index: i, quo: q, rem: r} } - // apportion left over to buckets with the highest remainder (to minimize error) + // 2) Calculate total left over from remainders, and apportion it to buckets with the highest remainder (to minimize error) + + // sort by decreasing remainder order sort.Slice(quotients, func(i, j int) bool { - return quotients[i].rem.GT(quotients[j].rem) // decreasing remainder order + return quotients[i].rem.GT(quotients[j].rem) }) + // calculate total left over from remainders allocated := sdk.ZeroInt() for _, qr := range quotients { allocated = allocated.Add(qr.quo) } leftToAllocate := amount.Sub(allocated) + // apportion according to largest remainder results := make([]sdk.Int, len(quotients)) for _, qr := range quotients { results[qr.index] = qr.quo diff --git a/x/auction/keeper/math_test.go b/x/auction/keeper/math_test.go index 6ff02765..1412988c 100644 --- a/x/auction/keeper/math_test.go +++ b/x/auction/keeper/math_test.go @@ -10,19 +10,97 @@ import ( func TestSplitIntIntoWeightedBuckets(t *testing.T) { testCases := []struct { - name string - amount sdk.Int - buckets []sdk.Int - want []sdk.Int + name string + amount sdk.Int + buckets []sdk.Int + want []sdk.Int + expectPanic bool }{ - {"2split1,1", i(2), is(1, 1), is(1, 1)}, - {"100split1,9", i(100), is(1, 9), is(10, 90)}, - {"7split1,2", i(7), is(1, 2), is(2, 5)}, - {"17split1,1,1", i(17), is(1, 1, 1), is(6, 6, 5)}, + { + name: "0split0", + amount: i(0), + buckets: is(0), + expectPanic: true, + }, + { + name: "5splitnil", + amount: i(5), + buckets: is(), + expectPanic: true, + }, + { + name: "-2split1,1", + amount: i(-2), + buckets: is(1, 1), + expectPanic: true, + }, + { + name: "2split1,-1", + amount: i(2), + buckets: is(1, -1), + expectPanic: true, + }, + { + name: "0split0,0,0,1", + amount: i(0), + buckets: is(0, 0, 0, 1), + want: is(0, 0, 0, 0), + }, + { + name: "2split1,1", + amount: i(2), + buckets: is(1, 1), + want: is(1, 1), + }, + { + name: "100split1,9", + amount: i(100), + buckets: is(1, 9), + want: is(10, 90), + }, + { + name: "100split9,1", + amount: i(100), + buckets: is(9, 1), + want: is(90, 10), + }, + { + name: "7split1,2", + amount: i(7), + buckets: is(1, 2), + want: is(2, 5), + }, + { + name: "17split1,1,1", + amount: i(17), + buckets: is(1, 1, 1), + want: is(6, 6, 5), + }, + { + name: "10split1000000,1", + amount: i(10), + buckets: is(1000000, 1), + want: is(10, 0), + }, + { + name: "334733353split730777,31547", + amount: i(334733353), + buckets: is(730777, 31547), + want: is(320881194, 13852159), + }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - got := splitIntIntoWeightedBuckets(tc.amount, tc.buckets) + var got []sdk.Int + run := func() { + got = splitIntIntoWeightedBuckets(tc.amount, tc.buckets) + } + if tc.expectPanic { + require.Panics(t, run) + } else { + require.NotPanics(t, run) + } + require.Equal(t, tc.want, got) }) } diff --git a/x/auction/types/auctions.go b/x/auction/types/auctions.go index 2e06b86e..ca5ea74f 100644 --- a/x/auction/types/auctions.go +++ b/x/auction/types/auctions.go @@ -287,15 +287,23 @@ func NewWeightedAddresses(addrs []sdk.AccAddress, weights []sdk.Int) (WeightedAd return wa, nil } -// Validate checks for that the weights are not negative and that lengths match. +// Validate checks for that the weights are not negative, not all zero, and the lengths match. func (wa WeightedAddresses) Validate() error { + if len(wa.Weights) < 1 { + return fmt.Errorf("must be at least 1 weighted address") + } if len(wa.Addresses) != len(wa.Weights) { return fmt.Errorf("number of addresses doesn't match number of weights, %d ≠ %d", len(wa.Addresses), len(wa.Weights)) } + totalWeight := sdk.ZeroInt() for _, w := range wa.Weights { if w.IsNegative() { return fmt.Errorf("weights contain a negative amount: %s", w) } + totalWeight = totalWeight.Add(w) + } + if !totalWeight.IsPositive() { + return fmt.Errorf("total weight must be positive") } return nil } diff --git a/x/auction/types/auctions_test.go b/x/auction/types/auctions_test.go index 49953cbe..7494612d 100644 --- a/x/auction/types/auctions_test.go +++ b/x/auction/types/auctions_test.go @@ -1,6 +1,7 @@ package types import ( + "fmt" "testing" "time" @@ -24,48 +25,65 @@ const ( TestAccAddress2 = "kava1pdfav2cjhry9k79nu6r8kgknnjtq6a7rcr0qlr" ) +func d(amount string) sdk.Dec { return sdk.MustNewDecFromStr(amount) } +func c(denom string, amount int64) sdk.Coin { return sdk.NewInt64Coin(denom, amount) } +func i(n int64) sdk.Int { return sdk.NewInt(n) } +func is(ns ...int64) (is []sdk.Int) { + for _, n := range ns { + is = append(is, sdk.NewInt(n)) + } + return +} + func TestNewWeightedAddresses(t *testing.T) { tests := []struct { - name string - addresses []sdk.AccAddress - weights []sdk.Int - expectpass bool + name string + addresses []sdk.AccAddress + weights []sdk.Int + expectedErr error }{ { - "normal", - []sdk.AccAddress{ + name: "normal", + addresses: []sdk.AccAddress{ sdk.AccAddress([]byte(TestAccAddress1)), sdk.AccAddress([]byte(TestAccAddress2)), }, - []sdk.Int{ - sdk.NewInt(6), - sdk.NewInt(8), - }, - true, + weights: is(6, 8), + expectedErr: nil, }, { - "mismatched", - []sdk.AccAddress{ + name: "mismatched", + addresses: []sdk.AccAddress{ sdk.AccAddress([]byte(TestAccAddress1)), sdk.AccAddress([]byte(TestAccAddress2)), }, - []sdk.Int{ - sdk.NewInt(6), - }, - false, + weights: is(6), + expectedErr: fmt.Errorf("number of addresses doesn't match number of weights, %d ≠ %d", 2, 1), }, { - "negativeWeight", - []sdk.AccAddress{ + name: "negativeWeight", + addresses: []sdk.AccAddress{ sdk.AccAddress([]byte(TestAccAddress1)), sdk.AccAddress([]byte(TestAccAddress2)), }, - []sdk.Int{ - sdk.NewInt(6), - sdk.NewInt(-8), + weights: is(6, -8), + expectedErr: fmt.Errorf("weights contain a negative amount: %s", i(-8)), + }, + { + name: "zero total weights", + addresses: []sdk.AccAddress{ + sdk.AccAddress([]byte(TestAccAddress1)), + sdk.AccAddress([]byte(TestAccAddress2)), }, - false, + weights: is(0, 0), + expectedErr: fmt.Errorf("total weight must be positive"), + }, + { + name: "no weights", + addresses: nil, + weights: nil, + expectedErr: fmt.Errorf("must be at least 1 weighted address"), }, } @@ -75,27 +93,16 @@ func TestNewWeightedAddresses(t *testing.T) { // Attempt to instantiate new WeightedAddresses weightedAddresses, err := NewWeightedAddresses(tc.addresses, tc.weights) - if tc.expectpass { - // Confirm there is no error - require.Nil(t, err) - + if tc.expectedErr != nil { + // Confirm the error + require.EqualError(t, err, tc.expectedErr.Error()) + } else { + require.NoError(t, err) // Check addresses, weights require.Equal(t, tc.addresses, weightedAddresses.Addresses) require.Equal(t, tc.weights, weightedAddresses.Weights) - } else { - // Confirm that there is an error - require.NotNil(t, err) - - switch tc.name { - case "mismatched": - require.Contains(t, err.Error(), "number of addresses doesn't match number of weights") - case "negativeWeight": - require.Contains(t, err.Error(), "weights contain a negative amount") - default: - // Unexpected error state - t.Fail() - } } + }) } } diff --git a/x/auction/types/msg_test.go b/x/auction/types/msg_test.go index 7ac2909c..4c5ee977 100644 --- a/x/auction/types/msg_test.go +++ b/x/auction/types/msg_test.go @@ -39,5 +39,3 @@ func TestMsgPlaceBid_ValidateBasic(t *testing.T) { }) } } - -func c(denom string, amount int64) sdk.Coin { return sdk.NewInt64Coin(denom, amount) } diff --git a/x/auction/types/params_test.go b/x/auction/types/params_test.go index f2d65091..88d30f43 100644 --- a/x/auction/types/params_test.go +++ b/x/auction/types/params_test.go @@ -5,8 +5,6 @@ import ( "time" "github.com/stretchr/testify/require" - - sdk "github.com/cosmos/cosmos-sdk/types" ) func TestParams_Validate(t *testing.T) { @@ -105,5 +103,3 @@ func TestParams_Validate(t *testing.T) { }) } } - -func d(amount string) sdk.Dec { return sdk.MustNewDecFromStr(amount) }