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.

Sign up for GitHub

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

Move allow list logic to msg server #550

Merged
merged 5 commits into from
Nov 4, 2024
Merged

Move allow list logic to msg server #550

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6cc6fc9
add all modules to allowlist exceptions
dssei Nov 1, 2024
a458efd
pass all modules list to keeper
dssei Nov 2, 2024
6fc1e8f
move the allow list logic to message sever
dssei Nov 3, 2024
c003cd7
remove unintended changes
dssei Nov 3, 2024
4d5e05d
add msg server tests
dssei Nov 4, 2024
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
252 changes: 109 additions & 143 deletions x/bank/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,108 +494,6 @@ func (suite *IntegrationTestSuite) TestInputOutputCoins() {
suite.Require().Equal(expected, acc3Balances)
}

func (suite *IntegrationTestSuite) TestInputOutputCoinsWithAllowList() {
app, ctx := suite.app, suite.ctx

addr1 := sdk.AccAddress("addr1_______________")
acc1 := app.AccountKeeper.NewAccountWithAddress(ctx, addr1)
app.AccountKeeper.SetAccount(ctx, acc1)
factoryCoin := newFactoryFooCoin(addr1, 100)
balances := sdk.NewCoins(factoryCoin)

addr2 := sdk.AccAddress("addr2_______________")
acc2 := app.AccountKeeper.NewAccountWithAddress(ctx, addr2)
app.AccountKeeper.SetAccount(ctx, acc2)

addr3 := sdk.AccAddress("addr3_______________")
acc3 := app.AccountKeeper.NewAccountWithAddress(ctx, addr3)
app.AccountKeeper.SetAccount(ctx, acc3)

addr4 := sdk.AccAddress("addr4_______________")
acc4 := app.AccountKeeper.NewAccountWithAddress(ctx, addr4)
app.AccountKeeper.SetAccount(ctx, acc4)

suite.Require().NoError(simapp.FundAccount(app.BankKeeper, ctx, addr1, balances))
app.BankKeeper.SetDenomAllowList(ctx, factoryCoin.Denom,
types.AllowList{
Addresses: []string{addr1.String(), addr2.String(), addr3.String()}})

inputs := []types.Input{
{Address: addr1.String(), Coins: sdk.NewCoins(newFactoryFooCoin(addr1, 30))},
{Address: addr1.String(), Coins: sdk.NewCoins(newFactoryFooCoin(addr1, 30))},
}
outputs := []types.Output{
{Address: addr2.String(), Coins: sdk.NewCoins(newFactoryFooCoin(addr1, 30))},
{Address: addr3.String(), Coins: sdk.NewCoins(newFactoryFooCoin(addr1, 30))},
}
suite.Require().NoError(app.BankKeeper.InputOutputCoins(ctx, inputs, outputs))

acc1Balances := app.BankKeeper.GetAllBalances(ctx, addr1)
expected := sdk.NewCoins(newFactoryFooCoin(addr1, 40))
suite.Require().Equal(expected, acc1Balances)

acc2Balances := app.BankKeeper.GetAllBalances(ctx, addr2)
expected = sdk.NewCoins(newFactoryFooCoin(addr1, 30))
suite.Require().Equal(expected, acc2Balances)

acc3Balances := app.BankKeeper.GetAllBalances(ctx, addr3)
expected = sdk.NewCoins(newFactoryFooCoin(addr1, 30))
suite.Require().Equal(expected, acc3Balances)

inputs1 := []types.Input{
{Address: addr1.String(), Coins: sdk.NewCoins(newFactoryFooCoin(addr1, 5))},
{Address: addr1.String(), Coins: sdk.NewCoins(newFactoryFooCoin(addr1, 10))},
}
outputs1 := []types.Output{
{Address: addr2.String(), Coins: sdk.NewCoins(newFactoryFooCoin(addr1, 5))},
{Address: addr4.String(), Coins: sdk.NewCoins(newFactoryFooCoin(addr1, 10))},
}

err := app.BankKeeper.InputOutputCoins(ctx, inputs1, outputs1)
suite.Require().Error(err)
suite.Require().Equal(
fmt.Sprintf("%s is not allowed to receive funds: unauthorized", addr4.String()), err.Error())

acc1Balances = app.BankKeeper.GetAllBalances(ctx, addr1)
expected = sdk.NewCoins(newFactoryFooCoin(addr1, 40))
suite.Require().Equal(expected, acc1Balances)

acc2Balances = app.BankKeeper.GetAllBalances(ctx, addr2)
expected = sdk.NewCoins(newFactoryFooCoin(addr1, 30))
suite.Require().Equal(expected, acc2Balances)

acc3Balances = app.BankKeeper.GetAllBalances(ctx, addr3)
expected = sdk.NewCoins(newFactoryFooCoin(addr1, 30))
suite.Require().Equal(expected, acc3Balances)

inputs2 := []types.Input{
{Address: addr4.String(), Coins: sdk.NewCoins(newFactoryFooCoin(addr1, 5))},
{Address: addr4.String(), Coins: sdk.NewCoins(newFactoryFooCoin(addr1, 10))},
}
outputs2 := []types.Output{
{Address: addr1.String(), Coins: sdk.NewCoins(newFactoryFooCoin(addr1, 5))},
{Address: addr2.String(), Coins: sdk.NewCoins(newFactoryFooCoin(addr1, 10))},
}

err = app.BankKeeper.InputOutputCoins(ctx, inputs2, outputs2)
suite.Require().Error(err)
suite.Require().Equal(
fmt.Sprintf("%s is not allowed to send funds: unauthorized", addr4.String()), err.Error())

acc1Balances = app.BankKeeper.GetAllBalances(ctx, addr1)
expected = sdk.NewCoins(newFactoryFooCoin(addr1, 40))
suite.Require().Equal(expected, acc1Balances)

acc2Balances = app.BankKeeper.GetAllBalances(ctx, addr2)
expected = sdk.NewCoins(newFactoryFooCoin(addr1, 30))
suite.Require().Equal(expected, acc2Balances)

acc3Balances = app.BankKeeper.GetAllBalances(ctx, addr3)
expected = sdk.NewCoins(newFactoryFooCoin(addr1, 30))
suite.Require().Equal(expected, acc3Balances)

}

