Repository navigation
Conversation
|
@codex review Please perform a comprehensive release audit of the entire tree at exact head |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex security review Audit exact head |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf6510bfe8
ℹ️ 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".
| await rateLimitExit(env); | ||
| const payload = await readUtf8BodyLimited(request, Number(parsePositiveDecimal(env.MAX_EXIT_PAYLOAD_BYTES, "exit_payload_limit", 2_097_152n))); | ||
| const budget: BudgetCoordinator = env.RELAY_BUDGET.getByName(budgetObjectName(env)); | ||
| const result = await executePreparedClaim(payload, env, budget); |
There was a problem hiding this comment.
The /v1/exit branch calls executePreparedClaim without checking SUBMIT_ENABLED, while that function constructs a signer and broadcasts the claim. In production, setting SUBMIT_ENABLED=false as prescribed by the rollback runbook therefore disables control/checkpoint submissions but leaves valid claim packages able to sign and spend from the neutral account; apply the same readiness and kill-switch gate before entering this path.
Useful? React with 👍 / 👎.
| allowanceBeforeFri: LOCKED_POOL_FEE_FRI, | ||
| allowanceAfterFri: 0n, |
There was a problem hiding this comment.
The exact audited Mainnet state is documented in docs/MAINNET.md as having zero neutral pool allowance, but every public claim calls this validator, which requires the allowance to equal 6 STRK. Consequently every newly funded vault's /v1/exit attempt now fails during readSnapshot, even though the web app continues accepting new 7 STRK funding flows; the release needs a bounded allowance-replenishment mechanism or must stop accepting reserves once claim capacity is exhausted.
Useful? React with 👍 / 👎.
| const prior = await budget.lookup(semanticKey); | ||
| if (prior.outcome === "found" && prior.state !== "released") { | ||
| return { status: "duplicate", transactionHash: prior.transactionHash }; |
There was a problem hiding this comment.
When a claim was marked SUBMITTED but receipt waiting or finalization timed out, retrying the exact package takes this early return instead of fetching the receipt and finalizing the reservation. The row therefore remains active indefinitely and RelayBudget.reserve blocks every later control, checkpoint, or claim on the shared nonce lane, despite the runbook promising that an exact retry reconciles the stored hash; handle submitted here like executeRelayPlan does.
Useful? React with 👍 / 👎.
| if (code === 53) return { category: "rpc_validate_resources", definitiveReject: true }; | ||
| if (code === 54) return { category: "rpc_account_balance", definitiveReject: true }; | ||
| if (code === 55) return { category: "rpc_validation", definitiveReject: true }; | ||
| if (code !== undefined) return { category: "rpc_other", definitiveReject: true }; |
There was a problem hiding this comment.
Treating every unrecognized JSON-RPC code as a definitive pre-acceptance rejection is unsafe: Starknet error code 59 (DUPLICATE_TX) specifically means the transaction was already received, and provider-specific/internal errors can likewise be returned after forwarding. In those cases the caller clears broadcastStarted and releases the reservation, allowing another transaction to be signed on an already-used or pending nonce; only explicitly pre-broadcast rejection codes should release exposure, while duplicate/unknown codes require hash and nonce reconciliation.
Useful? React with 👍 / 👎.
| if (proofOutput.length !== parsed.serializedActions.length + 1 || proofOutput[0] === 0n) { | ||
| throw new Error("proof_output_shape"); |
There was a problem hiding this comment.
The relayer accepts any nonzero pool class hash in proof.output, and readSnapshot verifies only the neutral and Afterlight class hashes, never the class currently installed at the transaction's pool target. If the canonical address is replaced or drifts, the public endpoint will continue signing calls to unreviewed pool code as long as it mimics the fee getters, exposing the neutral account's allowance and signing authority; enforce the documented pinned pool class both in the proof output and via getClassHashAt(POOL).
Useful? React with 👍 / 👎.
| const allowedVariants = new Set([0n, 1n, 7n, 10n]); | ||
| if (parsed.actions.some((item) => !allowedVariants.has(item.variant))) { | ||
| throw new Error("unexpected_value_or_event_action"); | ||
| } | ||
| const notes = parsed.actions.filter((item) => item.variant === 7n); | ||
| const invokes = parsed.actions.filter((item) => item.variant === 10n); | ||
| if (notes.length !== 1 || invokes.length !== 1) throw new Error("exit_action_cardinality"); |
There was a problem hiding this comment.
A package may contain any number and ordering of WriteOnce and Append actions because validation only requires one open-note event and one Afterlight invoke. A successor can therefore obtain a valid proof for a claim bundled with unrelated pool storage/channel mutations and have the neutral account sign and sponsor the whole batch; require the same exact [WriteOnce, EmitOpenNoteCreated, Invoke] shape and destination-note WriteOnce semantics already enforced by the client binder.
Useful? React with 👍 / 👎.
| maxFeeFri: networkCap.toString(), | ||
| perCallCapFri: policy.networkCapFri.toString(), | ||
| dailyBudgetFri: policy.networkCapFri.toString(), |
There was a problem hiding this comment.
Exit reservations use the same deployment-wide RelayBudget daily totals as controls but substitute the 7.5 STRK exit cap as the daily budget. After a normal claim spends about 2.8 STRK, every subsequent control/checkpoint reservation is compared against the configured 1.6 STRK control budget and rejected until UTC rollover; conversely earlier control spend reduces the available 7.5 STRK claim envelope. This contradicts the documented separate ceilings and can interrupt other users' lifecycle controls, so the two sponsorship classes need separate totals or a coherent combined budget.
Useful? React with 👍 / 👎.
| requireExitHeaders(request, env); | ||
| await rateLimitExit(env); | ||
| const payload = await readUtf8BodyLimited(request, Number(parsePositiveDecimal(env.MAX_EXIT_PAYLOAD_BYTES, "exit_payload_limit", 2_097_152n))); |
There was a problem hiding this comment.
The global two-per-minute exit limiter is consumed before the request body is even read or schema-validated. Because Origin and the intent header can be forged by non-browser clients, an attacker can continuously send two malformed requests per minute and make every legitimate, time-limited claim receive 429 without possessing a proof or application signature; perform bounded structural validation first and key the expensive quota by the validated binding while retaining a separate higher-volume ingress limit.
Useful? React with 👍 / 👎.
|
Closing this audit-only comparison PR after its findings were carried through the reviewed and merged product PR #2. This PR was never intended for merge. |
1 participant
Review-only PR for a comprehensive audit of the exact public release currently on main. This PR is not intended to be merged. Review the complete release delta and current tree, including contract, client, relayer, STRK20 integration, security boundaries, documentation, and mainnet evidence.