Merge pull request from GHSA-2f3p-6gfj-jccq

* x/bank: update SendCoinsFromModuleToAccount * x/bank: add TestSendCoinsFromModuleToAccount_Blacklist * CL++ * fix tests * godoc++
cosmos · Mar 24, 2021 · 56c312a · 56c312a
1 parent 863d7d0
commit 56c312a
Show file tree

Hide file tree

Showing 16 changed files with 115 additions and 75 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -51,7 +51,6 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * [\#8849](https://github.com/cosmos/cosmos-sdk/pull/8849) Upgrade module no longer supports time based upgrades. 
 * [\#8880](https://github.com/cosmos/cosmos-sdk/pull/8880) The CLI `simd migrate v0.40 ...` command has been renamed to `simd migrate v0.42`.
 
-
 ### API Breaking Changes
 
 * (keyring) [#\8662](https://github.com/cosmos/cosmos-sdk/pull/8662) `NewMnemonic` now receives an additional `passphrase` argument to secure the key generated by the bip39 mnemonic.
@@ -67,7 +66,6 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (x/auth) [\#8129](https://github.com/cosmos/cosmos-sdk/pull/8828) Updated `SigVerifiableTx.GetPubKeys` method signature to return error.
 * [\#8682](https://github.com/cosmos/cosmos-sdk/pull/8682) `ante.NewAnteHandler` updated to receive all positional params as `ante.HandlerOptions` struct. If required fields aren't set, throws error accordingly.
 
-
 ### State Machine Breaking
 
 * (x/{bank,distrib,gov,slashing,staking}) [\#8363](https://github.com/cosmos/cosmos-sdk/issues/8363) Store keys have been modified to allow for variable-length addresses.

diff --git a/simapp/test_helpers.go b/simapp/test_helpers.go
@@ -437,12 +437,30 @@ func (ao EmptyAppOptions) Get(o string) interface{} {
 	return nil
 }
 
-// FundAccount is a utility function that funds an account by minting and sending the coins to the address
-// TODO(fdymylja): instead of using the mint module account, which has the permission of minting, create a "faucet" account
+// FundAccount is a utility function that funds an account by minting and
+// sending the coins to the address. This should be used for testing purposes
+// only!
+//
+// TODO: Instead of using the mint module account, which has the
+// permission of minting, create a "faucet" account. (@fdymylja)
 func FundAccount(app *SimApp, ctx sdk.Context, addr sdk.AccAddress, amounts sdk.Coins) error {
-	err := app.BankKeeper.MintCoins(ctx, minttypes.ModuleName, amounts)
-	if err != nil {
+	if err := app.BankKeeper.MintCoins(ctx, minttypes.ModuleName, amounts); err != nil {
 		return err
 	}
+
 	return app.BankKeeper.SendCoinsFromModuleToAccount(ctx, minttypes.ModuleName, addr, amounts)
 }
+
+// FundModuleAccount is a utility function that funds a module account by
+// minting and sending the coins to the address. This should be used for testing
+// purposes only!
+//
+// TODO: Instead of using the mint module account, which has the
+// permission of minting, create a "faucet" account. (@fdymylja)
+func FundModuleAccount(app *SimApp, ctx sdk.Context, recipientMod string, amounts sdk.Coins) error {
+	if err := app.BankKeeper.MintCoins(ctx, minttypes.ModuleName, amounts); err != nil {
+		return err
+	}
+
+	return app.BankKeeper.SendCoinsFromModuleToModule(ctx, minttypes.ModuleName, recipientMod, amounts)
+}
diff --git a/x/bank/keeper/genesis_test.go b/x/bank/keeper/genesis_test.go
@@ -40,17 +40,17 @@ func (suite *IntegrationTestSuite) TestExportGenesis() {
 
 func (suite *IntegrationTestSuite) getTestBalancesAndSupply() ([]types.Balance, sdk.Coins) {
 	addr2, _ := sdk.AccAddressFromBech32("cosmos1f9xjhxm0plzrh9cskf4qee4pc2xwp0n0556gh0")
-	addr1, _ := sdk.AccAddressFromBech32("cosmos1fl48vsnmsdzcv85q5d2q4z5ajdha8yu34mf0eh")
+	addr1, _ := sdk.AccAddressFromBech32("cosmos1t5u0jfg3ljsjrh2m9e47d4ny2hea7eehxrzdgd")
 	addr1Balance := sdk.Coins{sdk.NewInt64Coin("testcoin3", 10)}
 	addr2Balance := sdk.Coins{sdk.NewInt64Coin("testcoin1", 32), sdk.NewInt64Coin("testcoin2", 34)}
 
 	totalSupply := addr1Balance
 	totalSupply = totalSupply.Add(addr2Balance...)
+
 	return []types.Balance{
 		{Address: addr2.String(), Coins: addr2Balance},
 		{Address: addr1.String(), Coins: addr1Balance},
 	}, totalSupply
-
 }
 
 func (suite *IntegrationTestSuite) TestInitGenesis() {
@@ -70,7 +70,7 @@ func (suite *IntegrationTestSuite) TestTotalSupply() {
 	defaultGenesis := types.DefaultGenesisState()
 	balances := []types.Balance{
 		{Coins: sdk.NewCoins(sdk.NewCoin("foocoin", sdk.NewInt(1))), Address: "cosmos1f9xjhxm0plzrh9cskf4qee4pc2xwp0n0556gh0"},
-		{Coins: sdk.NewCoins(sdk.NewCoin("barcoin", sdk.NewInt(1))), Address: "cosmos1fl48vsnmsdzcv85q5d2q4z5ajdha8yu34mf0eh"},
+		{Coins: sdk.NewCoins(sdk.NewCoin("barcoin", sdk.NewInt(1))), Address: "cosmos1t5u0jfg3ljsjrh2m9e47d4ny2hea7eehxrzdgd"},
 		{Coins: sdk.NewCoins(sdk.NewCoin("foocoin", sdk.NewInt(10)), sdk.NewCoin("barcoin", sdk.NewInt(20))), Address: "cosmos1m3h30wlvsf8llruxtpukdvsy0km2kum8g38c8q"},
 	}
 	totalSupply := sdk.NewCoins(sdk.NewCoin("foocoin", sdk.NewInt(11)), sdk.NewCoin("barcoin", sdk.NewInt(21)))

diff --git a/x/bank/keeper/keeper.go b/x/bank/keeper/keeper.go
@@ -268,7 +268,8 @@ func (k BaseKeeper) SetDenomMetaData(ctx sdk.Context, denomMetaData types.Metada
 }
 
 // SendCoinsFromModuleToAccount transfers coins from a ModuleAccount to an AccAddress.
-// It will panic if the module account does not exist.
+// It will panic if the module account does not exist. An error is returned if
+// the recipient address is black-listed or if sending the tokens fails.
 func (k BaseKeeper) SendCoinsFromModuleToAccount(
 	ctx sdk.Context, senderModule string, recipientAddr sdk.AccAddress, amt sdk.Coins,
 ) error {
@@ -278,6 +279,10 @@ func (k BaseKeeper) SendCoinsFromModuleToAccount(
 		panic(sdkerrors.Wrapf(sdkerrors.ErrUnknownAddress, "module account %s does not exist", senderModule))
 	}
 
+	if k.BlockedAddr(recipientAddr) {
+		return sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to receive funds", recipientAddr)
+	}
+
 	return k.SendCoins(ctx, senderAddr, recipientAddr, amt)
 }
 

diff --git a/x/bank/keeper/keeper_test.go b/x/bank/keeper/keeper_test.go
@@ -97,6 +97,36 @@ func (suite *IntegrationTestSuite) TestSupply() {
 	suite.Require().Equal(totalSupply, total)
 }
 
+func (suite *IntegrationTestSuite) TestSendCoinsFromModuleToAccount_Blacklist() {
+	app := simapp.Setup(false)
+	ctx := app.BaseApp.NewContext(false, tmproto.Header{Height: 1})
+	appCodec := app.AppCodec()
+
+	// add module accounts to supply keeper
+	maccPerms := simapp.GetMaccPerms()
+	maccPerms[holder] = nil
+	maccPerms[authtypes.Burner] = []string{authtypes.Burner}
+	maccPerms[authtypes.Minter] = []string{authtypes.Minter}
+	maccPerms[multiPerm] = []string{authtypes.Burner, authtypes.Minter, authtypes.Staking}
+	maccPerms[randomPerm] = []string{"random"}
+
+	addr1 := sdk.AccAddress([]byte("addr1_______________"))
+
+	authKeeper := authkeeper.NewAccountKeeper(
+		appCodec, app.GetKey(types.StoreKey), app.GetSubspace(types.ModuleName),
+		authtypes.ProtoBaseAccount, maccPerms,
+	)
+	keeper := keeper.NewBaseKeeper(
+		appCodec, app.GetKey(types.StoreKey), authKeeper,
+		app.GetSubspace(types.ModuleName), map[string]bool{addr1.String(): true},
+	)
+
+	suite.Require().NoError(keeper.MintCoins(ctx, minttypes.ModuleName, initCoins))
+	suite.Require().Error(keeper.SendCoinsFromModuleToAccount(
+		ctx, minttypes.ModuleName, addr1, initCoins,
+	))
+}
+
 func (suite *IntegrationTestSuite) TestSupply_SendCoins() {
 	app := simapp.Setup(false)
 	ctx := app.BaseApp.NewContext(false, tmproto.Header{Height: 1})

diff --git a/x/distribution/keeper/allocation_test.go b/x/distribution/keeper/allocation_test.go
@@ -83,7 +83,7 @@ func TestAllocateTokensToManyValidators(t *testing.T) {
 	require.NotNil(t, feeCollector)
 
 	// fund fee collector
-	require.NoError(t, simapp.FundAccount(app, ctx, feeCollector.GetAddress(), fees))
+	require.NoError(t, simapp.FundModuleAccount(app, ctx, feeCollector.GetName(), fees))
 
 	app.AccountKeeper.SetAccount(ctx, feeCollector)
 
@@ -163,7 +163,7 @@ func TestAllocateTokensTruncation(t *testing.T) {
 	feeCollector := app.AccountKeeper.GetModuleAccount(ctx, types.FeeCollectorName)
 	require.NotNil(t, feeCollector)
 
-	require.NoError(t, simapp.FundAccount(app, ctx, feeCollector.GetAddress(), fees))
+	require.NoError(t, simapp.FundModuleAccount(app, ctx, feeCollector.GetName(), fees))
 
 	app.AccountKeeper.SetAccount(ctx, feeCollector)
 

diff --git a/x/distribution/keeper/delegation_test.go b/x/distribution/keeper/delegation_test.go
@@ -280,7 +280,7 @@ func TestWithdrawDelegationRewardsBasic(t *testing.T) {
 
 	// set module account coins
 	distrAcc := app.DistrKeeper.GetDistributionAccount(ctx)
-	require.NoError(t, simapp.FundAccount(app, ctx, distrAcc.GetAddress(), sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, balanceTokens))))
+	require.NoError(t, simapp.FundModuleAccount(app, ctx, distrAcc.GetName(), sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, balanceTokens))))
 	app.AccountKeeper.SetModuleAccount(ctx, distrAcc)
 
 	// create validator with 50% commission
@@ -492,7 +492,7 @@ func TestCalculateRewardsMultiDelegatorMultWithdraw(t *testing.T) {
 
 	// set module account coins
 	distrAcc := app.DistrKeeper.GetDistributionAccount(ctx)
-	require.NoError(t, simapp.FundAccount(app, ctx, distrAcc.GetAddress(), sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(1000)))))
+	require.NoError(t, simapp.FundModuleAccount(app, ctx, distrAcc.GetName(), sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(1000)))))
 	app.AccountKeeper.SetModuleAccount(ctx, distrAcc)
 
 	tokens := sdk.DecCoins{sdk.NewDecCoinFromDec(sdk.DefaultBondDenom, sdk.NewDec(initial))}

diff --git a/x/distribution/keeper/keeper_test.go b/x/distribution/keeper/keeper_test.go
@@ -49,7 +49,7 @@ func TestWithdrawValidatorCommission(t *testing.T) {
 	// set module account coins
 	distrAcc := app.DistrKeeper.GetDistributionAccount(ctx)
 	coins := sdk.NewCoins(sdk.NewCoin("mytoken", sdk.NewInt(2)), sdk.NewCoin("stake", sdk.NewInt(2)))
-	require.NoError(t, simapp.FundAccount(app, ctx, distrAcc.GetAddress(), coins))
+	require.NoError(t, simapp.FundModuleAccount(app, ctx, distrAcc.GetName(), coins))
 
 	app.AccountKeeper.SetModuleAccount(ctx, distrAcc)
 

diff --git a/x/distribution/proposal_handler_test.go b/x/distribution/proposal_handler_test.go
@@ -33,7 +33,7 @@ func TestProposalHandlerPassed(t *testing.T) {
 	// add coins to the module account
 	macc := app.DistrKeeper.GetDistributionAccount(ctx)
 	balances := app.BankKeeper.GetAllBalances(ctx, macc.GetAddress())
-	require.NoError(t, simapp.FundAccount(app, ctx, macc.GetAddress(), amount))
+	require.NoError(t, simapp.FundModuleAccount(app, ctx, macc.GetName(), amount))
 
 	app.AccountKeeper.SetModuleAccount(ctx, macc)
 

diff --git a/x/distribution/simulation/operations_test.go b/x/distribution/simulation/operations_test.go
@@ -143,7 +143,7 @@ func (suite *SimTestSuite) testSimulateMsgWithdrawValidatorCommission(tokenName
 
 	// set module account coins
 	distrAcc := suite.app.DistrKeeper.GetDistributionAccount(suite.ctx)
-	suite.Require().NoError(simapp.FundAccount(suite.app, suite.ctx, distrAcc.GetAddress(), sdk.NewCoins(
+	suite.Require().NoError(simapp.FundModuleAccount(suite.app, suite.ctx, distrAcc.GetName(), sdk.NewCoins(
 		sdk.NewCoin(tokenName, sdk.NewInt(10)),
 		sdk.NewCoin("stake", sdk.NewInt(5)),
 	)))

diff --git a/x/staking/genesis_test.go b/x/staking/genesis_test.go
@@ -5,8 +5,6 @@ import (
 	"log"
 	"testing"
 
-	auth "github.com/cosmos/cosmos-sdk/x/auth/types"
-
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	abci "github.com/tendermint/tendermint/abci/types"
@@ -66,15 +64,12 @@ func TestInitGenesis(t *testing.T) {
 	log.Printf("%#v", len(validators))
 	// mint coins in the bonded pool representing the validators coins
 	require.NoError(t,
-		simapp.FundAccount(
+		simapp.FundModuleAccount(
 			app,
 			ctx,
-			auth.NewModuleAddress(types.BondedPoolName),
+			types.BondedPoolName,
 			sdk.NewCoins(
-				sdk.NewCoin(
-					params.BondDenom,
-					valTokens.MulRaw((int64)(len(validators))),
-				),
+				sdk.NewCoin(params.BondDenom, valTokens.MulRaw((int64)(len(validators)))),
 			),
 		),
 	)
@@ -182,16 +177,16 @@ func TestInitGenesisLargeValidatorSet(t *testing.T) {
 		// add bonded coins
 		bondedPoolAmt = bondedPoolAmt.Add(tokens)
 	}
+
 	genesisState := types.NewGenesisState(params, validators, delegations)
+
 	// mint coins in the bonded pool representing the validators coins
 	require.NoError(t,
-		simapp.FundAccount(
+		simapp.FundModuleAccount(
 			app,
 			ctx,
-			auth.NewModuleAddress(types.BondedPoolName),
-			sdk.NewCoins(
-				sdk.NewCoin(params.BondDenom, bondedPoolAmt),
-			),
+			types.BondedPoolName,
+			sdk.NewCoins(sdk.NewCoin(params.BondDenom, bondedPoolAmt)),
 		),
 	)
 

diff --git a/x/staking/handler_test.go b/x/staking/handler_test.go
@@ -38,7 +38,7 @@ func bootstrapHandlerGenesisTest(t *testing.T, power int64, numAddrs int, accAmo
 
 	// set non bonded pool balance
 	app.AccountKeeper.SetModuleAccount(ctx, notBondedPool)
-	require.NoError(t, simapp.FundAccount(app, ctx, notBondedPool.GetAddress(), totalSupply))
+	require.NoError(t, simapp.FundModuleAccount(app, ctx, notBondedPool.GetName(), totalSupply))
 	return app, ctx, addrDels, addrVals
 }