Skip to content

fix: F-2026-18786 | [Dual Defense] Unknown Selector Still Runs CommitWithCacheCtx Before ABI Fail - #48

Merged
0xNilesh merged 2 commits into
audit-fixesfrom
F-2026-18786
Aug 26, 2026
Merged

0xNilesh merged 2 commits into
audit-fixesfrom
F-2026-18786

Conversation

@0xNilesh

Copy link
Copy Markdown
Member

Finding

F-2026-18786 — Unknown Selector Still Runs CommitWithCacheCtx Before ABI Fail
Severity: Low (Impact 1 / Likelihood 2).

Every stateful precompile routes through cmn.Precompile.RunNativeAction, which runs the whole
expensive preamble before the ABI selector is ever resolved:

ctx, _ := stateDB.GetCacheContext()
snapshot := stateDB.MultiStoreSnapshot()
stateDB.AddPrecompileFn(snapshot, events)
stateDB.CommitWithCacheCtx()   // replays every dirty account + storage slot
initialGas := ctx.GasMeter().GasConsumed()
...
bz, err = action(ctx)          // SetupABI resolves the selector HERE, and fails
if err != nil { return bz, err }   // returns BEFORE contract.UseGas below

RequiredGas — charged by go-ethereum before Run — returns 0 for an unresolvable selector,
with a comment stating the assumption that fails: "This should never happen since this method is
going to fail during Run."
It does happen (four random bytes), and "during Run" is after the commit.
initialGas is also captured after CommitWithCacheCtx, so the preamble is outside the meter
even on the success path.

commitWithCtx runs with keepDirty=true, so each of up to 20 precompile frames per tx re-replays
the entire dirty set — a 20x multiplier.

Fix

Resolve the ABI method at the top of each concrete Run, before RunNativeAction is entered.
Feasible because each concrete precompile embeds abi.ABI directly, unlike the shared
cmn.Precompile wrapper.

Applied to the eight stateful precompiles that route through the wrapper: bank, gov, staking,
distribution, ics20, slashing, plus erc20 and werc20, which use the same wrapper and
were not covered by the original PoC.

The guard is fallback-aware, not a length check

The obvious form of this fix —

if len(contract.Input) < 4 { return nil, vm.ErrExecutionReverted }
if _, err := p.MethodById(contract.Input[:4]); err != nil { return nil, err }

— is wrong for werc20, whose ABI declares both a fallback and a receive. cmn.SetupABI
has a documented special case that werc20.Execute depends on: empty calldata resolves to
receive/fallback, calldata shorter than a selector resolves to fallback, and an unknown
selector resolves to fallback — all three behaving like deposit. There are twelve existing
integration specs covering exactly those paths.

So the guard goes through a new cmn.ResolveMethod, which is the selector-resolution half of
SetupABI lifted out verbatim; SetupABI now calls it, so the early check and the in-action check
cannot drift apart. Errors are returned through cmn.ReturnRevertError, matching what
RunNativeAction does today, so the revert reason and gas refund on the failure path are byte-identical.

Live verification (local node only)

Re-ran the PoC harness (Probe.sol dirties n storage slots, then calls the target with
0xdeadbeef; 20x amplification), on the same machine in the same session, against the unmodified
binary and against a pchaind built from this branch. receipt.gasUsed is unusable here (feemarket
min_gas_multiplier=0.5 floors it to gasLimit/2), so gas is eth_estimateGas and CPU is
eth_call round-trip median latency.

Dirty-set sweep, bank minus no-op target, 20 frames:

n slots Δgas before Δms before Δgas after Δms after
0 394 0.41 394 0.08
50 394 1.79 394 0.08
100 394 3.15 394 0.08
250 394 7.23 394 0.04
500 394 14.02 394 0.17
1000 394 28.64 394 -1.02

Gas is unchanged at every point — the fix costs nothing and charges nothing new. CPU cost, dead
linear before, is now flat and indistinguishable from the no-op baseline.
(The original PoC run measured +39.32 ms at n=1000 on a more loaded machine; the same-session
control above reproduces the same linear shape at +28.64 ms.)

Per-precompile map (n=500, k=20, garbage selector), Δms vs no-op:

