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.

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

ics29: gracefully handle escrow out of balance edge case during fee distribution #1251

Merged
merged 10 commits into from
Apr 14, 2022
2 changes: 1 addition & 1 deletion modules/apps/29-fee/ibc_module.go
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ func (im IBCModule) OnAcknowledgementPacket(
packetID := channeltypes.NewPacketId(packet.SourcePort, packet.SourceChannel, packet.Sequence)
feesInEscrow, found := im.keeper.GetFeesInEscrow(ctx, packetID)
if found {
im.keeper.DistributePacketFees(ctx, ack.ForwardRelayerAddress, relayer, feesInEscrow.PacketFees)
im.keeper.DistributePacketFeesOnAcknowledgement(ctx, ack.ForwardRelayerAddress, relayer, feesInEscrow.PacketFees)

// removes the fees from the store as fees are now paid
im.keeper.DeleteFeesInEscrow(ctx, packetID)
Expand Down
64 changes: 54 additions & 10 deletions modules/apps/29-fee/keeper/escrow.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,18 +49,41 @@ func (k Keeper) EscrowPacketFee(ctx sdk.Context, packetID channeltypes.PacketId,
return nil
}

// DistributePacketFees pays all the acknowledgement & receive fees for a given packetID while refunding the timeout fees to the refund account.
func (k Keeper) DistributePacketFees(ctx sdk.Context, forwardRelayer string, reverseRelayer sdk.AccAddress, feesInEscrow []types.PacketFee) {
// DistributePacketFeesOnAcknowledgement pays all the acknowledgement & receive fees for a given packetID while refunding the timeout fees to the refund account.
func (k Keeper) DistributePacketFeesOnAcknowledgement(ctx sdk.Context, forwardRelayer string, reverseRelayer sdk.AccAddress, packetFees []types.PacketFee) {
// cache context before trying to distribute fees
// if the escrow account has insufficient balance then we want to avoid partially distributing fees
cacheCtx, writeFn := ctx.CacheContext()

forwardAddr, _ := sdk.AccAddressFromBech32(forwardRelayer)

for _, packetFee := range feesInEscrow {
for _, packetFee := range packetFees {
if !k.EscrowAccountHasBalance(cacheCtx, packetFee.Fee.Total()) {
// if the escrow account does not have sufficient funds then there must exist a severe bug
// the fee module should be locked until manual intervention fixes the issue
// a locked fee module will simply skip fee logic, all channels will temporarily function as
// fee disabled channels
// NOTE: we use the uncached context to lock the fee module so that the state changes from
// locking the fee module are persisted
k.lockFeeModule(ctx)

return
}

// check if refundAcc address works
refundAddr, err := sdk.AccAddressFromBech32(packetFee.RefundAddress)
if err != nil {
panic(fmt.Sprintf("could not parse refundAcc %s to sdk.AccAddress", packetFee.RefundAddress))
}

k.distributePacketFeeOnAcknowledgement(ctx, refundAddr, forwardAddr, reverseRelayer, packetFee)
k.distributePacketFeeOnAcknowledgement(cacheCtx, refundAddr, forwardAddr, reverseRelayer, packetFee)
}

// write the cache
writeFn()

// NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context.
ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events())
}

// distributePacketFeeOnAcknowledgement pays the receive fee for a given packetID while refunding the timeout fee to the refund account associated with the Fee.
Expand All @@ -83,17 +106,38 @@ func (k Keeper) distributePacketFeeOnAcknowledgement(ctx sdk.Context, refundAddr

}

// DistributePacketsFeesOnTimeout pays all the timeout fees for a given packetID while refunding the acknowledgement & receive fees to the refund account.
func (k Keeper) DistributePacketFeesOnTimeout(ctx sdk.Context, timeoutRelayer sdk.AccAddress, feesInEscrow []types.PacketFee) {
for _, feeInEscrow := range feesInEscrow {
func (k Keeper) DistributePacketFeesOnTimeout(ctx sdk.Context, timeoutRelayer sdk.AccAddress, packetFees []types.PacketFee) {
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
// cache context before trying to distribute fees
// if the escrow account has insufficient balance then we want to avoid partially distributing fees
cacheCtx, writeFn := ctx.CacheContext()

for _, packetFee := range packetFees {
if !k.EscrowAccountHasBalance(cacheCtx, packetFee.Fee.Total()) {
// if the escrow account does not have sufficient funds then there must exist a severe bug
// the fee module should be locked until manual intervention fixes the issue
// a locked fee module will simply skip fee logic, all channels will temporarily function as
// fee disabled channels
// NOTE: we use the uncached context to lock the fee module so that the state changes from
// locking the fee module are persisted
k.lockFeeModule(ctx)

return
}

// check if refundAcc address works
refundAddr, err := sdk.AccAddressFromBech32(feeInEscrow.RefundAddress)
refundAddr, err := sdk.AccAddressFromBech32(packetFee.RefundAddress)
if err != nil {
panic(fmt.Sprintf("could not parse refundAcc %s to sdk.AccAddress", feeInEscrow.RefundAddress))
panic(fmt.Sprintf("could not parse refundAcc %s to sdk.AccAddress", packetFee.RefundAddress))
}

k.distributePacketFeeOnTimeout(ctx, refundAddr, timeoutRelayer, feeInEscrow)
k.distributePacketFeeOnTimeout(cacheCtx, refundAddr, timeoutRelayer, packetFee)
}

// write the cache
writeFn()

// NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context.
ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events())
}

// distributePacketFeeOnTimeout pays the timeout fee to the timeout relayer and refunds the acknowledgement & receive fee.
Expand Down
169 changes: 125 additions & 44 deletions modules/apps/29-fee/keeper/escrow_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ func (suite *KeeperTestSuite) TestEscrowPacketFee() {

// setup
refundAcc = suite.chainA.SenderAccount.GetAddress()
receiveFee = defaultReceiveFee
receiveFee = defaultRecvFee
ackFee = defaultAckFee
timeoutFee = defaultTimeoutFee
packetID = channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, uint64(1))
Expand Down Expand Up @@ -119,7 +119,7 @@ func (suite *KeeperTestSuite) TestEscrowPacketFee() {
}
}

func (suite *KeeperTestSuite) TestDistributeFee() {
func (suite *KeeperTestSuite) TestDistributePacketFeesOnAcknowledgement() {
var (
forwardRelayer string
forwardRelayerBal sdk.Coin
Expand All @@ -128,6 +128,7 @@ func (suite *KeeperTestSuite) TestDistributeFee() {
refundAcc sdk.AccAddress
refundAccBal sdk.Coin
packetFee types.PacketFee
packetFees []types.PacketFee
)

testCases := []struct {
Expand All @@ -148,7 +149,7 @@ func (suite *KeeperTestSuite) TestDistributeFee() {
forward, err := sdk.AccAddressFromBech32(forwardRelayer)
suite.Require().NoError(err)

expectedForwardAccBal := forwardRelayerBal.Add(defaultReceiveFee[0]).Add(defaultReceiveFee[0])
expectedForwardAccBal := forwardRelayerBal.Add(defaultRecvFee[0]).Add(defaultRecvFee[0])
balance = suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), forward, sdk.DefaultBondDenom)
suite.Require().Equal(expectedForwardAccBal, balance)

Expand All @@ -162,14 +163,28 @@ func (suite *KeeperTestSuite) TestDistributeFee() {
suite.Require().Equal(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(0)), balance)
},
},
{
"escrow account out of balance, fee module becomes locked - no distribution", func() {
// pass in an extra packet fee
packetFees = append(packetFees, packetFee)
},
func() {
suite.Require().True(suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(suite.chainA.GetContext()))

// check if the module acc contains all the fees
expectedModuleAccBal := packetFee.Fee.Total().Add(packetFee.Fee.Total()...)
balance := suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress())
suite.Require().Equal(expectedModuleAccBal, balance)
},
},
{
"invalid forward address",
func() {
forwardRelayer = "invalid address"
},
func() {
// check if the refund acc has been refunded the timeoutFee & recvFee
expectedRefundAccBal := refundAccBal.Add(defaultTimeoutFee[0]).Add(defaultReceiveFee[0]).Add(defaultTimeoutFee[0]).Add(defaultReceiveFee[0])
expectedRefundAccBal := refundAccBal.Add(defaultTimeoutFee[0]).Add(defaultRecvFee[0]).Add(defaultTimeoutFee[0]).Add(defaultRecvFee[0])
balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom)
suite.Require().Equal(expectedRefundAccBal, balance)
},
Expand All @@ -181,7 +196,7 @@ func (suite *KeeperTestSuite) TestDistributeFee() {
},
func() {
// check if the refund acc has been refunded the timeoutFee & recvFee
expectedRefundAccBal := refundAccBal.Add(defaultTimeoutFee[0]).Add(defaultReceiveFee[0]).Add(defaultTimeoutFee[0]).Add(defaultReceiveFee[0])
expectedRefundAccBal := refundAccBal.Add(defaultTimeoutFee[0]).Add(defaultRecvFee[0]).Add(defaultTimeoutFee[0]).Add(defaultRecvFee[0])
balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom)
suite.Require().Equal(expectedRefundAccBal, balance)
},
Expand All @@ -201,7 +216,8 @@ func (suite *KeeperTestSuite) TestDistributeFee() {
{
"invalid refund address: no-op, timeout fee remains in escrow",
func() {
packetFee.RefundAddress = suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress().String()
packetFees[0].RefundAddress = suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress().String()
packetFees[1].RefundAddress = suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress().String()
},
func() {
// check if the module acc contains the timeoutFee
Expand All @@ -225,10 +241,11 @@ func (suite *KeeperTestSuite) TestDistributeFee() {
refundAcc = suite.chainA.SenderAccount.GetAddress()

packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 1)
fee := types.NewFee(defaultReceiveFee, defaultAckFee, defaultTimeoutFee)
fee := types.NewFee(defaultRecvFee, defaultAckFee, defaultTimeoutFee)

// escrow the packet fee & store the fee in state
packetFee = types.NewPacketFee(fee, refundAcc.String(), []string{})
packetFees = []types.PacketFee{packetFee, packetFee}
err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee)
suite.Require().NoError(err)

Expand All @@ -244,59 +261,123 @@ func (suite *KeeperTestSuite) TestDistributeFee() {
reverseRelayerBal = suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), reverseRelayer, 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})
suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFeesOnAcknowledgement(suite.chainA.GetContext(), forwardRelayer, reverseRelayer, packetFees)

tc.expResult()
})
}
}

