From c049aad495cc4ca0f05e2f68cf96c4096a3a4f0b Mon Sep 17 00:00:00 2001 From: Kevin Davis Date: Thu, 4 Jun 2020 08:58:39 -0400 Subject: [PATCH] Minor audit fixes (#540) * use mod and quo to calculate how many auctions to start * check both return values in getDenomPrefix * split auctions using weighted buckets --- x/cdp/keeper/auctions.go | 41 ++++++++-------- x/cdp/keeper/auctions_test.go | 15 ++++++ x/cdp/keeper/cdp.go | 10 +++- x/cdp/keeper/integration_test.go | 21 ++++++++- x/cdp/keeper/keeper.go | 10 +++- x/cdp/keeper/math.go | 81 ++++++++++++++++++++++++++++++++ 6 files changed, 153 insertions(+), 25 deletions(-) create mode 100644 x/cdp/keeper/math.go diff --git a/x/cdp/keeper/auctions.go b/x/cdp/keeper/auctions.go index baacd1cf..b6906865 100644 --- a/x/cdp/keeper/auctions.go +++ b/x/cdp/keeper/auctions.go @@ -32,32 +32,33 @@ func (k Keeper) CreateAuctionsFromDeposit( ctx sdk.Context, collateral sdk.Coin, returnAddr sdk.AccAddress, debt, auctionSize sdk.Int, principalDenom string) (err error) { - amountToAuction := collateral.Amount - totalCollateralAmount := collateral.Amount - remainingDebt := debt - if !amountToAuction.IsPositive() { - return nil + // the number of auctions to start with lot = auctionSize + wholeAuctions := collateral.Amount.Quo(auctionSize) + // remaining collateral (< lot) to auction + partialAuctionAmount := collateral.Amount.Mod(auctionSize) + auctionLots := []sdk.Int{} + + for i := int64(0); i < wholeAuctions.Int64(); i++ { + auctionLots = append(auctionLots, auctionSize) } - for amountToAuction.GT(auctionSize) { - debtCoveredByAuction := (sdk.NewDecFromInt(auctionSize).Quo(sdk.NewDecFromInt(totalCollateralAmount))).Mul(sdk.NewDecFromInt(debt)).RoundInt() - penalty := k.ApplyLiquidationPenalty(ctx, collateral.Denom, debtCoveredByAuction) + if partialAuctionAmount.IsPositive() { + auctionLots = append(auctionLots, partialAuctionAmount) + } + // use the auction lots as weights to split the debt into buckets, + // where each bucket represents how much debt that auction will attempt to cover + debtAmounts := splitIntIntoWeightedBuckets(debt, auctionLots) + debtDenom := k.GetDebtDenom(ctx) + for i, debtAmount := range debtAmounts { + penalty := k.ApplyLiquidationPenalty(ctx, collateral.Denom, debtAmount) _, err := k.auctionKeeper.StartCollateralAuction( - ctx, types.LiquidatorMacc, sdk.NewCoin(collateral.Denom, auctionSize), sdk.NewCoin(principalDenom, debtCoveredByAuction.Add(penalty)), []sdk.AccAddress{returnAddr}, - []sdk.Int{auctionSize}, sdk.NewCoin(k.GetDebtDenom(ctx), debtCoveredByAuction)) + ctx, types.LiquidatorMacc, sdk.NewCoin(collateral.Denom, auctionLots[i]), + sdk.NewCoin(principalDenom, debtAmount.Add(penalty)), []sdk.AccAddress{returnAddr}, + []sdk.Int{auctionLots[i]}, sdk.NewCoin(debtDenom, debtAmount), + ) if err != nil { return err } - amountToAuction = amountToAuction.Sub(auctionSize) - remainingDebt = remainingDebt.Sub(debtCoveredByAuction) } - penalty := k.ApplyLiquidationPenalty(ctx, collateral.Denom, remainingDebt) - _, err = k.auctionKeeper.StartCollateralAuction( - ctx, types.LiquidatorMacc, sdk.NewCoin(collateral.Denom, amountToAuction), sdk.NewCoin(principalDenom, remainingDebt.Add(penalty)), []sdk.AccAddress{returnAddr}, - []sdk.Int{amountToAuction}, sdk.NewCoin(k.GetDebtDenom(ctx), remainingDebt)) - if err != nil { - return err - } - return nil } diff --git a/x/cdp/keeper/auctions_test.go b/x/cdp/keeper/auctions_test.go index 75e9fb21..57273c7b 100644 --- a/x/cdp/keeper/auctions_test.go +++ b/x/cdp/keeper/auctions_test.go @@ -13,6 +13,7 @@ import ( "github.com/stretchr/testify/suite" abci "github.com/tendermint/tendermint/abci/types" + "github.com/tendermint/tendermint/crypto" tmtime "github.com/tendermint/tendermint/types/time" ) @@ -22,14 +23,18 @@ type AuctionTestSuite struct { keeper keeper.Keeper app app.TestApp ctx sdk.Context + addrs []sdk.AccAddress } func (suite *AuctionTestSuite) SetupTest() { config := sdk.GetConfig() app.SetBech32AddressPrefixes(config) tApp := app.NewTestApp() + taddr := sdk.AccAddress(crypto.AddressHash([]byte("KavaTestUser1"))) + authGS := app.NewAuthGenState([]sdk.AccAddress{taddr}, []sdk.Coins{cs(c("usdx", 21000000000))}) ctx := tApp.NewContext(true, abci.Header{Height: 1, Time: tmtime.Now()}) tApp.InitializeFromGenesisStates( + authGS, NewPricefeedGenStateMulti(), NewCDPGenStateMulti(), ) @@ -37,6 +42,7 @@ func (suite *AuctionTestSuite) SetupTest() { suite.app = tApp suite.ctx = ctx suite.keeper = keeper + suite.addrs = []sdk.AccAddress{taddr} return } @@ -51,6 +57,15 @@ func (suite *AuctionTestSuite) TestNetDebtSurplus() { suite.Equal(cs(c("debt", 90)), acc.GetCoins()) } +func (suite *AuctionTestSuite) TestCollateralAuction() { + sk := suite.app.GetSupplyKeeper() + err := sk.MintCoins(suite.ctx, types.LiquidatorMacc, cs(c("debt", 21000000000), c("bnb", 190000000000))) + suite.Require().NoError(err) + testDeposit := types.NewDeposit(1, suite.addrs[0], c("bnb", 190000000000)) + err = suite.keeper.AuctionCollateral(suite.ctx, types.Deposits{testDeposit}, i(21000000000), "usdx") + suite.Require().NoError(err) +} + func (suite *AuctionTestSuite) TestSurplusAuction() { sk := suite.app.GetSupplyKeeper() err := sk.MintCoins(suite.ctx, types.LiquidatorMacc, cs(c("usdx", 600000000000))) diff --git a/x/cdp/keeper/cdp.go b/x/cdp/keeper/cdp.go index 37ac6f87..6445062e 100644 --- a/x/cdp/keeper/cdp.go +++ b/x/cdp/keeper/cdp.go @@ -302,14 +302,20 @@ func (k Keeper) RemoveCdpOwnerIndex(ctx sdk.Context, cdp types.CDP) { // IndexCdpByCollateralRatio sets the cdp id in the store, indexed by the collateral type and collateral to debt ratio func (k Keeper) IndexCdpByCollateralRatio(ctx sdk.Context, denom string, id uint64, collateralRatio sdk.Dec) { store := prefix.NewStore(ctx.KVStore(k.key), types.CollateralRatioIndexPrefix) - db, _ := k.GetDenomPrefix(ctx, denom) + db, found := k.GetDenomPrefix(ctx, denom) + if !found { + panic(fmt.Sprintf("denom %s prefix not found", denom)) + } store.Set(types.CollateralRatioKey(db, id, collateralRatio), types.GetCdpIDBytes(id)) } // RemoveCdpCollateralRatioIndex deletes the cdp id from the store's index of cdps by collateral type and collateral to debt ratio func (k Keeper) RemoveCdpCollateralRatioIndex(ctx sdk.Context, denom string, id uint64, collateralRatio sdk.Dec) { store := prefix.NewStore(ctx.KVStore(k.key), types.CollateralRatioIndexPrefix) - db, _ := k.GetDenomPrefix(ctx, denom) + db, found := k.GetDenomPrefix(ctx, denom) + if !found { + panic(fmt.Sprintf("denom %s prefix not found", denom)) + } store.Delete(types.CollateralRatioKey(db, id, collateralRatio)) } diff --git a/x/cdp/keeper/integration_test.go b/x/cdp/keeper/integration_test.go index 8b11a4d4..f72e2343 100644 --- a/x/cdp/keeper/integration_test.go +++ b/x/cdp/keeper/integration_test.go @@ -83,6 +83,7 @@ func NewPricefeedGenStateMulti() app.GenesisState { Markets: []pricefeed.Market{ {MarketID: "btc:usd", BaseAsset: "btc", QuoteAsset: "usd", Oracles: []sdk.AccAddress{}, Active: true}, {MarketID: "xrp:usd", BaseAsset: "xrp", QuoteAsset: "usd", Oracles: []sdk.AccAddress{}, Active: true}, + {MarketID: "bnb:usd", BaseAsset: "bnb", QuoteAsset: "usd", Oracles: []sdk.AccAddress{}, Active: true}, }, }, PostedPrices: []pricefeed.PostedPrice{ @@ -98,6 +99,12 @@ func NewPricefeedGenStateMulti() app.GenesisState { Price: sdk.MustNewDecFromStr("0.25"), Expiry: time.Now().Add(1 * time.Hour), }, + { + MarketID: "bnb:usd", + OracleAddress: sdk.AccAddress{}, + Price: sdk.MustNewDecFromStr("17.25"), + Expiry: time.Now().Add(1 * time.Hour), + }, }, } return app.GenesisState{pricefeed.ModuleName: pricefeed.ModuleCdc.MustMarshalJSON(pfGenesis)} @@ -105,7 +112,7 @@ func NewPricefeedGenStateMulti() app.GenesisState { func NewCDPGenStateMulti() app.GenesisState { cdpGenesis := cdp.GenesisState{ Params: cdp.Params{ - GlobalDebtLimit: sdk.NewInt64Coin("usdx", 1000000000000), + GlobalDebtLimit: sdk.NewInt64Coin("usdx", 1500000000000), SurplusAuctionThreshold: cdp.DefaultSurplusThreshold, SurplusAuctionLot: cdp.DefaultSurplusLot, DebtAuctionThreshold: cdp.DefaultDebtThreshold, @@ -136,6 +143,18 @@ func NewCDPGenStateMulti() app.GenesisState { LiquidationMarketID: "btc:usd", ConversionFactor: i(8), }, + { + Denom: "bnb", + LiquidationRatio: sdk.MustNewDecFromStr("1.5"), + DebtLimit: sdk.NewInt64Coin("usdx", 500000000000), + StabilityFee: sdk.MustNewDecFromStr("1.000000001547125958"), // %5 apr + LiquidationPenalty: d("0.05"), + AuctionSize: i(50000000000), + Prefix: 0x22, + SpotMarketID: "bnb:usd", + LiquidationMarketID: "bnb:usd", + ConversionFactor: i(8), + }, }, DebtParam: cdp.DebtParam{ Denom: "usdx", diff --git a/x/cdp/keeper/keeper.go b/x/cdp/keeper/keeper.go index 8e761880..b90afaea 100644 --- a/x/cdp/keeper/keeper.go +++ b/x/cdp/keeper/keeper.go @@ -45,7 +45,10 @@ func NewKeeper(cdc *codec.Codec, key sdk.StoreKey, paramstore subspace.Subspace, // CdpDenomIndexIterator returns an sdk.Iterator for all cdps with matching collateral denom func (k Keeper) CdpDenomIndexIterator(ctx sdk.Context, denom string) sdk.Iterator { store := prefix.NewStore(ctx.KVStore(k.key), types.CdpKeyPrefix) - db, _ := k.GetDenomPrefix(ctx, denom) + db, found := k.GetDenomPrefix(ctx, denom) + if !found { + panic(fmt.Sprintf("denom %s prefix not found", denom)) + } return sdk.KVStorePrefixIterator(store, types.DenomIterKey(db)) } @@ -53,7 +56,10 @@ func (k Keeper) CdpDenomIndexIterator(ctx sdk.Context, denom string) sdk.Iterato // matching denom and collateral:debt ratio LESS THAN targetRatio func (k Keeper) CdpCollateralRatioIndexIterator(ctx sdk.Context, denom string, targetRatio sdk.Dec) sdk.Iterator { store := prefix.NewStore(ctx.KVStore(k.key), types.CollateralRatioIndexPrefix) - db, _ := k.GetDenomPrefix(ctx, denom) + db, found := k.GetDenomPrefix(ctx, denom) + if !found { + panic(fmt.Sprintf("denom %s prefix not found", denom)) + } return store.Iterator(types.CollateralRatioIterKey(db, sdk.ZeroDec()), types.CollateralRatioIterKey(db, targetRatio)) } diff --git a/x/cdp/keeper/math.go b/x/cdp/keeper/math.go new file mode 100644 index 00000000..ac5ac497 --- /dev/null +++ b/x/cdp/keeper/math.go @@ -0,0 +1,81 @@ +package keeper + +import ( + "sort" + + sdk "github.com/cosmos/cosmos-sdk/types" +) + +// 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 +// note - copied from auction, tests are located there. +func splitIntIntoWeightedBuckets(amount sdk.Int, buckets []sdk.Int) []sdk.Int { + // 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") + } + } + + // 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") + } + + 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} + } + + // 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) + }) + + // 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 + if !leftToAllocate.IsZero() { + results[qr.index] = results[qr.index].Add(sdk.OneInt()) + leftToAllocate = leftToAllocate.Sub(sdk.OneInt()) + } + } + return results +} + +type quoRem struct { + index int + quo sdk.Int + rem sdk.Int +} + +// totalInts adds together sdk.Ints +func totalInts(is ...sdk.Int) sdk.Int { + total := sdk.ZeroInt() + for _, i := range is { + total = total.Add(i) + } + return total +}