diff --git a/modules/apps/29-fee/fee_test.go b/modules/apps/29-fee/fee_test.go index 9369fc16743..d2445adef22 100644 --- a/modules/apps/29-fee/fee_test.go +++ b/modules/apps/29-fee/fee_test.go @@ -21,7 +21,7 @@ type FeeTestSuite struct { chainB *ibctesting.TestChain chainC *ibctesting.TestChain - path *ibctesting.Path + path *ibctesting.Path pathAToC *ibctesting.Path } @@ -63,3 +63,11 @@ func (suite *FeeTestSuite) CreateMockPacket() channeltypes.Packet { 0, ) } + +// helper function +func lockFeeModule(chain *ibctesting.TestChain) { + ctx := chain.GetContext() + storeKey := chain.GetSimApp().GetKey(types.ModuleName) + store := ctx.KVStore(storeKey) + store.Set(types.KeyLocked(), []byte{1}) +} diff --git a/modules/apps/29-fee/ibc_module.go b/modules/apps/29-fee/ibc_module.go index bdb3bcadabd..f97423377bc 100644 --- a/modules/apps/29-fee/ibc_module.go +++ b/modules/apps/29-fee/ibc_module.go @@ -230,6 +230,16 @@ func (im IBCModule) OnAcknowledgementPacket( return sdkerrors.Wrapf(err, "cannot unmarshal ICS-29 incentivized packet acknowledgement: %v", ack) } + if im.keeper.IsLocked(ctx) { + // if the fee keeper is locked then fee logic should be skipped + // this may occur in the presence of a severe bug which leads invalid state + // the fee keeper will be unlocked after manual intervention + // the acknowledgement has been unmarshalled into an ics29 acknowledgement + // since the counterparty is still sending incentivized acknowledgements + // for fee enabled channels + return im.app.OnAcknowledgementPacket(ctx, packet, ack.Result, relayer) + } + packetID := channeltypes.NewPacketId(packet.SourceChannel, packet.SourcePort, packet.Sequence) feesInEscrow, found := im.keeper.GetFeesInEscrow(ctx, packetID) if !found { @@ -253,7 +263,10 @@ func (im IBCModule) OnTimeoutPacket( packet channeltypes.Packet, relayer sdk.AccAddress, ) error { - if !im.keeper.IsFeeEnabled(ctx, packet.SourcePort, packet.SourceChannel) { + // if the fee keeper is locked then fee logic should be skipped + // this may occur in the presence of a severe bug which leads invalid state + // the fee keeper will be unlocked after manual intervention + if !im.keeper.IsFeeEnabled(ctx, packet.SourcePort, packet.SourceChannel) || im.keeper.IsLocked(ctx) { return im.app.OnTimeoutPacket(ctx, packet, relayer) } diff --git a/modules/apps/29-fee/ibc_module_test.go b/modules/apps/29-fee/ibc_module_test.go index 6e0202a887e..68ff3207443 100644 --- a/modules/apps/29-fee/ibc_module_test.go +++ b/modules/apps/29-fee/ibc_module_test.go @@ -611,6 +611,15 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { }, true, }, + { + "success: fee module is disabled, skip fee logic", + func() { + lockFeeModule(suite.chainA) + + expectedBalance = originalBalance + }, + true, + }, { "fail on distribute receive fee (blocked address)", func() { @@ -724,6 +733,15 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() { }, false, }, + { + "fee module is disabled, skip fee logic", + func() { + lockFeeModule(suite.chainA) + + expectedBalance = originalBalance + }, + false, + }, { "no op if identified packet fee doesn't exist", func() { diff --git a/modules/apps/29-fee/keeper/escrow_test.go b/modules/apps/29-fee/keeper/escrow_test.go index 647b968f56d..8cdf4a9413e 100644 --- a/modules/apps/29-fee/keeper/escrow_test.go +++ b/modules/apps/29-fee/keeper/escrow_test.go @@ -124,22 +124,51 @@ func (suite *KeeperTestSuite) TestDistributeFee() { reverseRelayer sdk.AccAddress forwardRelayer string refundAcc sdk.AccAddress + refundAccBal sdk.Coin + fee types.Fee + packetID channeltypes.PacketId ) validSeq := uint64(1) testCases := []struct { - name string - malleate func() - expPass bool + name string + malleate func() + expResult func() }{ { - "success", func() {}, true, + "success", func() {}, func() { + // check if the reverse relayer is paid + hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), reverseRelayer, fee.AckFee[0].Add(fee.AckFee[0])) + suite.Require().True(hasBalance) + + // check if the forward relayer is paid + forward, err := sdk.AccAddressFromBech32(forwardRelayer) + suite.Require().NoError(err) + hasBalance = suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), forward, fee.RecvFee[0].Add(fee.RecvFee[0])) + suite.Require().True(hasBalance) + + // check if the refund acc has been refunded the timeoutFee + expectedRefundAccBal := refundAccBal.Add(fee.TimeoutFee[0].Add(fee.TimeoutFee[0])) + hasBalance = suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), refundAcc, expectedRefundAccBal) + suite.Require().True(hasBalance) + + // check the module acc wallet is now empty + hasBalance = suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(0)}) + suite.Require().True(hasBalance) + }, }, { "invalid forward address", func() { forwardRelayer = "invalid address" - }, false, + }, + func() { + // check if the refund acc has been refunded the timeoutFee & onRecvFee + expectedRefundAccBal := refundAccBal.Add(fee.TimeoutFee[0]).Add(fee.RecvFee[0]).Add(fee.TimeoutFee[0]).Add(fee.RecvFee[0]) + hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), refundAcc, expectedRefundAccBal) + suite.Require().True(hasBalance) + + }, }, } @@ -155,8 +184,8 @@ func (suite *KeeperTestSuite) TestDistributeFee() { reverseRelayer = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()) forwardRelayer = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()).String() - packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelID, suite.path.EndpointA.ChannelConfig.PortID, validSeq) - fee := types.Fee{ + packetID = channeltypes.NewPacketId(suite.path.EndpointA.ChannelID, suite.path.EndpointA.ChannelConfig.PortID, validSeq) + fee = types.Fee{ RecvFee: defaultReceiveFee, AckFee: defaultAckFee, TimeoutFee: defaultTimeoutFee, @@ -174,35 +203,12 @@ func (suite *KeeperTestSuite) TestDistributeFee() { tc.malleate() // refundAcc balance after escrow - refundAccBal := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom) + refundAccBal = suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom) suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFees(suite.chainA.GetContext(), forwardRelayer, reverseRelayer, []types.PacketFee{packetFee, packetFee}) - if tc.expPass { - // check if the reverse relayer is paid - hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), reverseRelayer, fee.AckFee[0].Add(fee.AckFee[0])) - suite.Require().True(hasBalance) - - // check if the forward relayer is paid - forward, err := sdk.AccAddressFromBech32(forwardRelayer) - suite.Require().NoError(err) - hasBalance = suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), forward, fee.RecvFee[0].Add(fee.RecvFee[0])) - suite.Require().True(hasBalance) - - // check if the refund acc has been refunded the timeoutFee - expectedRefundAccBal := refundAccBal.Add(fee.TimeoutFee[0].Add(fee.TimeoutFee[0])) - hasBalance = suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), refundAcc, expectedRefundAccBal) - suite.Require().True(hasBalance) + tc.expResult() - // check the module acc wallet is now empty - hasBalance = suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(0)}) - suite.Require().True(hasBalance) - } else { - // check if the refund acc has been refunded the timeoutFee & onRecvFee - expectedRefundAccBal := refundAccBal.Add(fee.TimeoutFee[0]).Add(fee.RecvFee[0]).Add(fee.TimeoutFee[0]).Add(fee.RecvFee[0]) - hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), refundAcc, expectedRefundAccBal) - suite.Require().True(hasBalance) - } }) } } diff --git a/modules/apps/29-fee/keeper/keeper.go b/modules/apps/29-fee/keeper/keeper.go index cb481543a23..dc02cd71a22 100644 --- a/modules/apps/29-fee/keeper/keeper.go +++ b/modules/apps/29-fee/keeper/keeper.go @@ -87,6 +87,19 @@ func (k Keeper) EscrowAccountHasBalance(ctx sdk.Context, coins sdk.Coins) bool { return true } +// lockFeeModule sets a flag to determine if fee handling logic should run for the given channel +// identified by channel and port identifiers. +func (k Keeper) lockFeeModule(ctx sdk.Context) { + store := ctx.KVStore(k.storeKey) + store.Set(types.KeyLocked(), []byte{1}) +} + +// IsLocked indicates if the fee module is locked +func (k Keeper) IsLocked(ctx sdk.Context) bool { + store := ctx.KVStore(k.storeKey) + return store.Has(types.KeyLocked()) +} + // SetFeeEnabled sets a flag to determine if fee handling logic should run for the given channel // identified by channel and port identifiers. func (k Keeper) SetFeeEnabled(ctx sdk.Context, portID, channelID string) { diff --git a/modules/apps/29-fee/keeper/keeper_test.go b/modules/apps/29-fee/keeper/keeper_test.go index 1f29a8872d5..4b5d5030e20 100644 --- a/modules/apps/29-fee/keeper/keeper_test.go +++ b/modules/apps/29-fee/keeper/keeper_test.go @@ -67,6 +67,14 @@ func TestKeeperTestSuite(t *testing.T) { suite.Run(t, new(KeeperTestSuite)) } +// helper function +func lockFeeModule(chain *ibctesting.TestChain) { + ctx := chain.GetContext() + storeKey := chain.GetSimApp().GetKey(types.ModuleName) + store := ctx.KVStore(storeKey) + store.Set(types.KeyLocked(), []byte{1}) +} + func (suite *KeeperTestSuite) TestEscrowAccountHasBalance() { fee := types.Fee{ AckFee: defaultAckFee, @@ -85,7 +93,6 @@ func (suite *KeeperTestSuite) TestEscrowAccountHasBalance() { // increase ack fee fee.AckFee = fee.AckFee.Add(defaultAckFee...) suite.Require().False(suite.chainA.GetSimApp().IBCFeeKeeper.EscrowAccountHasBalance(suite.chainA.GetContext(), fee.Total())) - } func (suite *KeeperTestSuite) TestFeesInEscrow() { @@ -111,6 +118,15 @@ func (suite *KeeperTestSuite) TestFeesInEscrow() { suite.Require().False(hasFeesInEscrow) } +func (suite *KeeperTestSuite) TestIsLocked() { + ctx := suite.chainA.GetContext() + suite.Require().False(suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(ctx)) + + lockFeeModule(suite.chainA) + + suite.Require().True(suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(ctx)) +} + func (suite *KeeperTestSuite) TestDisableAllChannels() { suite.chainA.GetSimApp().IBCFeeKeeper.SetFeeEnabled(suite.chainA.GetContext(), "port1", "channel1") suite.chainA.GetSimApp().IBCFeeKeeper.SetFeeEnabled(suite.chainA.GetContext(), "port2", "channel2") diff --git a/modules/apps/29-fee/keeper/msg_server.go b/modules/apps/29-fee/keeper/msg_server.go index fdac1d27874..c8ffb853799 100644 --- a/modules/apps/29-fee/keeper/msg_server.go +++ b/modules/apps/29-fee/keeper/msg_server.go @@ -31,6 +31,10 @@ func (k Keeper) RegisterCounterpartyAddress(goCtx context.Context, msg *types.Ms func (k Keeper) PayPacketFee(goCtx context.Context, msg *types.MsgPayPacketFee) (*types.MsgPayPacketFeeResponse, error) { ctx := sdk.UnwrapSDKContext(goCtx) + if k.IsLocked(ctx) { + return nil, types.ErrFeeModuleLocked + } + // get the next sequence sequence, found := k.GetNextSequenceSend(ctx, msg.SourcePortId, msg.SourceChannelId) if !found { @@ -57,6 +61,10 @@ func (k Keeper) PayPacketFee(goCtx context.Context, msg *types.MsgPayPacketFee) func (k Keeper) PayPacketFeeAsync(goCtx context.Context, msg *types.MsgPayPacketFeeAsync) (*types.MsgPayPacketFeeAsyncResponse, error) { ctx := sdk.UnwrapSDKContext(goCtx) + if k.IsLocked(ctx) { + return nil, types.ErrFeeModuleLocked + } + if err := k.EscrowPacketFee(ctx, msg.PacketId, msg.PacketFee); err != nil { return nil, err } diff --git a/modules/apps/29-fee/keeper/msg_server_test.go b/modules/apps/29-fee/keeper/msg_server_test.go index 1b371a2369f..65861e218b1 100644 --- a/modules/apps/29-fee/keeper/msg_server_test.go +++ b/modules/apps/29-fee/keeper/msg_server_test.go @@ -1,6 +1,8 @@ package keeper_test import ( + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/cosmos/ibc-go/v3/modules/apps/29-fee/types" channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" ibctesting "github.com/cosmos/ibc-go/v3/testing" @@ -62,6 +64,13 @@ func (suite *KeeperTestSuite) TestPayPacketFee() { true, func() {}, }, + { + "fee module is locked", + false, + func() { + lockFeeModule(suite.chainA) + }, + }, } for _, tc := range testCases { @@ -78,7 +87,8 @@ func (suite *KeeperTestSuite) TestPayPacketFee() { msg := types.NewMsgPayPacketFee(fee, suite.path.EndpointA.ChannelConfig.PortID, channelID, refundAcc.String(), []string{}) tc.malleate() - _, err := suite.chainA.SendMsgs(msg) + + _, err := suite.chainA.GetSimApp().IBCFeeKeeper.PayPacketFee(sdk.WrapSDKContext(suite.chainA.GetContext()), msg) if tc.expPass { suite.Require().NoError(err) // message committed @@ -99,6 +109,13 @@ func (suite *KeeperTestSuite) TestPayPacketFeeAsync() { true, func() {}, }, + { + "fee module is locked", + false, + func() { + lockFeeModule(suite.chainA) + }, + }, } for _, tc := range testCases { @@ -125,7 +142,8 @@ func (suite *KeeperTestSuite) TestPayPacketFeeAsync() { tc.malleate() msg := types.NewMsgPayPacketFeeAsync(packetID, packetFee) - _, err := suite.chainA.SendMsgs(msg) + + _, err := suite.chainA.GetSimApp().IBCFeeKeeper.PayPacketFeeAsync(sdk.WrapSDKContext(suite.chainA.GetContext()), msg) if tc.expPass { suite.Require().NoError(err) // message committed diff --git a/modules/apps/29-fee/types/errors.go b/modules/apps/29-fee/types/errors.go index 75fdd436c91..1e35c92e2b1 100644 --- a/modules/apps/29-fee/types/errors.go +++ b/modules/apps/29-fee/types/errors.go @@ -15,4 +15,5 @@ var ( ErrForwardRelayerAddressNotFound = sdkerrors.Register(ModuleName, 8, "forward relayer address not found") ErrFeeNotEnabled = sdkerrors.Register(ModuleName, 9, "fee module is not enabled for this channel. If this error occurs after channel setup, fee module may not be enabled") ErrRelayerNotFoundForAsyncAck = sdkerrors.Register(ModuleName, 10, "relayer address must be stored for async WriteAcknowledgement") + ErrFeeModuleLocked = sdkerrors.Register(ModuleName, 11, "the fee module is currently locked") ) diff --git a/modules/apps/29-fee/types/keys.go b/modules/apps/29-fee/types/keys.go index c9fab858919..c8577105688 100644 --- a/modules/apps/29-fee/types/keys.go +++ b/modules/apps/29-fee/types/keys.go @@ -38,6 +38,12 @@ const ( ForwardRelayerPrefix = "forwardRelayer" ) +// KeyLocked returns the key used to lock and unlock the fee module. This key is used +// in the presence of a severe bug. +func KeyLocked() []byte { + return []byte("locked") +} + // KeyFeeEnabled returns the key that stores a flag to determine if fee logic should // be enabled for the given port and channel identifiers. func KeyFeeEnabled(portID, channelID string) []byte {