From 971c40a46c9d5f680ff256b6c8e4cf37f9acbd8a Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 8 Jun 2022 09:56:58 +0100 Subject: [PATCH 1/8] fix: don't close channel on faulty refund address --- modules/apps/29-fee/keeper/escrow.go | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/modules/apps/29-fee/keeper/escrow.go b/modules/apps/29-fee/keeper/escrow.go index 130ac18e1b3..2e94b52c282 100644 --- a/modules/apps/29-fee/keeper/escrow.go +++ b/modules/apps/29-fee/keeper/escrow.go @@ -214,16 +214,9 @@ func (k Keeper) RefundFeesOnChannelClosure(ctx sdk.Context, portID, channelID st return err } - // if the refund address is blocked, skip and continue distribution - if k.bankKeeper.BlockedAddr(refundAddr) { - 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 { + continue } } From 85aba61303eba1de4888209fa850316cba29aabf Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 8 Jun 2022 15:35:56 +0100 Subject: [PATCH 2/8] test: Added test for invalid refund address --- modules/apps/29-fee/keeper/escrow_test.go | 27 +++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/modules/apps/29-fee/keeper/escrow_test.go b/modules/apps/29-fee/keeper/escrow_test.go index 9b67934c647..8b8929a0cec 100644 --- a/modules/apps/29-fee/keeper/escrow_test.go +++ b/modules/apps/29-fee/keeper/escrow_test.go @@ -2,11 +2,11 @@ package keeper_test import ( sdk "github.com/cosmos/cosmos-sdk/types" - "github.com/tendermint/tendermint/crypto/secp256k1" - + govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" "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() { @@ -407,6 +407,29 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() { expRefundBal = expRefundBal.Sub(fee.Total()) }, true, }, + { + name: "invalid refund address: no error is returned", + malleate: func() { + locked = true + account := suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), govtypes.ModuleName) + refundAcc = account.GetAddress() + + for i := 1; i < 6; i++ { + // store the fee in state & update escrow account balance + packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, uint64(i)) + packetFees := types.NewPacketFees([]types.PacketFee{types.NewPacketFee(fee, refundAcc.String(), nil)}) + identifiedPacketFees := types.NewIdentifiedPacketFees(packetID, packetFees.PacketFees) + + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, packetFees) + + err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), types.ModuleName, fee.Total()) + suite.Require().NoError(err) + + expIdentifiedPacketFees = append(expIdentifiedPacketFees, identifiedPacketFees) + } + }, + expPass: true, + }, } for _, tc := range testCases { From 33606ee647b76c1429edac0733e36b5f21abb99f Mon Sep 17 00:00:00 2001 From: chatton Date: Thu, 9 Jun 2022 10:31:32 +0100 Subject: [PATCH 3/8] test: updated existing test for blocked address --- modules/apps/29-fee/keeper/escrow.go | 7 ++-- modules/apps/29-fee/keeper/escrow_test.go | 43 ++++++----------------- 2 files changed, 16 insertions(+), 34 deletions(-) diff --git a/modules/apps/29-fee/keeper/escrow.go b/modules/apps/29-fee/keeper/escrow.go index 2e94b52c282..111e0b6ce74 100644 --- a/modules/apps/29-fee/keeper/escrow.go +++ b/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()) { @@ -216,12 +217,14 @@ func (k Keeper) RefundFeesOnChannelClosure(ctx sdk.Context, portID, channelID st // refund all fees to refund address 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. diff --git a/modules/apps/29-fee/keeper/escrow_test.go b/modules/apps/29-fee/keeper/escrow_test.go index 8b8929a0cec..2178637cce8 100644 --- a/modules/apps/29-fee/keeper/escrow_test.go +++ b/modules/apps/29-fee/keeper/escrow_test.go @@ -2,7 +2,6 @@ package keeper_test import ( sdk "github.com/cosmos/cosmos-sdk/types" - govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" "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" @@ -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 { @@ -389,6 +389,7 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() { }, { "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 @@ -407,29 +408,6 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() { expRefundBal = expRefundBal.Sub(fee.Total()) }, true, }, - { - name: "invalid refund address: no error is returned", - malleate: func() { - locked = true - account := suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), govtypes.ModuleName) - refundAcc = account.GetAddress() - - for i := 1; i < 6; i++ { - // store the fee in state & update escrow account balance - packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, uint64(i)) - packetFees := types.NewPacketFees([]types.PacketFee{types.NewPacketFee(fee, refundAcc.String(), nil)}) - identifiedPacketFees := types.NewIdentifiedPacketFees(packetID, packetFees.PacketFees) - - suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, packetFees) - - err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), types.ModuleName, fee.Total()) - suite.Require().NoError(err) - - expIdentifiedPacketFees = append(expIdentifiedPacketFees, identifiedPacketFees) - } - }, - expPass: true, - }, } for _, tc := range testCases { @@ -441,6 +419,7 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() { expIdentifiedPacketFees = []types.IdentifiedPacketFees{} expEscrowBal = sdk.Coins{} locked = false + expectEscrowFeesToBeDeleted = true // setup refundAcc = suite.chainA.SenderAccount.GetAddress() @@ -487,8 +466,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) } }) } From a640f3c4066f2031b22693d7a3658bd7980fb429 Mon Sep 17 00:00:00 2001 From: chatton Date: Fri, 10 Jun 2022 09:31:22 +0100 Subject: [PATCH 4/8] chore: Adding changelog entry. --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 104c9bb142e..5a185e32251 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -76,6 +76,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (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)[\#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 From 7ba892d8dbfcdcd04dc20325c728bde301601ab3 Mon Sep 17 00:00:00 2001 From: chatton Date: Fri, 10 Jun 2022 09:31:45 +0100 Subject: [PATCH 5/8] chore: Adding changelog entry. --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5a185e32251..ff1ca299127 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -75,7 +75,7 @@ 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 From ea6033f04d50202dce59cccc9de43a90d484d63b Mon Sep 17 00:00:00 2001 From: Cian Hatton Date: Mon, 13 Jun 2022 08:57:59 +0100 Subject: [PATCH 6/8] fix: invalid address now also does not block channel closure. --- modules/apps/29-fee/keeper/escrow.go | 3 ++- modules/apps/29-fee/keeper/escrow_test.go | 6 +++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/modules/apps/29-fee/keeper/escrow.go b/modules/apps/29-fee/keeper/escrow.go index 111e0b6ce74..9755fb45e8d 100644 --- a/modules/apps/29-fee/keeper/escrow.go +++ b/modules/apps/29-fee/keeper/escrow.go @@ -212,7 +212,8 @@ func (k Keeper) RefundFeesOnChannelClosure(ctx sdk.Context, portID, channelID st refundAddr, err := sdk.AccAddressFromBech32(packetFee.RefundAddress) if err != nil { - return err + failedToSendCoins = true + continue } // refund all fees to refund address diff --git a/modules/apps/29-fee/keeper/escrow_test.go b/modules/apps/29-fee/keeper/escrow_test.go index 2178637cce8..7ccf87aa91b 100644 --- a/modules/apps/29-fee/keeper/escrow_test.go +++ b/modules/apps/29-fee/keeper/escrow_test.go @@ -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,7 +386,10 @@ 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() { From c37dffc308b4a76bf3143bacf2e8977eb4381035 Mon Sep 17 00:00:00 2001 From: Cian Hatton Date: Mon, 13 Jun 2022 10:37:15 +0100 Subject: [PATCH 7/8] test: update existing test to succeed as an error is no longer returned --- modules/apps/29-fee/ibc_middleware_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/apps/29-fee/ibc_middleware_test.go b/modules/apps/29-fee/ibc_middleware_test.go index 17a961d0074..83e0bb8aedb 100644 --- a/modules/apps/29-fee/ibc_middleware_test.go +++ b/modules/apps/29-fee/ibc_middleware_test.go @@ -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() { From 577401197ecfc2106cf8eab98a563294c9d775f6 Mon Sep 17 00:00:00 2001 From: Cian Hatton Date: Mon, 13 Jun 2022 11:25:01 +0100 Subject: [PATCH 8/8] test: update existing test to succeed as an error is no longer returned --- modules/apps/29-fee/ibc_middleware_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/apps/29-fee/ibc_middleware_test.go b/modules/apps/29-fee/ibc_middleware_test.go index 83e0bb8aedb..37cfaf2f948 100644 --- a/modules/apps/29-fee/ibc_middleware_test.go +++ b/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() {