Skip to content

fix: F-2026-18825 | [Dual Defense] One Invalid Gateway Sibling Erases Valid Outbounds After Committed UEA Burn - #323

Open
0xNilesh wants to merge 2 commits into
audit-fixesfrom
F-2026-18825
Open

fix: F-2026-18825 | [Dual Defense] One Invalid Gateway Sibling Erases Valid Outbounds After Committed UEA Burn#323
0xNilesh wants to merge 2 commits into
audit-fixesfrom
F-2026-18825

Conversation

@0xNilesh

Copy link
Copy Markdown
Member

Root cause — the burn commits before the attach runs

Outbound creation reached through inbound handling is the one of three outbound-creation paths that is
broken; the direct MsgExecutePayload path and EVMHooks.PostTxProcessing both propagate errors and are safe.

ExecutePayloadV2 (x/uexecutor/keeper/execute_payload.go) wrapped EVM execution + fee deduction in a
CacheContext and committed via writeCache() before returning. Both callers then attached the
outbounds afterwards, outside any cache — so a payload that called UniversalGatewayPC had its PRC20
burn already permanent
by the time the attach ran.

BuildOutboundsFromReceipt (create_outbound.go) is all-or-nothing: four return nil, err points (log
decode, chain lookup, chain disabled, unregistered PRC20) discard every valid outbound already
accumulated
. So a UEA multicall carrying one valid gateway leg and one invalid sibling ended with:

  • both burns committed,
  • no OutboundTx and no PendingOutbounds row,
  • the failure stashed in UniversalTx.RevertError — a field with 9 writes / 0 reads chain-wide,
  • the payload PcTx marked SUCCESS, and the handler returning nil.

Nothing on chain recorded that anything had gone wrong, so neither rescue nor remint was eligible.

Why the fix is atomicity, not error propagation

The inbound handler runs inside MsgVoteInbound. Returning an error there would fail the vote tx and lose
the validator's vote, so the handler must keep returning nil. That consensus constraint is exactly the
design pressure that produced the swallow — which is why the fix has to make the burn and the attach commit
together, rather than bubble the error up. The handlers still return nil; the failure now surfaces as a
FAILED PcTx instead of a silent RevertError.

The change

x/uexecutor/keeper/execute_payload.goExecutePayloadV2 takes the types.UniversalTx it is
executing for and performs the attach inside its existing cacheCtx, before writeCache(). On attach
failure it returns outbound attach failed: … without committing, so the EVM state (including the gateway
burn), the fee deduction and the outbound rows all roll back together. ExecutePayloadV2 has exactly two
callers, both inbound handlers, so the signature change is contained.

execute_inbound_funds_and_payload.go / execute_inbound_gas_and_payload.go — the post-hoc
AttachOutboundsToExistingUniversalTx block is gone from the UEA branch of both handlers. A payload error
now records a FAILED PcTx carrying the real reason; nothing writes RevertError and nothing marks
SUCCESS on this path any more.

create_outbound.go — the bare collections.ErrNotFound from the PRC20 lookup is wrapped with the token
and chain, so the FAILED PcTx names the leg that could not be resolved instead of just saying not found.
Behaviour is unchanged.

BuildOutboundsFromReceipt deliberately left all-or-nothing — Hacken #1 declined

Once burn and attach are atomic, all-or-nothing is the correct semantics: one bad step reverts the tx,
exactly as ordinary EVM execution behaves. Hacken's recommendation #1 (skip the invalid leg and continue)
would commit the bad leg's burn with nothing to show for it, converting "lose everything" into "silently
lose one thing" — strictly worse, and it would keep the failure invisible. Hacken's #4 (wire RevertError
into a recovery path) is likewise skipped: the goal is to eliminate the swallow, not to build machinery
around it.

Resulting user outcome

The deposit happens before the payload cache, so the rollback undoes only the payload. Bridged funds stay
credited to the UEA, nothing is burned, no partial outbound row is left behind, the PcTx says FAILED with an
actionable reason, the ballot finalises normally, and the user retries without the bad leg.

Tests — test/integration/uexecutor/inbound_multicall_outbound_atomicity_test.go

The payloads are real UEA multicalls (bytes4(keccak256("UEA_MULTICALL")) + ABI-encoded Multicall[]), each
leg burning a PRC20 through UniversalGatewayPC. The mock gateway's outbound nonce (storage slot 2) is used
as the commit/rollback witness.

Test Asserts
headline, both handlers one valid + one unregistered-PRC20 leg ⇒ gateway nonce stays 0, no OutboundTx, no PendingOutbounds, no gas fee collected, PcTx FAILED naming the bad PRC20, RevertError empty, vote tx still commits, deposit still credited to the UEA
happy path, both handlers all-valid multicall ⇒ 2 OutboundTx with distinct ids + 2 PendingOutbounds rows, PcTx SUCCESS, both burns committed
regression a payload emitting no gateway event still succeeds, with no spurious rows and its own EVM work committed

Each headline assertion was verified as a genuine regression detector: with the fix reverted, all five fail
in exactly the shape of the finding — burn committed, fee collected, PcTx SUCCESS, empty ErrorMsg, and
RevertError holding the swallowed error.

./x/uexecutor/..., ./test/integration/... — all green.

⚠️ Overlap with #319 (F-2026-18195)

#319 is open against the same base and modifies the same two handler files — but the CEA
smart-contract branch
(CallExecuteUniversalTx), while this PR changes the UEA branch
(ExecutePayloadV2) plus execute_payload.go. Same atomicity fix, opposite branch of the same if/else, and
the naming (outbound attach failed: …, attach inside cacheCtx before writeCache()) is deliberately
mirrored so the two land consistently.

I trial-merged the two branches locally: the merge is clean, both changes survive intact, and the merged
tree builds and passes the full ./test/integration/uexecutor/... suite including both PRs' new tests.
Merge order does not matter for correctness, but whichever lands second should re-run that suite.

Ref: audits/TO_BE_FIXED.md## F-2026-18825.

…-18825)

The gateway burn was committed before the handlers attached the outbounds, so
one invalid leg of a multicall erased the valid ones with their burns already
final and the failure swallowed into RevertError.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant