Skip to content

scripts: exhaustive stuck-withdrawal scanner (OP-Stack / Orbit / Polygon) - #3733

Open
droplet-rl wants to merge 8 commits into
masterfrom
droplet/stuck-withdrawal-scanner
Open

scripts: exhaustive stuck-withdrawal scanner (OP-Stack / Orbit / Polygon)#3733
droplet-rl wants to merge 8 commits into
masterfrom
droplet/stuck-withdrawal-scanner

Conversation

@droplet-rl

Copy link
Copy Markdown
Contributor

Requested by @pxrl in Slack after the August investigation that surfaced five legacy SNX withdrawals (344.22 SNX) sitting unclaimed for over three years, plus three Maker-bridge DAI withdrawals (~20k DAI) the finalizer could not see.

Both sets were missed for structural reasons rather than bad luck, and each of those reasons is now a component of this scanner.

Why the existing finalizer misses these

Blind spot Consequence What this does
Token-layer discovery Misses token-specific bridges — Maker DAI on Optimism (0x46719477…), bridged-USDC on Lisk (0x3b1aC693…) Discovers at the canonical message layer, which no bridge can dodge
MessagePassed is Bedrock-only Pre-Bedrock withdrawals invisible at any block depth — this is what hid the SNX for 3 years Separate scanner + oracle per era, with eraBoundaryBlock per chain
TokensBridged shape drift l2TokenAddress widened addressbytes32 for Solana; one proxy emits both topic0s either side of its v3.5 upgrade Filters every address on all deployed variants
Only the current SpokePool Across has three generations; the gen-1 set is confirmed on-chain to have emitted TokensBridged Full historical registry in spokePools.ts
FINALIZER_WITHDRAWAL_TO_ADDRESSES 0xf7bAc63f… is in no finalizer allowlist, so its withdrawals are never discovered Watch-list is scanner-independent; add an address and every era picks it up

Structure

Generic core — rpc.ts, scanners.ts, oracles.ts — plus an editable registry (registry.ts, spokePools.ts) supplying addresses, event signatures and the watch-list. Adding a chain or an address does not touch the core.

Two invariants worth not regressing

  1. Every RPC failure becomes a recorded coverage gap. These endpoints return {"error":…} or time out on wide ranges, and jq '.result|length' maps null0 — so a timeout reads as a clean zero. That produced false all-clears three separate times during the investigation. coverage.exhaustive is only true with zero gaps and agreement from an independent monotonic-counter cross-check (OP messenger nonce as a lower bound; Orbit position contiguity).
  2. No status is reported from an unproven oracle. A subtly wrong portal address or a hash sliced at the wrong offset returns false for every query, which is a total false positive that reads as a dramatic finding — one sweep reported 19 stuck Ink withdrawals this way and all 19 were claimed. assertDiscriminates() demands a positive and negative control first.

Verification

--verify-fixtures reproduces 10 known cases (both stuck and claimed) and passes:

PASS  SNX 55.000000 (legacy, nonce 137125)
PASS  SNX 72.306130 x4 (legacy)
PASS  legacy SNX nonce 137125 (V1 versioned hash) — relayed
PASS  DAI 14,699.87 / 4,984.94 via Maker bridge
PASS  USDC.e 15,000 (standard bridge)
PASS  Arbitrum WBTC 4.6256 -> HubPool (outbox position)
All fixtures reproduced.

End-to-end smoke tests, both against mainnet:

  • Bedrock, Optimism 14 days: 147 MessagePassed, 0 failed chunks, 4 watch-list findings — including the Maker-bridge DAI correctly decoded to token/amount/recipient, i.e. the case that defeated the manual scan.
  • Legacy, Optimism pre-Bedrock window: 18 SentMessage, 0 failed chunks, one version-0 HubPool-destined message correctly resolved as relayed.

One bug this surfaced, worth reading

OP's CrossDomainMessenger.relayMessage() reads successfulMessages[v0Hash] as replay protection but writes successfulMessages[v1Hash]. A legacy withdrawal relayed post-migration therefore leaves the v0 key false forever. My first implementation checked only v0 — the control caught it, because the five SNX messages had demonstrably been claimed yet read as stuck. legacyRelayed() now checks both keys and a fixture pins it.

