Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add safety check: restriction fn in bank module for mint perms #50

Merged
merged 1 commit into from
Jan 13, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,7 @@ Security Release. No breaking changes related to 0.44.x.
* [\#9428](https://github.com/cosmos/cosmos-sdk/pull/9428) Optimize bank InitGenesis. Added `k.initBalances`.
* [\#9429](https://github.com/cosmos/cosmos-sdk/pull/9429) Add `cosmos_sdk_version` to node_info
* [\#9541](https://github.com/cosmos/cosmos-sdk/pull/9541) Bump tendermint dependency to v0.34.11.
* (x/bank) [\#10771](https://github.com/cosmos/cosmos-sdk/pull/10771) Add safety check on bank module perms to allow module-specific mint restrictions (e.g. only minting a certain denom).

### Bug Fixes

Expand Down
49 changes: 39 additions & 10 deletions x/bank/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ var _ Keeper = (*BaseKeeper)(nil)
// between accounts.
type Keeper interface {
SendKeeper
WithMintCoinsRestriction(BankMintingRestrictionFn) BaseKeeper

InitGenesis(sdk.Context, *types.GenesisState)
ExportGenesis(sdk.Context) *types.GenesisState
Expand Down Expand Up @@ -52,12 +53,15 @@ type Keeper interface {
type BaseKeeper struct {
BaseSendKeeper

ak types.AccountKeeper
cdc codec.BinaryCodec
storeKey sdk.StoreKey
paramSpace paramtypes.Subspace
ak types.AccountKeeper
cdc codec.BinaryCodec
storeKey sdk.StoreKey
paramSpace paramtypes.Subspace
mintCoinsRestrictionFn BankMintingRestrictionFn
}

type BankMintingRestrictionFn func(ctx sdk.Context, coins sdk.Coins) error

// GetPaginatedTotalSupply queries for the supply, ignoring 0 coins, with a given pagination
func (k BaseKeeper) GetPaginatedTotalSupply(ctx sdk.Context, pagination *query.PageRequest) (sdk.Coins, *query.PageResponse, error) {
store := ctx.KVStore(k.storeKey)
Expand Down Expand Up @@ -104,12 +108,32 @@ func NewBaseKeeper(
}

return BaseKeeper{
BaseSendKeeper: NewBaseSendKeeper(cdc, storeKey, ak, paramSpace, blockedAddrs),
ak: ak,
cdc: cdc,
storeKey: storeKey,
paramSpace: paramSpace,
BaseSendKeeper: NewBaseSendKeeper(cdc, storeKey, ak, paramSpace, blockedAddrs),
ak: ak,
cdc: cdc,
storeKey: storeKey,
paramSpace: paramSpace,
mintCoinsRestrictionFn: func(ctx sdk.Context, coins sdk.Coins) error { return nil },
}
}

// WithMintCoinsRestriction restricts the bank Keeper used within a specific module to
// have restricted permissions on minting speicified denoms.
func (k BaseKeeper) WithMintCoinsRestriction(NewRestrictionFn BankMintingRestrictionFn) BaseKeeper {
// this allows nesting restriction functions
oldRestrictionFn := k.mintCoinsRestrictionFn
k.mintCoinsRestrictionFn = func(ctx sdk.Context, coins sdk.Coins) error {
err := NewRestrictionFn(ctx, coins)
if err != nil {
return err
}
err = oldRestrictionFn(ctx, coins)
if err != nil {
return err
}
return nil
}
return k
}

// DelegateCoins performs delegation by deducting amt coins from an account with
Expand Down Expand Up @@ -391,6 +415,11 @@ func (k BaseKeeper) UndelegateCoinsFromModuleToAccount(
// MintCoins creates new coins from thin air and adds it to the module account.
// It will panic if the module account does not exist or is unauthorized.
func (k BaseKeeper) MintCoins(ctx sdk.Context, moduleName string, amounts sdk.Coins) error {
err := k.mintCoinsRestrictionFn(ctx, amounts)
if err != nil {
ctx.Logger().Error(fmt.Sprintf("Module %s attempted minting coins %s it did not have permission for with error %v", moduleName, amounts, err))
return err
}
acc := k.ak.GetModuleAccount(ctx, moduleName)
if acc == nil {
panic(sdkerrors.Wrapf(sdkerrors.ErrUnknownAddress, "module account %s does not exist", moduleName))
Expand All @@ -400,7 +429,7 @@ func (k BaseKeeper) MintCoins(ctx sdk.Context, moduleName string, amounts sdk.Co
panic(sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "module account %s does not have permissions to mint tokens", moduleName))
}

err := k.addCoins(ctx, acc.GetAddress(), amounts)
err = k.addCoins(ctx, acc.GetAddress(), amounts)
if err != nil {
return err
}
Expand Down
70 changes: 70 additions & 0 deletions x/bank/keeper/keeper_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package keeper_test

import (
"fmt"
"testing"
"time"

Expand Down Expand Up @@ -1221,6 +1222,75 @@ func (suite *IntegrationTestSuite) getTestMetadata() []types.Metadata {
}
}

func (suite *IntegrationTestSuite) TestMintCoinRestrictions() {
type BankMintingRestrictionFn func(ctx sdk.Context, coins sdk.Coins) error

maccPerms := simapp.GetMaccPerms()
maccPerms[multiPerm] = []string{authtypes.Burner, authtypes.Minter, authtypes.Staking}

suite.app.AccountKeeper = authkeeper.NewAccountKeeper(
suite.app.AppCodec(), suite.app.GetKey(authtypes.StoreKey), suite.app.GetSubspace(authtypes.ModuleName),
authtypes.ProtoBaseAccount, maccPerms)
suite.app.AccountKeeper.SetModuleAccount(suite.ctx, multiPermAcc)

type testCase struct {
coinsToTry sdk.Coin
expectPass bool
}

tests := []struct {
name string
restrictionFn BankMintingRestrictionFn
testCases []testCase
}{
{
"restriction",
func(ctx sdk.Context, coins sdk.Coins) error {
for _, coin := range coins {
if coin.Denom != fooDenom {
return fmt.Errorf("Module %s only has perms for minting %s coins, tried minting %s coins", types.ModuleName, fooDenom, coin.Denom)
}
}
return nil
},
[]testCase{
{
coinsToTry: newFooCoin(100),
expectPass: true,
},
{
coinsToTry: newBarCoin(100),
expectPass: false,
},
},
},
}

for _, test := range tests {
suite.app.BankKeeper = keeper.NewBaseKeeper(suite.app.AppCodec(), suite.app.GetKey(types.StoreKey),
suite.app.AccountKeeper, suite.app.GetSubspace(types.ModuleName), nil).WithMintCoinsRestriction(keeper.BankMintingRestrictionFn(test.restrictionFn))
for _, testCase := range test.testCases {
if testCase.expectPass {
suite.Require().NoError(
suite.app.BankKeeper.MintCoins(
suite.ctx,
multiPermAcc.Name,
sdk.NewCoins(testCase.coinsToTry),
),
)
} else {
suite.Require().Error(
suite.app.BankKeeper.MintCoins(
suite.ctx,
multiPermAcc.Name,
sdk.NewCoins(testCase.coinsToTry),
),
)
}
}
}
}

func TestKeeperTestSuite(t *testing.T) {
suite.Run(t, new(IntegrationTestSuite))
}
3 changes: 3 additions & 0 deletions x/bank/spec/02_keepers.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,14 @@ message Output {

The base keeper provides full-permission access: the ability to arbitrary modify any account's balance and mint or burn coins.

Restricted permission to mint per module could be achieved by using baseKeeper with `WithMintCoinsRestriction` to give specific restrictions to mint (e.g. only minting certain denom).

```go
// Keeper defines a module interface that facilitates the transfer of coins
// between accounts.
type Keeper interface {
SendKeeper
WithMintCoinsRestriction(NewRestrictionFn BankMintingRestrictionFn) BaseKeeper

InitGenesis(sdk.Context, *types.GenesisState)
ExportGenesis(sdk.Context) *types.GenesisState
Expand Down