From 509d2edbca1014b343a3fcc1186ce28f9d50e139 Mon Sep 17 00:00:00 2001 From: Ruaridh Date: Thu, 11 Mar 2021 04:13:21 +0000 Subject: [PATCH] fix bug that skipped minimum borrow check (#870) add tests for non-borrower repayer address --- x/hard/keeper/repay.go | 12 +-- x/hard/keeper/repay_test.go | 150 ++++++++++++++++++++++++++---------- 2 files changed, 116 insertions(+), 46 deletions(-) diff --git a/x/hard/keeper/repay.go b/x/hard/keeper/repay.go index 0d4c0400..7df0e6c5 100644 --- a/x/hard/keeper/repay.go +++ b/x/hard/keeper/repay.go @@ -23,13 +23,13 @@ func (k Keeper) Repay(ctx sdk.Context, sender, owner sdk.AccAddress, coins sdk.C // Refresh borrow after syncing interest borrow, _ = k.GetBorrow(ctx, owner) - // Validate that sender holds coins for repayment - err := k.ValidateRepay(ctx, sender, owner, coins) + // cap the repayment by what's available to repay (the borrow amount) + payment, err := k.CalculatePaymentAmount(borrow.Amount, coins) if err != nil { return err } - - payment, err := k.CalculatePaymentAmount(borrow.Amount, coins) + // Validate that sender holds coins for repayment + err = k.ValidateRepay(ctx, sender, owner, payment) if err != nil { return err } @@ -144,8 +144,10 @@ func (k Keeper) ValidateRepay(ctx sdk.Context, sender, owner sdk.AccAddress, coi // If the proposed repayment would results in a borrowed USD value below the minimum borrow USD value, reject it. // User can overpay their loan to close it out, but underpaying by such a margin that the USD value is in an // invalid range is not allowed + // Unless the user is fully repaying their loan proposedBorrowNewUSDValue := existingBorrowUSDValue.Sub(repayTotalUSDValue) - if proposedBorrowNewUSDValue.IsPositive() && proposedBorrowNewUSDValue.LT(k.GetMinimumBorrowUSDValue(ctx)) { + isFullRepayment := coins.IsEqual(existingBorrow.Amount) + if proposedBorrowNewUSDValue.LT(k.GetMinimumBorrowUSDValue(ctx)) && !isFullRepayment { return sdkerrors.Wrapf(types.ErrBelowMinimumBorrowValue, "the proposed borrow's USD value $%s is below the minimum borrow limit $%s", proposedBorrowNewUSDValue, k.GetMinimumBorrowUSDValue(ctx)) } diff --git a/x/hard/keeper/repay_test.go b/x/hard/keeper/repay_test.go index de8331b1..9ecb7fa8 100644 --- a/x/hard/keeper/repay_test.go +++ b/x/hard/keeper/repay_test.go @@ -1,7 +1,6 @@ package keeper_test import ( - "strings" "time" sdk "github.com/cosmos/cosmos-sdk/types" @@ -17,14 +16,14 @@ import ( func (suite *KeeperTestSuite) TestRepay() { type args struct { - borrower sdk.AccAddress - initialBorrowerCoins sdk.Coins - initialModuleCoins sdk.Coins - depositCoins []sdk.Coin - borrowCoins sdk.Coins - repayCoins sdk.Coins - expectedAccountBalance sdk.Coins - expectedModAccountBalance sdk.Coins + borrower sdk.AccAddress + repayer sdk.AccAddress + initialBorrowerCoins sdk.Coins + initialRepayerCoins sdk.Coins + initialModuleCoins sdk.Coins + depositCoins []sdk.Coin + borrowCoins sdk.Coins + repayCoins sdk.Coins } type errArgs struct { @@ -45,10 +44,30 @@ func (suite *KeeperTestSuite) TestRepay() { { "valid: partial repay", args{ - borrower: sdk.AccAddress(crypto.AddressHash([]byte("test"))), + borrower: sdk.AccAddress(crypto.AddressHash([]byte("borrower"))), + repayer: sdk.AccAddress(crypto.AddressHash([]byte("borrower"))), initialBorrowerCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(100*KAVA_CF))), + initialRepayerCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(100*KAVA_CF))), initialModuleCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(1000*KAVA_CF)), sdk.NewCoin("usdx", sdk.NewInt(1000*USDX_CF))), - depositCoins: []sdk.Coin{sdk.NewCoin("ukava", sdk.NewInt(100*KAVA_CF))}, + depositCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(100*KAVA_CF))), + borrowCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(50*KAVA_CF))), + repayCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(10*KAVA_CF))), + }, + errArgs{ + expectPass: true, + expectDelete: false, + contains: "", + }, + }, + { + "valid: partial repay by non borrower", + args{ + borrower: sdk.AccAddress(crypto.AddressHash([]byte("borrower"))), + repayer: sdk.AccAddress(crypto.AddressHash([]byte("repayer"))), + initialBorrowerCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(100*KAVA_CF))), + initialRepayerCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(100*KAVA_CF))), + initialModuleCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(1000*KAVA_CF)), sdk.NewCoin("usdx", sdk.NewInt(1000*USDX_CF))), + depositCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(100*KAVA_CF))), borrowCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(50*KAVA_CF))), repayCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(10*KAVA_CF))), }, @@ -61,10 +80,12 @@ func (suite *KeeperTestSuite) TestRepay() { { "valid: repay in full", args{ - borrower: sdk.AccAddress(crypto.AddressHash([]byte("test"))), + borrower: sdk.AccAddress(crypto.AddressHash([]byte("borrower"))), + repayer: sdk.AccAddress(crypto.AddressHash([]byte("borrower"))), initialBorrowerCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(100*KAVA_CF))), + initialRepayerCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(100*KAVA_CF))), initialModuleCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(1000*KAVA_CF)), sdk.NewCoin("usdx", sdk.NewInt(1000*USDX_CF))), - depositCoins: []sdk.Coin{sdk.NewCoin("ukava", sdk.NewInt(100*KAVA_CF))}, + depositCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(100*KAVA_CF))), borrowCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(50*KAVA_CF))), repayCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(50*KAVA_CF))), }, @@ -77,10 +98,12 @@ func (suite *KeeperTestSuite) TestRepay() { { "valid: overpayment is adjusted", args{ - borrower: sdk.AccAddress(crypto.AddressHash([]byte("test"))), + borrower: sdk.AccAddress(crypto.AddressHash([]byte("borrower"))), + repayer: sdk.AccAddress(crypto.AddressHash([]byte("borrower"))), initialBorrowerCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(100*KAVA_CF))), + initialRepayerCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(100*KAVA_CF))), initialModuleCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(1000*KAVA_CF)), sdk.NewCoin("usdx", sdk.NewInt(1000*USDX_CF))), - depositCoins: []sdk.Coin{sdk.NewCoin("ukava", sdk.NewInt(80*KAVA_CF))}, // Deposit less so user still has some KAVA + depositCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(80*KAVA_CF))), // Deposit less so user still has some KAVA borrowCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(50*KAVA_CF))), repayCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(60*KAVA_CF))), // Exceeds borrowed coins but not user's balance }, @@ -93,42 +116,48 @@ func (suite *KeeperTestSuite) TestRepay() { { "invalid: attempt to repay non-supplied coin", args{ - borrower: sdk.AccAddress(crypto.AddressHash([]byte("test"))), + borrower: sdk.AccAddress(crypto.AddressHash([]byte("borrower"))), + repayer: sdk.AccAddress(crypto.AddressHash([]byte("borrower"))), initialBorrowerCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(100*KAVA_CF))), + initialRepayerCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(100*KAVA_CF))), initialModuleCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(1000*KAVA_CF)), sdk.NewCoin("usdx", sdk.NewInt(1000*USDX_CF))), - depositCoins: []sdk.Coin{sdk.NewCoin("ukava", sdk.NewInt(100*KAVA_CF))}, + depositCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(100*KAVA_CF))), borrowCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(50*KAVA_CF))), repayCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(10*KAVA_CF)), sdk.NewCoin("bnb", sdk.NewInt(10*KAVA_CF))), }, errArgs{ expectPass: false, expectDelete: false, - contains: "account can only repay up to 0bnb", + contains: "no coins of this type borrowed", }, }, { - "invalid: insufficent balance for repay", + "invalid: insufficient balance for repay", args{ - borrower: sdk.AccAddress(crypto.AddressHash([]byte("test"))), + borrower: sdk.AccAddress(crypto.AddressHash([]byte("borrower"))), + repayer: sdk.AccAddress(crypto.AddressHash([]byte("repayer"))), initialBorrowerCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(100*KAVA_CF))), + initialRepayerCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(49*KAVA_CF))), initialModuleCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(1000*KAVA_CF)), sdk.NewCoin("usdx", sdk.NewInt(1000*USDX_CF))), - depositCoins: []sdk.Coin{sdk.NewCoin("ukava", sdk.NewInt(100*KAVA_CF))}, + depositCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(100*KAVA_CF))), borrowCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(50*KAVA_CF))), - repayCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(51*KAVA_CF))), // Exceeds user's KAVA balance + repayCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(50*KAVA_CF))), // Exceeds repayer's balance, but not borrow amount }, errArgs{ expectPass: false, expectDelete: false, - contains: "account can only repay up to 50000000ukava", + contains: "account can only repay up to 49000000ukava", }, }, { "invalid: repaying a single coin type results in borrow position below the minimum USD value", args{ - borrower: sdk.AccAddress(crypto.AddressHash([]byte("test"))), + borrower: sdk.AccAddress(crypto.AddressHash([]byte("borrower"))), + repayer: sdk.AccAddress(crypto.AddressHash([]byte("borrower"))), initialBorrowerCoins: sdk.NewCoins(sdk.NewCoin("usdx", sdk.NewInt(100*USDX_CF))), + initialRepayerCoins: sdk.NewCoins(sdk.NewCoin("usdx", sdk.NewInt(100*USDX_CF))), initialModuleCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(1000*KAVA_CF)), sdk.NewCoin("usdx", sdk.NewInt(1000*USDX_CF))), - depositCoins: []sdk.Coin{sdk.NewCoin("usdx", sdk.NewInt(100*USDX_CF))}, + depositCoins: sdk.NewCoins(sdk.NewCoin("usdx", sdk.NewInt(100*USDX_CF))), borrowCoins: sdk.NewCoins(sdk.NewCoin("usdx", sdk.NewInt(50*USDX_CF))), repayCoins: sdk.NewCoins(sdk.NewCoin("usdx", sdk.NewInt(45*USDX_CF))), }, @@ -141,12 +170,14 @@ func (suite *KeeperTestSuite) TestRepay() { { "invalid: repaying multiple coin types results in borrow position below the minimum USD value", args{ - borrower: sdk.AccAddress(crypto.AddressHash([]byte("test"))), + borrower: sdk.AccAddress(crypto.AddressHash([]byte("borrower"))), + repayer: sdk.AccAddress(crypto.AddressHash([]byte("borrower"))), initialBorrowerCoins: sdk.NewCoins(sdk.NewCoin("usdx", sdk.NewInt(100*USDX_CF))), + initialRepayerCoins: sdk.NewCoins(sdk.NewCoin("usdx", sdk.NewInt(100*USDX_CF)), sdk.NewCoin("ukava", sdk.NewInt(100*KAVA_CF))), initialModuleCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(1000*KAVA_CF)), sdk.NewCoin("usdx", sdk.NewInt(1000*USDX_CF))), - depositCoins: []sdk.Coin{sdk.NewCoin("usdx", sdk.NewInt(100*USDX_CF))}, + depositCoins: sdk.NewCoins(sdk.NewCoin("usdx", sdk.NewInt(100*USDX_CF))), borrowCoins: sdk.NewCoins(sdk.NewCoin("usdx", sdk.NewInt(50*USDX_CF)), sdk.NewCoin("ukava", sdk.NewInt(10*KAVA_CF))), // (50*$1)+(10*$2) = $70 - repayCoins: sdk.NewCoins(sdk.NewCoin("usdx", sdk.NewInt(45*USDX_CF)), sdk.NewCoin("ukava", sdk.NewInt(8*KAVA_CF))), // (45*$1)+(8*2) = $61 + repayCoins: sdk.NewCoins(sdk.NewCoin("usdx", sdk.NewInt(45*USDX_CF)), sdk.NewCoin("ukava", sdk.NewInt(8*KAVA_CF))), // (45*$1)+(8*$2) = $61 }, errArgs{ expectPass: false, @@ -154,6 +185,24 @@ func (suite *KeeperTestSuite) TestRepay() { contains: "proposed borrow's USD value $9.000000000000000000 is below the minimum borrow limit", }, }, + { + "invalid: overpaying multiple coin types results in borrow position below the minimum USD value", + args{ + borrower: sdk.AccAddress(crypto.AddressHash([]byte("borrower"))), + repayer: sdk.AccAddress(crypto.AddressHash([]byte("borrower"))), + initialBorrowerCoins: sdk.NewCoins(sdk.NewCoin("usdx", sdk.NewInt(100*USDX_CF))), + initialRepayerCoins: sdk.NewCoins(sdk.NewCoin("usdx", sdk.NewInt(100*USDX_CF)), sdk.NewCoin("ukava", sdk.NewInt(100*KAVA_CF))), + initialModuleCoins: sdk.NewCoins(sdk.NewCoin("ukava", sdk.NewInt(1000*KAVA_CF)), sdk.NewCoin("usdx", sdk.NewInt(1000*USDX_CF))), + depositCoins: sdk.NewCoins(sdk.NewCoin("usdx", sdk.NewInt(100*USDX_CF))), + borrowCoins: sdk.NewCoins(sdk.NewCoin("usdx", sdk.NewInt(50*USDX_CF)), sdk.NewCoin("ukava", sdk.NewInt(10*KAVA_CF))), // (50*$1)+(10*$2) = $70 + repayCoins: sdk.NewCoins(sdk.NewCoin("usdx", sdk.NewInt(500*USDX_CF)), sdk.NewCoin("ukava", sdk.NewInt(8*KAVA_CF))), // (500*$1)+(8*$2) = $516, or capping to borrowed amount, (50*$1)+(8*$2) = $66 + }, + errArgs{ + expectPass: false, + expectDelete: false, + contains: "proposed borrow's USD value $4.000000000000000000 is below the minimum borrow limit", + }, + }, } for _, tc := range testCases { @@ -163,9 +212,11 @@ func (suite *KeeperTestSuite) TestRepay() { ctx := tApp.NewContext(true, abci.Header{Height: 1, Time: tmtime.Now()}) // Auth module genesis state - authGS := app.NewAuthGenState( - []sdk.AccAddress{tc.args.borrower}, - []sdk.Coins{tc.args.initialBorrowerCoins}) + addrs, coinses := uniqueAddressCoins( + []sdk.AccAddress{tc.args.borrower, tc.args.repayer}, + []sdk.Coins{tc.args.initialBorrowerCoins, tc.args.initialRepayerCoins}, + ) + authGS := app.NewAuthGenState(addrs, coinses) // Hard module genesis state hardGS := types.NewGenesisState(types.NewParams( @@ -242,17 +293,19 @@ func (suite *KeeperTestSuite) TestRepay() { err = suite.keeper.Borrow(suite.ctx, tc.args.borrower, tc.args.borrowCoins) suite.Require().NoError(err) - err = suite.keeper.Repay(suite.ctx, tc.args.borrower, tc.args.borrower, tc.args.repayCoins) + previousRepayerCoins := suite.getAccount(tc.args.repayer).GetCoins() + + err = suite.keeper.Repay(suite.ctx, tc.args.repayer, tc.args.borrower, tc.args.repayCoins) if tc.errArgs.expectPass { suite.Require().NoError(err) // If we overpaid expect an adjustment repaymentCoins, err := suite.keeper.CalculatePaymentAmount(tc.args.borrowCoins, tc.args.repayCoins) suite.Require().NoError(err) - // Check borrower balance - expectedBorrowerCoins := tc.args.initialBorrowerCoins.Sub(tc.args.depositCoins).Add(tc.args.borrowCoins...).Sub(repaymentCoins) - acc := suite.getAccount(tc.args.borrower) - suite.Require().Equal(expectedBorrowerCoins, acc.GetCoins()) + // Check repayer balance + expectedRepayerCoins := previousRepayerCoins.Sub(repaymentCoins) + acc := suite.getAccount(tc.args.repayer) + suite.Require().Equal(expectedRepayerCoins, acc.GetCoins()) // Check module account balance expectedModuleCoins := tc.args.initialModuleCoins.Add(tc.args.depositCoins...).Sub(tc.args.borrowCoins).Add(repaymentCoins...) @@ -271,12 +324,11 @@ func (suite *KeeperTestSuite) TestRepay() { } } else { suite.Require().Error(err) - suite.Require().True(strings.Contains(err.Error(), tc.errArgs.contains)) + suite.Require().Contains(err.Error(), tc.errArgs.contains) - // Check borrower balance (no repay coins) - expectedBorrowerCoins := tc.args.initialBorrowerCoins.Sub(tc.args.depositCoins).Add(tc.args.borrowCoins...) - acc := suite.getAccount(tc.args.borrower) - suite.Require().Equal(expectedBorrowerCoins, acc.GetCoins()) + // Check repayer balance (no repay coins) + acc := suite.getAccount(tc.args.repayer) + suite.Require().Equal(previousRepayerCoins, acc.GetCoins()) // Check module account balance (no repay coins) expectedModuleCoins := tc.args.initialModuleCoins.Add(tc.args.depositCoins...).Sub(tc.args.borrowCoins) @@ -291,3 +343,19 @@ func (suite *KeeperTestSuite) TestRepay() { }) } } + +// uniqueAddressCoins removes duplicate addresses, and the corresponding elements in a list of coins. +func uniqueAddressCoins(addresses []sdk.AccAddress, coinses []sdk.Coins) ([]sdk.AccAddress, []sdk.Coins) { + uniqueAddresses := []sdk.AccAddress{} + filteredCoins := []sdk.Coins{} + + addrMap := map[string]bool{} + for i, a := range addresses { + if !addrMap[a.String()] { + uniqueAddresses = append(uniqueAddresses, a) + filteredCoins = append(filteredCoins, coinses[i]) + } + addrMap[a.String()] = true + } + return uniqueAddresses, filteredCoins +}