feat: allow ability to lock fee module in presence of severe bug

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})
+}

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)
 	}
 

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() {

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)
-			}
 		})
 	}
 }

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) {

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")

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
 	}

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

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")
 )

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 {