fix: F-2026-18822 | [Dual Defense] Native ERC20 Convert Skips Approval Log Check — Unbacked Mint on Malicious Pair

[0xNilesh]

The invariant

x/erc20 bridges an externally-owned ERC-20 to a bank denom by escrow-and-mint:

• In (convertERC20IntoCoinsForNativeToken): the user transfers tokens to the module's EVM
  address, the module mints erc20:<token> bank coins.
• Out (ConvertCoinNativeERC20): the module burns the coins and transfers the tokens back.

The invariant is module escrow balance == bank supply of that denom.

How it broke

After the transfer the keeper checked only that (a) the call reported success and (b) the
module's token balance rose by exactly the amount. Both hold for a malicious registered token
whose transfer also grants a third party an allowance over the recipient — i.e. over the
escrow module:

// contracts/solidity/x/erc20/keeper/testdata/ERC20MaliciousDelayed.sol  (already in-tree)
function transfer(address recipient, uint256 amount) public virtual override returns (bool) {
    // Any time a transaction happens, the thief account is granted allowance in secret.
    // Still emits an Approve!
    super._approve(recipient, _thief, _bigNum);
    return super.transfer(recipient, amount);
}

Register → convert 100 in (balance check passes, 100 coins minted) → later call
transferFrom(module, thief, 100) → escrow empty, 100 bank coins still outstanding, unbacked.
The balance check is a snapshot comparison taken before the allowance is ever used; nothing
looks wrong at the instant it is taken.

The intended defence was designed and never wired up. validateApprovalEventDoesNotExist
(x/erc20/keeper/util.go) is defined, unit-tested (x/erc20/keeper/util_test.go), and promised
by both convert doc-comments (msg_server.go — "check for unexpected Approval event in
logs") — but called from nowhere. The integration case even read "pass - delayed malicious contract" with expPass: true, so the suite asserted the vulnerable behaviour was correct.

Changes

1. Wire the guard into both convert directions (x/erc20/keeper/msg_server.go) — after the
transfer return-value validation, before the balance comparison:

// Check for unexpected `Approval` event in logs
if err := validateApprovalEventDoesNotExist(res.Logs); err != nil {
    return nil, err   // bare `err` in ConvertCoinNativeERC20
}

No false-positive risk: both paths call plain transfer, never transferFrom, so a compliant
ERC-20 emits only Transfer here. The legitimate mid-transfer Approval (an allowance
decrement) cannot arise on these paths.

2. Fix the tests that encoded the bug (tests/integration/x/erc20/test_msg_server.go) —
"pass - delayed malicious contract" becomes "fail - delayed malicious contract" with
expPass: false, and two new tests assert the invariant rather than just the error:

• TestConvertERC20MaliciousApprovalKeepsEscrowInvariant — ERC-20 → Coin. Runs the rejected
  message both directly (to pin the error) and through a real committed tx (to get baseapp's
  real rollback), then asserts the module's escrow balance is unchanged, the sender's token
  balance is unchanged, and the denom's bank supply is still zero — no unbacked coins minted.
• TestConvertCoinMaliciousApprovalKeepsEscrowInvariant — Coin → ERC-20, the direction that had
  no malicious-token coverage at all. Asserts nothing leaves escrow, the receiver gets nothing,
  and no coins are burned.

Both new tests fail on audit-fixes without change 1 (verified by stashing the keeper diff).

Small extra, worth flagging: validateApprovalEventDoesNotExist indexes log.Topics[0]
without a length check. That was harmless while the helper was dead and is still narrow in
validateTransferEventExists (only reached when the token returns no value), but the new call is
unconditional, so any registered token emitting an anonymous log (LOG0) from transfer would
have panicked in the keeper. Added a len(log.Topics) == 0 { continue } guard plus a unit-test
case rather than introduce a new panic path. The helper itself is otherwise untouched.

Known limitation — this is defence-in-depth, not a security boundary

The check is log-based and therefore bypassable. ERC-20 does not force an event on an
allowance write, so a malicious token can set allowance[module][thief] directly in storage,
emit nothing, and sail through. This restores the documented intent and catches the naive case;
it is not a boundary. The only real boundary is not letting arbitrary contracts become pairs
(PermissionlessRegistration), which is deliberately out of scope here — it is a product
decision, not a bug fix, and DefaultParams() is untouched by this PR.

Hacken's second remediation ("also assert module allowance(module, *) == 0") is not
implementable: allowance(owner, spender) requires a known spender, and the EVM offers no way
to enumerate the keys of a mapping. You can check "not this address", never "no address".

Provenance — upstream cosmos/evm gap, not Push-authored

• The call is present in v0.1.0 and v0.2.0.
• Zero calls in every tag from v0.3.0 (2025-07-16) through v0.6.2 and v0.7.2 (both
  2026-08-19) — ~13 months, across both current release lines. There is no patched upstream
  version to upgrade to.
• upstream/main @ 3e646c43 (2026-08-21) still carries the orphaned helper, its unit test and
  both doc-comments, and still asserts expPass: true on the ERC20MaliciousDelayed fixture.

Shape of the removal (swapping monitorApprovalEvent → validateTransferEventExists in one
edit, leaving helper, test and doc-comments behind) reads as a consolidation where the Approval
half was dropped and never re-wired — not a deliberate decision that the check was unnecessary.

Testing

go build ./...                                   # clean
go vet ./x/erc20/... ./tests/integration/x/erc20/...   # clean
go test ./x/erc20/...                            # ok (keeper, types)
cd evmd && go test -tags=test ./tests/integration/ -run 'ERC20|Erc20'   # ok, 15.5s
cd evmd && go test -tags=test ./tests/integration/ -run 'IBC'           # ok, 2.6s

x/erc20/keeper/ibc_callbacks.go also calls ConvertCoinNativeERC20 (OnRecvPacket /
OnAcknowledgementPacket), which is why the IBC suite is included above.

Audit finding: F-2026-18822 (Low, Impact 2 / Likelihood 2) · tracker RC-25 / PSHL1DDA-489