func (suite *KeeperTestSuite) TestDistributeTimeoutFee() {
suite.coordinator.Setup(suite.path) // setup channel
func (suite *KeeperTestSuite) TestDistributePacketFeesOnTimeout() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

duplicated TestDistributePacketFeesOnAcknowledgement and adjusted the tests

var (
timeoutRelayer sdk.AccAddress
timeoutRelayerBal sdk.Coin
refundAcc sdk.AccAddress
refundAccBal sdk.Coin
packetFee types.PacketFee
packetFees []types.PacketFee
)

// setup
refundAcc := suite.chainA.SenderAccount.GetAddress()
timeoutRelayer := sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address())
testCases := []struct {
name string
malleate func()
expResult func()
}{
{
"success",
func() {},
func() {
// check if the timeout relayer is paid
expectedTimeoutAccBal := timeoutRelayerBal.Add(defaultTimeoutFee[0]).Add(defaultTimeoutFee[0])
balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), timeoutRelayer, sdk.DefaultBondDenom)
suite.Require().Equal(expectedTimeoutAccBal, balance)

packetID := channeltypes.NewPacketId(
suite.path.EndpointA.ChannelConfig.PortID,
suite.path.EndpointA.ChannelID,
1,
)
// check if the refund acc has been refunded the recv/ack fees
expectedRefundAccBal := refundAccBal.Add(defaultAckFee[0]).Add(defaultAckFee[0]).Add(defaultRecvFee[0]).Add(defaultRecvFee[0])
balance = suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom)
suite.Require().Equal(expectedRefundAccBal, balance)

