From 67b89c2b9cd78c5ba5f193a0efb6945b6defc3a4 Mon Sep 17 00:00:00 2001 From: Denali Marsh Date: Wed, 20 May 2020 21:50:12 -0700 Subject: [PATCH] [R4R] Blacklist module accounts (#517) * implement module account blacklist * update cdp getModuleAccountCoins method --- app/app.go | 21 +++++++++++++++++++-- app/app_test.go | 2 +- x/bep3/handler.go | 5 ++--- x/bep3/keeper/keeper.go | 9 ++++----- x/bep3/keeper/swap.go | 5 +++++ x/bep3/keeper/swap_test.go | 35 ++++++++++++++++++++++++++++++++++- x/cdp/keeper/keeper.go | 5 ++++- x/cdp/keeper/savings.go | 23 +++++++++-------------- 8 files changed, 78 insertions(+), 27 deletions(-) diff --git a/app/app.go b/app/app.go index 63219115..5a1f2832 100644 --- a/app/app.go +++ b/app/app.go @@ -98,6 +98,11 @@ var ( bep3.ModuleName: nil, kavadist.ModuleName: {supply.Minter}, } + + // module accounts that are allowed to receive tokens + allowedReceivingModAcc = map[string]bool{ + distr.ModuleName: true, + } ) // Verify app interface at compile time @@ -199,7 +204,7 @@ func NewApp(logger log.Logger, db dbm.DB, traceStore io.Writer, loadLatest bool, app.bankKeeper = bank.NewBaseKeeper( app.accountKeeper, bankSubspace, - app.ModuleAccountAddrs(), + app.BlacklistedAccAddrs(), ) app.supplyKeeper = supply.NewKeeper( app.cdc, @@ -273,7 +278,7 @@ func NewApp(logger log.Logger, db dbm.DB, traceStore io.Writer, loadLatest bool, app.committeeKeeper = committee.NewKeeper( app.cdc, keys[committee.StoreKey], - committeeGovRouter, // TODO blacklist module addresses? + committeeGovRouter, app.paramsKeeper, ) @@ -321,6 +326,7 @@ func NewApp(logger log.Logger, db dbm.DB, traceStore io.Writer, loadLatest bool, app.auctionKeeper, app.supplyKeeper, app.accountKeeper, + mAccPerms, ) app.bep3Keeper = bep3.NewKeeper( app.cdc, @@ -328,6 +334,7 @@ func NewApp(logger log.Logger, db dbm.DB, traceStore io.Writer, loadLatest bool, app.supplyKeeper, app.accountKeeper, bep3Subspace, + app.ModuleAccountAddrs(), ) app.kavadistKeeper = kavadist.NewKeeper( app.cdc, @@ -505,6 +512,16 @@ func (app *App) ModuleAccountAddrs() map[string]bool { return modAccAddrs } +// BlacklistedAccAddrs returns all the app's module account addresses black listed for receiving tokens. +func (app *App) BlacklistedAccAddrs() map[string]bool { + blacklistedAddrs := make(map[string]bool) + for acc := range mAccPerms { + blacklistedAddrs[supply.NewModuleAddress(acc).String()] = !allowedReceivingModAcc[acc] + } + + return blacklistedAddrs +} + // Codec returns the application's sealed codec. func (app *App) Codec() *codec.Codec { return app.cdc diff --git a/app/app_test.go b/app/app_test.go index 0ca09394..82a6a939 100644 --- a/app/app_test.go +++ b/app/app_test.go @@ -31,7 +31,7 @@ func TestBlackListedAddrs(t *testing.T) { app := NewApp(log.NewTMLogger(log.NewSyncWriter(os.Stdout)), db, nil, true, map[int64]bool{}, 0) for acc := range mAccPerms { - require.True(t, app.bankKeeper.BlacklistedAddr(app.supplyKeeper.GetModuleAddress(acc))) + require.Equal(t, !allowedReceivingModAcc[acc], app.bankKeeper.BlacklistedAddr(app.supplyKeeper.GetModuleAddress(acc))) } } diff --git a/x/bep3/handler.go b/x/bep3/handler.go index 074c7240..805af1a2 100644 --- a/x/bep3/handler.go +++ b/x/bep3/handler.go @@ -24,9 +24,8 @@ func NewHandler(k Keeper) sdk.Handler { // handleMsgCreateAtomicSwap handles requests to create a new AtomicSwap func handleMsgCreateAtomicSwap(ctx sdk.Context, k Keeper, msg MsgCreateAtomicSwap) (*sdk.Result, error) { - err := k.CreateAtomicSwap(ctx, msg.RandomNumberHash, msg.Timestamp, msg.HeightSpan, msg.From, msg.To, - msg.SenderOtherChain, msg.RecipientOtherChain, msg.Amount, true) - + err := k.CreateAtomicSwap(ctx, msg.RandomNumberHash, msg.Timestamp, msg.HeightSpan, + msg.From, msg.To, msg.SenderOtherChain, msg.RecipientOtherChain, msg.Amount, true) if err != nil { return nil, err } diff --git a/x/bep3/keeper/keeper.go b/x/bep3/keeper/keeper.go index c748c118..7ea4d1a8 100644 --- a/x/bep3/keeper/keeper.go +++ b/x/bep3/keeper/keeper.go @@ -7,7 +7,6 @@ import ( "github.com/cosmos/cosmos-sdk/store/prefix" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/x/params/subspace" - "github.com/tendermint/tendermint/libs/log" "github.com/kava-labs/kava/x/bep3/types" @@ -20,13 +19,12 @@ type Keeper struct { paramSubspace subspace.Subspace supplyKeeper types.SupplyKeeper accountKeeper types.AccountKeeper + Maccs map[string]bool } // NewKeeper creates a bep3 keeper -func NewKeeper(cdc *codec.Codec, key sdk.StoreKey, - sk types.SupplyKeeper, ak types.AccountKeeper, - paramstore subspace.Subspace, -) Keeper { +func NewKeeper(cdc *codec.Codec, key sdk.StoreKey, sk types.SupplyKeeper, ak types.AccountKeeper, + paramstore subspace.Subspace, maccs map[string]bool) Keeper { if !paramstore.HasKeyTable() { paramstore = paramstore.WithKeyTable(types.ParamKeyTable()) } @@ -37,6 +35,7 @@ func NewKeeper(cdc *codec.Codec, key sdk.StoreKey, paramSubspace: paramstore, supplyKeeper: sk, accountKeeper: ak, + Maccs: maccs, } return keeper } diff --git a/x/bep3/keeper/swap.go b/x/bep3/keeper/swap.go index 725ec59d..3f9cfc3e 100644 --- a/x/bep3/keeper/swap.go +++ b/x/bep3/keeper/swap.go @@ -23,6 +23,11 @@ func (k Keeper) CreateAtomicSwap(ctx sdk.Context, randomNumberHash []byte, times return sdkerrors.Wrap(types.ErrAtomicSwapAlreadyExists, hex.EncodeToString(swapID)) } + // Cannot send coins to a module account + if k.Maccs[recipient.String()] { + return sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "%s is a module account", recipient) + } + // The heightSpan period should be more than 10 minutes and less than one week // Assume average block time interval is 10 second. 10 mins = 60 blocks, 1 week = 60480 blocks if heightSpan < k.GetMinBlockLock(ctx) || heightSpan > k.GetMaxBlockLock(ctx) { diff --git a/x/bep3/keeper/swap_test.go b/x/bep3/keeper/swap_test.go index 96bb5a45..dc89f0fd 100644 --- a/x/bep3/keeper/swap_test.go +++ b/x/bep3/keeper/swap_test.go @@ -24,6 +24,7 @@ type AtomicSwapTestSuite struct { keeper keeper.Keeper app app.TestApp ctx sdk.Context + randMacc sdk.AccAddress deputy sdk.AccAddress addrs []sdk.AccAddress timestamps []int64 @@ -62,6 +63,20 @@ func (suite *AtomicSwapTestSuite) SetupTest() { suite.deputy = deputy suite.addrs = addrs suite.keeper = suite.app.GetBep3Keeper() + + // Load a random module account to test blacklisting + i := 0 + var randModuleAcc sdk.AccAddress + for macc := range suite.keeper.Maccs { + if i == len(suite.keeper.Maccs)/2 { + acc, err := sdk.AccAddressFromBech32(macc) + suite.Nil(err) + randModuleAcc = acc + } + i = i + 1 + } + suite.randMacc = randModuleAcc + suite.GenerateSwapDetails() } @@ -286,6 +301,24 @@ func (suite *AtomicSwapTestSuite) TestCreateAtomicSwap() { false, true, }, + { + "recipient is module account", + currentTmTime, + args{ + randomNumberHash: suite.randomNumberHashes[8], + timestamp: suite.timestamps[8], + heightSpan: uint64(360), + sender: suite.deputy, + recipient: suite.randMacc, + senderOtherChain: TestSenderOtherChain, + recipientOtherChain: TestRecipientOtherChain, + coins: cs(c(BNB_DENOM, 5000)), + crossChain: true, + direction: types.Incoming, + }, + false, + false, + }, } for _, tc := range testCases { @@ -624,7 +657,7 @@ func (suite *AtomicSwapTestSuite) TestRefundAtomicSwap() { } err := suite.keeper.CreateAtomicSwap(suite.ctx, suite.randomNumberHashes[i], suite.timestamps[i], - uint64(360), sender, suite.addrs[8], TestSenderOtherChain, TestRecipientOtherChain, + uint64(360), sender, suite.addrs[9], TestSenderOtherChain, TestRecipientOtherChain, expectedRefundAmount, true) suite.NoError(err) diff --git a/x/cdp/keeper/keeper.go b/x/cdp/keeper/keeper.go index 0e028b59..8e761880 100644 --- a/x/cdp/keeper/keeper.go +++ b/x/cdp/keeper/keeper.go @@ -20,10 +20,12 @@ type Keeper struct { supplyKeeper types.SupplyKeeper auctionKeeper types.AuctionKeeper accountKeeper types.AccountKeeper + maccPerms map[string][]string } // NewKeeper creates a new keeper -func NewKeeper(cdc *codec.Codec, key sdk.StoreKey, paramstore subspace.Subspace, pfk types.PricefeedKeeper, ak types.AuctionKeeper, sk types.SupplyKeeper, ack types.AccountKeeper) Keeper { +func NewKeeper(cdc *codec.Codec, key sdk.StoreKey, paramstore subspace.Subspace, pfk types.PricefeedKeeper, + ak types.AuctionKeeper, sk types.SupplyKeeper, ack types.AccountKeeper, maccs map[string][]string) Keeper { if !paramstore.HasKeyTable() { paramstore = paramstore.WithKeyTable(types.ParamKeyTable()) } @@ -36,6 +38,7 @@ func NewKeeper(cdc *codec.Codec, key sdk.StoreKey, paramstore subspace.Subspace, auctionKeeper: ak, supplyKeeper: sk, accountKeeper: ack, + maccPerms: maccs, } } diff --git a/x/cdp/keeper/savings.go b/x/cdp/keeper/savings.go index f20898b0..c9d3fe53 100644 --- a/x/cdp/keeper/savings.go +++ b/x/cdp/keeper/savings.go @@ -7,12 +7,9 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" authexported "github.com/cosmos/cosmos-sdk/x/auth/exported" - authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" supplyexported "github.com/cosmos/cosmos-sdk/x/supply/exported" - auctiontypes "github.com/kava-labs/kava/x/auction/types" "github.com/kava-labs/kava/x/cdp/types" - kdtypes "github.com/kava-labs/kava/x/kavadist/types" ) // DistributeSavingsRate distributes surplus that has accumulated in the liquidator account to address holding stable coins according the the savings rate @@ -81,16 +78,14 @@ func (k Keeper) SetPreviousSavingsDistribution(ctx sdk.Context, distTime time.Ti store.Set([]byte{}, k.cdc.MustMarshalBinaryLengthPrefixed(distTime)) } +// getModuleAccountCoins gets the total coin balance of this coin currently held by module accounts func (k Keeper) getModuleAccountCoins(ctx sdk.Context, denom string) sdk.Coins { - // NOTE: these are the module accounts that could end up holding stable denoms at some point. - // Since there are currently no api methods to 'GetAllModuleAccounts', this function will need to be updated if a - // new module account is added which can hold stable denoms. - savingsRateMaccCoinAmount := k.supplyKeeper.GetModuleAccount(ctx, types.SavingsRateMacc).GetCoins().AmountOf(denom) - cdpMaccCoinAmount := k.supplyKeeper.GetModuleAccount(ctx, types.ModuleName).GetCoins().AmountOf(denom) - auctionMaccCoinAmount := k.supplyKeeper.GetModuleAccount(ctx, auctiontypes.ModuleName).GetCoins().AmountOf(denom) - liquidatorMaccCoinAmount := k.supplyKeeper.GetModuleAccount(ctx, types.LiquidatorMacc).GetCoins().AmountOf(denom) - feeMaccCoinAmount := k.supplyKeeper.GetModuleAccount(ctx, authtypes.FeeCollectorName).GetCoins().AmountOf(denom) - kavadistMaccCoinAmount := k.supplyKeeper.GetModuleAccount(ctx, kdtypes.ModuleName).GetCoins().AmountOf(denom) - totalModAccountAmount := savingsRateMaccCoinAmount.Add(cdpMaccCoinAmount).Add(auctionMaccCoinAmount).Add(liquidatorMaccCoinAmount).Add(feeMaccCoinAmount).Add(kavadistMaccCoinAmount) - return sdk.NewCoins(sdk.NewCoin(denom, totalModAccountAmount)) + totalModCoinBalance := sdk.NewCoins(sdk.NewCoin(denom, sdk.ZeroInt())) + for macc := range k.maccPerms { + modCoinBalance := k.supplyKeeper.GetModuleAccount(ctx, macc).GetCoins().AmountOf(denom) + if modCoinBalance.IsPositive() { + totalModCoinBalance = totalModCoinBalance.Add(sdk.NewCoin(denom, modCoinBalance)) + } + } + return totalModCoinBalance }