fix: F-2026-18819 | [Dual Defense] IBC Refund Conversion Swallows EVM Error After Escrowing Coins Into erc20 Module

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

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")
+}

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")

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,