fee := types.Fee{
RecvFee: defaultReceiveFee,
AckFee: defaultAckFee,
TimeoutFee: defaultTimeoutFee,
// check the module acc wallet is now empty
balance = suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.DefaultBondDenom)
suite.Require().Equal(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(0)), balance)
},
},
{
"escrow account out of balance, fee module becomes locked - no distribution", func() {
// pass in an extra packet fee
packetFees = append(packetFees, packetFee)
},
func() {
suite.Require().True(suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(suite.chainA.GetContext()))

// check if the module acc contains all the fees
expectedModuleAccBal := packetFee.Fee.Total().Add(packetFee.Fee.Total()...)
balance := suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress())
suite.Require().Equal(expectedModuleAccBal, balance)
},
},
{
"invalid timeout relayer address: timeout fee returned to sender",
func() {
timeoutRelayer = suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress()
},
func() {
// check if the refund acc has been refunded the all the fees
expectedRefundAccBal := sdk.Coins{refundAccBal}.Add(packetFee.Fee.Total()...).Add(packetFee.Fee.Total()...)[0]
balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom)
suite.Require().Equal(expectedRefundAccBal, balance)
},
},
{
"invalid refund address: no-op, recv and ack fees remain in escrow",
func() {
packetFees[0].RefundAddress = suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress().String()
packetFees[1].RefundAddress = suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress().String()
},
func() {
// check if the module acc contains the timeoutFee
expectedModuleAccBal := sdk.NewCoin(sdk.DefaultBondDenom, defaultRecvFee.Add(defaultRecvFee[0]).Add(defaultAckFee[0]).Add(defaultAckFee[0]).AmountOf(sdk.DefaultBondDenom))
balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.DefaultBondDenom)
suite.Require().Equal(expectedModuleAccBal, balance)
},
},
}

