ibc-go icon indicating copy to clipboard operation
ibc-go copied to clipboard

Refund stuck fees when ics29 is downgraded in an upgrade (edge case scenario)

Open colin-axner opened this issue 1 year ago • 1 comments

Summary

There is an edge case scenario where a user incentivizes the next packet to be sent, but before the packet is sent, the channel undergoes an upgrade which removes ics29 from the channel version. This will result in packet fees being left in escrow for a packet that can never be incentivized. We may optionally refund any packet fees leftover in state when downgrading. Because all in-flight packets are flushed, only the next packet to be sent should be capable of having fees in escrow.

It might be possible to reuse the logic in RefundFeeOnChannelClosure

For now we have decided to leave the behaviour as is.

Discussed during our internal security audit


For Admin Use

  • [ ] Not duplicate issue
  • [ ] Appropriate labels applied
  • [ ] Appropriate contributors tagged/assigned

colin-axner avatar Jan 25 '24 16:01 colin-axner

The changes would basically be:

diff --git a/modules/apps/29-fee/ibc_middleware.go b/modules/apps/29-fee/ibc_middleware.go
index 4ac1e7dd9..13f92c644 100644
--- a/modules/apps/29-fee/ibc_middleware.go
+++ b/modules/apps/29-fee/ibc_middleware.go
@@ -429,6 +429,9 @@ func (im IBCMiddleware) OnChanUpgradeOpen(ctx sdk.Context, portID, channelID str
 
        versionMetadata, err := types.MetadataFromVersion(proposedVersion)
        if err != nil {
+               // refund any fees left in escrow that might be in state for the next packet
+               // to be sent and that can no longer be incentivized after the downgrade succeeds
+               im.keeper.RefundFeesOnChannelClosure(ctx, portID, channelID)
                // set fee disabled and passthrough to the next middleware or application in callstack.
                im.keeper.DeleteFeeEnabled(ctx, portID, channelID)
                cbs.OnChanUpgradeOpen(ctx, portID, channelID, proposedOrder, proposedConnectionHops, proposedVersion)

Plus a test here. Fees may be left in escrow if the next sequence is incentivised with MsgPayPacketFee and the packet for next sequence is not sent.

crodriguezvega avatar Apr 18 '24 20:04 crodriguezvega