Skip to content

docs: F-2026-18801 | [Dual Defense] REJECTED Inbounds Land in ExpiredInbounds With No Shipped Refund Hatch - #334

Open
0xNilesh wants to merge 1 commit into
audit-fixesfrom
F-2026-18801
Open

docs: F-2026-18801 | [Dual Defense] REJECTED Inbounds Land in ExpiredInbounds With No Shipped Refund Hatch#334
0xNilesh wants to merge 1 commit into
audit-fixesfrom
F-2026-18801

Conversation

@0xNilesh

Copy link
Copy Markdown
Member

Finding

BallotHooks.afterInboundBallotTerminal files both terminal-failure statuses into ExpiredInbounds:

// All variants are terminal-failure (EXPIRED or REJECTED). Preserve
// the full audit trail in ExpiredInbounds for the future escape-hatch
// refund flow.

No UniversalTx is created. But the admin hatch accepts only one of them (admin_revert.go:47):

if ballot.Status != uvalidatortypes.BallotStatus_BALLOT_STATUS_EXPIRED {

and AttachRescueOutboundFromReceipt needs a UTX that doesn't exist. So a REJECTED-terminal inbound would have no shipped refund path, admin or user.

Why this is documentation rather than a code fix

The state is unreachable for inbounds. REJECTED is produced solely by Ballot.IsFinalizingVote's threshold-FAILURE branch, and VoteOnInboundBallot hardcodes VOTE_RESULT_SUCCESS. RecomputeBallotQuorum only yields EXPIRED. An inbound ballot terminates PASSED or EXPIRED, never REJECTED.

That isn't an accident of implementation — it's the shape of the protocol. An inbound vote is a claim about what you saw. Disagreement forks the ballot key into a separate variant rather than voting against one, and a validator that believes an event is fabricated simply doesn't vote. There is no "I assert this did not happen" for inbounds. REJECTED is vocabulary inherited from the generic x/uvalidator ballot layer, where only fund migration actually casts FAILURE (x/utss/keeper/voting.go:126-129, derived from an observed success flag).

And the refusal is correct, not an oversight. The two statuses mean opposite things. EXPIRED is uncertainty — quorum never formed, the deposit may be real, funds may be stuck in the source gateway; refunding is right. REJECTED is a supermajority affirmatively rejecting the observation; building a revert outbound there would pay out of the TSS-controlled vault against a deposit the validator set concluded never happened. With F-2026-18198 (Solana inbound event forgery) open on exactly this surface, that is the wrong direction.

So Hacken's recommendation 1 — extend the hatch to accept REJECTED — is declined as harmful, not deferred.

What this PR changes

Nothing behavioural. It makes an undocumented deliberate decision legible, so it can't be silently reversed:

  1. admin_revert.go — records why REJECTED is refused, why it is currently unreachable, and that a future negative-vote path must re-make this decision rather than inherit it.
  2. ballot_hooks.go — an Error-level log if a REJECTED variant ever reaches terminal routing, naming the consequence (no shipped refund path) and this finding. Today that branch cannot fire; if someone adds inbound negative voting, it becomes an alarm instead of a silent trap.
  3. revert_stuck_inbound_test.goTestRevertStuckInbound_RejectedBallot_RefusedDeliberately, seeding a REJECTED ballot directly (no vote path can produce one) and asserting the hatch refuses it, names the required status, and leaves no UniversalTx behind.

Mutation check

The mutation is Hacken's own recommendation — relaxing the hatch to also accept REJECTED:

--- FAIL: TestRevertStuckInbound_RejectedBallot_RefusedDeliberately
    Error:    An error is expected but got nil.
    Messages: admin revert must refuse a REJECTED ballot

So the test actively defends the decision rather than merely recording it.

Tests

go test -mod=readonly -tags="ledger test_ledger_mock test" ./x/... ./test/integration/...15 packages ok, 0 failures.

gofmt flags ballot_hooks.go, a doc-comment reflow that is pre-existing on audit-fixes; left alone to keep the diff focused.

Other recommendations

  • Rec 2 (auto-schedule INBOUND_REVERT when routing to ExpiredInbounds) — not taken; same objection as rec 1 for the REJECTED half, and the EXPIRED half already has the admin hatch.
  • Rec 3 (ops runbook) — reasonable and cheap; AllExpiredInbounds already exists as a query. Not code, so not in this PR.

REJECTED is a quorum asserting the observation is invalid, not a stuck
deposit, so refunding it would pay out against a deposit the validator set
said never happened. Adds an alarm if the unreachable state ever occurs.
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