// escrow the packet fee & store the fee in state
packetFee := types.NewPacketFee(fee, refundAcc.String(), []string{})
for _, tc := range testCases {
tc := tc

suite.Run(tc.name, func() {
suite.SetupTest() // reset
suite.coordinator.Setup(suite.path) // setup channel

// setup accounts
timeoutRelayer = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address())
refundAcc = suite.chainA.SenderAccount.GetAddress()

err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee)
suite.Require().NoError(err)
// escrow a second packet fee to test with multiple fees distributed
err = suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee)
suite.Require().NoError(err)
packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 1)
fee := types.NewFee(defaultRecvFee, defaultAckFee, defaultTimeoutFee)

// refundAcc balance after escrow
refundAccBal := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom)
// escrow the packet fee & store the fee in state
packetFee = types.NewPacketFee(fee, refundAcc.String(), []string{})
packetFees = []types.PacketFee{packetFee, packetFee}
err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee)
suite.Require().NoError(err)

suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFeesOnTimeout(suite.chainA.GetContext(), timeoutRelayer, []types.PacketFee{packetFee, packetFee})
// escrow a second packet fee to test with multiple fees distributed
err = suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee)
suite.Require().NoError(err)

// check if the timeoutRelayer has been paid
hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), timeoutRelayer, fee.TimeoutFee[0])
suite.Require().True(hasBalance)
tc.malleate()

// check if the refund acc has been refunded the recv & ack fees
expectedRefundAccBal := refundAccBal.Add(fee.AckFee[0]).Add(fee.AckFee[0])
expectedRefundAccBal = refundAccBal.Add(fee.RecvFee[0]).Add(fee.RecvFee[0])
hasBalance = suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), refundAcc, expectedRefundAccBal)
suite.Require().True(hasBalance)
// fetch the account balances before fee distribution (forward, reverse, refund)
timeoutRelayerBal = suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), timeoutRelayer, sdk.DefaultBondDenom)
refundAccBal = suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom)

// 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)
suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFeesOnTimeout(suite.chainA.GetContext(), timeoutRelayer, packetFees)

tc.expResult()
})
}
}

func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() {
Expand Down Expand Up @@ -446,7 +527,7 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() {
expRefundBal = suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), refundAcc)

fee = types.Fee{
RecvFee: defaultReceiveFee,
RecvFee: defaultRecvFee,
AckFee: defaultAckFee,
TimeoutFee: defaultTimeoutFee,
}
Expand Down
4 changes: 2 additions & 2 deletions modules/apps/29-fee/keeper/genesis_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ func (suite *KeeperTestSuite) TestInitGenesis() {
refundAcc := suite.chainA.SenderAccount.GetAddress()
packetID := channeltypes.NewPacketId(ibctesting.MockFeePort, ibctesting.FirstChannelID, 1)
fee := types.Fee{
RecvFee: defaultReceiveFee,
RecvFee: defaultRecvFee,
AckFee: defaultAckFee,
TimeoutFee: defaultTimeoutFee,
}
Expand Down Expand Up @@ -73,7 +73,7 @@ func (suite *KeeperTestSuite) TestExportGenesis() {
refundAcc := suite.chainA.SenderAccount.GetAddress()
packetID := channeltypes.NewPacketId(ibctesting.MockFeePort, ibctesting.FirstChannelID, 1)
fee := types.Fee{
RecvFee: defaultReceiveFee,
RecvFee: defaultRecvFee,
AckFee: defaultAckFee,
TimeoutFee: defaultTimeoutFee,
}
Expand Down
Loading