Skip to content

fix: adding check for blocked addresses before escrowing fees #1890

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

Merged
crodriguezvega merged 9 commits into from
Aug 9, 2022
Merged

fix: adding check for blocked addresses before escrowing fees #1890

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8525e4e
fix: adding check for blocked addresses before escrowing fees
seantking Aug 4, 2022
42d6666
Merge branch 'main' into sean/issue#1889-fix-module-acc-refund
seantking Aug 9, 2022
808ee69
refactor: move check below isLocked check
seantking Aug 9, 2022
337df64
refactor: use sdk error instead of fee error
seantking Aug 9, 2022
e9248c4
feat: adding check to RegisterPayee
seantking Aug 9, 2022
8a6b1f1
Merge branch 'main' into sean/issue#1889-fix-module-acc-refund
seantking Aug 9, 2022
e3115f5
chore: format import
seantking Aug 9, 2022
6309549
Merge branch 'main' into sean/issue#1889-fix-module-acc-refund
seantking Aug 9, 2022
ffb9e33
refactor: remove dist module from unblocked addr
seantking Aug 9, 2022
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
27 changes: 27 additions & 0 deletions modules/apps/29-fee/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,15 @@ var _ types.MsgServer = Keeper{}
func (k Keeper) RegisterPayee(goCtx context.Context, msg *types.MsgRegisterPayee) (*types.MsgRegisterPayeeResponse, error) {
ctx := sdk.UnwrapSDKContext(goCtx)

payee, err := sdk.AccAddressFromBech32(msg.Payee)
if err != nil {
return nil, err
}

if k.bankKeeper.BlockedAddr(payee) {
return nil, sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "%s is not authorized to be a payee", payee)
}

// only register payee address if the channel exists and is fee enabled
if _, found := k.channelKeeper.GetChannel(ctx, msg.PortId, msg.ChannelId); !found {
return nil, channeltypes.ErrChannelNotFound
Expand Down Expand Up @@ -78,6 +87,15 @@ func (k Keeper) PayPacketFee(goCtx context.Context, msg *types.MsgPayPacketFee)
return nil, types.ErrFeeModuleLocked
}

refundAcc, err := sdk.AccAddressFromBech32(msg.Signer)
if err != nil {
return nil, err
}

if k.bankKeeper.BlockedAddr(refundAcc) {
Copy link
Copy Markdown
Contributor

@crodriguezvega crodriguezvega Aug 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe instead of adding a new error, do something like?

if k.bankKeeper.BlockedAddr(refundAcc) {
 	return sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to escrow fees", refundAcc)
 }

Copy link
Copy Markdown
Contributor Author

@seantking seantking Aug 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤝 done

return nil, sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to escrow fees", refundAcc)
}

// get the next sequence
sequence, found := k.GetNextSequenceSend(ctx, msg.SourcePortId, msg.SourceChannelId)
if !found {
Expand Down Expand Up @@ -110,6 +128,15 @@ func (k Keeper) PayPacketFeeAsync(goCtx context.Context, msg *types.MsgPayPacket
return nil, types.ErrFeeModuleLocked
}

refundAcc, err := sdk.AccAddressFromBech32(msg.PacketFee.RefundAddress)
if err != nil {
return nil, err
}

if k.bankKeeper.BlockedAddr(refundAcc) {
return nil, sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to escrow fees", refundAcc)
}
Comment on lines +118 to 138
Copy link
Copy Markdown
Contributor

@damiannolan damiannolan Aug 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move the check to below k.IsLocked(ctx) for consistency with PayPacketFee rpc?

Copy link
Copy Markdown
Contributor

@colin-axner colin-axner Aug 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed


nextSeqSend, found := k.GetNextSequenceSend(ctx, msg.PacketId.PortId, msg.PacketId.ChannelId)
if !found {
return nil, sdkerrors.Wrapf(channeltypes.ErrSequenceSendNotFound, "channel does not exist, portID: %s, channelID: %s", msg.PacketId.PortId, msg.PacketId.ChannelId)
Expand Down
31 changes: 31 additions & 0 deletions modules/apps/29-fee/keeper/msg_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/cosmos/ibc-go/v5/modules/apps/29-fee/types"
transfertypes "github.com/cosmos/ibc-go/v5/modules/apps/transfer/types"
clienttypes "github.com/cosmos/ibc-go/v5/modules/core/02-client/types"
Comment thread
colin-axner marked this conversation as resolved.
channeltypes "github.com/cosmos/ibc-go/v5/modules/core/04-channel/types"
ibctesting "github.com/cosmos/ibc-go/v5/testing"
Expand Down Expand Up @@ -37,6 +38,20 @@ func (suite *KeeperTestSuite) TestRegisterPayee() {
suite.chainA.GetSimApp().IBCFeeKeeper.DeleteFeeEnabled(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID)
},
},
{
"given payee is not an sdk address",
false,
func() {
msg.Payee = "invalid-addr"
},
},
{
"payee is a blocked address",
false,
func() {
msg.Payee = suite.chainA.GetSimApp().AccountKeeper.GetModuleAddress(transfertypes.ModuleName).String()
},
},
}

