fix: mint and transfer funds back to escrow account on timeout or ack error

.github/workflows/packet-forward-middleware.yml

@@ -1,23 +1,23 @@
 name: packet-forward-middleware
-on: 
+on:
   pull_request:
     paths:
       - 'middleware/packet-forward-middleware/**'
       - '.github/workflows/packet-forward-middleware.yml'
 
 env:
-    LINT_VERSION: v1.52
+    LINT_VERSION: v1.55.2
     GO_VERSION: 1.21.0
     WORKING_DIRECTORY: middleware/packet-forward-middleware
-    
+
     DOCKER_TAG: pfm:local
     TAR_PATH: /tmp/pfm-docker-image.tar
     IMAGE_NAME: pfm-docker-image
 
 jobs:
   golangci:
-    name: Linter    
-    runs-on: ubuntu-latest    
+    name: Linter
+    runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@v3
 
@@ -51,14 +51,14 @@ jobs:
       - name: Setup Go ${{ env.GO_VERSION }}
         uses: actions/setup-go@v4
         with:
-          go-version: ${{ env.GO_VERSION }}          
+          go-version: ${{ env.GO_VERSION }}
 
       - name: Set up Docker Buildx
-        uses: docker/setup-buildx-action@v3        
+        uses: docker/setup-buildx-action@v3
 
       - name: Build and export
         uses: docker/build-push-action@v5
-        with:          
+        with:
           context: ${{ env.WORKING_DIRECTORY }}
           tags: ${{ env.DOCKER_TAG }}
           outputs: type=docker,dest=${{ env.TAR_PATH }}
@@ -68,15 +68,15 @@ jobs:
         with:
           name: ${{ env.IMAGE_NAME }}
           path: ${{ env.TAR_PATH }}
-  
+
   e2e-tests:
     needs: build-docker
     runs-on: ubuntu-latest
     defaults:
       run:
         working-directory: ${{ env.WORKING_DIRECTORY }}
     strategy:
-      matrix:          
+      matrix:
         test:
           - "ictest-forward"
           - "ictest-timeout"
@@ -88,8 +88,8 @@ jobs:
       - name: Set up Go ${{ env.GO_VERSION }}
         uses: actions/setup-go@v4
         with:
-          go-version: ${{ env.GO_VERSION }}          
-      
+          go-version: ${{ env.GO_VERSION }}
+
       - uses: actions/checkout@v4
 
       - name: Download Tarball Artifact

middleware/packet-forward-middleware/e2e/forward_timeout_test.go

