fix: adding check for blocked addresses before escrowing fees (backport

#1890) (#1950) * fix: adding check for blocked addresses before escrowing fees (#1890) * fix: adding check for blocked addresses before escrowing fees * refactor: move check below isLocked check * refactor: use sdk error instead of fee error * feat: adding check to RegisterPayee * chore: format import * refactor: remove dist module from unblocked addr (cherry picked from commit 7694903) # Conflicts: # modules/apps/29-fee/keeper/msg_server_test.go * fix conflicts Co-authored-by: Sean King <[email protected]> Co-authored-by: Carlos Rodriguez <[email protected]>
cosmos · Aug 10, 2022 · 3830bd7 · 3830bd7
1 parent a3941f0
commit 3830bd7
Show file tree

Hide file tree

Showing 3 changed files with 60 additions and 4 deletions.
diff --git a/modules/apps/29-fee/keeper/msg_server.go b/modules/apps/29-fee/keeper/msg_server.go
@@ -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
@@ -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) {
+		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 {
@@ -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)
+	}
+
 	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)

diff --git a/modules/apps/29-fee/keeper/msg_server_test.go b/modules/apps/29-fee/keeper/msg_server_test.go
@@ -4,16 +4,15 @@ import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 
 	"github.com/cosmos/ibc-go/v4/modules/apps/29-fee/types"
+	transfertypes "github.com/cosmos/ibc-go/v4/modules/apps/transfer/types"
 	clienttypes "github.com/cosmos/ibc-go/v4/modules/core/02-client/types"
 	channeltypes "github.com/cosmos/ibc-go/v4/modules/core/04-channel/types"
 	ibctesting "github.com/cosmos/ibc-go/v4/testing"
 	ibcmock "github.com/cosmos/ibc-go/v4/testing/mock"
 )
 
 func (suite *KeeperTestSuite) TestRegisterPayee() {
-	var (
-		msg *types.MsgRegisterPayee
-	)
+	var msg *types.MsgRegisterPayee
 
 	testCases := []struct {
 		name     string
@@ -39,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 {
@@ -222,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() {
@@ -401,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() {

diff --git a/testing/simapp/app.go b/testing/simapp/app.go
@@ -655,7 +655,7 @@ 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 {
 			continue