func (suite *IntegrationTestSuite) TestSendCoins() {
app, ctx := suite.app, suite.ctx
balances := sdk.NewCoins(newFooCoin(100), newBarCoin(50))
Expand Down Expand Up @@ -661,68 +559,136 @@ func (suite *IntegrationTestSuite) TestSendCoinsWithAllowList() {
suite.Require().Equal(expected, acc2Balances)
}

func (suite *IntegrationTestSuite) TestSendCoinsWithSenderNotInAllowList() {
app, ctx := suite.app, suite.ctx
// Test that creating allowlist does not block module to module transfers
func (suite *IntegrationTestSuite) TestSendCoinsModuleToModuleWithAllowList() {
// add module accounts to supply keeper
ctx := suite.ctx
_, keeper := suite.initKeepersWithmAccPerms(make(map[string]bool))
app := suite.app
app.BankKeeper = keeper

addr1 := sdk.AccAddress("addr1_______________")
acc1 := app.AccountKeeper.NewAccountWithAddress(ctx, addr1)
app.AccountKeeper.SetAccount(ctx, acc1)
factoryCoin := newFactoryFooCoin(addr1, 100)
balances := sdk.NewCoins(factoryCoin, newBarCoin(50))
suite.Require().NoError(simapp.FundAccount(app.BankKeeper, ctx, addr1, balances))
app.BankKeeper.SetDenomAllowList(ctx, factoryCoin.Denom, types.AllowList{
Addresses: []string{addr1.String()}})

addr2 := sdk.AccAddress("addr2_______________")
acc2 := app.AccountKeeper.NewAccountWithAddress(ctx, addr2)
app.AccountKeeper.SetAccount(ctx, acc2)
// set up bank balances
suite.Require().NoError(simapp.FundAccount(app.BankKeeper, ctx, multiPermAcc.GetAddress(), balances))

app.BankKeeper.SetDenomAllowList(ctx, factoryCoin.Denom,
types.AllowList{Addresses: []string{addr2.String()}})
sendCoins := sdk.NewCoins(newFactoryFooCoin(addr1, 50), newBarCoin(20))
suite.Require().NoError(app.BankKeeper.SendCoinsFromModuleToModule(ctx, multiPerm, randomPerm, sendCoins))
expectedBankBalances := sdk.NewCoins(newFactoryFooCoin(addr1, 50), newBarCoin(30))
// assert module balances correct
bals := app.BankKeeper.GetAllBalances(ctx, multiPermAcc.GetAddress())
suite.Require().Equal(expectedBankBalances, bals)
// assert receiver balances correct
userBals := app.BankKeeper.GetAllBalances(ctx, randomPermAcc.GetAddress())
suite.Require().Equal(sendCoins, userBals)
}

sendAmt := sdk.NewCoins(newFactoryFooCoin(addr1, 5), newBarCoin(5))
err := app.BankKeeper.SendCoins(ctx, addr1, addr2, sendAmt)
suite.Require().Error(err)
suite.Require().Equal(
fmt.Sprintf("%s is not allowed to send funds: unauthorized", addr1.String()), err.Error())
// Test that creating allowlist does not block sending from module to account even though we are not explicitly adding
// the module account to the allowlist
func (suite *IntegrationTestSuite) TestSendCoinsModuleToAccountWithAllowList() {
// add module accounts to supply keeper
ctx := suite.ctx
moduleAddresses := make(map[string]bool)
moduleAddresses[multiPermAcc.GetAddress().String()] = true
moduleAddresses[suite.app.AccountKeeper.GetModuleAddress("mint").String()] = true
_, keeper := suite.initKeepersWithmAccPerms(make(map[string]bool))
app := suite.app
app.BankKeeper = keeper

// Balances should remain the same
acc1Balances := app.BankKeeper.GetAllBalances(ctx, addr1)
expected := sdk.NewCoins(newFactoryFooCoin(addr1, 100), newBarCoin(50))
suite.Require().Equal(expected, acc1Balances)
addr1 := sdk.AccAddress("addr1_______________")
acc1 := app.AccountKeeper.NewAccountWithAddress(ctx, addr1)
app.AccountKeeper.SetAccount(ctx, acc1)
factoryCoin := newFactoryFooCoin(addr1, 100)
balances := sdk.NewCoins(factoryCoin, newBarCoin(50))
app.BankKeeper.SetDenomAllowList(ctx, factoryCoin.Denom, types.AllowList{
Addresses: []string{addr1.String()}})

acc2Balances := app.BankKeeper.GetAllBalances(ctx, addr2)
expected = sdk.NewCoins(newFactoryFooCoin(addr1, 0), newBarCoin(0))
suite.Require().Equal(expected, acc2Balances)
// set up bank balances
suite.Require().NoError(simapp.FundAccount(app.BankKeeper, ctx, multiPermAcc.GetAddress(), balances))

sendCoins := sdk.NewCoins(newFactoryFooCoin(addr1, 50), newBarCoin(20))
suite.Require().NoError(app.BankKeeper.SendCoinsFromModuleToAccount(ctx, multiPerm, addr1, sendCoins))

expectedBankBalances := sdk.NewCoins(newFactoryFooCoin(addr1, 50), newBarCoin(30))
// assert module balances correct
bals := app.BankKeeper.GetAllBalances(ctx, multiPermAcc.GetAddress())
suite.Require().Equal(expectedBankBalances, bals)
// assert receiver balances correct
userBals := app.BankKeeper.GetAllBalances(ctx, addr1)
suite.Require().Equal(sendCoins, userBals)
}

func (suite *IntegrationTestSuite) TestSendCoinsWithReceiverNotInAllowList() {
app, ctx := suite.app, suite.ctx
// Test that creating allowlist does not block sending from account to module even though we are not explicitly adding
// the module account to the allowlist
func (suite *IntegrationTestSuite) TestSendCoinsAccountToModuleWithAllowList() {
// add module accounts to supply keeper
ctx := suite.ctx
moduleAddresses := make(map[string]bool)
moduleAddresses[multiPermAcc.GetAddress().String()] = true
moduleAddresses[suite.app.AccountKeeper.GetModuleAddress("mint").String()] = true
_, keeper := suite.initKeepersWithmAccPerms(make(map[string]bool))
app := suite.app
app.BankKeeper = keeper

addr1 := sdk.AccAddress("addr1_______________")
acc1 := app.AccountKeeper.NewAccountWithAddress(ctx, addr1)
app.AccountKeeper.SetAccount(ctx, acc1)
factoryCoin := newFactoryFooCoin(addr1, 100)
balances := sdk.NewCoins(factoryCoin, newBarCoin(50))
app.BankKeeper.SetDenomAllowList(ctx, factoryCoin.Denom, types.AllowList{
Addresses: []string{addr1.String()}})

// set up bank balances
suite.Require().NoError(simapp.FundAccount(app.BankKeeper, ctx, addr1, balances))

addr2 := sdk.AccAddress("addr2_______________")
acc2 := app.AccountKeeper.NewAccountWithAddress(ctx, addr2)
app.AccountKeeper.SetAccount(ctx, acc2)
sendCoins := sdk.NewCoins(newFactoryFooCoin(addr1, 50), newBarCoin(20))
suite.Require().NoError(app.BankKeeper.SendCoinsFromAccountToModule(ctx, addr1, multiPerm, sendCoins))
expectedBankBalances := sdk.NewCoins(newFactoryFooCoin(addr1, 50), newBarCoin(30))
// assert account balances correct
bals := app.BankKeeper.GetAllBalances(ctx, addr1)
suite.Require().Equal(expectedBankBalances, bals)
// assert module balances correct
userBals := app.BankKeeper.GetAllBalances(ctx, multiPermAcc.GetAddress())
suite.Require().Equal(sendCoins, userBals)
}

app.BankKeeper.SetDenomAllowList(ctx, factoryCoin.Denom,
types.AllowList{Addresses: []string{addr1.String()}})
// Test that creating allowlist does not block sending from account to module even though we are not explicitly adding
// the module account to the allowlist
func (suite *IntegrationTestSuite) TestDeferredSendCoinsAccountToModuleWithAllowList() {
// add module accounts to supply keeper
ctx := suite.ctx
_, keeper := suite.initKeepersWithmAccPerms(make(map[string]bool))
app := suite.app
app.BankKeeper = keeper

sendAmt := sdk.NewCoins(newFactoryFooCoin(addr1, 5), newBarCoin(5))
err := app.BankKeeper.SendCoins(ctx, addr1, addr2, sendAmt)
suite.Require().Error(err)
suite.Require().Equal(
fmt.Sprintf("%s is not allowed to receive funds: unauthorized", addr2.String()), err.Error())
addr1 := sdk.AccAddress("addr1_______________")
acc1 := app.AccountKeeper.NewAccountWithAddress(ctx, addr1)
app.AccountKeeper.SetAccount(ctx, acc1)
factoryCoin := newFactoryFooCoin(addr1, 100)
balances := sdk.NewCoins(factoryCoin, newBarCoin(50))
app.BankKeeper.SetDenomAllowList(ctx, factoryCoin.Denom, types.AllowList{
Addresses: []string{addr1.String()}})

// Balances should remain the same
acc1Balances := app.BankKeeper.GetAllBalances(ctx, addr1)
expected := sdk.NewCoins(newFactoryFooCoin(addr1, 100), newBarCoin(50))
suite.Require().Equal(expected, acc1Balances)
// set up bank balances
suite.Require().NoError(simapp.FundAccount(app.BankKeeper, ctx, addr1, balances))

acc2Balances := app.BankKeeper.GetAllBalances(ctx, addr2)
expected = sdk.NewCoins(newFactoryFooCoin(addr1, 0), newBarCoin(0))
suite.Require().Equal(expected, acc2Balances)
sendCoins := sdk.NewCoins(newFactoryFooCoin(addr1, 50), newBarCoin(20))
suite.Require().NoError(app.BankKeeper.DeferredSendCoinsFromAccountToModule(ctx, addr1, multiPerm, sendCoins))
app.BankKeeper.WriteDeferredBalances(ctx)

expectedBankBalances := sdk.NewCoins(newFactoryFooCoin(addr1, 50), newBarCoin(30))
// assert account balances correct
bals := app.BankKeeper.GetAllBalances(ctx, addr1)
suite.Require().Equal(expectedBankBalances, bals)
// assert module balances correct
userBals := app.BankKeeper.GetAllBalances(ctx, multiPermAcc.GetAddress())
suite.Require().Equal(sendCoins, userBals)
}

func (suite *IntegrationTestSuite) TestSendCoinsModuleToAccount() {
Expand Down Expand Up @@ -1728,11 +1694,11 @@ func (suite *IntegrationTestSuite) TestBaseKeeper_IsAllowedToSendCoins() {
}
denomToAllowedAddressesCache := make(map[string]keeper.AllowedAddresses)
isAllowed :=
app.BankKeeper.IsAllowedToSendCoins(ctx, tt.args.addr, coins, denomToAllowedAddressesCache)
app.BankKeeper.IsInDenomAllowList(ctx, tt.args.addr, coins, denomToAllowedAddressesCache)

// Use suite.Require to assert the results
suite.Require().Equal(tt.isAllowed, isAllowed,
fmt.Errorf("IsAllowedToSendCoins() isAllowed = %v, want %v",
fmt.Errorf("IsInDenomAllowList() isAllowed = %v, want %v",
isAllowed, tt.isAllowed))
})
}
Expand Down
17 changes: 13 additions & 4 deletions x/bank/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,12 @@
return nil, err
}

if k.BlockedAddr(to) {
allowListCache := make(map[string]AllowedAddresses)
if !k.IsInDenomAllowList(ctx, from, msg.Amount, allowListCache) {
return nil, sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to send funds", msg.ToAddress)
}

Check warning on line 45 in x/bank/keeper/msg_server.go

View check run for this annotation

Codecov / codecov/patch

x/bank/keeper/msg_server.go#L42-L45 

Added lines #L42 - L45 were not covered by tests

if k.BlockedAddr(to) || !k.IsInDenomAllowList(ctx, to, msg.Amount, allowListCache) {

Check warning on line 47 in x/bank/keeper/msg_server.go

View check run for this annotation

Codecov / codecov/patch

x/bank/keeper/msg_server.go#L47 

Added line #L47 was not covered by tests
return nil, sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to receive funds", msg.ToAddress)
}

Expand Down Expand Up @@ -72,19 +77,23 @@

func (k msgServer) MultiSend(goCtx context.Context, msg *types.MsgMultiSend) (*types.MsgMultiSendResponse, error) {
ctx := sdk.UnwrapSDKContext(goCtx)

denomToAllowListCache := make(map[string]AllowedAddresses)

Check warning on line 80 in x/bank/keeper/msg_server.go

View check run for this annotation

Codecov / codecov/patch

x/bank/keeper/msg_server.go#L80 

Added line #L80 was not covered by tests
// NOTE: totalIn == totalOut should already have been checked
for _, in := range msg.Inputs {
if err := k.IsSendEnabledCoins(ctx, in.Coins...); err != nil {
return nil, err
}
accAddr := sdk.MustAccAddressFromBech32(in.Address)
if !k.IsInDenomAllowList(ctx, accAddr, in.Coins, denomToAllowListCache) {
return nil, sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to send funds", accAddr)
}

Check warning on line 89 in x/bank/keeper/msg_server.go

View check run for this annotation

Codecov / codecov/patch

x/bank/keeper/msg_server.go#L86-L89 

Added lines #L86 - L89 were not covered by tests
}

for _, out := range msg.Outputs {
accAddr := sdk.MustAccAddressFromBech32(out.Address)

if k.BlockedAddr(accAddr) {
return nil, sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to receive transactions", out.Address)
if k.BlockedAddr(accAddr) || !k.IsInDenomAllowList(ctx, accAddr, out.Coins, denomToAllowListCache) {
return nil, sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to receive funds", out.Address)

Check warning on line 96 in x/bank/keeper/msg_server.go

View check run for this annotation

Codecov / codecov/patch

x/bank/keeper/msg_server.go#L95-L96 

Added lines #L95 - L96 were not covered by tests
}
}

Expand Down
Loading
Loading