@@ -214,7 +214,7 @@ func TestTimeoutOnForward(t *testing.T) {
 	time.Sleep(time.Second * 11)
 
 	// Restart the relayer
-	err = r.StartRelayer(ctx, eRep, pathAB, pathBC)
+	err = r.StartRelayer(ctx, eRep, pathAB, pathBC, pathCD)
 	require.NoError(t, err)
 
 	chainAHeight, err := chainA.Height(ctx)
@@ -263,4 +263,149 @@ func TestTimeoutOnForward(t *testing.T) {
 	require.True(t, firstHopEscrowBalance.Equal(zeroBal))
 	require.True(t, secondHopEscrowBalance.Equal(zeroBal))
 	require.True(t, thirdHopEscrowBalance.Equal(zeroBal))
+
+	// Send IBC transfer from ChainA -> ChainB -> ChainC -> ChainD that will succeed
+	secondHopMetadata = &PacketMetadata{
+		Forward: &ForwardMetadata{
+			Receiver: userD.FormattedAddress(),
+			Channel:  cdChan.ChannelID,
+			Port:     cdChan.PortID,
+		},
+	}
+	nextBz, err = json.Marshal(secondHopMetadata)
+	require.NoError(t, err)
+	next = string(nextBz)
+
+	firstHopMetadata = &PacketMetadata{
+		Forward: &ForwardMetadata{
+			Receiver: userC.FormattedAddress(),
+			Channel:  bcChan.ChannelID,
+			Port:     bcChan.PortID,
+			Next:     &next,
+		},
+	}
+
+	memo, err = json.Marshal(firstHopMetadata)
+	require.NoError(t, err)
+
+	opts = ibc.TransferOptions{
+		Memo: string(memo),
+	}
+
+	chainAHeight, err = chainA.Height(ctx)
+	require.NoError(t, err)
+
+	transferTx, err = chainA.SendIBCTransfer(ctx, abChan.ChannelID, userA.KeyName(), transfer, opts)
+	require.NoError(t, err)
+
+	_, err = testutil.PollForAck(ctx, chainA, chainAHeight, chainAHeight+30, transferTx.Packet)
+	require.NoError(t, err)
+
+	err = testutil.WaitForBlocks(ctx, 5, chainA)
+	require.NoError(t, err)
+
+	// Assert balances are updated to reflect tokens now being on ChainD
+	chainABalance, err = chainA.GetBalance(ctx, userA.FormattedAddress(), chainA.Config().Denom)
+	require.NoError(t, err)
+
+	chainBBalance, err = chainB.GetBalance(ctx, userB.FormattedAddress(), firstHopIBCDenom)
+	require.NoError(t, err)
+
+	chainCBalance, err = chainC.GetBalance(ctx, userC.FormattedAddress(), secondHopIBCDenom)
+	require.NoError(t, err)
+
+	chainDBalance, err = chainD.GetBalance(ctx, userD.FormattedAddress(), thirdHopIBCDenom)
+	require.NoError(t, err)
+
+	require.True(t, chainABalance.Equal(initBal.Sub(transferAmount)))
+	require.True(t, chainBBalance.Equal(zeroBal))
+	require.True(t, chainCBalance.Equal(zeroBal))
+	require.True(t, chainDBalance.Equal(transferAmount))
+
+	firstHopEscrowBalance, err = chainA.GetBalance(ctx, firstHopEscrowAccount, chainA.Config().Denom)
+	require.NoError(t, err)
+
+	secondHopEscrowBalance, err = chainB.GetBalance(ctx, secondHopEscrowAccount, firstHopIBCDenom)
+	require.NoError(t, err)
+
+	thirdHopEscrowBalance, err = chainC.GetBalance(ctx, thirdHopEscrowAccount, secondHopIBCDenom)
+	require.NoError(t, err)
+
+	require.True(t, firstHopEscrowBalance.Equal(transferAmount))
+	require.True(t, secondHopEscrowBalance.Equal(transferAmount))
+	require.True(t, thirdHopEscrowBalance.Equal(transferAmount))
+
+	// Compose IBC tx that will attempt to go from ChainD -> ChainC -> ChainB -> ChainA but timeout between ChainB->ChainA
+	transfer = ibc.WalletAmount{
+		Address: userC.FormattedAddress(),
+		Denom:   thirdHopDenom,
+		Amount:  transferAmount,
+	}
+
+	secondHopMetadata = &PacketMetadata{
+		Forward: &ForwardMetadata{
+			Receiver: userA.FormattedAddress(),
+			Channel:  baChan.ChannelID,
+			Port:     baChan.PortID,
+			Timeout:  1 * time.Second,
+		},
+	}
+	nextBz, err = json.Marshal(secondHopMetadata)
+	require.NoError(t, err)
+	next = string(nextBz)
+
+	firstHopMetadata = &PacketMetadata{
+		Forward: &ForwardMetadata{
+			Receiver: userB.FormattedAddress(),
+			Channel:  cbChan.ChannelID,
+			Port:     cbChan.PortID,
+			Next:     &next,
+		},
+	}
+
+	memo, err = json.Marshal(firstHopMetadata)
+	require.NoError(t, err)
+
+	chainDHeight, err := chainD.Height(ctx)
+	require.NoError(t, err)
+
+	transferTx, err = chainD.SendIBCTransfer(ctx, dcChan.ChannelID, userD.KeyName(), transfer, ibc.TransferOptions{Memo: string(memo)})
+	require.NoError(t, err)
+
+	_, err = testutil.PollForAck(ctx, chainD, chainDHeight, chainDHeight+25, transferTx.Packet)
+	require.NoError(t, err)
+
+	err = testutil.WaitForBlocks(ctx, 5, chainD)
+	require.NoError(t, err)
+
+	// Assert balances to ensure timeout happened and user funds are still present on ChainD
+	chainABalance, err = chainA.GetBalance(ctx, userA.FormattedAddress(), chainA.Config().Denom)
+	require.NoError(t, err)
+
+	chainBBalance, err = chainB.GetBalance(ctx, userB.FormattedAddress(), firstHopIBCDenom)
+	require.NoError(t, err)
+
+	chainCBalance, err = chainC.GetBalance(ctx, userC.FormattedAddress(), secondHopIBCDenom)
+	require.NoError(t, err)
+
+	chainDBalance, err = chainD.GetBalance(ctx, userD.FormattedAddress(), thirdHopIBCDenom)
+	require.NoError(t, err)
+
+	require.True(t, chainABalance.Equal(initBal.Sub(transferAmount)))
+	require.True(t, chainBBalance.Equal(zeroBal))
+	require.True(t, chainCBalance.Equal(zeroBal))
+	require.True(t, chainDBalance.Equal(transferAmount))
+
+	firstHopEscrowBalance, err = chainA.GetBalance(ctx, firstHopEscrowAccount, chainA.Config().Denom)
+	require.NoError(t, err)
+
+	secondHopEscrowBalance, err = chainB.GetBalance(ctx, secondHopEscrowAccount, firstHopIBCDenom)
+	require.NoError(t, err)
+
+	thirdHopEscrowBalance, err = chainC.GetBalance(ctx, thirdHopEscrowAccount, secondHopIBCDenom)
+	require.NoError(t, err)
+
+	require.True(t, firstHopEscrowBalance.Equal(transferAmount))
+	require.True(t, secondHopEscrowBalance.Equal(transferAmount))
+	require.True(t, thirdHopEscrowBalance.Equal(transferAmount))
 }

middleware/packet-forward-middleware/e2e/go.mod

@@ -12,6 +12,7 @@ require (
 	github.com/strangelove-ventures/interchaintest/v7 v7.0.1-0.20231121220910-a334ab4006ae
 	github.com/stretchr/testify v1.8.4
 	go.uber.org/zap v1.26.0
+	golang.org/x/sync v0.4.0
 )
 
 require (
@@ -202,7 +203,6 @@ require (
 	golang.org/x/mod v0.12.0 // indirect
 	golang.org/x/net v0.17.0 // indirect
 	golang.org/x/oauth2 v0.11.0 // indirect
-	golang.org/x/sync v0.4.0 // indirect
 	golang.org/x/sys v0.13.0 // indirect
 	golang.org/x/term v0.13.0 // indirect
 	golang.org/x/text v0.13.0 // indirect

middleware/packet-forward-middleware/packetforward/keeper/keeper.go

@@ -224,41 +224,40 @@ func (k *Keeper) WriteAcknowledgementForForwardedPacket(
 			}
 		}
 
+		amount, ok := sdk.NewIntFromString(data.Amount)
+		if !ok {
+			return fmt.Errorf("failed to parse amount from packet data for forward refund: %s", data.Amount)
+		}
+
+		denomTrace := transfertypes.ParseDenomTrace(fullDenomPath)
+		token := sdk.NewCoin(denomTrace.IBCDenom(), amount)
+
+		escrowAddress := transfertypes.GetEscrowAddress(packet.SourcePort, packet.SourceChannel)
+		refundEscrowAddress := transfertypes.GetEscrowAddress(inFlightPacket.RefundPortId, inFlightPacket.RefundChannelId)
+
+		newToken := sdk.NewCoins(token)
+
 		if transfertypes.SenderChainIsSource(packet.SourcePort, packet.SourceChannel, fullDenomPath) {
 			// funds were moved to escrow account for transfer, so they need to either:
 			// - move to the other escrow account, in the case of native denom
 			// - burn
-
-			amount, ok := sdk.NewIntFromString(data.Amount)
-			if !ok {
-				return fmt.Errorf("failed to parse amount from packet data for forward refund: %s", data.Amount)
-			}
-			denomTrace := transfertypes.ParseDenomTrace(fullDenomPath)
-			token := sdk.NewCoin(denomTrace.IBCDenom(), amount)
-
-			escrowAddress := transfertypes.GetEscrowAddress(packet.SourcePort, packet.SourceChannel)
-
 			if transfertypes.SenderChainIsSource(inFlightPacket.RefundPortId, inFlightPacket.RefundChannelId, fullDenomPath) {
 				// transfer funds from escrow account for forwarded packet to escrow account going back for refund.
-
-				refundEscrowAddress := transfertypes.GetEscrowAddress(inFlightPacket.RefundPortId, inFlightPacket.RefundChannelId)
-
 				if err := k.bankKeeper.SendCoins(
-					ctx, escrowAddress, refundEscrowAddress, sdk.NewCoins(token),
+					ctx, escrowAddress, refundEscrowAddress, newToken,
 				); err != nil {
 					return fmt.Errorf("failed to send coins from escrow account to refund escrow account: %w", err)
 				}
 			} else {
 				// transfer the coins from the escrow account to the module account and burn them.
-
 				if err := k.bankKeeper.SendCoinsFromAccountToModule(
-					ctx, escrowAddress, transfertypes.ModuleName, sdk.NewCoins(token),
+					ctx, escrowAddress, transfertypes.ModuleName, newToken,
 				); err != nil {
 					return fmt.Errorf("failed to send coins from escrow to module account for burn: %w", err)
 				}
 
 				if err := k.bankKeeper.BurnCoins(
-					ctx, transfertypes.ModuleName, sdk.NewCoins(token),
+					ctx, transfertypes.ModuleName, newToken,
 				); err != nil {
 					// NOTE: should not happen as the module account was
 					// retrieved on the step above and it has enough balace
@@ -270,6 +269,20 @@ func (k *Keeper) WriteAcknowledgementForForwardedPacket(
 			// We move funds from the escrowAddress in both cases,
 			// update the total escrow amount for the denom.
 			k.unescrowToken(ctx, token)
+		} else {
+			// Funds in the escrow account were burned,
+			// so on a timeout or acknowledgement error we need to mint the funds back to the escrow account.
+			if err := k.bankKeeper.MintCoins(ctx, transfertypes.ModuleName, newToken); err != nil {
+				return fmt.Errorf("cannot mint coins to the %s module account: %v", transfertypes.ModuleName, err)
+			}
+
+			if err := k.bankKeeper.SendCoinsFromModuleToAccount(ctx, transfertypes.ModuleName, refundEscrowAddress, newToken); err != nil {
+				return fmt.Errorf("cannot send coins from the %s module to the escrow account %s: %v", transfertypes.ModuleName, refundEscrowAddress, err)
+			}
+
+			currentTotalEscrow := k.transferKeeper.GetTotalEscrowForDenom(ctx, token.GetDenom())
+			newTotalEscrow := currentTotalEscrow.Add(token)
+			k.transferKeeper.SetTotalEscrowForDenom(ctx, newTotalEscrow)
 		}
 	}