Known gaps, also in the README

  • Polygon PoS is candidates-only. An exit is keyed on a Merkle proof against a checkpointed root, so processedExits() cannot be keyed from the burn log alone. Falls back to reconciling burn amounts against L1 predicate transfers — treat as a review queue, not a verdict.
  • Pre-Nitro Arbitrum is wired but its topic0/outbox addresses are from docs, not confirmed by a passing control.
  • Redstone (690) / Aleph Zero (41455) SpokePool addresses unconfirmed — both chains sunset, public RPCs dead. Marked verified: false; a zero there is unproven.
  • zkSync / Scroll / Linea have oracles but no controls and no fixtures, so lower confidence.

Not self-merging — this wants a review from someone who knows the pre-Bedrock migration better than I do, particularly the value=0, minGasLimit=0 assumption in legacyVersionedHash() which is empirical (n=2) rather than read from the migration spec.

🤖 Generated with Claude Code

Finds Across L2->L1 withdrawals that were initiated but never claimed, for the
relayer EOAs and the HubPool, across OP-Stack / Orbit / Polygon.

Motivated by five legacy SNX withdrawals (344.22 SNX) that sat unclaimed for
3+ years, and three Maker-bridge DAI withdrawals (~20k DAI) that the finalizer
could not see. Both were invisible for structural reasons, not bad luck:

- Token-layer discovery misses token-specific bridges (Maker DAI on Optimism,
  bridged-USDC on Lisk). This scans the canonical message layer instead, which
  no bridge can dodge.
- MessagePassed is a Bedrock-era predeploy, so pre-Bedrock withdrawals are
  invisible to it at any block depth. Separate scanner + oracle per era.
- TokensBridged widened l2TokenAddress address->bytes32, and one proxy emits
  both topic0s either side of its v3.5 upgrade.
- Across has three SpokePool generations, not two; the gen-1 set is confirmed
  on-chain to have emitted TokensBridged.

Structure: generic core (rpc/scanners/oracles) plus an editable registry
(registry.ts, spokePools.ts) supplying addresses, event shapes and watch-list.

Two invariants worth preserving:
- Every RPC failure becomes a recorded coverage gap. An error is otherwise
  indistinguishable from "no events" and reads as a false all-clear.
- No status is reported from an oracle that has not been shown to return both
  true and false. A wrong portal address or hash offset returns false for
  everything, which looks exactly like "everything is stuck".

fixtures.ts pins known stuck/claimed cases; --verify-fixtures must pass before
any scan output is trusted. One fixture specifically guards the legacy
successfulMessages v0-vs-v1 key bug, which would otherwise flag every legacy
message as stuck.

Co-Authored-By: Claude <noreply@anthropic.com>
droplet-rl and others added 2 commits August 18, 2026 12:51
…rbit-classic

Follow-up archaeology corrected two things in the first commit and closed
several gaps. All claims below are on-chain verified and fixture-pinned;
--verify-fixtures now covers 18 cases across 5 families and passes.

Corrections to the first commit:

- Polygon is NOT candidates-only. The exit key is
  keccak256(abi.encodePacked(blockNumber, nibbles(rlp(txIndex)), receiptLogIndex))
  and is computable from the burn receipt alone -- no Merkle proof, no API.
  Verified round trip + negative control. Also adds the checkpoint gate
  (getLastChildBlock, 15-40min lag) and rejects LayerZero OFT burns, which
  are burns but not exits.

- Linea's inboxL2L1MessageStatus() was the WRONG oracle. It is callable but
  returns 0 for every real message including confirmed-claimed ones -- it
  would have flagged every Linea message as stuck. Replaced with the
  nonce-keyed isMessageClaimed(uint256).

New / hardened:

- Orbit era routing is now enforced. Classic and Nitro outbox index spaces
  overlap numerically, so cross-era isSpent() returns plausible garbage in
  both directions; and isSpent does not exist on classic outboxes, where a
  permissive eth_call turns the revert into a silent "not claimed". Classic
  candidates are reported unknown rather than guessed. Classic claims are
  still arriving (481 in the last 500k L1 blocks).

