Skip distribution if refund fails to send on channel closure (#1523)

CHANGELOG.md

@@ -79,7 +79,8 @@ Ref: https://keepachangelog.com/en/1.0.0/
 ### Bug Fixes
 
 * (modules/core/04-channel) [\#1130](https://github.com/cosmos/ibc-go/pull/1130) Call `packet.GetSequence()` rather than passing func in `WriteAcknowledgement` log output
-* (apps/29-fee) [\#1278](https://github.com/cosmos/ibc-go/pull/1278) The URI path for the query to get all incentivized packets for a specifc channel did not follow the same format as the rest of queries.
+* (apps/29-fee) [\#1278](https://github.com/cosmos/ibc-go/pull/1278) The URI path for the query to get all incentivized packets for a specific channel did not follow the same format as the rest of queries.
+* (apps/29-fee)[\#1343](https://github.com/cosmos/ibc-go/pull/1523) Fixed an issue where a bad refund address would prevent channel closure.
 
 ## [v3.0.0](https://github.com/cosmos/ibc-go/releases/tag/v3.0.0) - 2022-03-15
 

modules/apps/29-fee/ibc_middleware_test.go

@@ -345,7 +345,7 @@ func (suite *FeeTestSuite) TestOnChanCloseInit() {
 			}, false,
 		},
 		{
-			"RefundFeesOnChannelClosure fails - invalid refund address", func() {
+			"RefundFeesOnChannelClosure continues - invalid refund address", func() {
 				// store the fee in state & update escrow account balance
 				packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, uint64(1))
 				packetFees := types.NewPacketFees([]types.PacketFee{types.NewPacketFee(fee, "invalid refund address", nil)})
@@ -354,7 +354,7 @@ func (suite *FeeTestSuite) TestOnChanCloseInit() {
 				err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total())
 				suite.Require().NoError(err)
 			},
-			false,
+			true,
 		},
 		{
 			"fee module locked", func() {
@@ -434,7 +434,7 @@ func (suite *FeeTestSuite) TestOnChanCloseConfirm() {
 			}, false,
 		},
 		{
-			"RefundChannelFeesOnClosure fails - refund address is invalid", func() {
+			"RefundChannelFeesOnClosure continues - refund address is invalid", func() {
 				// store the fee in state & update escrow account balance
 				packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, uint64(1))
 				packetFees := types.NewPacketFees([]types.PacketFee{types.NewPacketFee(fee, "invalid refund address", nil)})
@@ -443,7 +443,7 @@ func (suite *FeeTestSuite) TestOnChanCloseConfirm() {
 				err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total())
 				suite.Require().NoError(err)
 			},
-			false,
+			true,
 		},
 		{
 			"fee module locked", func() {

modules/apps/29-fee/keeper/escrow.go

@@ -194,6 +194,7 @@ func (k Keeper) RefundFeesOnChannelClosure(ctx sdk.Context, portID, channelID st
 	cacheCtx, writeFn := ctx.CacheContext()
 
 	for _, identifiedPacketFee := range identifiedPacketFees {
+		var failedToSendCoins bool
 		for _, packetFee := range identifiedPacketFee.PacketFees {
 
 			if !k.EscrowAccountHasBalance(cacheCtx, packetFee.Fee.Total()) {
@@ -211,24 +212,20 @@ func (k Keeper) RefundFeesOnChannelClosure(ctx sdk.Context, portID, channelID st
 
 			refundAddr, err := sdk.AccAddressFromBech32(packetFee.RefundAddress)
 			if err != nil {
-				return err
-			}
-
-			// if the refund address is blocked, skip and continue distribution
-			if k.bankKeeper.BlockedAddr(refundAddr) {
+				failedToSendCoins = true
 				continue
 			}
 
 			// refund all fees to refund address
-			// Use SendCoins rather than the module account send functions since refund address may be a user account or module address.
-			moduleAcc := k.GetFeeModuleAddress()
-			if err = k.bankKeeper.SendCoins(cacheCtx, moduleAcc, refundAddr, packetFee.Fee.Total()); err != nil {
-				return err
+			if err = k.bankKeeper.SendCoinsFromModuleToAccount(cacheCtx, types.ModuleName, refundAddr, packetFee.Fee.Total()); err != nil {
+				failedToSendCoins = true
+				continue
 			}
-
 		}
 
-		k.DeleteFeesInEscrow(cacheCtx, identifiedPacketFee.PacketId)
+		if !failedToSendCoins {
+			k.DeleteFeesInEscrow(cacheCtx, identifiedPacketFee.PacketId)
+		}
 	}
 
 	// NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context.

modules/apps/29-fee/keeper/escrow_test.go

@@ -2,11 +2,10 @@ package keeper_test
 
 import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
-	"github.com/tendermint/tendermint/crypto/secp256k1"
-
 	"github.com/cosmos/ibc-go/v3/modules/apps/29-fee/types"
 	transfertypes "github.com/cosmos/ibc-go/v3/modules/apps/transfer/types"
 	channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types"
+	"github.com/tendermint/tendermint/crypto/secp256k1"
 )
 
 func (suite *KeeperTestSuite) TestDistributeFee() {
@@ -277,12 +276,13 @@ func (suite *KeeperTestSuite) TestDistributePacketFeesOnTimeout() {
 
 func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() {
 	var (
-		expIdentifiedPacketFees []types.IdentifiedPacketFees
-		expEscrowBal            sdk.Coins
-		expRefundBal            sdk.Coins
-		refundAcc               sdk.AccAddress
-		fee                     types.Fee
-		locked                  bool
+		expIdentifiedPacketFees     []types.IdentifiedPacketFees
+		expEscrowBal                sdk.Coins
+		expRefundBal                sdk.Coins
+		refundAcc                   sdk.AccAddress
+		fee                         types.Fee
+		locked                      bool
+		expectEscrowFeesToBeDeleted bool
 	)
 
 	testCases := []struct {
@@ -375,6 +375,7 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() {
 		{
 			"invalid refund acc address", func() {
 				// store the fee in state & update escrow account balance
+				expectEscrowFeesToBeDeleted = false
 				packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, uint64(1))
 				packetFees := types.NewPacketFees([]types.PacketFee{types.NewPacketFee(fee, "invalid refund address", nil)})
 				identifiedPacketFees := types.NewIdentifiedPacketFees(packetID, packetFees.PacketFees)
@@ -385,10 +386,14 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() {
 				suite.Require().NoError(err)
 
 				expIdentifiedPacketFees = []types.IdentifiedPacketFees{identifiedPacketFees}
-			}, false,
+
+				expEscrowBal = fee.Total()
+				expRefundBal = expRefundBal.Sub(fee.Total())
+			}, true,
 		},
 		{
 			"distributing to blocked address is skipped", func() {
+				expectEscrowFeesToBeDeleted = false
 				blockedAddr := suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress().String()
 
 				// store the fee in state & update escrow account balance
@@ -418,6 +423,7 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() {
 			expIdentifiedPacketFees = []types.IdentifiedPacketFees{}
 			expEscrowBal = sdk.Coins{}
 			locked = false
+			expectEscrowFeesToBeDeleted = true
 
 			// setup
 			refundAcc = suite.chainA.SenderAccount.GetAddress()
@@ -464,8 +470,8 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() {
 				suite.Require().Equal(expEscrowBal, escrowBal) // escrow balance should be empty
 				suite.Require().Equal(expRefundBal, refundBal) // all packets should have been refunded
 
-				// all fees in escrow should be deleted for this channel
-				suite.Require().Empty(suite.chainA.GetSimApp().IBCFeeKeeper.GetIdentifiedPacketFeesForChannel(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID))
+				// all fees in escrow should be deleted if expected for this channel
+				suite.Require().Equal(expectEscrowFeesToBeDeleted, len(suite.chainA.GetSimApp().IBCFeeKeeper.GetIdentifiedPacketFeesForChannel(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID)) == 0)
 			}
 		})
 	}