Merge PR #4797: blacklist module accounts from receiving txs

Author: fedekunze

.pending/bugfixes/modules/_4795-restrict-txs-m

@@ -0,0 +1,2 @@
+#4795 restrict module accounts from receiving transactions.
+Allowing this would cause an invariant on the module account coins.

docs/spec/supply/01_concepts.md

@@ -2,10 +2,11 @@
 
 ## Supply
 
-The `supply` module: 
- - passively tracks the total supply of coins within a chain, 
- - provides a pattern for modules to hold/interact with `Coins`, and 
- - introduces the invariant check to verify a chain's total supply.
+The `supply` module:
+
+- passively tracks the total supply of coins within a chain,
+- provides a pattern for modules to hold/interact with `Coins`, and
+- introduces the invariant check to verify a chain's total supply.
 
 ### Total Supply
 
@@ -32,11 +33,14 @@ The `ModuleAccount` interface is defined as follows:
 type ModuleAccount interface {
   auth.Account               // same methods as the Account interface
   GetName() string           // name of the module; used to obtain the address
-  GetPermissions() []string  // permissions of module account 
-  HasPermission(string) bool 
+  GetPermissions() []string  // permissions of module account
+  HasPermission(string) bool
 }
 ```
 
+> **WARNING!**
+Any module or message handler that allows either direct or indirect sending of funds must explicitly guarantee those funds cannot be sent to module accounts (unless allowed).
+
 The supply `Keeper` also introduces new wrapper functions for the auth `Keeper`
 and the bank `Keeper` that are related to `ModuleAccount`s in order to be able
 to:

simapp/app.go

@@ -141,13 +141,13 @@ func NewSimApp(
 
 	// add keepers
 	app.accountKeeper = auth.NewAccountKeeper(app.cdc, keys[auth.StoreKey], authSubspace, auth.ProtoBaseAccount)
-	app.bankKeeper = bank.NewBaseKeeper(app.accountKeeper, bankSubspace, bank.DefaultCodespace)
+	app.bankKeeper = bank.NewBaseKeeper(app.accountKeeper, bankSubspace, bank.DefaultCodespace, app.ModuleAccountAddrs())
 	app.supplyKeeper = supply.NewKeeper(app.cdc, keys[supply.StoreKey], app.accountKeeper, app.bankKeeper, supply.DefaultCodespace, maccPerms)
 	stakingKeeper := staking.NewKeeper(app.cdc, keys[staking.StoreKey], tkeys[staking.TStoreKey],
 		app.supplyKeeper, stakingSubspace, staking.DefaultCodespace)
 	app.mintKeeper = mint.NewKeeper(app.cdc, keys[mint.StoreKey], mintSubspace, &stakingKeeper, app.supplyKeeper, auth.FeeCollectorName)
 	app.distrKeeper = distr.NewKeeper(app.cdc, keys[distr.StoreKey], distrSubspace, &stakingKeeper,
-		app.supplyKeeper, distr.DefaultCodespace, auth.FeeCollectorName)
+		app.supplyKeeper, distr.DefaultCodespace, auth.FeeCollectorName, app.ModuleAccountAddrs())
 	app.slashingKeeper = slashing.NewKeeper(app.cdc, keys[slashing.StoreKey], &stakingKeeper,
 		slashingSubspace, slashing.DefaultCodespace)
 	app.crisisKeeper = crisis.NewKeeper(crisisSubspace, invCheckPeriod, app.supplyKeeper, auth.FeeCollectorName)

x/bank/app_test.go

@@ -5,11 +5,8 @@ import (
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/cosmos/cosmos-sdk/x/auth"
-	"github.com/cosmos/cosmos-sdk/x/bank"
-	"github.com/cosmos/cosmos-sdk/x/bank/internal/keeper"
 	"github.com/cosmos/cosmos-sdk/x/bank/internal/types"
 	"github.com/cosmos/cosmos-sdk/x/mock"
-	"github.com/cosmos/cosmos-sdk/x/supply"
 
 	"github.com/stretchr/testify/require"
 
@@ -50,6 +47,7 @@ var (
 	freeFee   = auth.NewStdFee(100000, sdk.Coins{sdk.NewInt64Coin("foocoin", 0)})
 
 	sendMsg1 = types.NewMsgSend(addr1, addr2, coins)
+	sendMsg2 = types.NewMsgSend(addr1, moduleAccAddr, coins)
 
 	multiSendMsg1 = types.MsgMultiSend{
 		Inputs:  []types.Input{types.NewInput(addr1, coins)},
@@ -88,26 +86,15 @@ var (
 			types.NewOutput(addr2, manyCoins),
 		},
 	}
-)
-
-// initialize the mock application for this module
-func getMockApp(t *testing.T) *mock.App {
-	mapp, err := getBenchmarkMockApp()
-	supply.RegisterCodec(mapp.Cdc)
-	require.NoError(t, err)
-	return mapp
-}
-
-// overwrite the mock init chainer
-func getInitChainer(mapp *mock.App, keeper keeper.BaseKeeper) sdk.InitChainer {
-	return func(ctx sdk.Context, req abci.RequestInitChain) abci.ResponseInitChain {
-		mapp.InitChainer(ctx, req)
-		bankGenesis := bank.DefaultGenesisState()
-		bank.InitGenesis(ctx, keeper, bankGenesis)
-
-		return abci.ResponseInitChain{}
+	multiSendMsg6 = types.MsgMultiSend{
+		Inputs: []types.Input{
+			types.NewInput(addr1, coins),
+		},
+		Outputs: []types.Output{
+			types.NewOutput(moduleAccAddr, coins),
+		},
 	}
-}
+)
 
 func TestSendNotEnoughBalance(t *testing.T) {
 	mapp := getMockApp(t)
@@ -140,14 +127,53 @@ func TestSendNotEnoughBalance(t *testing.T) {
 	require.True(t, res2.GetSequence() == origSeq+1)
 }
 
+func TestSendToModuleAcc(t *testing.T) {
+	mapp := getMockApp(t)
+	acc := &auth.BaseAccount{
+		Address: addr1,
+		Coins:   coins,
+	}
+
+	macc := &auth.BaseAccount{
+		Address: moduleAccAddr,
+	}
+
+	mock.SetGenesis(mapp, []auth.Account{acc, macc})
+
+	ctxCheck := mapp.BaseApp.NewContext(true, abci.Header{})
+
+	res1 := mapp.AccountKeeper.GetAccount(ctxCheck, addr1)
+	require.NotNil(t, res1)
+	require.Equal(t, acc, res1.(*auth.BaseAccount))
+
+	origAccNum := res1.GetAccountNumber()
+	origSeq := res1.GetSequence()
+
+	header := abci.Header{Height: mapp.LastBlockHeight() + 1}
+	mock.SignCheckDeliver(t, mapp.Cdc, mapp.BaseApp, header, []sdk.Msg{sendMsg2}, []uint64{origAccNum}, []uint64{origSeq}, false, false, priv1)
+
+	mock.CheckBalance(t, mapp, addr1, coins)
+	mock.CheckBalance(t, mapp, moduleAccAddr, sdk.Coins(nil))
+
+	res2 := mapp.AccountKeeper.GetAccount(mapp.NewContext(true, abci.Header{}), addr1)
+	require.NotNil(t, res2)
+
+	require.True(t, res2.GetAccountNumber() == origAccNum)
+	require.True(t, res2.GetSequence() == origSeq+1)
+}
+
 func TestMsgMultiSendWithAccounts(t *testing.T) {
 	mapp := getMockApp(t)
 	acc := &auth.BaseAccount{
 		Address: addr1,
 		Coins:   sdk.Coins{sdk.NewInt64Coin("foocoin", 67)},
 	}
 
-	mock.SetGenesis(mapp, []auth.Account{acc})
+	macc := &auth.BaseAccount{
+		Address: moduleAccAddr,
+	}
+
+	mock.SetGenesis(mapp, []auth.Account{acc, macc})
 
 	ctxCheck := mapp.BaseApp.NewContext(true, abci.Header{})
 
@@ -176,6 +202,14 @@ func TestMsgMultiSendWithAccounts(t *testing.T) {
 			expPass:    false,
 			privKeys:   []crypto.PrivKey{priv1},
 		},
+		{
+			msgs:       []sdk.Msg{multiSendMsg6},
+			accNums:    []uint64{0},
+			accSeqs:    []uint64{0},
+			expSimPass: false,
+			expPass:    false,
+			privKeys:   []crypto.PrivKey{priv1},
+		},
 	}
 
 	for _, tc := range testCases {

x/bank/bench_test.go

@@ -3,6 +3,7 @@ package bank_test
 import (
 	"testing"
 
+	"github.com/stretchr/testify/require"
 	abci "github.com/tendermint/tendermint/abci/types"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
@@ -11,18 +12,44 @@ import (
 	"github.com/cosmos/cosmos-sdk/x/bank/internal/keeper"
 	"github.com/cosmos/cosmos-sdk/x/bank/internal/types"
 	"github.com/cosmos/cosmos-sdk/x/mock"
+	"github.com/cosmos/cosmos-sdk/x/supply"
 )
 
+var moduleAccAddr = sdk.AccAddress([]byte("moduleAcc"))
+
+// initialize the mock application for this module
+func getMockApp(t *testing.T) *mock.App {
+	mapp, err := getBenchmarkMockApp()
+	supply.RegisterCodec(mapp.Cdc)
+	require.NoError(t, err)
+	return mapp
+}
+
+// overwrite the mock init chainer
+func getInitChainer(mapp *mock.App, keeper keeper.BaseKeeper) sdk.InitChainer {
+	return func(ctx sdk.Context, req abci.RequestInitChain) abci.ResponseInitChain {
+		mapp.InitChainer(ctx, req)
+		bankGenesis := bank.DefaultGenesisState()
+		bank.InitGenesis(ctx, keeper, bankGenesis)
+
+		return abci.ResponseInitChain{}
+	}
+}
+
 // getBenchmarkMockApp initializes a mock application for this module, for purposes of benchmarking
 // Any long term API support commitments do not apply to this function.
 func getBenchmarkMockApp() (*mock.App, error) {
 	mapp := mock.NewApp()
-
 	types.RegisterCodec(mapp.Cdc)
+
+	blacklistedAddrs := make(map[string]bool)
+	blacklistedAddrs[moduleAccAddr.String()] = true
+
 	bankKeeper := keeper.NewBaseKeeper(
 		mapp.AccountKeeper,
 		mapp.ParamsKeeper.Subspace(types.DefaultParamspace),
 		types.DefaultCodespace,
+		blacklistedAddrs,
 	)
 	mapp.Router().AddRoute(types.RouterKey, bank.NewHandler(bankKeeper))
 	mapp.SetInitChainer(getInitChainer(mapp, bankKeeper))

x/bank/handler.go

@@ -33,6 +33,10 @@ func handleMsgSend(ctx sdk.Context, k keeper.Keeper, msg types.MsgSend) sdk.Resu
 		return types.ErrSendDisabled(k.Codespace()).Result()
 	}
 
+	if k.BlacklistedAddr(msg.ToAddress) {
+		return sdk.ErrUnauthorized(fmt.Sprintf("%s is not allowed to receive transactions", msg.ToAddress)).Result()
+	}
+
 	err := k.SendCoins(ctx, msg.FromAddress, msg.ToAddress, msg.Amount)
 	if err != nil {
 		return err.Result()
@@ -55,6 +59,12 @@ func handleMsgMultiSend(ctx sdk.Context, k keeper.Keeper, msg types.MsgMultiSend
 		return types.ErrSendDisabled(k.Codespace()).Result()
 	}
 
+	for _, out := range msg.Outputs {
+		if k.BlacklistedAddr(out.Address) {
+			return sdk.ErrUnauthorized(fmt.Sprintf("%s is not allowed to receive transactions", out.Address)).Result()
+		}
+	}
+
 	err := k.InputOutputCoins(ctx, msg.Inputs, msg.Outputs)
 	if err != nil {
 		return err.Result()

x/bank/internal/keeper/common_test.go

@@ -0,0 +1,59 @@
+package keeper
+
+// DONTCOVER
+
+import (
+	abci "github.com/tendermint/tendermint/abci/types"
+	dbm "github.com/tendermint/tendermint/libs/db"
+	"github.com/tendermint/tendermint/libs/log"
+
+	"github.com/cosmos/cosmos-sdk/codec"
+	"github.com/cosmos/cosmos-sdk/store"
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	"github.com/cosmos/cosmos-sdk/x/auth"
+	"github.com/cosmos/cosmos-sdk/x/bank/internal/types"
+	"github.com/cosmos/cosmos-sdk/x/params"
+)
+
+type testInput struct {
+	cdc *codec.Codec
+	ctx sdk.Context
+	k   Keeper
+	ak  auth.AccountKeeper
+	pk  params.Keeper
+}
+
+func setupTestInput() testInput {
+	db := dbm.NewMemDB()
+
+	cdc := codec.New()
+	auth.RegisterCodec(cdc)
+	codec.RegisterCrypto(cdc)
+
+	authCapKey := sdk.NewKVStoreKey("authCapKey")
+	keyParams := sdk.NewKVStoreKey("params")
+	tkeyParams := sdk.NewTransientStoreKey("transient_params")
+
+	ms := store.NewCommitMultiStore(db)
+	ms.MountStoreWithDB(authCapKey, sdk.StoreTypeIAVL, db)
+	ms.MountStoreWithDB(keyParams, sdk.StoreTypeIAVL, db)
+	ms.MountStoreWithDB(tkeyParams, sdk.StoreTypeTransient, db)
+	ms.LoadLatestVersion()
+
+	blacklistedAddrs := make(map[string]bool)
+	blacklistedAddrs[sdk.AccAddress([]byte("moduleAcc")).String()] = true
+
+	pk := params.NewKeeper(cdc, keyParams, tkeyParams, params.DefaultCodespace)
+
+	ak := auth.NewAccountKeeper(
+		cdc, authCapKey, pk.Subspace(auth.DefaultParamspace), auth.ProtoBaseAccount,
+	)
+	ctx := sdk.NewContext(ms, abci.Header{ChainID: "test-chain-id"}, false, log.NewNopLogger())
+
+	ak.SetParams(ctx, auth.DefaultParams())
+
+	bankKeeper := NewBaseKeeper(ak, pk.Subspace(types.DefaultParamspace), types.DefaultCodespace, blacklistedAddrs)
+	bankKeeper.SetSendEnabled(ctx, true)
+
+	return testInput{cdc: cdc, ctx: ctx, k: bankKeeper, ak: ak, pk: pk}
+}

x/bank/internal/keeper/invariants.go

@@ -7,7 +7,7 @@ import (
 	"github.com/cosmos/cosmos-sdk/x/bank/internal/types"
 )
 
-// register bank invariants
+// RegisterInvariants registers the bank module invariants
 func RegisterInvariants(ir sdk.InvariantRegistry, ak types.AccountKeeper) {
 	ir.RegisterRoute(types.ModuleName, "nonnegative-outstanding",
 		NonnegativeBalanceInvariant(ak))

x/bank/internal/keeper/keeper.go

@@ -34,11 +34,11 @@ type BaseKeeper struct {
 // NewBaseKeeper returns a new BaseKeeper
 func NewBaseKeeper(ak types.AccountKeeper,
 	paramSpace params.Subspace,
-	codespace sdk.CodespaceType) BaseKeeper {
+	codespace sdk.CodespaceType, blacklistedAddrs map[string]bool) BaseKeeper {
 
 	ps := paramSpace.WithKeyTable(types.ParamKeyTable())
 	return BaseKeeper{
-		BaseSendKeeper: NewBaseSendKeeper(ak, ps, codespace),
+		BaseSendKeeper: NewBaseSendKeeper(ak, ps, codespace, blacklistedAddrs),
 		ak:             ak,
 		paramSpace:     ps,
 	}
@@ -145,6 +145,8 @@ type SendKeeper interface {
 
 	GetSendEnabled(ctx sdk.Context) bool
 	SetSendEnabled(ctx sdk.Context, enabled bool)
+
+	BlacklistedAddr(addr sdk.AccAddress) bool
 }
 
 var _ SendKeeper = (*BaseSendKeeper)(nil)
@@ -156,16 +158,20 @@ type BaseSendKeeper struct {
 
 	ak         types.AccountKeeper
 	paramSpace params.Subspace
+
+	// list of addresses that are restricted from receiving transactions
+	blacklistedAddrs map[string]bool
 }
 
 // NewBaseSendKeeper returns a new BaseSendKeeper.
 func NewBaseSendKeeper(ak types.AccountKeeper,
-	paramSpace params.Subspace, codespace sdk.CodespaceType) BaseSendKeeper {
+	paramSpace params.Subspace, codespace sdk.CodespaceType, blacklistedAddrs map[string]bool) BaseSendKeeper {
 
 	return BaseSendKeeper{
-		BaseViewKeeper: NewBaseViewKeeper(ak, codespace),
-		ak:             ak,
-		paramSpace:     paramSpace,
+		BaseViewKeeper:   NewBaseViewKeeper(ak, codespace),
+		ak:               ak,
+		paramSpace:       paramSpace,
+		blacklistedAddrs: blacklistedAddrs,
 	}
 }
 
@@ -321,6 +327,12 @@ func (keeper BaseSendKeeper) SetSendEnabled(ctx sdk.Context, enabled bool) {
 	keeper.paramSpace.Set(ctx, types.ParamStoreKeySendEnabled, &enabled)
 }
 
+// BlacklistedAddr checks if a given address is blacklisted (i.e restricted from
+// receiving funds)
+func (keeper BaseSendKeeper) BlacklistedAddr(addr sdk.AccAddress) bool {
+	return keeper.blacklistedAddrs[addr.String()]
+}
+
 var _ ViewKeeper = (*BaseViewKeeper)(nil)
 
 // ViewKeeper defines a module interface that facilitates read only access to