Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 11 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
f1b196b
feat: allow fee module to become locked
colin-axner Feb 28, 2022
1ff4fa9
add test for IsLocked
colin-axner Mar 2, 2022
b140255
add test for msg server
colin-axner Mar 2, 2022
5466343
add tests for onacknowledgement and ontimeout
colin-axner Mar 2, 2022
fbce570
add test for distributeFee
colin-axner Mar 2, 2022
6974982
fix merge conflicts
colin-axner Mar 2, 2022
7399b52
address review comments
colin-axner Mar 3, 2022
3cc256f
fix merge conflicts
colin-axner Mar 3, 2022
b128ea0
fix merge conflicts
colin-axner Mar 3, 2022
e81e4e5
add more documentation as per review request
colin-axner Mar 3, 2022
8e59f13
remove IsLocked check from distributeFee
colin-axner Mar 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion modules/apps/29-fee/fee_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ type FeeTestSuite struct {
chainB *ibctesting.TestChain
chainC *ibctesting.TestChain

path *ibctesting.Path
path *ibctesting.Path
pathAToC *ibctesting.Path
}

Expand Down Expand Up @@ -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})
}
12 changes: 11 additions & 1 deletion modules/apps/29-fee/ibc_module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,13 @@ 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
return im.app.OnAcknowledgementPacket(ctx, packet, ack.Result, relayer)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also document why we're still unmarshalling the acknowledgement because the counterparty is still sending the incentivized ack over

}

packetID := channeltypes.NewPacketId(packet.SourceChannel, packet.SourcePort, packet.Sequence)
feesInEscrow, found := im.keeper.GetFeesInEscrow(ctx, packetID)
if !found {
Expand All @@ -253,7 +260,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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document why we're just passing through to underlying app in case of locked module

return im.app.OnTimeoutPacket(ctx, packet, relayer)
}

Expand Down
18 changes: 18 additions & 0 deletions modules/apps/29-fee/ibc_module_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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() {
Expand Down
4 changes: 4 additions & 0 deletions modules/apps/29-fee/keeper/escrow.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,10 @@ func (k Keeper) DistributePacketFeesOnTimeout(ctx sdk.Context, timeoutRelayer sd
// If the distribution fails for any reason (such as the receiving address being blocked),
// the state changes will be discarded.
func (k Keeper) distributeFee(ctx sdk.Context, receiver sdk.AccAddress, fee sdk.Coins) {
if k.IsLocked(ctx) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how much extra computation this does. It is a little annoying we need to check if the fee module is locked for each single fee distribution, but otherwise we have to rework the API of all our other functions to properly abort a transaction without returning an error

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an extra state read. From a efficiency point of view, this will probs be cached so i wouldn't worry about it. But it is additional gas

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this line even necessary? I believe in both Ack and Timeout we're checking locked before we even call this function.

Why don't we just add it as CONTRACT that we must check locked before calling this function?

Copy link
Contributor Author

@colin-axner colin-axner Mar 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is. The module may become locked during fee distribution. I'd have to add checks around every call to distribute fee (7 calls total). If the balance becomes negative due to locking, I'd need to return immediately

I find this to be much easier to reason about from a code point of view. I also don't like the idea of function contracts since they are unenforceable and prone to misuse

Copy link
Contributor Author

@colin-axner colin-axner Mar 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I should actually go with a solution similar to this 2b9aa5d

Since we now have multiple fees per ID, we probably want to revert all state changes if a negative balance would be reached and then return immediately.

If I use the cached context then I can remove the is locked check since the entry points are blocked and the state changes are dropped upon locking the fee module during distribution

It's partially annoying that this same format will be duplicated 3 times, DistributePacketFees, DistributePacketFeesOnTimeout, RefundFeesOnChannelClosure, but I don't see a way to deduplicate the code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I think caching and discarding a partially executed tx is much cleaner.

We should cache the context and if the balance goes negative. Let's discard state changes and commit just the write to the locked flag

return
}

// cache context before trying to distribute fees
cacheCtx, writeFn := ctx.CacheContext()

Expand Down
81 changes: 49 additions & 32 deletions modules/apps/29-fee/keeper/escrow_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,22 +124,62 @@ 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()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made these changes to test distributeFee. No longer necessary for this pr, but will be for future pr's (implementing the locking functionality) so I will leave the changes

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

},
},
{
"fee module is locked, no distribution occurs", func() {
lockFeeModule(suite.chainA)
},
func() {
// check the module acc wallet still has the fee
hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), fee.Total()[0])
suite.Require().True(hasBalance)

},
},
}

Expand All @@ -155,8 +195,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,
Expand All @@ -174,35 +214,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)
}
})
}
}
Expand Down
13 changes: 13 additions & 0 deletions modules/apps/29-fee/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,19 @@ func (k Keeper) GetFeeModuleAddress() sdk.AccAddress {
return k.authKeeper.GetModuleAddress(types.ModuleName)
}

// 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) {
Expand Down
17 changes: 17 additions & 0 deletions modules/apps/29-fee/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) TestFeesInEscrow() {
suite.coordinator.Setup(suite.path)

Expand All @@ -90,6 +98,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")
Expand Down
8 changes: 8 additions & 0 deletions modules/apps/29-fee/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move the IsFeeEnabled check from EscrowPacketFee to here and make EscrowPacketFee private?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1059

return nil, err
}
Expand Down
22 changes: 20 additions & 2 deletions modules/apps/29-fee/keeper/msg_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"
Expand Down Expand Up @@ -62,6 +64,13 @@ func (suite *KeeperTestSuite) TestPayPacketFee() {
true,
func() {},
},
{
"fee module is locked",
false,
func() {
lockFeeModule(suite.chainA)
},
},
}

for _, tc := range testCases {
Expand All @@ -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
Expand All @@ -99,6 +109,13 @@ func (suite *KeeperTestSuite) TestPayPacketFeeAsync() {
true,
func() {},
},
{
"fee module is locked",
false,
func() {
lockFeeModule(suite.chainA)
},
},
}

for _, tc := range testCases {
Expand All @@ -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
Expand Down
1 change: 1 addition & 0 deletions modules/apps/29-fee/types/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
)
6 changes: 6 additions & 0 deletions modules/apps/29-fee/types/keys.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down