From 3f781c907f4740a357bde3e1408475b26334afed Mon Sep 17 00:00:00 2001 From: Nilesh Gupta Date: Wed, 26 Aug 2026 07:50:42 +0530 Subject: [PATCH] fix: roll back the bank escrow when ConvertCoinNativeERC20's EVM call fails The escrow was applied to the parent context before CallEVM, so a VM failure left the sender's coins stranded on the erc20 module account - permanently on the IBC refund path, which swallows the error. --- CHANGELOG.md | 7 + .../integration/x/erc20/test_ibc_callback.go | 127 ++++++++++++++++++ x/erc20/keeper/ibc_callbacks.go | 19 ++- x/erc20/keeper/msg_server.go | 43 +++++- 4 files changed, 190 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4064e1bbc..cf96e8922 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,13 @@ ### BUG FIXES +- Make `ConvertCoinNativeERC20` atomic. It escrowed the sender's coins onto the erc20 module + account before calling the EVM, and `CallEVMWithData` caches only the EVM execution, so a VM + failure discarded the EVM cache while leaving the bank escrow committed. On the IBC error-ACK / + timeout refund path, which deliberately swallows a conversion error so a failed re-wrap cannot + undo the refund, that stranded the sender's coins on the module account permanently - contradicting + the `OnAcknowledgementPacket` / `OnTimeoutPacket` comments promising the user keeps the bank token. + The escrow, the EVM transfer and the burn now all run on a branched context and commit together. - Run the gov precompile's `getTallyResult` against a throwaway cache. The SDK's `TallyResult` query delegates to `Keeper.Tally`, which deletes every vote it counts; that is safe on the SDK's own query paths because a gRPC query runs against a context that is never committed, but precompiles diff --git a/tests/integration/x/erc20/test_ibc_callback.go b/tests/integration/x/erc20/test_ibc_callback.go index 0086d4b78..7af9916a2 100644 --- a/tests/integration/x/erc20/test_ibc_callback.go +++ b/tests/integration/x/erc20/test_ibc_callback.go @@ -769,3 +769,130 @@ func (s *KeeperTestSuite) TestOnTimeoutPacket() { }) } } + +// setupUnbackedPairAndFundSender registers a native-ERC20 pair and funds the +// sender with the paired bank denom, while deliberately leaving the erc20 module +// account with NO token balance to unescrow from. +// +// That produces the exact failure shape F-2026-18819 needs. Walking +// convertCoinNativeERC20 in order: +// +// balanceOf(receiver) -> succeeds (a plain view) +// SendCoinsFromAccountToModule -> succeeds, coins now on the module +// CallEVM "transfer" -> REVERTS, module holds nothing to send +// +// The failure therefore lands strictly after the bank mutation, which is the +// only ordering that can strand the sender's coins. Pausing the token does not +// work here: it fails the balanceOf read instead, before any escrow happens, and +// the test would pass vacuously. +// +// It returns a context fetched *after* the deployment: the suite's contract +// deployment only becomes visible to EVM reads on a context taken afterwards, so +// reusing an earlier one makes balanceOf return nil and the conversion fail +// before it ever escrows. +func (s *KeeperTestSuite) setupUnbackedPairAndFundSender( + sender sdk.AccAddress, fund math.Int, +) (sdk.Context, types.TokenPair) { + contractAddr, err := s.setupRegisterERC20Pair(contractMinterBurner) + s.Require().NoError(err, "failed to register pair") + + ctx := s.network.GetContext() + + id := s.network.App.GetErc20Keeper().GetTokenPairID(ctx, contractAddr.String()) + pair, found := s.network.App.GetErc20Keeper().GetTokenPair(ctx, id) + s.Require().True(found) + + s.Require().NoError(testutil.FundAccount( + ctx, s.network.App.GetBankKeeper(), sender, + sdk.NewCoins(sdk.NewCoin(pair.Denom, fund)), + )) + + // Sanity: the module must hold no tokens, or the transfer would succeed and + // the test would prove nothing. + moduleTokens := s.network.App.GetErc20Keeper().BalanceOf( + ctx, contracts.ERC20MinterBurnerDecimalsContract.ABI, contractAddr, types.ModuleAddress, + ) + s.Require().NotNil(moduleTokens, "balanceOf must work - the failure has to come from transfer") + s.Require().Zero(moduleTokens.Sign(), "module must have nothing to unescrow") + + return ctx, pair +} + +// TestConvertCoinNativeERC20RollsBackEscrowOnEVMFailure is the direct guard for +// F-2026-18819. +// +// ConvertCoinNativeERC20 escrows the sender's coins onto the erc20 module +// account and only then calls the EVM. CallEVM caches solely the EVM execution, +// so before the fix a VM failure discarded the EVM cache but left the bank +// escrow committed - the sender's coins ended up stranded on a module account +// with no user-facing way to reclaim them. +// +// Asserting on the error alone would not catch that. The assertions that matter +// are the balances. +func (s *KeeperTestSuite) TestConvertCoinNativeERC20RollsBackEscrowOnEVMFailure() { + s.mintFeeCollector = true + defer func() { s.mintFeeCollector = false }() + s.SetupTest() + + bankKeeper := s.network.App.GetBankKeeper() + moduleAcc := authtypes.NewModuleAddress(types.ModuleName) + + senderPk := secp256k1.GenPrivKey() + sender := sdk.AccAddress(senderPk.PubKey().Address()) + + ctx, pair := s.setupUnbackedPairAndFundSender(sender, math.NewInt(100)) + + senderBefore := bankKeeper.GetBalance(ctx, sender, pair.Denom) + moduleBefore := bankKeeper.GetBalance(ctx, moduleAcc, pair.Denom) + supplyBefore := bankKeeper.GetSupply(ctx, pair.Denom) + s.Require().Equal(math.NewInt(100), senderBefore.Amount) + + err := s.network.App.GetErc20Keeper().ConvertCoinNativeERC20( + ctx, pair, math.NewInt(10), common.BytesToAddress(sender), sender, + ) + s.Require().Error(err, "a paused token must fail the unescrow transfer") + + s.Require().Equal(senderBefore, bankKeeper.GetBalance(ctx, sender, pair.Denom), + "sender must keep every coin - the escrow has to roll back with the EVM failure") + s.Require().Equal(moduleBefore, bankKeeper.GetBalance(ctx, moduleAcc, pair.Denom), + "no coins may be stranded on the erc20 module account") + s.Require().Equal(supplyBefore, bankKeeper.GetSupply(ctx, pair.Denom), + "a failed conversion must not burn supply") +} + +// TestConvertCoinToERC20FromPacketLeavesRefundWithSender covers the same defect +// one layer up, on the path that actually made it permanent. +// +// The IBC error-ACK / timeout wrapper deliberately swallows a conversion failure +// so a failed re-wrap cannot undo the refund itself, and the block commits. The +// doc comments on OnAcknowledgementPacket / OnTimeoutPacket promise that the +// user "receives the corresponding bank token from the TokenPair instead" - this +// pins that promise down. +func (s *KeeperTestSuite) TestConvertCoinToERC20FromPacketLeavesRefundWithSender() { + s.mintFeeCollector = true + defer func() { s.mintFeeCollector = false }() + s.SetupTest() + + bankKeeper := s.network.App.GetBankKeeper() + moduleAcc := authtypes.NewModuleAddress(types.ModuleName) + + senderPk := secp256k1.GenPrivKey() + sender := sdk.AccAddress(senderPk.PubKey().Address()) + + ctx, pair := s.setupUnbackedPairAndFundSender(sender, math.NewInt(100)) + + senderBefore := bankKeeper.GetBalance(ctx, sender, pair.Denom) + moduleBefore := bankKeeper.GetBalance(ctx, moduleAcc, pair.Denom) + + data := transfertypes.NewFungibleTokenPacketData(pair.Denom, "10", sender.String(), "", "") + + // The wrapper swallows the conversion error by design; the IBC lifecycle + // must continue. + err := s.network.App.GetErc20Keeper().ConvertCoinToERC20FromPacket(ctx, data) + s.Require().NoError(err, "the refund path must not surface the conversion failure") + + s.Require().Equal(senderBefore, bankKeeper.GetBalance(ctx, sender, pair.Denom), + "the refunded coins must still be with the sender, who can retry the conversion") + s.Require().Equal(moduleBefore, bankKeeper.GetBalance(ctx, moduleAcc, pair.Denom), + "the refund must not be stranded on the erc20 module account") +} diff --git a/x/erc20/keeper/ibc_callbacks.go b/x/erc20/keeper/ibc_callbacks.go index 4848dc10b..6270ed5ae 100644 --- a/x/erc20/keeper/ibc_callbacks.go +++ b/x/erc20/keeper/ibc_callbacks.go @@ -159,8 +159,12 @@ func (k Keeper) OnRecvPacket( // is refunded and then the IBC Coins are converted to ERC20. // If the ERC20 conversion fails for whatever reason, such as an attempt to call // a self-destructed ERC20 contract or an invalid function, OnAcknowledgementPacket -// still succeeds, but the user receives the corresponding bank token from the -// TokenPair instead. A user may then manually re-attempt the conversion. +// still succeeds and the user keeps the corresponding bank token from the +// TokenPair, so they may manually re-attempt the conversion. That holds only +// because ConvertCoinNativeERC20 is atomic: it escrows the coins onto the module +// account before touching the EVM, so without the rollback a VM failure would +// leave the refund stranded on the module account rather than with the sender +// (F-2026-18819). func (k Keeper) OnAcknowledgementPacket( ctx sdk.Context, _ channeltypes.Packet, data transfertypes.FungibleTokenPacketData, @@ -181,8 +185,9 @@ func (k Keeper) OnAcknowledgementPacket( // since the original packet sent was never received and has been timed out. // If the ERC20 conversion fails for whatever reason, such as an attempt to call // a self-destructed ERC20 contract or an invalid function, OnTimeoutPacket still -// succeeds, but the user receives the corresponding bank token from the TokenPair -// instead. A user may then manually re-attempt the conversion. +// succeeds and the user keeps the corresponding bank token from the TokenPair, +// so they may manually re-attempt the conversion. As with OnAcknowledgementPacket +// that depends on ConvertCoinNativeERC20 being atomic (F-2026-18819). func (k Keeper) OnTimeoutPacket(ctx sdk.Context, _ channeltypes.Packet, data transfertypes.FungibleTokenPacketData) error { return k.ConvertCoinToERC20FromPacket(ctx, data) } @@ -235,6 +240,12 @@ func (k Keeper) ConvertCoinToERC20FromPacket(ctx sdk.Context, data transfertypes // Convert from Coin to ERC20 if err := k.ConvertCoinNativeERC20(ctx, pair, coin.Amount, common.BytesToAddress(sender), sender); err != nil { + // Swallowing the error is deliberate: the refund itself already + // succeeded and must not be undone by a failure to re-wrap it into + // the ERC20 representation. This is only safe because + // ConvertCoinNativeERC20 rolls back on failure, so the sender still + // holds the bank coins here (F-2026-18819). + // // We want to record only the failed attempt to reconvert the coins during IBC. defer func() { telemetry.IncrCounter(1, types.ModuleName, "ibc", "error", "total") diff --git a/x/erc20/keeper/msg_server.go b/x/erc20/keeper/msg_server.go index 51c8621e4..65f04aba4 100644 --- a/x/erc20/keeper/msg_server.go +++ b/x/erc20/keeper/msg_server.go @@ -228,13 +228,52 @@ func (k Keeper) ConvertCoin( } // ConvertCoinNativeERC20 handles the coin conversion for a native ERC20 token -// pair: +// pair. It runs the conversion atomically: the bank escrow, the EVM transfer and +// the burn either all commit or none of them do. +// +// The atomicity is load-bearing, not cosmetic. The body escrows coins onto the +// module account before calling the EVM, and CallEVM caches only the EVM +// execution — so on a VM failure the discarded EVM cache used to leave the bank +// escrow applied, stranding the sender's coins on the erc20 module account with +// no user-facing way to reclaim them. The IBC refund path (see +// ConvertCoinToERC20FromPacket) swallows this error and commits the block, which +// made that stranding permanent (F-2026-18819). +func (k Keeper) ConvertCoinNativeERC20( + ctx sdk.Context, + pair types.TokenPair, + amount math.Int, + receiver common.Address, + sender sdk.AccAddress, +) error { + // CacheContext branches the multistore and installs a fresh EventManager; + // writeCache replays those events onto the parent and commits the branch. + // Nothing is written unless the whole conversion succeeded. + // + // Note the gas meter is *shared* with the parent, so work done on a + // discarded branch is still charged. That is intended — a failed conversion + // should not be free. + cacheCtx, writeCache := ctx.CacheContext() + + if err := k.convertCoinNativeERC20(cacheCtx, pair, amount, receiver, sender); err != nil { + // Branch discarded: the escrow, the EVM state and any events go with it. + return err + } + + writeCache() + + return nil +} + +// convertCoinNativeERC20 is the non-atomic body of ConvertCoinNativeERC20: // - escrow Coins on module account // - unescrow Tokens that have been previously escrowed with ConvertERC20 and send to receiver // - burn escrowed Coins // - check if token balance increased by amount // - check for unexpected `Approval` event in logs -func (k Keeper) ConvertCoinNativeERC20( +// +// It mutates ctx directly and must only be called on a branched context — see +// ConvertCoinNativeERC20. +func (k Keeper) convertCoinNativeERC20( ctx sdk.Context, pair types.TokenPair, amount math.Int,