for _, tc := range testCases {
Expand Down Expand Up @@ -220,6 +235,14 @@ func (suite *KeeperTestSuite) TestPayPacketFee() {
},
false,
},
{
"refund account is a blocked address",
func() {
blockedAddr := suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress()
msg.Signer = blockedAddr.String()
},
false,
},
{
"acknowledgement fee balance not found",
func() {
Expand Down Expand Up @@ -399,6 +422,14 @@ func (suite *KeeperTestSuite) TestPayPacketFeeAsync() {
},
false,
},
{
"refund account is a blocked address",
func() {
blockedAddr := suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress()
msg.PacketFee.RefundAddress = blockedAddr.String()
},
false,
},
{
"acknowledgement fee balance not found",
func() {
Expand Down
4 changes: 2 additions & 2 deletions testing/simapp/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -672,9 +672,9 @@ func (app *SimApp) LoadHeight(height int64) error {
func (app *SimApp) ModuleAccountAddrs() map[string]bool {
modAccAddrs := make(map[string]bool)
for acc := range maccPerms {
// do not add mock module to blocked addresses
// do not add the following modules to blocked addresses
// this is only used for testing
if acc == ibcmock.ModuleName {
if acc == ibcmock.ModuleName || acc == distrtypes.ModuleName {
Copy link
Copy Markdown
Contributor Author

@seantking seantking Aug 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty hacky I just left it in for discussion. I'm guessing we don't want to unblock the distribution module?

We have the following test case. We ensure a non-blocked module account can escrow fees.

When I try to use the mock module account I get an AuthKeeper error saying the account doesn't exist. I'm not sure why exactly. I've left the test case as is using the distribution module account for now.

cc: @colin-axner

Copy link
Copy Markdown
Contributor

@damiannolan damiannolan Aug 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was looking at the test case "refund account is a module account" and trying to use ibcmock.ModuleName instead of disttypes.ModuleName:

"refund account is module account",
func() {
	msg.Signer = suite.chainA.GetSimApp().AccountKeeper.GetModuleAddress(disttypes.ModuleName).String()
	expPacketFee := types.NewPacketFee(fee, msg.Signer, nil)
	expFeesInEscrow = []types.PacketFee{expPacketFee}
},
true,

The account does not exist error is being returned from here in escrow.go. And interestingly if we actually use AccountKeeper.GetModuleAccount() we will not encounter that error as it calls directly into GetModuleAccountAndPermissions() which creates and sets the account in x/auth.

This differs from GetModuleAddress which just checks the permAddrs map created on keeper initialisation.

It's definitely pretty ambiguous as to what constitutes a blocked address and what doesn't. Without this logical OR added for distrtypes.ModuleName it is and will be in the blockedAddrs map in x/bank. However, its usage in x/distribution when for example, withdrawing delegation rewards uses SendCoinsFromModuleToAccount which only checks that the recipientAddr is not a blocked address.

When I use the following in the malleate() func, we don't fail on the account not existing but instead fail with an "insufficient funds" error.

moduleAcc := suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), ibcmock.ModuleName)
msg.Signer = moduleAcc.GetAddress().String()
expPacketFee := types.NewPacketFee(fee, msg.Signer, nil)
expFeesInEscrow = []types.PacketFee{expPacketFee}

It would be nice to get some more clarity around module accounts in general. We could use the above and also fund the ibcmock.ModuleName module account to get past the error.

Copy link
Copy Markdown
Contributor

@colin-axner colin-axner Aug 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the blocked address logic gets removed from the SDK, what test case would we want to have? I'd say probably still wise to just use the mock module account since it may not make sense long term to use distr (as community pool might be removed from it)

Maybe we can add the mock module to the list of genAccs in NewTestChainWithValSet?

Also I think you can use GetModuleAddress().String()

continue
}

Expand Down