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

[0xNilesh]

Finding

ConvertCoinNativeERC20 mutates bank state on the parent context before touching the EVM:

// Escrow Coins on module account
if err := k.bankKeeper.SendCoinsFromAccountToModule(ctx, sender, types.ModuleName, coins); err != nil {
    return sdkerrors.Wrap(err, "failed to escrow coins")
}

// Unescrow Tokens and send to receiver
res, err := k.evmKeeper.CallEVM(ctx, erc20, types.ModuleAddress, contract, true, nil, "transfer", receiver, amount.BigInt())
if err != nil {
    return err   // <- escrow already committed, never undone
}

CallEVMWithData caches only the EVM execution. On a VM failure it discards the EVM cache and returns an error, leaving the bank send applied. Every later failure in the function has the same shape — the return-value check, the balance-invariance check, and the burn.

Called directly via MsgConvertCoin this is survivable, because baseapp discards the whole message on error. The IBC refund path is where it becomes permanent: ConvertCoinToERC20FromPacket swallows the error, emits FailedConvertERC20, returns nil, and the block commits.

That swallow is correct in intent — the refund itself already succeeded and must not be undone by a failure to re-wrap it into the ERC20 representation. But it was resting on an assumption the code did not honour. The doc comments on OnAcknowledgementPacket and OnTimeoutPacket both stated:

the user receives the corresponding bank token from the TokenPair instead. A user may then manually re-attempt the conversion.

False once the escrow had succeeded. The coins were on the erc20 module account, the ERC20 transfer never credited the user, and there is no shipped message to reclaim a module balance. Recovery would have needed gov or an upgrade handler.

OnRecvPacket does the opposite for the identical error (return channeltypes.NewErrorAcknowledgement(err) at ibc_callbacks.go:137) — the swallow is specific to the refund path.

Changes

ConvertCoinNativeERC20 is now atomic. The body moved to an unexported convertCoinNativeERC20; the exported function wraps it in a CacheContext and calls writeCache() only on full success. Escrow, EVM transfer and burn commit together or not at all.

Structured deliberately as a thin wrapper plus an untouched inner body, so that PR #44's validateApprovalEventDoesNotExist insertion lands inside the inner function and merges mechanically.

Comments corrected. The OnAcknowledgementPacket / OnTimeoutPacket promises are now true, and each says why — they hold only because the conversion is atomic. The swallow in ConvertCoinToERC20FromPacket states the precondition it depends on.

Two behaviours worth naming explicitly, since neither is obvious:

• CacheContext's writeCache replays the branch's events onto the parent, so events survive success and vanish on failure. No manual handling needed.
• The gas meter is shared with the parent, so work on a discarded branch is still charged. Intended — a failed conversion should not be free.

Tests

Two integration tests in tests/integration/x/erc20/test_ibc_callback.go: one at the keeper level, one on the IBC refund path that made this permanent. Both assert balances, not just the error — an error assertion alone would not have caught this.

Getting the failure to land in the right place took two corrections worth recording:

Pausing the token does not work. It fails the balanceOf read at msg_server.go:292, which runs before the escrow, so nothing is ever escrowed and the test passes vacuously. The setup instead leaves the module account with no token balance, so balanceOf succeeds, the escrow succeeds, and transfer reverts with nothing to send — the only ordering that can strand coins.

The context must be fetched after setupRegisterERC20Pair. A context taken earlier does not see the deployed contract and balanceOf returns nil. The helper returns its own context for this reason.

Mutation-verified — wrap removed, tests re-run, fix restored:

mutation	result
replace the CacheContext wrap with a direct call	both tests FAIL: "sender must keep every coin - the escrow has to roll back with the EVM failure" and "the refunded coins must still be with the sender"

The first version of these tests passed under that mutation. They were rewritten until they didn't.

go test ./x/erc20/... and the ERC20|IBC integration suites are green.

Hacken's remediations

1. Run the whole body in a CacheContext, write only on success — done.
2. Or explicitly send the coins back on failure — not taken. It leaves the non-atomicity in place and needs a compensating path per failure mode; rec 1 covers every one uniformly.
3. Fix the comments — done, on all three sites.
4. Regression: forced EVM failure must leave coins with the sender — done, and mutation-verified.

Merge order

PR #44 (F-2026-18822) touches the same function. Land #44 first; the split into wrapper + inner body should let this rebase cleanly.