- Verified addresses filled in: Aleph Zero + Robinhood outboxes, zkSync
  L1Nullifier (the upgraded L1SharedBridge, not the AssetRouter), Scroll and
  Linea L2 messengers, Polygon RootChain + ERC20Predicate, classic outboxes.

- Documented a second Optimism era boundary BELOW the legacy era: the 2021
  regenesis seeded messageNonce at 100000, so nonces 0..99999 live on the
  pre-regenesis chain and are undiscoverable by any eth_getLogs scan.

- Bedrock migration struct confirmed byte-for-byte, incl. gasLimit =
  16*len(data) + 1_300_873, so legacyVersionedHash's zeros are no longer an
  n=2 guess.

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c95788a651

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

portal: string,
withdrawalHash: string
): Promise<boolean> {
return isTrue(await ethCall(l1, portal, SELECTORS.finalizedWithdrawals + pad32(withdrawalHash)));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve failed oracle calls as unknown

When a per-candidate eth_call times out or reverts, ethCall returns undefined and this converts it to false, so even a run whose initial control passed reports that withdrawal as STUCK. This violates the scanner's RPC-gap invariant and can trigger false-positive recovery actions during transient L1 RPC failures; propagate an error/unknown result instead of treating every failed call as an unfinalized withdrawal.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in d8c0622. This was the worst of the batch — agreed.

ethCall now distinguishes the two cases instead of collapsing them: it returns undefined only for a genuine revert (still needed, since that is legitimately how "this function does not exist here" presents on the classic outboxes) and throws RpcError when the node could not answer, after retrying. Callers route that through a status() helper that records the candidate as finalized: "unknown" with the reason, and unknowns are printed as loudly as findings rather than being omitted.

So a transient L1 blip now downgrades the run to exit 2 instead of manufacturing a STUCK line.

Comment thread scripts/stuckWithdrawals/index.ts Outdated
Comment on lines +173 to +174
const finalized = cfg.l1.portal ? await opFinalized(l1, cfg.l1.portal, c.key) : "unknown";
const f: Finding = { ...c, finalized };

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Gate OP statuses on the portal control

When assertDiscriminates returns ok=false—for example on Blast, where no fallback hashes are supplied despite the non-standard portal events—the code still queries the portal and stores false; main then includes those candidates in unclaimed and prints them as STUCK. The warning does not prevent false findings in either console or JSON output, so status must remain unknown unless controls.portal.ok is true.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in d8c0622. Controls are gates now, not warnings — controls.portal.ok !== true forces finalized: "unknown" for every bedrock candidate with the reason attached.

On Blast specifically: rather than leave it permanently unknown, I went and found the missing fallback hashes. Its controls.finalizedWithdrawalHashes now carries three withdrawal hashes extracted from real L2 MessagePassed logs with the scanner’s own withdrawalHashSlice and confirmed to read true through finalizedWithdrawals — which incidentally end-to-end validates that slice offset. A live Blast scan now passes its control.

}
findings.push({ ...c, finalized });
}
} else if (fam === "orbit-nitro" || fam === "orbit-classic") {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Dispatch Polygon to its burn scanner

For --chains 137, the configured polygon-pos family matches none of the branches in this dispatch, and the separately implemented scanPolygonBurns is never called anywhere. The later TokensBridged sweep only records coverage and warnings, so Polygon burns can never become candidates and the advertised Polygon scan can finish with zero findings regardless of actual withdrawals.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one was already addressed — the review ran against c95788a, and commit 3024dda on the branch added the fam === "polygon-pos" branch that calls scanPolygonBurns. Sorry for the stale target.

I did verify it end to end rather than take it on trust: --chains 137 --since 1 --tokens 0x2791Bca1… dispatches, and the exit oracle passes its control. Polygon also now needs --tokens to be documented, so I added it to the README flag list — without it the family is skipped, and that skip now counts as incomplete coverage (exit 2) instead of a silent zero.

Comment thread scripts/stuckWithdrawals/index.ts Outdated
Comment on lines +207 to +212
if (cfg.l1.l1XDM && c.key) {
const st = await legacyRelayed(l1, cfg.l1.l1XDM, {
v0: c.extra?.hashV0 ?? c.key,
v1: c.extra?.hashV1,
});
finalized = st.successful;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Run a legacy control before reporting statuses

During a normal --era legacy scan, this path calls legacyRelayed without ever invoking the implemented assertLegacyDiscriminates control. If the configured L1 messenger is wrong, its calls fail, or the empirical legacy hash conversion does not apply to a message, every candidate becomes finalized=false and is reported as stuck; use the known-relayed fixture to prove this oracle first and otherwise leave statuses unknown.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in d8c0622. assertLegacyDiscriminates is now actually called for any chain with op-legacy + l1XDM, and legacy statuses are gated on it.

The fixture it needs is in controls.relayedLegacyHashes for Optimism: the v1 versioned hash of legacy SNX nonce 137125, which I confirmed on-chain reads true while its v0 counterpart reads false. Both hashes were re-derived from the keeper’s actual L1 finalizeWithdrawalTransactionExternalProof calldata rather than copied, so the values and the derivation are both pinned.

Comment thread scripts/stuckWithdrawals/package.json Outdated
"description": "Exhaustive scanner for un-finalized Across L2->L1 withdrawals",
"type": "commonjs",
"scripts": {
"scan": "tsx src/index.ts",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Point the standalone command at the actual entrypoint

When following the documented standalone flow (npm install && npm run scan), this command requests src/index.ts, but all of the new TypeScript files are directly in scripts/stuckWithdrawals/ and no src/ directory exists. Consequently the advertised standalone entrypoint fails with a module-not-found error after dependencies are installed; invoke index.ts from the package root instead.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in d8c0622"scan": "tsx index.ts", and tsconfig.json include moved from ["src/**/*.ts"] to ["*.ts"] for the same reason (npm run typecheck was compiling nothing).

Verified the documented flow properly: copied the directory to a bare temp dir, npm install && npm run scan -- --verify-fixtures, and it installs, typechecks, and reproduces all 26 fixtures.

Comment thread scripts/stuckWithdrawals/scanners.ts Outdated
Comment on lines +236 to +238
const contiguous =
positions.length === 0 ||
positions.every((p, i) => i === 0 || p === positions[i - 1] + 1 || p === positions[i - 1]);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Do not treat an empty Orbit sequence as an independent proof

When the log query returns zero positions, this sets contiguous=true and later marks coverage exhaustive, even though there is no monotonic-counter observation to independently corroborate the zero. A wrong event address/topic or a silently empty RPC response therefore satisfies the cross-check vacuously; report the independent check as unavailable for an empty sequence, or compare against an external position boundary before claiming exhaustiveness.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in d8c0622 — agreed, a zero corroborating itself is not a cross-check.

Contiguity now requires at least two observations. Below that the independent field is omitted, exhaustive is false, and the note explains that the zero is not independently corroborated and suggests widening the range or confirming the surrounding positions by hand.

Separately, while decoding the classic event for your other comment I found classic does have a usable counter — uniqueId in topic2 is the global monotonic id (verified adjacent: 83530, 83531), whereas batchNumber in topic3 is not and repeats. So classic gets a real contiguity oracle too rather than being exempted.

Observed both branches live: a 15k-block Arbitrum window with 2 positions reports uniqueId contiguity: AGREES / exhaustive=true, and an empty 10k-block window reports the UNAVAILABLE note and exits 2 where it previously claimed exhaustive.

Comment thread scripts/stuckWithdrawals/index.ts Outdated
Comment on lines +77 to +78
// Probe both keys, as the scanner does.
actual = (await legacyRelayed(l1, cfg.l1.l1XDM, { v0: f.key, v1: f.key })).successful;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Exercise discovery and hash derivation in fixture verification

The fixture command directly queries precomputed claim keys instead of running the corresponding scanner, so broken event topics, payload matching, era boundaries, or withdrawal-hash extraction can all pass verification. The legacy case is especially ineffective because the supplied V1 hash is passed as both v0 and v1, meaning an implementation regressed to checking only v0 would still pass; fixtures should start from raw L2 events or provide distinct expected V0/V1 inputs.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partly fixed in d8c0622, and I want to be straight about which part.

Fixed — the v0/v1 conflation, which was the concrete hole. Legacy fixtures now declare keys: { v0, v1 } separately with two real, distinct hashes that disagree on-chain (v1 0xdde3a671… true, v0 0xc14724b1… false). A v0-only implementation now fails this fixture, which was the whole point of it.

Fixed — the derivation gap, offline. DERIVATION_FIXTURES pins the exact relayMessage arguments decoded from the keeper’s real L1 finalization tx (0x0a2992ee…) against the expected v0 and v1 hashes, so a regression in legacyXDomainCalldataHash/legacyVersionedHash fails with no RPC at all. I also added offline coverage of the chunking loop for the same reason. Both run first in --verify-fixtures.

Not fixed — starting fixtures from raw L2 events, so broken topic0s, payload matching and era boundaries are still not covered by the fixture command. That needs pinned L2 block ranges per chain and archive RPCs, which is a bigger change than I want to bundle into a review response. I did exercise those paths manually against live RPCs (bedrock, legacy, nitro, classic, polygon) and the withdrawal-hash slice is now indirectly validated by the Blast control hashes, which were extracted from raw logs with that slice. Happy to do the event-sourced fixtures as a follow-up if someone assigns me.

Comment thread scripts/stuckWithdrawals/scanners.ts Outdated
Comment on lines +216 to +218
// Nitro L2ToL1Tx: position is topic3. Classic differs; guard defensively.
const pos = l.topics[3] ? Number(BigInt(l.topics[3])) : NaN;
if (!Number.isNaN(pos)) positions.push(pos);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Decode Classic withdrawals with their own claim key

When scanning orbit-classic, this still interprets topic3 as Nitro's global position, but the Classic event uses that topic for batchNumber and identifies a message using its separate indexInBatch. Passing the resulting batch number to the Nitro-style isSpent(uint256) oracle can mark unrelated state as spent or report claimed Classic withdrawals as stuck; decode the Classic batch/index pair and resolve it through a Classic-specific outbox oracle.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in d8c0622. Two notes.

The specific hazard you name — feeding a Classic batch number to the Nitro isSpent(uint256) — was already closed in 3024dda, which stopped resolving classic against the Nitro outbox at all and left those candidates unknown by design. The review targeted c95788a, before that commit.

What was still wrong is the part you identified about the key, and it was real: the scanner stored topic3 as position, so the recorded key was a batch number masquerading as a Nitro id. Classic candidates now use the composite key batchNumber:indexInBatch, decoded correctly — and note indexInBatch is data word 1, not word 0, because the non-indexed caller occupies word 0. I had this wrong on the first pass and caught it because the key came out as 16378:61802288871334…; verified against a real log, it is now 16378:1.

I also went ahead and built the Classic-specific oracle you suggest, behind --resolve-classic: match L1 OutBoxTransactionExecuted on indexed topic3 (batchNumber) and compare data to indexInBatch, returning undefined rather than false if the log scan hit a gap. It has its own control, because a log scan whose empty result is indistinguishable from a wrong topic0 is exactly the silent all-stuck oracle this script exists to avoid — batch 16378 on Outbox2 is partially executed (indices 4,5,6,7 claimed, 1,2,3 not), so one query proves discrimination in both directions.

Heads-up: with that enabled, a pre-Nitro Arbitrum window surfaces three HubPool-destined classic messages in batch 16378 (indices 1,2,3) that read as never executed, consistent with the raw L1 logs. Reported as findings rather than acted on — worth a human look.

Comment thread scripts/stuckWithdrawals/index.ts Outdated
Comment on lines +191 to +193
const pv = await opProvenAt(l1, cfg.l1.portal, c.key, submitter);
if (pv?.timestamp)
f.claimableAt = new Date((pv.timestamp + 7 * 86400) * 1000).toISOString();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Derive claimability from the portal's actual clocks

For Portal-2 fault-proof chains, proof timestamp + 7 days is not sufficient to determine finalizability because the backing dispute game may still be unresolved or inside its separate finality airgap. The production finalizer handles this by taking the later of proof maturity and the dispute-game finalizable time in src/finalizer/utils/opStack.ts, so this scanner can currently advertise a claimableAt that is too early and lead operators to submit reverting finalizations.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in d8c0622. Confirmed the numbers on mainnet OP: proofMaturityDelaySeconds = 604800 and disputeGameFinalityDelaySeconds = 302400, so the hardcoded seven days happened to match the first clock and ignored the second entirely.

opClaimability() now takes the later of provenAt + proofMaturityDelaySeconds and disputeGame.resolvedAt() + disputeGameFinalityDelaySeconds, mirroring getDisputeGameFinalizableAt() in src/finalizer/utils/opStack.ts. When the game has not resolved the airgap clock has not started, so it reports claimableBlockedOn (naming the game and the earliest possible proof-maturity time) rather than inventing a timestamp. Pre-fault-proof portals with no proofMaturityDelaySeconds fall back to the single 7-day clock.

Comment thread scripts/stuckWithdrawals/index.ts Outdated
// proofSubmitters[hash][0] is authoritative when set. Fall back to probing known
// third-party provers — a withdrawal proven by someone else needs
// finalizeWithdrawalTransactionExternalProof(_tx, thatAddress), NOT the plain variant.
let submitter = await opProofSubmitter(l1, cfg.l1.portal, c.key);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Select a valid proof rather than always index zero

When a withdrawal has been reproven, proofSubmitters[hash][0] can refer to an older proof backed by an invalidated or unresolved dispute game, while a later submitter has the usable proof. The existing finalizer deliberately reads numProofSubmitters(hash) - 1, but this scanner always requests index zero and then reports that address and its timestamp, which can recommend the wrong external-proof finalization; enumerate submitters or select a currently valid proof.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in d8c0622. opProofSubmitters() reads numProofSubmitters(hash) and enumerates, then opClaimability() walks them newest-first and picks the first with a proof, so a reproven withdrawal reports the current submitter rather than the superseded index-0 one. When more than one exists the finding carries proofSubmitters and reproven: "true" so it is visible in the JSON.

Slightly more than numProofSubmitters - 1 because enumerating lets it skip a newest entry whose provenWithdrawals timestamp is zero. Portals with no numProofSubmitters (pre-Portal-2) fall back to index 0, which is the only proof that can exist there. The known-third-party-prover probe is retained as a fallback for when the array is empty.

droplet-rl and others added 2 commits August 18, 2026 13:22
Addresses the Codex review on #3733. The theme across the P1s: several paths
could emit a `STUCK` finding from an oracle that had not been shown to work, or
from an RPC call that never actually answered.

Controls are now gates rather than warnings. A failed control forces every
candidate on that chain to `unknown` instead of reporting it, and the
known-claimed control values move into per-chain `ChainConfig.controls`:

- The Orbit positive control was the literal position 164622 for every Orbit
  chain. That is Arbitrum's, and fixtures.ts simultaneously pins it as UNSPENT
  because it was inside its challenge window — so the control asked "is this
  unclaimed withdrawal claimed?", correctly got false, and declared the oracle
  broken on every Arbitrum scan. Replaced with verified known-spent positions
  per chain (Arbitrum 119534, Aleph Zero 35, Robinhood 0).
- assertLegacyDiscriminates was implemented but never called; op-legacy statuses
  were reported unproven. Now run, with Optimism's verified relayed v1 hash.
- Blast's portal cannot supply the self-emitted positive control, so it now
  ships three verified fallback hashes; previously its control could only come
  back "unavailable" and statuses were reported anyway.
- Polygon exits and the new classic path are gated the same way.

RPC failures no longer read as findings. ethCall returned undefined on ANY
failure and isTrue() turned that into false, so a transient L1 timeout was
indistinguishable from an unclaimed withdrawal. It now returns undefined only
for a genuine revert and throws RpcError when the node could not answer, after
retrying; callers record those candidates as `unknown`.

getLogsChunked could livelock: it grew the window whenever no chunk had failed
yet, including the iteration that had just shrunk, so an endpoint that rejects
`chunk` but accepts `chunk/4` oscillated forever without issuing the narrower
request or recording a gap. Now grows only after a successful batch. selfTest.ts
pins this offline against a fake capped endpoint (the old code hangs on it).

Exit codes distinguish "we do not know" from "all clear": 2 for incomplete or
unproven, 1 for findings, 0 for a clean scan. A missing NODE_URL_<chain> used to
produce zero findings and exit 0.

Also:
- Classic Orbit is keyed on (batchNumber, indexInBatch), decoded from topic3 and
  data word 1 — word 0 is the non-indexed `caller`. Resolvable via L1
  OutBoxTransactionExecuted logs behind --resolve-classic, with its own control
  (batch 16378 is partially executed: 4,5,6,7 claimed, 1,2,3 not).
- Orbit contiguity no longer passes vacuously on an empty sequence, and classic
  gets a real counter via uniqueId (topic2).
- claimableAt reads both Portal-2 clocks off the portal instead of assuming
  proof + 7 days, and proof submitters are enumerated rather than taking index 0,
  matching getDisputeGameFinalizableAt() in src/finalizer/utils/opStack.ts.
- TokensBridged coverage honours `verified: false` spokes; the flag was being
  dropped by a duplicate SpokeDeployment interface in registry.ts.
- Legacy fixtures declare v0 and v1 separately (previously one hash was passed as
  both, so a v0-only regression still passed), plus offline hash-derivation
  fixtures and permanent negatives; time-dependent negatives report DRIFT.
- package.json/tsconfig point at the real entrypoint, so the documented
  standalone `npm install && npm run scan` flow works.

Verified: 26/26 fixtures pass, typecheck clean, and an A/B run shows a broken
control turning the same real Arbitrum finding from STUCK into UNKNOWN.

Co-Authored-By: Claude <noreply@anthropic.com>
`yarn lint` runs `prettier --list-different .` over the whole repo in CI, and
these files were over the 120-char printWidth. Content is unchanged apart from
line wrapping and trailing commas — split out from the review-response commit so
that diff stays readable.

Co-Authored-By: Claude <noreply@anthropic.com>
@droplet-rl

Copy link
Copy Markdown
Contributor Author

Addressed the Codex review — 15 inline threads, all replied to individually. Two commits: d8c0622 (fixes) and 612f2e1 (prettier only, split out to keep the review diff readable).

13 of 15 were real and are fixed. 1 was stale. 1 is partially fixed.

The P1s shared a theme worth stating plainly: several paths could emit a STUCK finding from an oracle that had never been shown to work, or from an RPC call that never actually answered. That is the exact failure class this script's README claims to defend against, so the fixes are structural rather than local.

  • Controls are now gates, not warnings. A failed control forces every candidate on that chain to unknown. Previously the code warned and reported anyway, with the disclaimer buried in a separate warnings array.
  • The Orbit control was inverted. It used the literal position 164622 for every Orbit chain — which is Arbitrum's, and which fixtures.ts simultaneously pins as unspent. I confirmed isSpent(164622) == false on-chain: the control asked "is this unclaimed withdrawal claimed?", correctly got false, and declared the oracle broken on every Arbitrum scan. Control values moved into per-chain ChainConfig.controls with positions verified via OutBoxTransactionExecuted logs.
  • ethCall swallowed transport errors into false. A five-second L1 timeout was indistinguishable from an unclaimed withdrawal. It now returns undefined only for a genuine revert and throws otherwise.
  • getLogsChunked could hang forever. It grew the window on the same iteration it had just shrunk, so an endpoint that rejects chunk but accepts chunk/4 oscillated indefinitely without ever issuing the narrower request. Codex called this one precisely.
  • Exit codes now separate "we don't know" from "all clear"2 incomplete/unproven, 1 findings, 0 clean. A missing NODE_URL_<chain> used to produce zero findings and exit 0.

Rather than assert the fixes by inspection I verified them:

  • 26/26 fixtures pass; typecheck clean; no unused imports.
  • The livelock is pinned by a new offline selfTest.ts (runs in ~1ms as part of --verify-fixtures). The pre-fix code hangs on that test until killed.
  • A/B run over a window containing a real Arbitrum finding, to prove the gate works in both directions: correct control → STUCK, exit 1; old broken control → UNKNOWN, exit 2.
  • Live scans exercised across bedrock, legacy, nitro, classic and polygon; the documented standalone npm install && npm run scan flow verified from a bare temp dir (it was module-not-found before).

Two things worth your attention rather than mine:

  1. One finding I could not fully close. Codex's point that fixture verification never runs the scanners is correct and I only partly fixed it. The concrete hole — a single hash passed as both v0 and v1, so a v0-only regression still passed — is fixed with two real, distinct, on-chain-disagreeing hashes, plus new offline hash-derivation fixtures. But sourcing fixtures from raw L2 events needs pinned block ranges and archive RPCs per chain, which I don't think belongs in a review response. Flagged as a follow-up; happy to take it if someone assigns me.
  2. A possible real finding surfaced by the new Classic oracle. With --resolve-classic, a pre-Nitro Arbitrum window shows three HubPool-destined messages in batch 16378 (indices 1, 2, 3) reading as never executed — consistent with the raw L1 logs, where indices 4–7 of that same batch were claimed. The oracle passes its control, so this isn't the stuck-at-false failure mode, but I've reported it rather than acted on it. Worth a human look.

Per the one-automated-round rule I haven't re-summoned Codex.

@droplet-rl

Copy link
Copy Markdown
Contributor Author

🔎 View trace

droplet-rl and others added 3 commits August 20, 2026 12:08
…g it

Re-running the scanner over the four watched addresses surfaced three ways it
returns a confident wrong answer. Each is the failure mode the README is about,
so each gets a permanent fixture rather than just a fix.

1. Legacy ETH withdrawals hashed with value = 0 -> mass false positive.
   hashCrossDomainMessageV1 takes the message value, which for a pre-Bedrock ETH
   withdrawal is the amount, not 0. The 0 was empirical from n=2, and both
   samples were SNX, i.e. ERC20 — the code said as much and it was still wrong.
   Effect: five genuinely-claimed pre-Bedrock withdrawals totalling 1,552.38 ETH
   (SpokePool -> HubPool, all relayed 2023-06-14) reported STUCK. Optimism
   full-history legacy now returns 0 findings instead of 5.

2. Polygon skipped every SpokePool -> HubPool return.
   scanPolygonBurns matched on the burn's `from` against WATCH + spokePools, but
   Across exits Polygon through PolygonTokenBridger 0x0330E9b4…, which is neither.
   The README's "watch-list matching is payload-based, so a new address is picked
   up by every scanner" is not true of Polygon, whose only identifying field is
   `from`. Adds ChainConfig.extraSenders. Recovers 4 tokens the token-derivation
   step would otherwise never have seen.

3. zk-stack / scroll / linea exited 0 without scanning anything.
   Those families have verified L1 oracles but no L2 discovery scanner, and no
   dispatch branch either — so they scanned nothing, found nothing, skipped
   nothing, and reported "scan complete, nothing unclaimed". Now counted as
   skipped, so they exit 2 with an explicit warning.

Also: the Linea negative fixture was pinned to live state without mutable:true,
so a legal claim failed the suite. Marked mutable and backed by a permanent
negative, and ORACLE_FIXTURES now honours the same drift rule as FIXTURES.

Co-Authored-By: Claude <noreply@anthropic.com>
Adds 14b (legacy ETH claim key is value-dependent), 15b (Polygon keys on the
burn sender, so the payload-matching watch-list rule does not apply there) and
15c (an unimplemented family exited 0 rather than 2). Corrects the zkSync /
Scroll / Linea line in Known gaps: they have oracles but no L2 scanner, so they
are never scanned and always exit 2.

Co-Authored-By: Claude <noreply@anthropic.com>
…hood zero

Robinhood (4663) was in CHAINS but had no NODE_URL_4663, so its L2 side had
never been scanned. The registry recorded that as "zero ACROSS withdrawals",
which was a zero produced by not looking. With the public RPC wired in, full
history reads 1,345 L2ToL1Tx events (contiguous, exhaustive) and 6 watch-list
withdrawals totalling 1,308 WETH to 0x07a since 2026-08-15, none claimed. All
six were inside the 7-day window when found and arbStackFinalizer is configured
for the chain, so this is probably just early — but there is no precedent of a
completed Robinhood withdrawal, and position 1210 (matures 2026-08-22 07:46Z)
is the first test. Note corrected accordingly.

Lens (232) was absent from CHAINS entirely while being a live Across chain. A
chain the registry does not know about is not reported as skipped — it is
invisible, which is strictly worse than exit 2. Added as zk-stack; it settles
through the same L1Nullifier as Era since chainId is part of the key. Like the
other zk-stack/scroll/linea entries it has no L2 scanner, so it exits 2.

Co-Authored-By: Claude <noreply@anthropic.com>
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