Skip to content

fix: F-2026-18823 | [Dual Defense] buildRevertOutbound Fail-Open Leaves Unsignable INBOUND_REVERT and Blocks Rescue - #330

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

fix: F-2026-18823 | [Dual Defense] buildRevertOutbound Fail-Open Leaves Unsignable INBOUND_REVERT and Blocks Rescue#330
0xNilesh merged 2 commits into
audit-fixesfrom
F-2026-18823

Conversation

@0xNilesh

Copy link
Copy Markdown
Member

Fixes F-2026-18823buildRevertOutbound fail-open leaves an unsignable INBOUND_REVERT and blocks rescue.

Root cause — fail-open

buildRevertOutbound built the INBOUND_REVERT as PENDING first, then looked up gas metadata. On failure of either lookup — GetTokenConfig, or GetGasFeeInfoForRevertOutbound (UniversalCore.getOutboundTxGasAndFees) — it logged "proceeding without gas fields" and returned the outbound anyway, with GasToken / GasFee / GasPrice / GasLimit empty.

attachOutboundsToUtx then wrote it into PendingOutbounds unconditionally (no status check). The universal validators' outbound builder rejects it (gas price is zero or missing), so it is never signed, never broadcast, and sits PENDING forever. Re-resolving the metadata later does not rewrite the fields already stored on the outbound, so the row is dead on arrival.

Why fail-closed alone is not enough

Non-CEA rescue was gated on an INBOUND_REVERT having reached REVERTED. Simply not attaching the unsignable revert would clean the queue and still leave the user stuck: rescue would keep refusing with "has no reverted inbound-revert outbound". Recovery has to be restored alongside the queue hygiene.

The fix (four parts)

  1. buildRevertOutbound returns (*OutboundTx, error) — failure is explicit instead of a silently half-built struct. All six callers updated; admin_revert.go's previously-unreachable nil check is now real.
  2. On gas-metadata failure the revert is marked Status_ABORTED with an AbortReason instead of PENDING, and the gas fields are left empty rather than half-written. This reuses the existing shape from AbortOutbound.
  3. attachOutboundsToUtx indexes only PENDING outbounds into PendingOutbounds, and emits the same outbound_aborted event AbortOutbound does for the rest, so monitoring sees it. Correct generally, not just for this case.
  4. The non-CEA rescue gate accepts REVERTED or ABORTED. Rescue exists for "funds never reached Push and are still locked at source" — a non-CEA inbound that failed and whose revert could not even be built is exactly that. This avoids needing a new admin message to patch gas fields or force a status (Hacken rec 2, deliberately out of scope).

The outbound is still attached in the abort case: the attempt stays in the audit trail, and it is what makes the UTX rescue-eligible. The healthy path — metadata resolves — is untouched: still PENDING, still indexed, same gas values.

Cross-cutting note

This removes one of the three confirmed ways to create an outbound that can never leave PendingOutbounds — no ballot can form for an unsignable row, and there is no admin abort for outbounds — alongside F-2026-18184 and F-2026-18146. It also removes one input to the F-2026-18827 keygen-guard deadlock.

Tests

x/uexecutor/keeper/build_revert_outbound_test.go (new) drives buildRevertOutbound with the UniversalCore call mocked both ways:

  • healthy path regression: metadata resolves → PENDING, no abort reason, gas fields exactly as the contract returned them, and the outbound is indexed in PendingOutbounds.
  • gas fee lookup reverts → error returned, ABORTED, reason names the failure, gas fields empty, not indexed, outbound_aborted emitted.
  • token config missing / token config without a native representation → ABORTED with the right reason.
  • nil inbound → (nil, error), the contract admin_revert.go relies on.

test/integration/uexecutor/inbound_revert_abort_test.go (new) covers it end-to-end through the vote path:

  • a failed non-CEA inbound produces an ABORTED revert with a reason, empty gas fields, and leaves PendingOutbounds completely empty.
  • with that ABORTED revert present, a non-CEA RESCUE_FUNDS is accepted (refused before this change) and the rescue itself is queued while the aborted revert stays out.

Existing tests updated where they asserted the old fail-open behaviour (vote_inbound_validation_test.go, execute_inbound_gas_test.go, revert_stuck_inbound_test.go, and the rescue-gate error string). Each now also asserts the revert is not queued.

Regression-detector check: with the three behavioural changes reverted (fail-open restored, unconditional indexing, narrow rescue gate) every new/updated assertion fails, and the two unchanged-behaviour tests still pass. Reverting only the PendingOutbounds guard independently fails every "not queued" assertion.

Results: ./x/uexecutor/... and ./test/integration/... pass. TestGaslessExecutePayloadWithModuleSender fails identically on audit-fixes without this change (pre-existing, unrelated).

Notes for review

  • The integration harness's UniversalCore stub cannot serve getOutboundTxGasAndFees (its PRC20 stub has no SOURCE_CHAIN_NAMESPACE), so every INBOUND_REVERT built at that level now takes the abort path. That is why several existing integration assertions flip from PENDING to ABORTED — it is the harness telling the truth, not a semantic change. The resolvable path is covered by the keeper unit test, which mocks the lookup.
  • The unit fixture was reading keys[authtypes.StoreKey] ("acc") from a map keyed by module names ("auth"), yielding a nil store key that panicked the first time the account keeper was touched. Fixed, and the real account keeper is now wired into the keeper under test so UniversalCore calls resolve the module address.
  • Trade-off worth naming: a transient oracle outage (e.g. chain meta not yet voted, ZeroGasPrice) now aborts the revert rather than parking it. That is not a regression — the parked row was permanently unsignable anyway, since the empty fields are never rewritten — and the abort is observable and rescue-eligible. A retry path for transient failures could be a follow-up.

…them

buildRevertOutbound failed open: when the gas metadata lookup failed it
returned a PENDING outbound with empty gas fields, which attachOutboundsToUtx
indexed into PendingOutbounds unconditionally. UVs refuse to sign it, so the
row sat there forever, and non-CEA rescue was gated on a REVERTED
inbound-revert so the user had no way out either.

- buildRevertOutbound returns (outbound, error)
- on gas-metadata failure the revert is marked ABORTED with an AbortReason
- attachOutboundsToUtx indexes only PENDING outbounds, and emits
  outbound_aborted for the rest
- the non-CEA rescue gate accepts REVERTED or ABORTED
- keeper unit tests drive buildRevertOutbound with the gas lookup mocked both
  ways: resolvable stays PENDING with exact gas fields and is indexed,
  unresolvable aborts with a reason and is not
- integration tests assert the revert is ABORTED, absent from PendingOutbounds,
  and that a non-CEA RESCUE_FUNDS is then accepted
- fix the unit fixture's auth store key (authtypes.StoreKey != ModuleName) and
  wire the real account keeper so UniversalCore calls work
@0xNilesh
0xNilesh merged commit 4f6a125 into audit-fixes Aug 26, 2026
7 checks passed
0xNilesh added a commit that referenced this pull request Aug 26, 2026
…026-18823

The two F-2026-18147 tests were written before #330 landed and asserted the
revert outbound is PENDING and queued. #330 aborts a revert whose gas metadata
is unresolvable, which is what the harness's UniversalCore stub produces, so
they now assert ABORTED and not-queued, matching the sibling happy-path test.
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