From 8e503d4e409293776a5a44411a5b68759f2977c1 Mon Sep 17 00:00:00 2001 From: Justin Tieri <37750742+jtieri@users.noreply.github.com> Date: Tue, 19 Mar 2024 14:21:31 -0500 Subject: [PATCH] fix: mint and transfer funds back to escrow account on timeout or ack error (#172) * chore: fix deps * chore: fix func sig --- .github/workflows/golangci-lint.yaml | 2 +- .../e2e/forward_timeout_test.go | 147 +++++++++++++++++- .../packetforward/keeper/keeper.go | 43 +++-- .../packetforward/types/expected_keepers.go | 2 + .../test/mock/bank_keeper.go | 28 ++++ 5 files changed, 203 insertions(+), 19 deletions(-) diff --git a/.github/workflows/golangci-lint.yaml b/.github/workflows/golangci-lint.yaml index c2883b12..c96b2814 100644 --- a/.github/workflows/golangci-lint.yaml +++ b/.github/workflows/golangci-lint.yaml @@ -25,6 +25,6 @@ jobs: - name: golangci-lint uses: golangci/golangci-lint-action@v2 with: - version: v1.52 + version: v1.55.2 working-directory: ${{ matrix.workdir }} args: --timeout=5m \ No newline at end of file diff --git a/middleware/packet-forward-middleware/e2e/forward_timeout_test.go b/middleware/packet-forward-middleware/e2e/forward_timeout_test.go index d71ae64d..6da6d92b 100644 --- a/middleware/packet-forward-middleware/e2e/forward_timeout_test.go +++ b/middleware/packet-forward-middleware/e2e/forward_timeout_test.go @@ -201,7 +201,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) @@ -249,4 +249,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)) } diff --git a/middleware/packet-forward-middleware/packetforward/keeper/keeper.go b/middleware/packet-forward-middleware/packetforward/keeper/keeper.go index 5f651dfc..1a0cb81c 100644 --- a/middleware/packet-forward-middleware/packetforward/keeper/keeper.go +++ b/middleware/packet-forward-middleware/packetforward/keeper/keeper.go @@ -140,41 +140,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 @@ -182,6 +181,16 @@ func (k *Keeper) WriteAcknowledgementForForwardedPacket( panic(fmt.Sprintf("cannot burn coins after a successful send from escrow account to module account: %v", err)) } } + } 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) + } } } diff --git a/middleware/packet-forward-middleware/packetforward/types/expected_keepers.go b/middleware/packet-forward-middleware/packetforward/types/expected_keepers.go index c7c07428..bf52aaf6 100644 --- a/middleware/packet-forward-middleware/packetforward/types/expected_keepers.go +++ b/middleware/packet-forward-middleware/packetforward/types/expected_keepers.go @@ -32,6 +32,8 @@ type DistributionKeeper interface { // BankKeeper defines the expected bank keeper type BankKeeper interface { SendCoins(ctx sdk.Context, fromAddr sdk.AccAddress, toAddr sdk.AccAddress, amt sdk.Coins) error + SendCoinsFromModuleToAccount(ctx sdk.Context, senderModule string, recipientAddr sdk.AccAddress, amt sdk.Coins) error SendCoinsFromAccountToModule(ctx sdk.Context, senderAddr sdk.AccAddress, recipientModule string, amt sdk.Coins) error + MintCoins(ctx sdk.Context, moduleName string, amt sdk.Coins) error BurnCoins(ctx sdk.Context, moduleName string, amt sdk.Coins) error } diff --git a/middleware/packet-forward-middleware/test/mock/bank_keeper.go b/middleware/packet-forward-middleware/test/mock/bank_keeper.go index 4af0456e..51a88912 100644 --- a/middleware/packet-forward-middleware/test/mock/bank_keeper.go +++ b/middleware/packet-forward-middleware/test/mock/bank_keeper.go @@ -48,6 +48,20 @@ func (mr *MockBankKeeperMockRecorder) BurnCoins(arg0, arg1, arg2 interface{}) *g return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "BurnCoins", reflect.TypeOf((*MockBankKeeper)(nil).BurnCoins), arg0, arg1, arg2) } +// MintCoins mocks base method. +func (m *MockBankKeeper) MintCoins(arg0 types.Context, arg1 string, arg2 types.Coins) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "MintCoins", arg0, arg1, arg2) + ret0, _ := ret[0].(error) + return ret0 +} + +// MintCoins indicates an expected call of MintCoins. +func (mr *MockBankKeeperMockRecorder) MintCoins(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "MintCoins", reflect.TypeOf((*MockBankKeeper)(nil).MintCoins), arg0, arg1, arg2) +} + // SendCoins mocks base method. func (m *MockBankKeeper) SendCoins(arg0 types.Context, arg1, arg2 types.AccAddress, arg3 types.Coins) error { m.ctrl.T.Helper() @@ -75,3 +89,17 @@ func (mr *MockBankKeeperMockRecorder) SendCoinsFromAccountToModule(arg0, arg1, a mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SendCoinsFromAccountToModule", reflect.TypeOf((*MockBankKeeper)(nil).SendCoinsFromAccountToModule), arg0, arg1, arg2, arg3) } + +// SendCoinsFromModuleToAccount mocks base method. +func (m *MockBankKeeper) SendCoinsFromModuleToAccount(arg0 types.Context, arg1 string, arg2 types.AccAddress, arg3 types.Coins) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "SendCoinsFromModuleToAccount", arg0, arg1, arg2, arg3) + ret0, _ := ret[0].(error) + return ret0 +} + +// SendCoinsFromModuleToAccount indicates an expected call of SendCoinsFromModuleToAccount. +func (mr *MockBankKeeperMockRecorder) SendCoinsFromModuleToAccount(arg0, arg1, arg2, arg3 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SendCoinsFromModuleToAccount", reflect.TypeOf((*MockBankKeeper)(nil).SendCoinsFromModuleToAccount), arg0, arg1, arg2, arg3) +}