precompile before after
staking 0x…0800 +13.98 +0.03
distribution 0x…0801 +14.97 +0.04
ics20 0x…0802 +13.99 +0.00
bank 0x…0804 +13.87 +0.03
gov 0x…0805 +13.92 +0.01
slashing 0x…0806 +13.96 +0.01
p256 0x…0100 (stateless control) -0.06 -0.05
bech32 0x…0400 (stateless control) -0.07 -0.08
usigverifier 0xEC…01 (Push control) -0.05 -0.07
no-op baseline 0.00 0.00

All six affected precompiles now sit on the usigverifier / no-op line.

Valid dispatch on the live node is unaffected: bank.balances, bank.totalSupply and
slashing.getParams return real data, and the MaxPrecompileCalls cap still reads
k=1→1, 5→5, 19→19, 20→20, 21→20, 25→20, 40→20, 64→20 — identical to the pre-fix numbers.
Direct EOA→precompile calls with a garbage selector still revert with status=0 and
gasUsed at the gasLimit/2 floor, i.e. gas is refunded rather than consumed, confirming the
revert semantics did not change.

Tests

New *_selector_test.go in each of the eight precompile packages. They drive Run with a nil
StateDB: the early guard never looks at evm.StateDB, whereas the first thing RunNativeAction
does is type-assert on it, so a revert reason of not run in EVM proves the call reached the
preamble and any other reason proves it was rejected before it.

  • unknown selector → rejected before the native action, reason no method with id
  • calldata shorter than a selector → rejected before the native action
  • empty calldata → rejected before the native action
  • a valid selector → still dispatched into the native action
  • werc20 instead asserts that all of empty / short / unknown calldata still reach the native
    action, because its fallback/receive legitimately handle them

Mutation-verified both ways:

  • removing the guard from bank fails 3 of its 4 subtests (should not contain "not run in EVM"),
    while the valid-selector subtest keeps passing;
  • putting the naive len < 4 || MethodById template on werc20 fails all four of its
    fallback/receive subtests and 10 specs of the real werc20 integration suite.

Suites run (-tags=test): the whole precompiles/... tree green; integration suites for werc20
(81/81 specs), erc20, slashing, ics20, gov, distribution and staking all green. The bank
integration suite has 4 pre-existing failures (stale total-supply constants) that reproduce
identically on unmodified audit-fixes.

Declined recommendations

  • Rec 2 — account for the preamble on the error path. Declined. initialGas is captured
    after CommitWithCacheCtx, so charging on the error path would not cover the preamble anyway.
    Making it correct means moving the gas baseline, which changes gas accounting for every
    stateful precompile call including successful ones — far too much risk for an Impact 1 finding.
    This PR deletes the wasted work instead of pricing it, so no accounting changes at all.
  • Rec 3 — non-zero RequiredGas floor. Deferred as the weaker half. It prices the waste rather
    than preventing it, and there is no honest value for it — the cost covers "a snapshot plus a
    commit whose size depends on the caller's dirty set". Optional belt-and-braces after this fix.
  • Rec 4 — backport upstream gas-alignment fixes. N/A, nothing to backport (see below).

Provenance

Inherited from cosmos/evm. Unfixed in every tag and on upstream/main @ 3e646c43; not
Push-authored. Push's own precompiles are unaffected because they already implement this pattern:
precompiles/usigverifier/usigverifier.go (0xEC…01) and the utxverifier precompiles
(0x…CA / 0x…CB) override Run and resolve the selector first — they are the in-repo precedent
this fix mirrors, and they were the PoC's flat controls.

Interaction with #47

#47 merges develop into audit-fixes for
cosmos/evm v0.6.0 and also touches precompiles/common/precompile.go, but only inside
runNativeAction (AddPrecompileFn loses its events argument, CommitWithCacheCtx becomes
FlushToCacheCtx). This PR's edits to that file are SetupABI and the new ResolveMethod beneath
it, so the two should not conflict. The finding survives v0.6.0 unchanged — FlushToCacheCtx is the
same replay, still ahead of selector resolution — so this fix stays correct and necessary after the
merge either way.

Stateful precompiles ran the full native-action preamble - including a state DB commit that replays the caller's dirty set - before SetupABI resolved the selector, and RequiredGas returns 0 for an unresolvable one so none of it was charged.
@0xNilesh
0xNilesh merged commit 2e5db83 into audit-fixes Aug 26, 2026
6 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant