audit.md

# audit.md — the CloudsForge defect register An audit of every repository in the estate, read against its own documentation. This document is the register of what is wrong. It is not a plan — the plan is [upgrade.md](upgrade.md), and the blockchain testing track is [CHAIN-TESTPLAN.md](CHAIN-TESTPLAN.md). ## How this was produced, and how much to trust it Ten auditors read the estate independently — one per repository or domain — under a house rule this codebase already keeps: every claim cites `path:line`. Each finding was then attacked by two further reviewers who did not write it. The first was asked only "is the code as described", and told to default to refuting when uncertain. The second was asked "does this matter" — whether anything reaches the path in a deployed configuration, and whether the stated consequence is real. A single refutation killed the finding. **73 findings were raised. 21 were killed. 52 survived, and five of those were three descriptions of one defect and two of another, so 49 tickets remain.** The register is narrower than the survey on purpose: a finding that reads plausibly and is wrong costs more than one nobody wrote down. Two severities here are mine rather than the reviewers'. CF-18 (the password-reset token in logs) and CF-17 (the plaintext RSA signing key) were both scored down on the grounds that they need log or database access first. Both are raised back to P1: the reset token is a live account-takeover credential that Lantern stores and every backup carries, and the signing key mints identity for every product in the estate. "You need one other thing first" is the shape of most real incidents. Everything here was true when read. Two tickets (CF-13, CF-19) were merged from findings whose line numbers predate changes made in the same session; every P0 and the highest-consequence P1s were re-checked against the current tree by hand. ## Summary | ID | Sev | Category | Component | Title | Effort | | --- | --- | --- | --- | --- | --- | | [CF-01](#cf-01) | P0 | broken | `forge-mint/forge-mint` | Give the customer the token: supply and ownership are minted to a vault key they can never use | XL | | [CF-02](#cf-02) | P0 | broken | `ninety-days-after/game` | Add `methods` to the game service's CORS: every browser PUT preflights to GET,HEAD,POST and is blocked | S | | [CF-03](#cf-03) | P1 | broken | `crucible/crucible` | Do not assess a new settlement charge while an older row for the same bot is unresolved | S | | [CF-04](#cf-04) | P1 | broken | `crucible/crucible` | Stop writing a settlement off as `uncollectable` when Pay's answer never arrived | M | | [CF-05](#cf-05) | P1 | broken | `forge-keyvault/forge-keyvault` | Give per-address holder containers a restart policy, or start them on read | S | | [CF-06](#cf-06) | P1 | security | `forge-pay/pay` | Abandon can refund a withdrawal whose signed bytes can still be applied | M | | [CF-07](#cf-07) | P1 | broken | `forge-pay/pay` | Un-stick a `stuck` sweep: it freezes its deposit address and the admin remedy is a silent no-op | M | | [CF-08](#cf-08) | P1 | security | `forge-pay/pay` | Verify the manual-sweep amount against the transaction — an overstated one mints coin balance | M | | [CF-09](#cf-09) | P1 | security | `hearth/node` | Cap RPC execution gas and add a time budget — one eth_estimateGas can stall the node for seconds | S | | [CF-10](#cf-10) | P1 | broken | `hearth/node` | Gate a full-block execution benchmark: StateDB still re-roots both tries on every mutation | L | | [CF-11](#cf-11) | P1 | unexposed | `hearth/node` | Run and measure the production PoW parameters before the mainnet genesis — they have never executed | L | | [CF-12](#cf-12) | P1 | security | `hearth/node` | Run eth_call/eth_estimateGas against a scratch node store — each call leaks state permanently | M | | [CF-13](#cf-13) | P1 | broken | `hearth/web + stack/docker-compose` | Point the explorer and browser wallet at the JSON-RPC port, and stop pinning it to localhost | S | | [CF-14](#cf-14) | P1 | security | `ninety-days-after/game` | Make the `trade` action require the target's consent and a non-empty offer | M | | [CF-15](#cf-15) | P1 | missing | `ninety-days-after/game` | Stop selling private worlds until something provisions them, or provision them | L | | [CF-16](#cf-16) | P1 | security | `platform/nimbus` | Encrypt or externalise the RSA signing key: plaintext in a shared-superuser Postgres, and unrotatable | L | | [CF-17](#cf-17) | P1 | security | `platform/nimbus` | Stop logging the password-reset token: GET /reset?token=… is written verbatim to the log stream | S | | [CF-18](#cf-18) | P1 | security | `stack/lantern` | Clamp Lantern's client-ingest statusCode and strip NUL bytes before they poison the batch insert | S | | [CF-19](#cf-19) | P2 | broken | `crucible/crucible` | Retry the final fee settlement of a stopped bot — nothing ever does, despite the log line | M | | [CF-20](#cf-20) | P2 | inaccurate | `forge-keyvault/forge-keyvault` | Fix README: deposit addresses are signable now, and the Ember chain-id gap is closed | S | | [CF-21](#cf-21) | P2 | broken | `forge-mint/forge-mint` | Let a reverted or dropped deploy be retried: a set `txHash` bricks the order forever | M | | [CF-22](#cf-22) | P2 | unexposed | `forge-pay/pay` | Expose deposit addresses and their ids — the only remedy for a frozen address is keyed by an id nothing shows | L | | [CF-23](#cf-23) | P2 | missing | `forge-pay/pay` | Give the deposit sweeper and withdrawal worker a Beacon signal — today they alert nowhere | L | | [CF-24](#cf-24) | P2 | broken | `forge-pay/pay` | Mark a build-failing `pending` withdrawal stuck — today it retries forever and blocks the chain | S | | [CF-25](#cf-25) | P2 | broken | `forge-pay/pay` | Stop minting deposit addresses on a network the deployment does not run | S | | [CF-26](#cf-26) | P2 | inaccurate | `hearth/docs` | Rewrite the status paragraphs in five docs — they all say no chain exists, and the code produces blocks | M | | [CF-27](#cf-27) | P2 | missing | `hearth/node` | Put the genesis hash in the p2p handshake — a genesis mismatch is currently silent | S | | [CF-28](#cf-28) | P2 | security | `hearth/node` | Refuse a genesis.json whose chainId disagrees with HEARTH_NETWORK — it silently re-enables cross-net replay | S | | [CF-29](#cf-29) | P2 | broken | `hearth/tools` | Fix the failing explorer-api suite — the Developer kit CI job is red on every push | M | | [CF-30](#cf-30) | P2 | drift | `hearth/web + stack/docker-compose` | Stop hardcoding chain id 7411 in the browser wallet/explorer while compose runs testnet 7412 | M | | [CF-31](#cf-31) | P2 | inaccurate | `ninety-days-after/game` | Grant the commune join stipend once per player, not once per join | S | | [CF-32](#cf-32) | P2 | broken | `ninety-days-after/game` | Make POST /worlds/:id/join atomic — unique (world_id, user_id) and a conditional tile claim | M | | [CF-33](#cf-33) | P2 | inaccurate | `platform/admin` | Fetch withdrawals by status instead of filtering the newest 200 client-side — "Nothing is stuck" can be false | S | | [CF-34](#cf-34) | P2 | unexposed | `platform/admin` | Give the console a way to abandon a stuck withdrawal — it reports the queue and offers no action | M | | [CF-35](#cf-35) | P2 | inaccurate | `platform/admin` | Stop swallowing treasury/withdrawal load failures into empty arrays — an outage renders as reassuring text | S | | [CF-36](#cf-36) | P2 | broken | `platform/nimbus` | Give refresh rotation a reuse grace window: two tabs refreshing at once burn the whole family | M | | [CF-37](#cf-37) | P2 | inaccurate | `platform/site` | Correct the public roadmap: EMBER now credits at 60 confirmations and cosmetics are server-side | S | | [CF-38](#cf-38) | P2 | drift | `shared-libs/shared` | Delete the invoice/fiat contract from shared-libs and CONTRACTS.md — the endpoints 404 | S | | [CF-39](#cf-39) | P2 | unexposed | `stack/beacon` | Monitor the 8545 JSON-RPC surface: beacon probes only REST 8645 and nothing asserts chain id 7412 | S | | [CF-40](#cf-40) | P2 | inaccurate | `stack/env + stack/docs` | Correct .env.example and MAP.md: keyvault signs sweeps now, so the withdrawal advice is wrong | S | | [CF-41](#cf-41) | P2 | broken | `stack/lantern` | Key Lantern's ingest quota on the forwarded client IP, not the tunnel's socket address | S | | [CF-42](#cf-42) | P3 | unexposed | `forge-keyvault/forge-keyvault` | Expose a way to mint a second treasury address, or the rotation UI can never be used | M | | [CF-43](#cf-43) | P3 | drift | `forge-mint/forge-mint` | Remove or wire up EVM_RPC_URL / SOLANA_RPC_URL — they cannot take effect | S | | [CF-44](#cf-44) | P3 | drift | `forge-pay/pay` | MAP.md §9 still says ETH cannot be sent, which the same file and outbound.ts contradict | S | | [CF-45](#cf-45) | P3 | broken | `forge-pay/pay` | XRP mark rebase can collide the synthetic deposit txid and latch the address on `duplicate` | S | | [CF-46](#cf-46) | P3 | drift | `platform/nimbus` | Correct the pay-proxy rationale in two files: forge-pay's CORS now allows PUT | S | | [CF-47](#cf-47) | P3 | drift | `platform/nimbus` | Derive the portal origin from the configured apex instead of hardcoding account.cloudsforge.online | S | | [CF-48](#cf-48) | P3 | broken | `stack/beacon` | Fix Beacon's incident rendering to read camelCase fields — timestamps vanish exactly when Postgres is down | S | | [CF-49](#cf-49) | P3 | unexposed | `stack/docker-compose + shared-libs` | Add Beacon to the shared surface registry so something actually links to the status page | S | **By severity** — P0: 2 · P1: 16 · P2: 23 · P3: 8 **By category** — broken: 18 · security: 9 · inaccurate: 7 · unexposed: 6 · drift: 6 · missing: 3 **By repository** — hearth: 10 · forge-pay: 9 · platform: 9 · stack: 6 · ninety-days-after: 5 · forge-mint: 3 · crucible: 3 · forge-keyvault: 3 · shared-libs: 1 ## The three to read first **A customer who buys a token on ForgeMint cannot reach it** (CF-01). The supply and the `Ownable` owner both go to a keyvault-held deployer address, and that address is bound to the `creation` shape — it may sign contract creations and nothing else, forever. There is no signature it can ever produce that transfers a token or hands over ownership. The product page says the key is "never exposed"; the repository map says management happens in the customer's own wallet. The second is not true, and that is why this survived long enough for an audit to find it. **90 Days After cannot be played in a browser** (CF-02). Its CORS registration omits `methods`, so the preflight advertises `GET,HEAD,POST` and all three of the game's `PUT` routes are refused by the browser before the server sees them. Pay and Nimbus each hit this and each fixed it for themselves; the game was never given the same line. It fails silently by construction — no server log, nothing in Lantern. **Money can be created by an operator typing the wrong number** (CF-08). The manual-sweep route checks that the transaction id is real, unrejected and deep enough, then trusts the amount typed beside it. Nothing compares that amount against what the transaction moved, and nothing caps it at what the address is believed to hold; the watcher credits the excess to the user on its next tick. The consequence is stated in a comment two functions away. No guard implements it. --- ## Tickets ### CF-01 — Give the customer the token: supply and ownership are minted to a vault key they can never use **P0** · broken · `forge-mint/forge-mint` · effort XL · user-visible **What is wrong** Every deployed token mints its entire initial supply to `deployerAddress` and, on the Forge and Foundry tiers, passes that same address to `Ownable`. `deployerAddress` is a keyvault-held EOA whose row carries `purpose: 'deployer'`, which selects the `creation` EVM shape — `assertCreation` refuses any transaction with `to != null`. So that address can sign contract creations and nothing else: it can never call `transfer`, `transferOwnership`, `mint`, `pause`, or send its leftover native gas anywhere. The customer funds it with real gas but is told outright (Order.tsx:236) that its key is never exposed. The Solana path is the same: `createInitializeMint2Instruction(mint, decimals, payer, payer)` makes the vault key both mint and freeze authority, and `signSolana` explicitly refuses SetAuthority(6) and Transfer(3). The result is that the thing the customer paid Shards for — a token they own — does not exist in any form they can reach. MAP.md:443-444 asserts the opposite ("management happens in the customer's own wallet"), which is the doc defect that has hidden this. **Evidence** - `repos/forge-mint/services/forge-mint/src/chain/evm.ts:76` — `owner: deployerAddress,` - `repos/forge-keyvault/services/forge-keyvault/src/routes/vault.ts:60` — `deployer: 'creation',` - `repos/forge-keyvault/services/forge-keyvault/src/signing.ts:224` — `if (tx.to != null) refuse('deployer addresses may only sign contract creations — `to` must be null')` - `repos/forge-mint/services/forge-mint/contracts/ForgeTokens.sol:49` — `_mint(owner_, initialSupply_);` - `repos/forge-mint/apps/forge-mint/src/pages/Order.tsx:236` — `only inside the hardened vault — never exposed.` - `repos/forge-mint/MAP.md:443` — `(`chain/erc20.ts:22-25`). The customer's deployer address is the owner and the` **How it fails** A customer buys the Forge tier, pays 4000 Shards, provisions a deployer, sends real ETH to it, and deploys on Ethereum mainnet. The contract exists and its 1,000,000,000 tokens sit at the deployer address; `owner()` is the deployer address. The customer opens MetaMask and has nothing: no key, no balance, no ownership. Listing the token, sending it to anyone, or calling `mint`/`pause` requires a signature the vault will refuse by design, and the ETH they sent for gas is equally stranded. Their only recourse is a Nimbus-admin `POST /admin/keys/:address/reveal`, which is an operator handing a customer a raw private key. **Suggested fix** Take a customer-controlled `ownerAddress` on `createTokenOrderSchema` (validated with `ethers.isAddress` / `PublicKey` and required for EVM tiers), and pass it as the constructor's `recipient_`/`owner_` in `encodeDeployData` and as the SPL mint/freeze authority. The deployer then only pays gas and never holds value, which is exactly what the `creation` shape is safe for. Until that ships, stop selling the tiers whose value is post-deploy control, and correct MAP.md:443-444 and Order.tsx:234-237 to say the deployer — not the customer — holds the supply. **Done when** - [ ] Take a customer-controlled `ownerAddress` on `createTokenOrderSchema` (validated with `ethers.isAddress` / `PublicKey` and required for EVM tiers), and pass it as the constructor's `recipient_`/`owner_` in `encodeDeployData` and as the SPL mint/freeze authority. - [ ] The deployer then only pays gas and never holds value, which is exactly what the `creation` shape is safe for. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: Two refinements. (1) The mainnet failure scenario is not reachable by an ordinary customer in the shipped configuration — `FORGE_MINT_MAINNET_ENABLED` is unset in docker-compose.yml and, with no allowlist, only the `admin` role passes `mainnetDenial`. The real-ETH stranding therefore currently lands on the operator; the customer-facing loss today is the Shard price ($14.99/$39.99/$89.99 per order) plus total non-delivery on testnet, which every order reaches. (2) The Solana half is stronger than stated: the vault does not just hold an unusable mint authority, it holds a live one — `signSolana` permits `MintTo` (tag 7) with the payer as authority, so the platform retains a permanent, unrevocable power to inflate any customer's SPL token, while `SetAuthority(6)` makes handing it over impossible. > Reviewer note: Two refinements, neither of which changes the substance. (1) The doc-defect claim is half right: MAP.md:447-449 already documents the stranded gas ("It does not sweep the deployer. Leftover gas stays at the per-order address and is only recoverable through keyvault's admin reveal"), so the leftover-ETH half is a known, written-down limitation. What MAP.md gets wrong is only the sentence at :443-444 about the token itself — "management happens in the customer's own wallet" — which is false for all three tiers. (2) The finding frames ownership as a Forge/Foundry issue; the Spark tier has no Ownable, but its entire supply is minted to the same unusable address (ForgeTokens.sol:29), so all three tiers are equally unusable and Spark's loss is total supply rather than supply plus ownership. --- ### CF-02 — Add `methods` to the game service's CORS: every browser PUT preflights to GET,HEAD,POST and is blocked **P0** · broken · `ninety-days-after/game` · effort S · user-visible **What is wrong** The game service registers @fastify/cors with `origin` and `exposedHeaders` but no `methods`. In @fastify/cors 11.3.0 the default is `GET,HEAD,POST`, so the preflight it returns advertises exactly those three. The game serves three PUTs — `/worlds/:id/actions`, `/cosmetics`, `/admin/worlds/:id/bots` — and all three are called from a browser on a different origin: the SPA resolves `HOSTS.api` (api. in prod, localhost:4002 in dev) while it is served from play./localhost:3001. This is the identical defect forge-pay already found and fixed for itself with an explicit `methods` list (pay/src/index.ts:31), and that nimbus fixed for itself (nimbus/src/index.ts:102) — the game was never given the same line. The failure is silent by construction: the preflight 204s and the handler is never entered, so nothing appears in the game's log or in Lantern. **Evidence** - `repos/ninety-days-after/services/game/src/index.ts:33` — `await app.register(cors, { origin: env.corsOrigins,` - `repos/ninety-days-after/node_modules/.pnpm/@fastify+cors@11.3.0/node_modules/@fastify/cors/index.js:11` — `methods: 'GET,HEAD,POST',` - `repos/ninety-days-after/services/game/src/routes/worlds.ts:194` — `app.put<{ Params: { id: string } }>('/worlds/:id/actions', async (req, reply) => {` - `repos/ninety-days-after/apps/game/src/pages/WorldView.tsx:155` — `method: 'PUT',` - `repos/ninety-days-after/apps/game/src/lib/api.ts:8` — `const API_URL = HOSTS.api` - `repos/ninety-days-after/services/game/src/routes/cosmetics.ts:45` — `app.put('/cosmetics', async (req, reply) => {` **How it fails** A player on play. queues actions and clicks save. `saveQueue()` PUTs to https://api./worlds//actions with `authorization` + `content-type: application/json`, so the browser preflights. The OPTIONS answer carries `access-control-allow-methods: GET,HEAD,POST`, the browser refuses to send the PUT, and the user sees "Could not save your orders" with no server-side trace. The same blocks equipping any cosmetic (`PUT /cosmetics`) and the admin console's bot config (`api.ts` setBots), i.e. the game's core action loop cannot be played from a browser at all. **Suggested fix** Add `methods: ['GET', 'HEAD', 'OPTIONS', 'POST', 'PUT']` to the cors registration in repos/ninety-days-after/services/game/src/index.ts:33, matching what pay and nimbus already state. Then audit the other two services that register cors bare — crucible and forge-mint have no PUT/DELETE routes today, so they are fine, but forge-keyvault does (routes/admin.ts:115) and only escapes because it is proxied. **Done when** - [ ] Add `methods: ['GET', 'HEAD', 'OPTIONS', 'POST', 'PUT']` to the cors registration in repos/ninety-days-after/services/game/src/index.ts:33, matching what pay and nimbus already state. - [ ] Then audit the other two services that register cors bare — crucible and forge-mint have no PUT/DELETE routes today, so they are fine, but forge-keyvault does (routes/admin.ts:115) and only escapes because it is proxied. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: Two small precisions. (1) The cited PUT callers are WorldView.tsx:155 (saveQueue), apps/game/src/lib/equip.tsx:99 (PUT /cosmetics — the finding says the SPA equips cosmetics but cites only the route, not this caller), and Admin.tsx:387 (PUT /admin/worlds/:id/bots). (2) The admin bots PUT is operator-only and is served from the same play/3001 SPA, so the user-visible blast radius is the two player-facing PUTs; the phrase "the game's core action loop cannot be played from a browser at all" is accurate for saving orders and equipping cosmetics, while all GET/POST traffic (joining, reading, ticking) is unaffected. > Reviewer note: One detail is imprecise: the finding says nothing appears in the game's log. Fastify's default request logging is enabled (services/game/src/obs.ts never sets disableRequestLogging), so the OPTIONS request IS logged — as a successful 204. The absence is of any error signal, not of any log line, which makes the failure more misleading rather than less. The Lantern claim is correct for a different reason than stated: apps/game wires no client-side error reporting at all. --- ### CF-03 — Do not assess a new settlement charge while an older row for the same bot is unresolved **P1** · broken · `crucible/crucible` · effort S · user-visible **What is wrong** `resolvePending` handles an unresolved row by logging and moving on (fees.ts:164-167) — correctly, since the outcome is unknown. But `settle()` then continues unconditionally: it computes `due = fee + feeOwed`, where `feeOwed` still carries the debt booked by that very unresolved row, inserts a brand-new settlement row, and charges it under a brand-new idempotency key (`settlement:`). The one-row-one-key invariant the module header states is preserved per row, but nothing prevents two rows from representing the same arrears and both landing at Pay. **Evidence** - `repos/crucible/services/crucible/src/fees.ts:164` — `ctx.log.warn(` - `repos/crucible/services/crucible/src/fees.ts:167` — `'fee settlement still unresolved',` - `repos/crucible/services/crucible/src/fees.ts:187` — `const due = fee + feeOwed` - `repos/crucible/services/crucible/src/fees.ts:204` — `const row = await insertSettlement({` - `repos/crucible/services/crucible/src/fees.ts:95` — `const ref = `settlement:${row.id}`` **How it fails** Pass 1 charges 200 Shards under `crucible:settlement:R1`; Pay begins committing and answers 409 `idempotency_in_flight`, so R1 stays pending and `feeOwed = 200`. The debit commits. Pass 2's `resolvePending` re-sends R1's key but that single request hits a connection reset (502), so R1 is left pending and `feeOwed` is untouched. `settle` proceeds: `due = 0 + 200`, inserts R2, and `crucible:settlement:R2` succeeds — a second 200-Shard debit for the same gain. Pass 3 then resolves R1 as `charged` too, and `feeOwed` goes negative-clamped to 0 while the wallet is 400 short. **Suggested fix** Have `resolvePending` report whether any row was left unresolved, and make `settle` return early (`{ settlement: null, deferred: … }`) when it was — the gain is already booked into `highWaterMark`/`feeOwed`, so nothing is lost by waiting a pass. That also makes the `inFlight` early return at fees.ts:218-225 a special case of one rule rather than a lone guard. **Done when** - [ ] Have `resolvePending` report whether any row was left unresolved, and make `settle` return early (`{ settlement: null, deferred: … }`) when it was — the gain is already booked into `highWaterMark`/`feeOwed`, so nothing is lost by waiting a pass. - [ ] That also makes the `inFlight` early return at fees.ts:218-225 a special case of one rule rather than a lone guard. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: Two refinements. (1) The defect is wider than the pending-row case: on any hard failure that is not 409 (a 502 from a timeout or reset that nevertheless committed at Pay), fees.ts:227-230 writes the row off as `uncollectable` with collected:0 and restores feeOwed to the full `due`. That row leaves the pending set permanently, so resolvePending never retries it, and the next pass re-bills the identical arrears under a brand-new key — the same double charge from a SINGLE failure, with no in-flight state involved. A fix that only suppresses new assessment while a `pending` row exists will not close that path; the `uncollectable` write-off on an unknown (non-refusal) outcome is itself unsound. (2) Minor: in the stated timeline feeOwed is already 0 when pass 3 resolves R1 (pass 2 set feeOwed = due - collected = 0 at :231), so the max(0, …) clamp at :151 is a no-op rather than absorbing a negative; the visible damage is feePaid inflated by 200 against no recorded debt, plus the 400-short wallet. > Reviewer note: Two mechanics in the write-up are off, both in the direction of the bug being MORE reachable, not less. (1) Pay's withIdempotency (forge-pay/services/pay/src/store.ts:155-193) claims the key inside the same transaction as the debit, and a concurrent duplicate blocks then replays the stored response — so idempotency_in_flight on /internal/charge means the original rolled back and nothing committed, and it cannot occur at all on the first send of a fresh settlement: key. Pass 1 of the scenario ("Pay answers 409 in_flight ... the debit commits") is therefore not how a row becomes pending; the realistic origin is a crucible crash/restart or DB error between insertSettlement (fees.ts:204) and updateSettlement (fees.ts:230). Everything after that holds exactly as written, since obs.ts:354 turns every timeout/reset into UpstreamError(502) which move() reports as a plain failure. (2) The same double charge exists with no pending row involved: collect returns {error} for any non-insufficient, non-inFlight failure (fees.ts:100), and settle then writes status 'uncollectable' with feeOwed = due (fees.ts:228-231). A 10s timeout on a charge Pay actually committed is written off as not-charged, the row is never retried under its own key (pendingSettlements selects only 'pending'), and the next sweep re-bills under a new key. So the fix must gate on "outcome unknown" generally — including 5xx inside collect — not just on the existence of a pending row. --- ### CF-04 — Stop writing a settlement off as `uncollectable` when Pay's answer never arrived **P1** · broken · `crucible/crucible` · effort M · user-visible **What is wrong** `collect()` branches on only two of Pay's answers: `insufficient` and `inFlight`. Every other failure — a 10s timeout, a 502 from `fetchJson`, a Pay 500 — falls through line 100 with `collected: 0`, and `settle()` then stamps the settlement row `uncollectable` (line 228). `uncollectable` is terminal: `pendingSettlements` only selects `status = 'pending'`, so that row's idempotency key `crucible:settlement:` is never re-sent. The debt survives on `bot.feeOwed`, and the next hourly sweep inserts a *new* row with a *new* UUID and therefore a *new* idempotency key. If the original charge had in fact committed inside Pay before the response was lost, the user is debited twice for one gain. The runner already gets this right for trades (`runner.ts:357` zeroes a fill only for a definitive 4xx); fees.ts does not. MAP.md §5.4 documents the identical bug for the `idempotency_in_flight` variant as "Window 2 — closed"; the transport-failure variant of the same shape is still open. **Evidence** - `repos/crucible/services/crucible/src/fees.ts:100` — `if (!first.insufficient) return { ...full, error: first.error }` - `repos/crucible/services/crucible/src/fees.ts:228` — `collected >= due ? 'charged' : collected > 0 ? 'partial' : 'uncollectable'` - `repos/crucible/services/crucible/src/store.ts:376` — `.where(and(eq(feeSettlements.botId, botId), eq(feeSettlements.status, 'pending')))` - `repos/crucible/services/crucible/src/clients/pay.ts:83` — `inFlight: err.statusCode === 409 && parsed?.code === 'idempotency_in_flight',` - `repos/crucible/services/crucible/src/runner.ts:357` — `const refused = result.status >= 400 && result.status < 500` - `repos/crucible/services/crucible/src/clients/pay.ts:16` — `const MOVE_TIMEOUT_MS = 10_000` **How it fails** Live bot gains 1000 Shards; `settle` writes row R (fee 1000, attempted 1000, pending) and POSTs `/internal/charge` with key `crucible:settlement:R`. Pay commits the 1000-Shard debit, then the pod is rotated / the response exceeds `MOVE_TIMEOUT_MS`. `move()` returns `{ok:false, insufficient:false, inFlight:false, status:502}`. R is stamped `uncollectable`, `bot.feeOwed` stays 1000. One hour later `resolvePending` finds nothing (R is not pending), `due = 0 + 1000`, row R2 is inserted, and `crucible:settlement:R2` debits another 1000. Wallet is down 2000 for a 1000 fee; `/billing` reports `feePaid: 1000`. **Suggested fix** In `collect()`, distinguish a definitive refusal from an unknown outcome the same way `runner.ts:357` does. Return a third state (`unresolved`) for anything that is not `insufficient` and not a 4xx, and have `settle()` leave the row `pending` for it — exactly the treatment `inFlight` already gets at fees.ts:218-225. Only a 4xx that is not `idempotency_in_flight`, or a wallet that genuinely cannot cover 1 Shard, may reach `uncollectable`. **Done when** - [ ] In `collect()`, distinguish a definitive refusal from an unknown outcome the same way `runner.ts:357` does. - [ ] Return a third state (`unresolved`) for anything that is not `insufficient` and not a 4xx, and have `settle()` leave the row `pending` for it — exactly the treatment `inFlight` already gets at fees.ts:218-225. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: Accurate on mechanism, with two refinements. A Pay 500 is not part of the harmful set: `chargeShards` throws inside its transaction, so nothing committed and the debt survives correctly in `feeOwed` — the only money-losing vector is a charge that COMMITS and whose response is lost (the 10s `MOVE_TIMEOUT_MS` firing while Pay's tx commits, or the pod rotating mid-response), which `fetchJson` reports as a synthetic 502. And the blast radius is zero in the shipped configuration: every caller of `settle()` is gated on live mode, and `CRUCIBLE_LIVE_ENABLED` defaults to false in env.ts, .env.example and docker-compose.yml, with live billing scheduled for M6. This is a latent defect that starts costing users real Shards the moment that switch is turned on, not one taking money today. > Reviewer note: The evidence and the failure scenario hold, but the enumeration of triggers is over-broad. A Pay 500 or 502 where the transaction rolled back charges nothing: `feeOwed` survives at fees.ts:231, the next sweep re-bills the same amount under a new key, and the user is charged exactly once — the only damage is a settlement row misleadingly labelled `uncollectable` and a `deferred`-less warn log. The double debit requires specifically a charge that COMMITTED inside Pay whose response never reached Crucible: the 10s `MOVE_TIMEOUT_MS` firing after commit, a pod rotation mid-response, or a connection reset after commit. That is the scenario the finding actually describes, so the defect is real; the fix is the runner.ts:357 rule — leave the row `pending` (outcome unknown) unless Pay definitively refused with a 4xx, and only write `uncollectable` on `insufficient`. --- ### CF-05 — Give per-address holder containers a restart policy, or start them on read **P1** · broken · `forge-keyvault/forge-keyvault` · effort S · user-visible **What is wrong** DockerVault creates one `alpine` holder container per address with `HostConfig` carrying only NetworkMode, Binds and AutoRemove — no `RestartPolicy`, so Docker's default of "no" applies. `read()` then does `getContainer(...)` followed immediately by `container.exec(...)` with no attempt to start it. Docker's exec API answers 409 for a container that is not running, so every read fails. The compose `restart: unless-stopped` at line 38 is an anchor applied to the compose services only; it does not reach containers keyvault creates through the mounted socket at line 357, and keyvault picks DockerVault whenever that socket answers. **Evidence** - `repos/forge-keyvault/services/forge-keyvault/src/vault/docker.ts:150` — `HostConfig: { NetworkMode: 'none', Binds: [`${this.volumeName(address)}:/keys`], AutoRemove: false, },` - `repos/forge-keyvault/services/forge-keyvault/src/vault/docker.ts:176` — `const out = await this.exec(container, ['cat', '/keys/key.enc'], 'read key.enc')` - `repos/forge-keyvault/services/forge-keyvault/src/vault/docker.ts:183` — `throw new Error(`unable to read custody storage for ${address}`)` - `docker-compose.yml:38` — `restart: unless-stopped` - `docker-compose.yml:357` — `- /var/run/docker.sock:/var/run/docker.sock` **How it fails** The host reboots (or dockerd is restarted). Compose brings `cloudsforge-forge-keyvault` back, the socket answers, so `selectProvisioner` chooses DockerVault again — but every `keyvault-` holder container is stopped, because none has a restart policy. From that moment every `POST /sign` and every `POST /admin/keys/:address/reveal` for an address provisioned before the reboot throws `unable to read custody storage for ` and returns 500: no deposit can be swept, no withdrawal can be signed, no forge-mint deploy can be signed. Addresses minted *after* the reboot work fine, so the outage looks intermittent rather than total. The ciphertext is intact in the named volumes, but nothing in the service starts them and no log line names the cause. **Suggested fix** Add `RestartPolicy: { Name: 'unless-stopped' }` to the holder container's HostConfig, and make `read()` tolerant of the existing fleet: inspect the container and `container.start()` (ignoring a 304 'already started') before the exec, retrying the exec once. Both are needed — the policy fixes new addresses, the start-on-read fixes every address already provisioned without one. **Done when** - [ ] Add `RestartPolicy: { Name: 'unless-stopped' }` to the holder container's HostConfig, and make `read()` tolerant of the existing fleet: inspect the container and `container.start()` (ignoring a 304 'already started') before the exec, retrying the exec once. - [ ] Both are needed — the policy fixes new addresses, the start-on-read fixes every address already provisioned without one. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: "every read fails" should be "every read for an address whose row says storage='docker' fails"; addresses that fell back to file storage during provisioning, and any address minted after the restart, continue to work — which is what makes the outage present as intermittent. Also worth adding to the finding: /health (index.ts:25) keeps returning ok:true with vault:'docker' throughout, so the compose healthcheck and Beacon never go red, and the admin reveal endpoint fails the same way, leaving no in-product path to recover a key. > Reviewer note: Two details in the evidence are imprecise. (1) The 409 surfaces at exec *create* -- `container.exec({...})` on docker.ts:107 -- not at `ex.start()`. (2) "no log line names the cause" is wrong: docker.ts:182 logs 'docker custody read failed' with dockerErr() carrying statusCode 409 and the daemon's "Container ... is not running" text, so the cause IS diagnosable from logs; what is missing is recovery, not observability. The blast radius is also wider than a reboot: any `docker stop` of a holder, an OOM-kill of its `sleep infinity` PID 1, or routine docker maintenance leaves that address permanently unsignable until an operator starts the container by hand. Fix is either RestartPolicy { Name: 'unless-stopped' } at creation plus a one-time reconcile pass at boot for already-created holders, or an inspect-and-start step at the top of read(). --- ### CF-06 — Abandon can refund a withdrawal whose signed bytes can still be applied **P1** · security · `forge-pay/pay` · effort M · user-visible **What is wrong** `POST /admin/withdrawals/:id/abandon` refunds from `signed` and `broadcast`, and its entire safety rests on the `outboundStatus` check. Two holes. (a) For EVM, `unknown` only means "no receipt" — the signed bytes are still valid, the nonce is unconsumed, and a legacy transaction has no expiry, so a node that still holds them can land the payment after the refund. The only proof an EVM transaction can never apply is a consumed nonce (`eth_getTransactionCount` at `latest` vs the nonce in `raw_tx`), and that check is not made. (b) `buildAndSignXrp` returns `txid: null`, so an XRP withdrawal that was signed but whose broadcast response was lost has `txid = null` and the chain is not asked at all — the branch is skipped entirely, contradicting the route's own docstring. **Evidence** - `repos/forge-pay/services/pay/src/routes/admin.ts:330` — `The chain is asked first, every time, and a transaction it can still see is never` - `repos/forge-pay/services/pay/src/routes/admin.ts:340` — `if (withdrawal.txid) {` - `repos/forge-pay/services/pay/src/routes/admin.ts:357` — `['pending', 'signed', 'broadcast', 'stuck'],` - `repos/forge-pay/services/pay/src/outbound.ts:364` — `if (!receipt || typeof receipt.blockNumber !== 'string') return { kind: 'unknown' }` - `repos/forge-pay/services/pay/src/outbound.ts:98` — `txid: string | null` **How it fails** An XRP withdrawal is signed; `submit` reaches rippled and is applied, but the HTTP response is lost and the node then goes unreachable, so `txid` stays null and the row goes `stuck` at 60 minutes. `PAY_XRP_LEDGER_WINDOW` defaults to 1000 ledgers (~67 min), so the blob is still applicable. The operator abandons it: `if (withdrawal.txid)` is false, the chain is never asked, and `refundWithdrawal` credits the full amount back. The user has both the on-chain payment and the refund. The EVM variant is the same outcome via `unknown` on a transaction still in a mempool. **Suggested fix** Refuse to abandon a row that has `raw_tx` set unless the chain proves the bytes can never apply: for EVM, decode the nonce from `raw_tx` and require `eth_getTransactionCount(from,'latest') > nonce` with a different txid mined at it; for XRP, require the current ledger index to exceed the blob's `LastLedgerSequence`. Never skip the chain check just because `txid` is null — a null txid with a non-null `raw_tx` is the most dangerous case, not the safest. **Done when** - [ ] Refuse to abandon a row that has `raw_tx` set unless the chain proves the bytes can never apply: for EVM, decode the nonce from `raw_tx` and require `eth_getTransactionCount(from,'latest') > nonce` with a different txid mined at it; for XRP, require the current ledger index to exceed the blob's `LastLedgerSequence`. - [ ] Never skip the chain check just because `txid` is null — a null txid with a non-null `raw_tx` is the most dangerous case, not the safest. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: Two refinements. The XRP half is wider than stated: abandon accepts `signed`, so the operator need not wait for the 60-minute stuck deadline — any XRP row in `signed` with a null txid refunds with no chain evidence at all. It is also narrower in reachability than the scenario suggests: broadcastXrp returns tx_json.hash for the whole tef* family (it throws only on tem/tel), so the next worker tick re-submits an already-applied blob, recovers the hash, and records it via markWithdrawalBroadcast — a null txid persists only while submits keep failing outright, and in that same outage an abandon with a txid would 500 on outboundStatus rather than refund. The EVM half needs no such contrivance and carries the severity on its own. Also note the finding is a re-statement of the repo's own docs/money-review.md finding 5, which was reviewed and left unfixed; the new contribution is the XRP null-txid branch, which that document explicitly (and wrongly) declares protected by LastLedgerSequence. > Reviewer note: Two refinements. First, the XRP consequence is understated: the ledger-window framing implies a bounded ~7-minute residual exposure after the 60-minute stuck deadline, but in the case the scenario itself describes — submit applied, response lost — LastLedgerSequence protects nothing and the double-pay is permanent and time-unbounded. Second, the XRP fix is not a one-line chain query: the row stores only raw_tx and pay has no XRP deserializer, so it cannot recover Sequence or LastLedgerSequence from the blob. The remedy is either to persist them at signing time or to fail closed and refuse abandon whenever raw_tx is non-null and txid is null. Worth noting the asymmetry that makes this pure downside: with a txid and an unreachable node, outboundStatus throws and the route 500s with no refund; with a null txid the same outage silently refunds. --- ### CF-07 — Un-stick a `stuck` sweep: it freezes its deposit address and the admin remedy is a silent no-op **P1** · broken · `forge-pay/pay` · effort M · user-visible **What is wrong** A sweep that misses `PAY_WITHDRAWAL_STUCK_MINUTES` before reaching maturity depth is set to `stuck`. `stuck` is in neither `UNFINISHED_SWEEP_STATUSES` (so `activeSweeps()` stops returning it and nothing ever matures it) nor `UNMATURED_SWEEP_STATUSES` (so it stops explaining the address's shortfall and the watcher reports `regression`). The money has left the address, `deposit_addresses.swept` was never advanced, and crediting for that address stops. The documented remedy — `POST /admin/deposit-addresses/:id/sweeps` — inserts a `sweeps` row with the same `address_id` and the same `txid` as the stuck row, which collides with `sweeps_address_txid_idx`; `onConflictDoNothing` returns no row, so `recordSweepIn` is never called and the route answers `replayed: true` having changed nothing. `matureSweep` refuses it too (status guard is `broadcast|confirmed`). Only a hand-written `UPDATE sweeps` clears it. **Evidence** - `repos/forge-pay/services/pay/src/sweeper.ts:456` — `await markSweepStuck(row.id, 'the sweep has not reached the depth its accounting needs')` - `repos/forge-pay/services/pay/src/store.ts:1039` — `const UNMATURED_SWEEP_STATUSES = ['signed', 'broadcast', 'confirmed']` - `repos/forge-pay/services/pay/src/store.ts:1787` — `.where(and(eq(sweeps.id, id), inArray(sweeps.status, ['broadcast', 'confirmed'])))` - `repos/forge-pay/services/pay/src/db/migrate.ts:251` — `CREATE UNIQUE INDEX IF NOT EXISTS sweeps_address_txid_idx ON sweeps (address_id, txid)` - `repos/forge-pay/services/pay/src/store.ts:1255` — `return { swept: existing?.swept ?? '0', replayed: true }` - `repos/forge-pay/services/pay/src/store.ts:1660` — `const UNFINISHED_SWEEP_STATUSES = ['pending', 'signed', 'broadcast', 'confirmed']` **How it fails** An EMBER sweep is broadcast and mined. Hearth block production slows (or the pay process is restarted, leaving `updated_at` old) so 60 minutes pass before 61 confirmations. `advanceSweep` marks the sweep `stuck`. The deposit watcher now logs `regression` for that address every 30s and credits nothing further — the depositor's next deposit is never credited. The operator follows the logged instruction and POSTs the real txid and amount to `/admin/deposit-addresses/:id/sweeps`; it returns 200 `{ replayed: true, swept: '0' }`, `swept` is unchanged, and the address is still frozen. There is no route or worker that can move the row out of `stuck`. **Suggested fix** Give `matureSweep` a `stuck` arm (or an admin `POST /admin/sweeps/:id/retry` that returns a stuck sweep to `broadcast` after re-checking `outboundStatus`), and make `recordManualSweep` reconcile against an existing `(address_id, txid)` row — if the conflicting row is unrecorded, mature it in place rather than reporting a replay. Also derive the sweep stuck deadline from `sweepMaturityConfirmations(coin)` rather than borrowing `PAY_WITHDRAWAL_STUCK_MINUTES`. **Done when** - [ ] Give `matureSweep` a `stuck` arm (or an admin `POST /admin/sweeps/:id/retry` that returns a stuck sweep to `broadcast` after re-checking `outboundStatus`), and make `recordManualSweep` reconcile against an existing `(address_id, txid)` row — if the conflicting row is unrecorded, mature it in place rather than reporting a replay. - [ ] Also derive the sweep stuck deadline from `sweepMaturityConfirmations(coin)` rather than borrowing `PAY_WITHDRAWAL_STUCK_MINUTES`. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: Two imprecisions. (1) The "or the pay process is restarted, leaving `updated_at` old" alternative is not independent: after a restart the first tick calls `matureSweep` and returns before the stuck check if the tx is by then deep enough, so the sweep only strands under the same slow-chain precondition (still below maturity depth 60+ minutes after being mined). (2) "the depositor's next deposit is never credited" is slightly overstated — a later deposit larger than the shortfall does eventually push `observed + swept` past the high-water mark and credits the excess, silently swallowing the missing sweep amount; deposits smaller than the shortfall are never credited. Line numbers have drifted: store.ts 1044/1260/1665/1792, migrate.ts 251. > Reviewer note: Two precisions. (1) Reachability: the whole path requires PAY_SWEEP_ENABLED=true, which is false by default (env.ts:111) and set nowhere in deploy/, infra/ or either compose file — no sweep rows exist in any current deployment, so this is latent until sweeping is switched on (a pending roadmap step). (2) The returned `swept` is the deposit address's current cumulative total, which is '0' only when this was the address's first sweep; in general it is the unchanged prior total. Everything else in the finding — the index collision, the 200 no-op, matureSweep's guard, and the absence of any un-stick route — is accurate. --- ### CF-08 — Verify the manual-sweep amount against the transaction — an overstated one mints coin balance **P1** · security · `forge-pay/pay` · effort M **What is wrong** `POST /admin/deposit-addresses/:id/sweeps` verifies that the supplied `txid` exists on chain, was not rejected, and is `sweepIsMature` deep. It never verifies that the transaction moved the supplied `amount` — `outboundStatus` does not carry a value — and nothing caps the recorded amount at what the address is believed to hold (`last_seen - swept`). The amount goes straight into `deposit_addresses.swept`, and the watcher's next tick computes `observed = confirmed + swept`, so any excess is credited to the user as a new deposit. The code comment two functions away already states this consequence, but no guard implements it. **Evidence** - `repos/forge-pay/services/pay/src/routes/admin.ts:233` — `const amountSmallest = parseUnits(parsed.data.amount, info.decimals)` - `repos/forge-pay/services/pay/src/store.ts:1297` — `const next = (BigInt(row.swept || '0') + amountSmallest).toString()` - `repos/forge-pay/services/pay/src/store.ts:948` — `const observed = probe.confirmed + recorded` - `repos/forge-pay/services/pay/src/store.ts:1188` — `await moveCoinBalance(` - `repos/forge-pay/services/pay/src/store.ts:1210` — `and an overstated `swept` credits the user the difference on the very next watcher tick` **How it fails** An operator reconciling a frozen EMBER address moves 100 EMBER by hand and types `1000` (or pastes a wei figure into a field that expects whole coins). The route accepts it: the txid is real and deep. `swept` becomes 1000e18 while `last_seen` is 100e18 and the confirmed probe reads 0. The next watcher tick computes `observed = 0 + 1000e18`, `delta = 900e18`, writes a `deposit_payments` row and credits 900 EMBER to that user's `coin_balances`. It is immediately convertible to Shards and withdrawable from the treasury. Nothing reconciles it back. **Suggested fix** Refuse an `amount` greater than `BigInt(address.last_seen) - BigInt(address.swept)` (plus a small fee allowance), and prefer reading the moved value from the chain: extend `OutboundStatus` with the transaction's `value + fee` for the EVM and XRP paths and compare it against the posted amount before recording. **Done when** - [ ] Refuse an `amount` greater than `BigInt(address.last_seen) - BigInt(address.swept)` (plus a small fee allowance), and prefer reading the moved value from the chain: extend `OutboundStatus` with the transaction's `value + fee` for the EVM and XRP paths and compare it against the posted amount before recording. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: Two precisions. (1) The proposed cap is the wrong fix as stated: bounding the amount at `last_seen - swept` would refuse the legitimate case this route exists for — an address whose crediting is paused by `regression`, or one that received funds after the mark last advanced, holds more than `last_seen - swept`, and a hand sweep of its full balance is exactly what an operator must record. The workable guard is the finding's other half: verify the amount (and, for EVM, the `from`) against the transaction — `OutboundStatus` (outbound.ts:72-86) currently carries only kind/confirmations/minedHeight, and the route never checks the transaction touched this deposit address at all. (2) The gap is wider than "the amount is unverified": because the verification block is gated on `canSend(coin)`, SOL manual sweeps skip `outboundStatus` entirely, so an arbitrary amount is recorded against an unverified txid string with no chain contact whatsoever (BTC takes the same path but is inert, since `recordSweepIn` no-ops on cumulative probes). The actor is an admin-role operator only — an error or an insider, never an external attacker, and an insider is not escalated by it since `PUT /admin/prices/EMBER` is an equivalent unbounded mint. > Reviewer note: Two amendments. (1) The finding understates the SOL path: `canSend('SOL')` is false (outbound.ts:48-68) so the entire chain-verification block is skipped, while `PROBE_IS_CUMULATIVE.solana` is false (chains.ts:54-60) so `recordSweepIn` still adds to `swept` — on SOL an entirely fabricated txid with an arbitrary amount is accepted with no chain check at all. BTC is the only coin immune, because `recordSweepIn` returns early on a cumulative probe. (2) A minor precision point on the mechanism: the excess is credited only to the extent the recorded amount exceeds the genuine shortfall (`prev - probe.confirmed`), which is exactly what a straight overstatement does, so the scenario as written holds. Cited line numbers should be store.ts:1302, :953, :1193. --- ### CF-09 — Cap RPC execution gas and add a time budget — one eth_estimateGas can stall the node for seconds **P1** · security · `hearth/node` · effort S · user-visible **What is wrong** `eth_call` defaults its gas to the full 30,000,000 block limit and `eth_estimateGas` re-executes the message up to 33 times (one probe at the cap plus 32 bisection steps). The interpreter has no step counter, no deadline and no interruption point, and the JSON-RPC server has no rate limiting or concurrency cap. hearthd is a single Node process, so the same event loop serves p2p gossip, the miner and both HTTP servers — an RPC execution blocks all of them. geth caps this with `--rpc.gascap` and `--rpc.evmtimeout` for exactly this reason; neither exists here. **Evidence** - `repos/hearth/node/src/chain/rpcadapter.js:343` — `const cap = msg.gas === null || msg.gas === undefined ? this.chain.gasLimit : msg.gas;` - `repos/hearth/node/src/chain/rpcadapter.js:366` — `for (let i = 0; i < ESTIMATE_ITERATIONS && lo + 1n < hi; i++) {` - `repos/hearth/node/src/chain/rpcadapter.js:49` — `const ESTIMATE_ITERATIONS = 32;` - `repos/hearth/node/src/jsonrpc/server.js:163` — `async _serve(req, res) {` - `repos/hearth/node/src/evmnode.js:209` — `this.jsonrpcServer = http.createServer(this.jsonrpc.httpListener());` **How it fails** Measured: 30,000,000 gas of a tight `JUMPDEST/PUSH1 0/JUMP` loop takes 371 ms in this interpreter, and the repo's own blake2f suite reports 6.4 s for a 30M-gas block of blake2f. An anonymous caller posting `eth_estimateGas` with 30M gas of blake2f-heavy calldata blocks the node's event loop for tens of seconds per request — during which the node answers no RPC, relays no blocks and mines nothing. A handful of concurrent requests takes a node off the network without any invalid input, any funds, or any peer connection. **Suggested fix** Add an RPC gas cap (a `rpcGasCap` option, defaulting well below the block limit) applied to both `call` and `estimateGas`, plus a wall-clock deadline checked in the interpreter loop that returns `{ ok:false, error:'execution timeout' }`. Cheap addition: a per-IP in-flight limit on the JSON-RPC listener. **Done when** - [ ] Add an RPC gas cap (a `rpcGasCap` option, defaulting well below the block limit) applied to both `call` and `estimateGas`, plus a wall-clock deadline checked in the interpreter loop that returns `{ ok:false, error:'execution timeout' }`. - [ ] Cheap addition: a per-IP in-flight limit on the JSON-RPC listener. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: Keep the finding, but fix the reachability claim. The failure scenario's "an anonymous caller" is not true of the current deployment: JSON-RPC is bound to 127.0.0.1 (docker-compose.yml:462,488,510), the rpc.cloudsforge.online ingress is commented out (deploy/cloudflared/config.example.yml:98-103), and the public explorer's /rpc/ proxy targets the REST port 8645 (repos/hearth/web/Dockerfile:22), which 404s all JSON-RPC POSTs. Today the caller must be on the host or the compose network (forge-pay, forge-mint), or be honest load — every legitimate eth_estimateGas is up to 33 executions on the single loop that also mines and relays. It becomes an anonymous internet kill switch the moment the settled public RPC ingress is enabled, which quickstart.md:542 lists as the pending step — so this is a launch blocker rather than a live exploit. Also: the repo's own robustness-review.md:337 measures blake2f at 10.7 s per 30M gas, not 6.4 s; and jsonrpc/server.js:70-83 processes an unbounded batch array sequentially under a 5 MB body, which multiplies one request into hundreds of such executions. Finally, note in the fix that the time budget must apply only to the RPC path — the same interpreter validates blocks, and a deadline there would fork the chain. > Reviewer note: Three corrections, two of which strengthen the finding. (1) The estimateGas bisection is not needed at all: a single eth_call with to = 0x…0009 and 213 bytes of calldata setting rounds = 30,000,000 blocks the event loop for one uninterruptible precompile invocation. I measured 10,819 ms. (2) The "6.4 s for a 30M-gas block of blake2f" figure is wrong — the repo's own table at docs/robustness-review.md says 2.8 Mgas/s and 10.7 s, matching my measurement. (3) The 33x amplification claim needs a caveat: rpcadapter.js:361-362 returns early when the probe at the cap fails, so the full 33 executions only occur for a message that succeeds near the cap; a message that always runs out of gas costs one execution, not 33. On reachability, "anonymous caller" is not true of the deployed estate today: docker-compose.yml:461 publishes the JSON-RPC as 127.0.0.1:8545:8545 and deploy/cloudflared/config.example.yml:102-103 still has the rpc.cloudsforge.online ingress commented out. Today the attacker must be on the compose network or on the host. That comment says the rule is commented "until phase 5 lands the listener" — and the listener has landed — so this is a pre-publication window rather than a permanent mitigation, which is why P1 still holds: it must be fixed before 8545 is tunnelled, not after. --- ### CF-10 — Gate a full-block execution benchmark: StateDB still re-roots both tries on every mutation **P1** · broken · `hearth/node` · effort L · user-visible **What is wrong** `setStorage` writes the slot, immediately materialises the storage trie root, then writes the account — which re-roots the state trie too. The robustness review measured 443 MB retained and 65 s of CPU for a single 30,000,000-gas transaction. The chain now produces blocks (`evmnode.js` mounts the miner and the state transition), so this is no longer latent, and `npm test` contains no performance assertion at all — the suite passes while a legal block takes four times the block interval to validate. **Evidence** - `repos/hearth/node/src/state/statedb.js:367` — `a.storageRoot = st.root(); this._write(hex, a);` - `repos/hearth/docs/robustness-review.md:42` — `### 1. `StateDB` re-roots both tries on every single mutation: 1.66 KB permanently retained and 245 µs of CPU for 112 gas` - `repos/hearth/docs/testing.md:142` — `**Performance is not gated.** `docs/robustness-review.md` measured StateDB re-rooting the trie on every mutation: **443 MB retained and 65 seconds …` - `repos/hearth/node/src/params.js:295` — `EVM_BLOCK_GAS_LIMIT: 30_000_000,` **How it fails** A single transaction that fills the 30M block gas limit with SSTOREs is mined by any miner. Every other node spends ~65 s and ~443 MB validating that one block against a 15 s target, so blocks arrive faster than they can be validated, peers fall behind, orphan pools fill, and the LWMA retarget reacts to a chain nobody can keep up with. A stranger on the public testnet can produce this block for the price of the gas. **Suggested fix** Keep the tries dirty and hash once at the end of the transaction (Ethereum client behaviour), then add a CI benchmark that applies a gas-saturating SSTORE transaction and fails if wall-clock exceeds a fraction of TARGET_BLOCK_TIME or peak heap exceeds a fixed budget. This must be green before the testnet is public and re-measured at production PoW parameters before mainnet. **Done when** - [ ] Keep the tries dirty and hash once at the end of the transaction (Ethereum client behaviour), then add a CI benchmark that applies a gas-saturating SSTORE transaction and fails if wall-clock exceeds a fraction of TARGET_BLOCK_TIME or peak heap exceeds a fixed budget. - [ ] This must be green before the testnet is public and re-measured at production PoW parameters before mainnet. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: The facts are right but the title mis-states the fix. Gating a full-block execution benchmark is detection, not remediation: a failing benchmark would leave the DoS wide open. The actual fix, which robustness-review.md:119-125 already prescribes, is to defer hashing -- have setStorage/setBalance mark the account dirty and compute storageRoot and the state root once per transaction or per block, as finalize() already ends the transaction. The benchmark is the regression guard that comes after. Two details are also understated rather than overstated: the 443 MB is retained permanently because the node store is the append-only MemoryDB that blockchain.js:76 uses for all state (there is no pruning by design), and the attacker does not need the transaction to succeed -- a reverting out-of-gas transaction retains every node it wrote, because db.put is outside the journal. > Reviewer note: Two things are overstated. (1) "A stranger on the public testnet can produce this block" is not true today: every EVM port in docker-compose.yml:462-510 binds 127.0.0.1, the compose comment states none are routed through the tunnel, and deploy/cloudflared/config.example.yml:103 keeps rpc.cloudsforge.online -> :8545 commented out. The attacker must already be on the compose network (any faucet-funded service key) or on the host. (2) The framing should say the cost is per-mutation, not per-gas: ordinary traffic is unaffected (a 30M-gas block of ~1,400 plain transfers costs well under a second), so this is a crafted-transaction and storage-heavy-contract hazard, not a general throughput ceiling. Also, the title asks to gate a benchmark, but a test fixes nothing — the fix is deferring storageRoot/state-root computation to finalize(). Measured numbers on this machine are 36-52 s and 250-313 MB retained, not 65 s / 443 MB, but the conclusion is unchanged. --- ### CF-11 — Run and measure the production PoW parameters before the mainnet genesis — they have never executed **P1** · unexposed · `hearth/node` · effort L · user-visible **What is wrong** Every block ever produced — in tests, in the compose testnet, everywhere — used a 64 KiB scratchpad and a 256-step walk. The intended mainnet values are 2 GiB and 2,048+ steps, a 32,768× increase in per-attempt memory. Nothing has ever run at those values: not the node miner, not the browser miner (which reimplements Homefire in a Worker and is CI-checked only against the dev pad), not the block verification path that pays a full Homefire evaluation per received block. `COINBASE_MATURITY` is likewise 10 rather than ~100. All three are consensus, so changing them after launch is a fork. **Evidence** - `repos/hearth/node/src/params.js:125` — `POW_SCRATCH_KIB: IS_TEST_NETWORK ? 1 : 64, // (dev) production: ~2,097,152 (2 GiB)` - `repos/hearth/node/src/params.js:126` — `POW_WALK_STEPS: IS_TEST_NETWORK ? 8 : 256, // (dev) production: ~2048+` - `repos/hearth/node/src/params.js:186` — `COINBASE_MATURITY: 10, // (dev) coinbase unspendable until N deep; prod ~100` - `repos/hearth/docs/listing-checklist.md:253` — `| M1 | **Raise `POW_SCRATCH_KIB` from 64 to the production ~2 GiB** and `POW_WALK_STEPS` from 256 to 2,048+ ... A 64 KiB pad fits in L2 cache and i…` - `repos/hearth/TESTNET.md:47` — `A **production mainnet** would require swapping those dev PoW params for the hardened values noted alongside them in `params.js`, **plus** setting …` - `repos/hearth/docs/mining.md:1` — `# Mining` **How it fails** Mainnet genesis is cut with a 2 GiB pad. Each verification of a peer's block now allocates and chains 2 GiB of SHA-256 — the p2p verification budget (`P2P_BLOCK_VERIFY_BURST: 200`) was sized against a ~5 ms evaluation and is now sized against something orders of magnitude larger, so a single peer's getblocks page becomes a memory-exhaustion event. Separately, the browser miner — the coin's entire distribution thesis — cannot allocate 2 GiB in a Worker tab and mines nothing while appearing busy. **Suggested fix** Stand up a fourth network (`hearth-prepare`) at the intended production PoW values and run the full 3-node compose against it: measure per-attempt memory and time, block verification cost, the p2p verification-budget behaviour under a hostile peer, and whether the browser miner can run at all. Re-derive `P2P_BLOCK_VERIFY_BURST`/`PER_S` from the measured cost. Do this before the genesis is written, and record the measurements as the justification for the final constants. **Done when** - [ ] Stand up a fourth network (`hearth-prepare`) at the intended production PoW values and run the full 3-node compose against it: measure per-attempt memory and time, block verification cost, the p2p verification-budget behaviour under a hostile peer, and whether the browser miner can run at all. - [ ] Re-derive `P2P_BLOCK_VERIFY_BURST`/`PER_S` from the measured cost. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: Two corrections. (a) The failure mode is event-loop starvation, not memory exhaustion: the pad is allocated and released per `homefireHash` call, so RSS peaks around 2-4 GiB (survivable); it is the ~195 s of blocked single-threaded CPU per block — and ~10.8 h per 200-block page — that kills the node. (b) COINBASE_MATURITY is understated. On the account model, the chain actually being launched, there is NO maturity at all: `_creditReward` (blockchain.js:268-275) adds the subsidy straight to the balance, spendable in the next block. `COINBASE_MATURITY` is read only by the retired UTXO path (tx.js:141, wallet.js:92, rpc.js:216, chain.js:290), so listing-checklist M2 "raise 10 to ~100" is a no-op on the launch chain. And the framing shared by listing-checklist.md M1, roadmap.md, MAP.md:1106, TESTNET.md:43 and coinnomics.md:179 — that this is raising two constants, "free before launch" — is false. Because Homefire's per-attempt cost is O(pad) with no amortization, a 2 GiB pad cannot coexist with millisecond verification; reaching it requires an epoch-cached/amortized dataset, i.e. a redesign of node/src/pow.js plus web/assets/mining/homefire.js plus rust/hearthd/src/pow.rs, not an edit to params.js. The finding's real weight is not "an untested knob" but "the documented mainnet plan is arithmetically impossible and the checklist calls it a two-line edit." > Reviewer note: Two corrections. (1) The category "unexposed" is wrong — the dev-vs-production parameter gap is documented in six places (docs/listing-checklist.md:253 M1/M2, docs/roadmap.md:60, TESTNET.md:38-47, WHITEPAPER.md:159 §2.4, docs/coinnomics.md:178 §8, MAP.md:1105), and WHITEPAPER.md §2.4 already concedes it is "an argument about the construction, not a measured property of the running testnet." What is genuinely new is the measurement, and the measurement changes the conclusion: at ~0.098 ms/KiB the intended 2 GiB pad costs ~205 s per evaluation, so the production values recorded in the params.js comments are not merely unrun — they are unachievable with this construction, and M1 as written ("raise the constant") cannot be executed. (2) "a single peer's getblocks page becomes a memory-exhaustion event" is the wrong mechanism: pow.js:30 allocates one pad per call and releases it inside a synchronous loop, so peak RSS stays ~2 GiB rather than 200 x 2 GiB. The 200-block page fails on CPU — roughly 11 hours of one core — not on allocation. --- ### CF-12 — Run eth_call/eth_estimateGas against a scratch node store — each call leaks state permanently **P1** · security · `hearth/node` · effort M · user-visible **What is wrong** `RpcChain._run` executes a speculative `eth_call`/`eth_estimateGas` against a `StateDB` opened over `Blockchain.db` — the one process-wide `MemoryDB` that also holds consensus state. Every SSTORE, account touch and code deposit inside that speculative run calls `Trie._ref` → `this.db.put(...)`, which inserts nodes into that Map. `MemoryDB` has `get`/`put`/`has` and no delete, and nothing prunes it. So garbage produced by an unauthenticated, never-mined RPC call is retained for the life of the process, indistinguishable from real state. The JSON-RPC server is CORS `*` with no auth and no rate limit. **Evidence** - `repos/hearth/node/src/chain/rpcadapter.js:300` — `const state = this.chain.stateAt(entry.id);` - `repos/hearth/node/src/chain/blockchain.js:146` — `return new StateDB(this.db, hexBuf(e.stateRoot));` - `repos/hearth/node/src/state/trie.js:168` — `this.db.put(h, enc);` - `repos/hearth/node/src/state/trie.js:93` — `class MemoryDB {` - `repos/hearth/node/src/chain/blockchain.js:36` — `The cost is that nothing is ever pruned; spec §9 puts pruning out of` **How it fails** Against a live `hearthd --evm` (measured on this machine): POST `eth_call` with `data=0x60005b808055600101600256` (a tight SSTORE loop) and `gas=0x1c9c380`. One call adds 7,135 nodes / ~1.6 MB to `chain.db`; process RSS went 145 MB → 196 MB after one `eth_call` plus one `eth_estimateGas`, and → 346 MB after ten more requests, never returning. A few hundred requests OOM the node — including `hearth-testnet-seed`, which forge-pay's deposit watcher depends on for `service_healthy`. **Suggested fix** Give speculative execution its own store: wrap the chain's `MemoryDB` in an overlay (`get` falls through to the real db, `put` goes to a per-call Map that is dropped when `_run` returns). That also makes eth_call incapable of influencing consensus state at all, which is the property you actually want. **Done when** - [ ] Give speculative execution its own store: wrap the chain's `MemoryDB` in an overlay (`get` falls through to the real db, `put` goes to a per-call Map that is dropped when `_run` returns). - [ ] That also makes eth_call incapable of influencing consensus state at all, which is the property you actually want. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: Two corrections, one softening and one hardening. (1) The node store is content-addressed, so repeating the IDENTICAL eth_call adds zero new nodes — I measured three identical calls holding at the same 7,135 nodes / 1.90 MB. The stated scenario ("one eth_call plus one eth_estimateGas, then ten more requests -> 346 MB") only grows if the payload varies between requests; it does with a 2-byte change to the loop counter (10 varied calls -> 29,530 nodes / 9.77 MB retained, RSS 238 MB), so the attack stands but the recipe as written does not. (2) The finding badly understates the sharp edge: `RpcChain.call` (rpcadapter.js:343-344) computes `cap = msg.gas ?? this.chain.gasLimit` and never clamps it to `chain.gasLimit` — only `estimateGas` clamps, at rpcadapter.js:359. So per-request retention is not bounded by the block gas limit and "a few hundred requests" is far too many. Measured: one eth_call with `gas: 0x11e1a300` (300M) retains 83,173 nodes / 25.6 MB in 2.0 s; one eth_call with `gas: 2000000000` retains 615,613 nodes / 205 MB, drives RSS to 1.5 GB and blocks the single-threaded event loop for 15.5 s. A single unauthenticated request can therefore OOM the node, and the missing clamp is a second, independently fixable defect (an rpc gas cap) alongside the unpruned store. > Reviewer note: Two amendments. First, "each call leaks state permanently" is only true for DISTINCT calls: the node store is content-addressed, so re-sending the same eth_call adds nothing (measured delta 0 over 6 identical calls); the attacker varies one byte, and each distinct call then retains ~3.2k nodes / ~1 MB. The cited "145 MB -> 196 MB -> 346 MB" figures conflate RSS churn with retained store growth. Second, and more important, the amplifier is a missing gas cap, not just the shared store: `RpcChain.call` at rpcadapter.js:343 uses the caller's `gas` verbatim (`const cap = msg.gas ?? this.chain.gasLimit`) while `estimateGas` at line 359 clamps with `if (hi > this.chain.gasLimit) hi = this.chain.gasLimit`. Measured: gas=0x23c34600 (20x the block limit) blocked the single-threaded event loop for 4.6 s and retained 49 MB from ONE request. So the correct failure scenario is one request, not a few hundred, and the first thing to fall over is the event loop (p2p, mining, and the :8645/info healthcheck the compose file uses), not the heap. Exposure should read: not internet-facing today (docker-compose.yml:462 binds 127.0.0.1; the rpc.cloudsforge.online ingress in deploy/cloudflared/config.example.yml is commented; the public explorer proxies /rpc/ to the 8645 REST port, which has no eth_call), but reachable from any compose peer, any host process, and any browser page on the host via the wildcard CORS — and one uncommented line from the public internet by the deploy file's own stated plan. --- ### CF-13 — Point the explorer and browser wallet at the JSON-RPC port, and stop pinning it to localhost **P1** · broken · `hearth/web + stack/docker-compose` · effort S · user-visible · found independently 3 times **What is wrong** The explorer is now `eth_*`-native — every read goes through `rpc.call('eth_getBlockByNumber', …)`. Its endpoint resolution falls back to same-origin `/rpc/`, which nginx proxies to `HEARTH_RPC_UPSTREAM`, baked in the image as `hearth-testnet-seed:8645`. 8645 is the REST server, which answers any POST with HTTP 404 and `{err: 'this is the REST API — the Ethereum JSON-RPC endpoint is a different port'}`. Meanwhile `web/index.html` (the explorer root, per pages.yml) pins `http://localhost:8545`, which is the user's own machine, not the node. The same defect was reached independently from two other directions. Two independent defects stack. First, `resolveEndpoint` checks the `hearth-eth-rpc` meta tag *before* falling back to the same-origin `/rpc/` proxy, and `index.html` hardcodes that meta to `http://localhost:8545` — so the deployed explorer asks the visitor's own machine for the chain and the nginx proxy that exists to prevent exactly this (see its comment in `web/nginx.conf`) is dead code. Second, even with the meta removed, `/rpc/` proxies to `HEARTH_RPC_UPSTREAM`, defaulted to port **8645** — the REST server — while the explorer speaks `eth_*` JSON-RPC, which only 8545 answers. And from the compose side: The explorer and the non-custodial browser wallet both speak Ethereum JSON-RPC (`eth_getBalance`, `eth_sendRawTransaction`) and default their endpoint to same-origin `/rpc/`. nginx proxies `/rpc/` to `${HEARTH_RPC_UPSTREAM}`, whose only definition anywhere is the image default `hearth-testnet-seed:8645` — the REST port. The account-model node answers any unrecognised POST on 8645 with a 404 whose body literally says "this is the REST API — the Ethereum JSON-RPC endpoint is a different port". The `hearth-web` service in docker-compose.yml declares no `environment:` block at all, so nothing corrects it; the identical fix was already applied to Forge Pay (line 229 moved from 8645 to 8545) and the web container was left behind. **Evidence** - `repos/hearth/web/Dockerfile:22` — `ENV HEARTH_RPC_UPSTREAM=hearth-testnet-seed:8645` - `repos/hearth/web/nginx.conf:64` — `proxy_pass http://${HEARTH_RPC_UPSTREAM}/;` - `repos/hearth/web/assets/explorer/rpc.js:42` — `if (location.protocol === 'http:' || location.protocol === 'https:') return location.origin + '/rpc/';` - `repos/hearth/web/index.html:33` — `` - `repos/hearth/node/src/evmnode.js:380` — `err: 'this is the REST API — the Ethereum JSON-RPC endpoint is a different port',` - `repos/hearth/docker-compose.testnet.yml:54` — `- "8545:8545" # Ethereum JSON-RPC` - `repos/hearth/web/assets/explorer/rpc.js:41` — `if (m && m.content) return m.content.replace(/\/$/, '') || '/';` - `repos/hearth/docker-compose.yml:58` — `HEARTH_RPC_UPSTREAM: seed:8645` - `docker-compose.yml:669` — `build: repos/hearth/web` - `docker-compose.yml:229` — `HEARTH_TESTNET_RPC: http://hearth-testnet-seed:8545` **How it fails** Load explorer.cloudsforge.online (tunnel → :8080 → hearth-web). Every page fetches `http://localhost:8545` from the visitor's browser and gets a connection refusal, so the whole explorer shows `RpcUnreachable`. Removing the meta does not fix it: POSTing `{"method":"eth_blockNumber"}` to the REST port returns HTTP 404 with `{"err":"this is the REST API …"}` (verified against a running node), which `unwrap()` turns into `MalformedResponse: response carried neither result nor error`. The wallet path fails the same way: `docker compose up`, open http://localhost:8080/wallet.html (or explorer.cloudsforge.online in production). Every `eth_*` call POSTs to /rpc/ -> hearth-testnet-seed:8645 -> HTTP 404 with a non-JSON-RPC body. rpc.js raises RpcUnreachable/MalformedResponse, the page shows "NO NODE", and no balance, nonce, block or broadcast works. The chain is up and healthy the whole time. **Suggested fix** Delete the `hearth-eth-rpc` meta from `web/index.html` (or set it to `/rpc/`) so the same-origin proxy is used, and change `HEARTH_RPC_UPSTREAM` to `hearth-testnet-seed:8545` in `web/Dockerfile` and to the EVM node's 8545 in `repos/hearth/docker-compose.yml`. Add a smoke check that the deployed explorer's first `eth_chainId` returns a result. Also: Add `environment: HEARTH_RPC_UPSTREAM: hearth-testnet-seed:8545` to the `hearth-web` service in docker-compose.yml (and change the Dockerfile default to :8545), keeping a second location block on 8645 only if the explorer still needs REST/SSE paths such as /events. **Done when** - [ ] Delete the `hearth-eth-rpc` meta from `web/index.html` (or set it to `/rpc/`) so the same-origin proxy is used, and change `HEARTH_RPC_UPSTREAM` to `hearth-testnet-seed:8545` in `web/Dockerfile` and to the EVM node's 8545 in `repos/hearth/docker-compose.yml`. - [ ] Add a smoke check that the deployed explorer's first `eth_chainId` returns a result. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: Drop the index.html half entirely: the `` at web/index.html:33 is inside an HTML comment (lines 22-34), so querySelector never matches it and the explorer falls through to same-origin /rpc/ like every other page. The public explorer is broken solely because nginx proxies /rpc/ to the REST port. Also drop 'there is no configuration under which the deployed explorer reads the chain' — HEARTH_RPC_UPSTREAM is deliberately an envsubst template variable (documented in Dockerfile lines 15-21), so setting it to hearth-testnet-seed:8545 on the hearth-web service is a valid deploy-time fix, and ?rpc= still overrides per page. Finally, the title's remedy is incomplete: repointing /rpc/ at 8545 would break mine.html, which loads web/assets/api.js and fetches /info and EventSource('/events') through the same /rpc prefix — those are REST-only routes. The fix needs two proxy locations (REST/SSE on 8645, JSON-RPC on 8545) and a matching same-origin default in resolveEndpoint(). > Reviewer note: The core defect and its severity stand, but two details are wrong. (1) `web/index.html:33` is INSIDE an HTML comment — the block opens at line 22 (``), so `` is a commented-out example, not an active pin. `document.querySelector` never sees it. The public explorer therefore does NOT aim at the visitor's loopback; it falls through to same-origin `/rpc/` exactly like the compose case, and breaks for the same single reason (nginx → 8645). Drop the second half of the failure scenario; the conclusion ("there is no configuration under which the deployed explorer reads the chain") survives on the upstream-port bug alone. (2) The proposed fix as titled — repoint `/rpc/` at 8545 — would break `mine.html`: `assets/api.js` (`GET /rpc/info`, `EventSource /rpc/events`) and `assets/mining/miner.js` (`${rpc}/mining/template`, `${rpc}/mining/submit`) use the same `/rpc/` base for the REST protocol. The fix needs a second nginx location for the JSON-RPC port (and a matching default in `resolveEndpoint`), not a port flip on the existing one. --- ### CF-14 — Make the `trade` action require the target's consent and a non-empty offer **P1** · security · `ninety-days-after/game` · effort M · user-visible **What is wrong** A queued `trade` action is executed unilaterally by the tick: the only checks are that the actor holds their own `offer` and the target holds the `request`. The target never accepts anything, and the server places no lower bound on `offer` — `sanitizeBag({})` yields an all-zero bag, and `hasResources(actor.resources, {0,0,0,0,0,0})` is trivially true. The `offerTotal > 0` rule that makes this look like a two-sided swap lives only in the browser (TradeModal.tsx:28); the wire schema in shared-libs permits an empty record. Unlike `raid`, the trade path performs no defense check, costs no reputation, and never consults `isSpawnProtected` — it in fact awards the thief +2 reputation and +1 contribution (which feeds `survivalScore`). **Evidence** - `repos/ninety-days-after/services/game/src/engine/resolve.ts:429` — `if (hasResources(actor.resources, offer) && hasResources(target.resources, request)) {` - `repos/ninety-days-after/services/game/src/engine/resolve.ts:422` — `const offer = sanitizeBag(action.offer)` - `repos/ninety-days-after/services/game/src/util.ts:37` — `if (typeof v === 'number' && v > 0) bag[key] = Math.floor(v)` - `repos/ninety-days-after/apps/game/src/components/TradeModal.tsx:28` — `const valid = offerTotal > 0 && requestTotal > 0` - `repos/shared-libs/packages/shared/src/game.ts:127` — `offer: z.record(z.string(), z.number().int().min(0)),` - `repos/ninety-days-after/services/game/src/engine/resolve.ts:437` — `actor.reputation += 2 + b.tradeRep + (flags.caravanSeverity > 0 ? 1 : 0)` **How it fails** An authenticated player PUTs to /worlds/:id/actions with `{"actions":[{"type":"trade","targetPlayerId":"","offer":{},"request":{"food":12,"water":12,"materials":8,"fuel":2,"medicine":1,"seeds":5}}]}`. On the next tick the victim's entire starting bag transfers to the attacker for nothing. The victim is inside the 3-day spawn-protection window, so a `raid` against them would have been refused at resolve.ts:354 — the trade path has no such guard. With six AP the attacker empties six different survivors per day and gains +12 reputation and +6 contribution doing it, while the public report reads "traded goods with". **Suggested fix** Reject the trade at resolve time unless the offer is non-empty and the target has agreed. Minimum viable: tighten `actionSchema`'s trade branch in shared-libs to `z.record(z.enum(RESOURCE_KEYS), …)` with a refinement that the offer sums > 0, and add the same server-side check in resolve.ts before the transfer. Properly: make trade a two-phase proposal (a `trade_offers` row the target accepts with its own queued action), and apply `isSpawnProtected` to it as raid already does. **Done when** - [ ] Reject the trade at resolve time unless the offer is non-empty and the target has agreed. - [ ] Minimum viable: tighten `actionSchema`'s trade branch in shared-libs to `z.record(z.enum(RESOURCE_KEYS), …)` with a refinement that the offer sums > 0, and add the same server-side check in resolve.ts before the transfer. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: Two small imprecisions worth carrying into the fix, neither of which changes the verdict. (a) The victim is not purely a loser of reputation — `target.reputation += 2` at resolve.ts:438 also fires, so the theft silently credits both parties; the fix should reconsider both rep awards, not just the actor's. (b) The blast radius is slightly wider than "spawn-protected day-one settlers": because the trade branch has no defense check, no reputation cost, and no `isSpawnProtected` call, any living player at any age with any fortification level can be emptied — spawn protection is just the most galling case. Severity P1 is right: any authenticated player can drain up to six arbitrary victims per day with a two-line request body, and the public report frames it as a friendly trade, but the loss is in-game state only (no funds, keys, or PII), so it is not P0. --- ### CF-15 — Stop selling private worlds until something provisions them, or provision them **P1** · missing · `ninety-days-after/game` · effort L · user-visible **What is wrong** `POST /private-worlds/rent` debits 1800–2500 Shards (US$14.99–19.99 of on-chain-funded balance) and writes a `private_world` entitlement row whose `meta` carries the requested world name. Nothing anywhere reads entitlements of that kind: a grep for `private_world` across all nine repos hits only forge-pay's own write path, shared-libs' type, and the game client's shop. The only world-creation path in the estate is the admin-only `POST /admin/worlds`, and neither the game admin page nor the platform admin console surfaces pending rentals, so no operator can even see that a world was bought. **Evidence** - `repos/forge-pay/services/pay/src/routes/monetization.ts:99` — `// The game service later reads this to actually provision the world (out of scope here).` - `repos/shared-libs/packages/shared/src/pay.ts:169` — `{ id: 'private_saga', name: 'Private Saga World', priceShards: 2500, priceUsd: 19.99, seasonDays: 90, maxPlayers: 40, description: 'A private full-…` - `repos/ninety-days-after/apps/game/src/pages/Shop.tsx:183` — ``“${worldName}” is being raised — invite your friends.`,` - `repos/ninety-days-after/services/game/src/routes/admin.ts:54` — `app.post('/admin/worlds', ADMIN_RATE_LIMIT, async (req, reply) => {` **How it fails** A player converts a deposit into 2500 Shards, opens Shop → Private Worlds, types "Ashvale Refuge" and pays. The UI answers "“Ashvale Refuge” is being raised — invite your friends." The Shards are gone, a row exists in forge-pay's entitlements table that nothing polls, and /worlds never lists the world. There is no refund route and no operator view of the request. **Suggested fix** Either hide the Private Worlds section and the `/private-worlds` catalog until provisioning exists, or close the loop: give forge-pay an internal `GET /internal/entitlements?kind=private_world&unprovisioned=1`, have the game service claim and create the world from `meta.worldName`/`seasonDays`/`maxPlayers`, and mark the entitlement provisioned. Whichever is chosen, the success copy must stop promising a world that is being raised. **Done when** - [ ] Either hide the Private Worlds section and the `/private-worlds` catalog until provisioning exists, or close the loop: give forge-pay an internal `GET /internal/entitlements?kind=private_world&unprovisioned=1`, have the game service claim and create the world from `meta.worldName`/`seasonDays`/`maxPlayers`, and mark the entitlement provisioned. - [ ] Whichever is chosen, the success copy must stop promising a world that is being raised. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: The finding's "There is no refund route" should read: no *discoverable* refund path — POST /internal/credit exists as a service-token refund primitive, but no admin route or console lists entitlements, so no operator can know to call it. Also add that the rental is `ownOnce: false` (monetization.ts:105) with no cap or pending-rental guard, so the same player can be charged repeatedly for the same undelivered world, making the per-user loss unbounded. > Reviewer note: Two refinements. (1) The finding reads as newly discovered; it is in fact already tracked by the estate's own docs (ECOSYSTEM.md:313 as the top Phase 5 item, ninety-days-after/MAP.md:519, ROADMAP.md:849 — where the game repo files it as P2, below this finding's P1). The value here is confirming it is still live in the working tree, not novelty. (2) "There is no refund route" is right for any user- or operator-facing path, but a manual remediation lever technically exists: forge-pay's /internal/credit could hand the Shards back, and an admin could hand-create the world via POST /admin/worlds. Neither is wired to rentals and /internal/credit has no caller anywhere in the estate (ECOSYSTEM.md §7.6), so in practice the player's money is stranded — but the fix does not require new payment primitives, only a reader that joins private_world entitlements to world creation plus an operator view. --- ### CF-16 — Encrypt or externalise the RSA signing key: plaintext in a shared-superuser Postgres, and unrotatable **P1** · security · `platform/nimbus` · effort L **What is wrong** `signing_keys.private_jwk` holds the RS256 private key as plaintext JSONB. Every service in the estate connects to the same Postgres instance as the same role, `cloudsforge`, whose password defaults to the literal `cloudsforge` and lives in the `.env` that eight containers mount whole. That role owns every database, so anything that can reach postgres:5432 can `SELECT private_jwk FROM nimbus.signing_keys`. That key is the estate's universal forging credential: every service verifies `iss`+`aud:'cloudsforge'` and nothing else, so a self-minted `{sub, roles:['admin']}` token is accepted by pay, game, crucible, forge-mint and forge-keyvault alike. Contrast keyvault, which encrypts its custody keys at rest under `KEYVAULT_MASTER_SECRET` and refuses to boot on a placeholder — nimbus's key, which unlocks keyvault, has no equivalent. There is also no rotation path: no admin route, and `getJwks()` publishes exactly one key (`keys.ts:60`) while `getSigningKey()` caches for process lifetime, so replacing a leaked key means hand-editing the row plus a restart, which invalidates every live access token estate-wide at once. **Evidence** - `repos/platform/services/nimbus/src/db/schema.ts:71` — `privateJwk: jsonb('private_jwk').$type>().notNull(),` - `repos/platform/services/nimbus/src/keys.ts:38` — `const privateJwk = await exportJWK(privateKey)` - `docker-compose.yml:120` — `POSTGRES_USER: cloudsforge` - `docker-compose.yml:148` — `NIMBUS_DATABASE_URL: postgres://cloudsforge:${POSTGRES_PASSWORD:-cloudsforge}@postgres:5432/nimbus` - `repos/forge-keyvault/services/forge-keyvault/src/auth.ts:45` — `audience: 'cloudsforge',` - `repos/platform/services/nimbus/src/keys.ts:60` — `return { keys: [key.publicJwk] }` **How it fails** An RCE or SQLi in any one service (or a leaked/default POSTGRES_PASSWORD, or a stolen pg backup) yields the private JWK. The attacker signs `{iss: NIMBUS_ISSUER, aud: 'cloudsforge', sub: , roles: ['admin'], exp: +15m}` with the kid from the public JWKS, POSTs it to forge-keyvault's `/admin/keys/:address/reveal`, and walks away with every plaintext custody private key — no nimbus account, no password, no lockout, nothing in nimbus's audit log. Remediation requires manual DB surgery and signs out every user of every product. **Suggested fix** Wrap `private_jwk` in the same envelope keyvault uses (a `NIMBUS_KEY_SECRET`-derived AES-GCM key, refused when placeholder/short), and give each service its own least-privilege Postgres role instead of the shared `cloudsforge` superuser. Then make rotation possible: an `active`/`retired` flag with `getJwks()` publishing all non-retired public keys ordered deterministically, so a new key can be published one access-token TTL before it starts signing. **Done when** - [ ] Wrap `private_jwk` in the same envelope keyvault uses (a `NIMBUS_KEY_SECRET`-derived AES-GCM key, refused when placeholder/short), and give each service its own least-privilege Postgres role instead of the shared `cloudsforge` superuser. - [ ] Then make rotation possible: an `active`/`retired` flag with `getJwks()` publishing all non-retired public keys ordered deterministically, so a new key can be published one access-token TTL before it starts signing. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: Three details are wrong or overstated. (1) Six containers mount .env whole (nimbus, game, pay, forge-mint, crucible, forge-keyvault), not eight. (2) The reveal is not traceless: forge-keyvault/routes/admin.ts:66-77 writes a key_reveals row and a warn-level structured log before returning the key — attribution is forgeable (attacker picks `sub`) but the event is recorded; only nimbus's own log is silent. (3) "Replacing the key ... signs out every user of every product" is false. Access tokens are 15 minutes (tokens.ts:19-29) and refresh tokens are opaque SHA-256 DB rows, not signed (tokens.ts:40-48), so a key swap costs at most a 15-minute window of 401s that clients recover from via /auth/refresh. Rotation is a row replacement plus a restart, not an estate-wide logout. Also worth stating: an attacker with write-capable SQL access already gets admin by inserting a users row, so the incremental gain from the plaintext JWK is stealth and offline usability from a read-only artifact (stolen backup, SELECT-only injection) — which is what makes encrypting at rest under an env secret worthwhile, not a new capability ceiling. > Reviewer note: Three corrections to the failure scenario, none of which reduce severity. (1) The forge-keyvault reveal is not directly reachable: keyvault binds 127.0.0.1:4005 with no tunnel ingress rule, and nimbus's relay of it is off unless NIMBUS_VAULT_REVEAL_PROXY=true (repos/platform/services/nimbus/src/routes/vault.ts:123-135, src/env.ts:156), so that step needs host/compose-network access or an opt-in flag. Replace it with the paths that are public and ungated: forged user token -> POST pay /withdrawals (drains custodied BTC/ETH/SOL/XRP), and forged admin token -> POST nimbus /admin/users/:id/password-reset, which returns the reset URL in the response body. (2) "nothing in nimbus's audit log" is right, but a reveal that does land is audited by keyvault (key_reveals row plus a warn line, routes/admin.ts:60-75) — it just records the forged `sub`. (3) In the RCE-in-a-container vector specifically, the plaintext JWK is not the only universal credential: eight containers mount .env whole and that file also carries KEYVAULT_MASTER_SECRET and KEYVAULT_SERVICE_TOKEN. The JWK's distinct marginal harm is in the read-only vectors (backup theft, SQLi, a copied dump), where it converts DB-read into full write-level, silent, unrevocable impersonation across pay, game, crucible, forge-mint and keyvault. --- ### CF-17 — Stop logging the password-reset token: GET /reset?token=… is written verbatim to the log stream **P1** · security · `platform/nimbus` · effort S **What is wrong** The reset token is delivered as a query parameter on GET /reset, and nimbus's pino request serializer logs `url: req.url` — the full path *with* the query string. `disableRequestLogging` is not set, so Fastify emits an "incoming request" line for every hit. The redact list in obs.ts covers headers only (`req.headers.authorization`, `cookie`, `x-service-token`), never `req.url`, and `scrub()` only runs over error messages. So the one secret the code explicitly says must never be logged (`routes/admin.ts:205`) is logged by the framework on the very request that spends it. The design goes to real lengths elsewhere to protect it: only the SHA-256 is stored so "not even an operator with database access can recover an issued link" (schema.ts:55), and the page does `history.replaceState` to strip it from the address bar (portal.ts:874). **Evidence** - `repos/platform/services/nimbus/src/passwordReset.ts:76` — `return `${base}/reset?token=${encodeURIComponent(token)}`` - `repos/platform/services/nimbus/src/obs.ts:119` — `url: req.url,` - `repos/platform/services/nimbus/src/routes/portal.ts:829` — `app.get('/reset', (req, reply) => {` - `repos/platform/services/nimbus/src/routes/admin.ts:205` — `// The URL carries the token, so it is the one thing here that must never be` - `infra/lantern/src/docker.js:69` — `return request(`/v1.43/containers/${encodeURIComponent(id)}/logs?${params}`, { stream: true })` - `repos/platform/services/nimbus/src/passwordReset.ts:56` — `// Only the hash is stored, so an issued link cannot be recovered from here.` **How it fails** An admin issues a link from the console; the user opens https://account./reset?token=9f2c… . Nimbus writes `{"req":{"method":"GET","url":"/reset?token=9f2c…"}}` to stdout at info. Lantern's collector streams that container's logs into the `lantern` database and stores the raw line, where it is searchable from lantern. and included in every pg backup. The token is still unspent at that moment (it is redeemed by the later POST with the token in the body) and lives 30 minutes, so anyone with log or backup access — a lower-trust admin, a restored dump — can redeem it and take over that account, including another admin's. **Suggested fix** Either strip the query string in the `req` serializer (log `req.routeOptions?.url` plus a sanitised path, or redact `token`/`code` params), or stop carrying the token in the URL: render /reset with no parameter and have the page read it from the fragment (`/reset#token=…`), which browsers never send to the server. The fragment form also removes it from the Referer and from any reverse-proxy access log. **Done when** - [ ] Either strip the query string in the `req` serializer (log `req.routeOptions?.url` plus a sanitised path, or redact `token`/`code` params), or stop carrying the token in the URL: render /reset with no parameter and have the page read it from the fragment (`/reset#token=…`), which browsers never send to the server. - [ ] The fragment form also removes it from the Referer and from any reverse-proxy access log. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: Two refinements, neither of which weakens it. (1) The blast radius is slightly narrower than "anyone with log access": deliverPasswordReset (passwordReset.ts:178-192) delivers nothing today, so the only live issuance path is the admin console, and Lantern's reader is gated on the nimbus `admin` role or the static LANTERN_TOKEN (infra/lantern/src/auth.js). The realistic attacker is a break-glass LANTERN_TOKEN holder, a pg-backup holder, or anyone with `docker logs` on the host — not an anonymous one — and a full admin could already mint such a link themselves, so for that class the gain is an unaudited takeover rather than new capability. (2) The finding understates the storage: parse.js:59 lifts `o.req.url` into the `route` column, so the token is persisted and searchable in a dedicated indexed column, not only inside the 8000-char `raw` blob. The fix belongs in the canonical obs.ts serializer (strip the query string, e.g. log `req.url.split('?')[0]` or the routeOptions path plus an allowlisted query), since obs.ts is vendored into every service and the same serializer would leak any other query-borne credential. > Reviewer note: Severity is P2, not P1. Exploitation requires an actor who can read logs but is *not* already a nimbus admin (LANTERN_TOKEN break-glass holder, shared-postgres reader, Cloudflare edge logs), because any nimbus admin can already mint and redeem a reset link for any account via POST /admin/users/:id/password-reset, which returns the raw token in its body. The exposure window is the token's unspent life — typically seconds between the GET and the redeeming POST, capped at 30 minutes if abandoned — so the practical vector is racing Lantern's live tail, not a restored backup: a daily pg_dumpall can only ever capture expired or already-spent tokens. Also more precise than the finding: the token lands in three places in lantern's `events` table — the indexed `route` column, `fields.req.url`, and `raw` — under 7-day retention. --- ### CF-18 — Clamp Lantern's client-ingest statusCode and strip NUL bytes before they poison the batch insert **P1** · security · `stack/lantern` · effort S **What is wrong** `/ingest/client` is served at server.js:182, before `authorise()` at server.js:192, so it is reachable unauthenticated from the internet (cloudflared routes lantern. to it). The handler copies `item.statusCode` into the record after only a `Number.isFinite` check — `1e30` and `1.5` both pass — and copies `item.message` verbatim, including a JSON ``. `status_code` is an INTEGER column and Postgres TEXT cannot hold a NUL byte, so either value makes the whole batch INSERT throw. `flush()` catches, marks the DB unhealthy, and puts the identical batch back on the queue, so it throws again on the next tick and every tick after, until `pending` grows past `bufferSize` and the entire buffer — every service's logs, not just the poison — is discarded. **Evidence** - `infra/lantern/src/server.js:99` — `statusCode: Number.isFinite(item.statusCode) ? item.statusCode : null,` - `infra/lantern/src/server.js:182` — `if (path === '/ingest/client' && req.method === 'POST') return handleClientIngest(req, res)` - `infra/lantern/src/db.js:73` — `status_code INTEGER,` - `infra/lantern/src/store.js:114` — `if (pending.length < env.bufferSize) pending = rows.concat(pending)` - `infra/lantern/src/server.js:96` — `msg: String(item.message ?? '(no message)').slice(0, 2000),` **How it fails** `curl -X POST https://lantern./ingest/client -H 'content-type: text/plain' -d '{"app":"x","message":"hi","statusCode":1e30}'` once a second. Every `flush()` raises 22003, `dbHealthy()` latches false, and Lantern stops persisting a single log line from any of the nineteen containers for as long as the attacker keeps sending. Nothing in the estate has a durable record of anything that happens during that window. Each request may carry 50 items and the quota allows 120 requests/minute, so one host is far more than enough. **Suggested fix** In `handleClientIngest`, coerce statusCode with `Number.isInteger(n) && n >= 0 && n < 1000 ? n : null`, and run every string field through `.replace(//g, '')`. Separately, make `flush()` fault-tolerant: on a non-connection error (anything that is not a Postgres connection failure), drop the offending batch rather than requeueing it, so one malformed record can never wedge the writer. **Done when** - [ ] In `handleClientIngest`, coerce statusCode with `Number.isInteger(n) && n >= 0 && n < 1000 ? n : null`, and run every string field through `.replace(//g, '')`. - [ ] Separately, make `flush()` fault-tolerant: on a non-connection error (anything that is not a Postgres connection failure), drop the offending batch rather than requeueing it, so one malformed record can never wedge the writer. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: Two overstatements. (1) "the entire buffer - every service's logs - is discarded" is too strong: the in-memory ring and the SSE live tail keep working, and /api/events falls back to the buffer when dbHealthy() is false (server.js:230), so the operator still sees recent lines. What is actually lost is durable persistence and issue grouping for the duration of the attack. Also the eviction that finally drops the poison is usually the ingest-side cap at store.js:68 (bufferSize*4 = 20000, trimmed to 10000), not the store.js:114 guard. (2) The finding under-reports one effect: on a failed flush only `rows` are re-queued; the issueCounts groups are discarded and never retried (store.js:100-102), so grouped-issue counts are permanently lost on every poisoned tick. Separately, the defect is broader than client ingest: parse.js:74 uses `typeof statusCode === 'number'`, which likewise admits 1.5, so a container emitting a fractional statusCode in its JSON logs poisons the same batch with no attacker involved. A fix should clamp/round in both places and strip NUL bytes at ingest(). > Reviewer note: Three corrections to the mechanism, none of which reduce the severity. 1. The shedding path in the evidence is the wrong one. `flush()` sets `pending = []` before awaiting (store.js:99-102), so at store.js:114 `pending` holds only the handful of records that arrived during the insert — it is essentially never >= bufferSize, and the guard never fires. The batch that actually gets discarded is shed in `ingest()` at store.js:68-71: once `pending` reaches `bufferSize * 4` (20,000) it splices the oldest down to 10,000. That does discard every service's logs as claimed, and it incidentally sheds the poison row from the head, so a ONE-SHOT request self-heals — after 20,000 records have queued. On a quiet estate that is tens of minutes of nothing persisted from a single curl; a sustained attacker re-poisons every batch and it never recovers. 2. Unreported permanent loss: on failure `pending = rows.concat(pending)` requeues the events, but `issueCounts` was already reset to a new Map at store.js:102 and `groups` is dropped on the floor. Grouped issue counts for every failed flush are lost forever, even after Postgres recovers. That is data the `issues` table can never reconstruct. 3. The trigger surface is wider than `/ingest/client`. `parse.js:74` uses `typeof statusCode === 'number'`, which is looser than the client path — it accepts NaN and floats — and `statusCode` is read from `fields.statusCode`, i.e. any JSON key of that name in any container's log line (parse.js:60). There is no NUL stripping anywhere in `infra/lantern/src` (grep for /\x00 returns nothing), so any container that emits a NUL byte on stdout jams the same pipeline with no attacker involved. The fix therefore belongs in `db.js`/`store.js` sanitisation for both paths, not only in `handleClientIngest`, and `flush()` should not requeue a batch that failed with a 22xxx data error — otherwise one malformed line from a trusted service reproduces this without anyone attacking. --- ### CF-19 — Retry the final fee settlement of a stopped bot — nothing ever does, despite the log line **P2** · broken · `crucible/crucible` · effort M · user-visible **What is wrong** `settle()` has exactly two call sites: `settlementSweep`, which iterates `runningBots()` (`WHERE status = 'running'`), and the stop action. Once a bot is `stopped` it is out of the sweep, and the stop action short-circuits at line 276 for an already-stopped bot, so `resolvePending` can never run for it again. A settlement row left `pending` at stop time — the `inFlight` early return at fees.ts:218-225, or a crash between the charge and `updateSettlement` — is therefore orphaned permanently, and the arrears on `bot.feeOwed` are never collected or cleared. `GET /billing` sums `feeOwed` across every bot the user has ever had, so the figure it shows is wrong in both directions depending on whether the in-flight charge committed. **Evidence** - `repos/crucible/services/crucible/src/routes/bots.ts:305` — `request.log.error({ err, botId: bot.id }, 'final fee settlement failed — it will be retried')` - `repos/crucible/services/crucible/src/runner.ts:420` — `const entries = await runningBots()` - `repos/crucible/services/crucible/src/store.ts:280` — `const rows = await db.select().from(bots).where(eq(bots.status, 'running'))` - `repos/crucible/services/crucible/src/routes/bots.ts:276` — `if (bot.status === 'stopped') return bot` - `repos/crucible/services/crucible/src/store.ts:406` — `owed: rows.reduce((sum, b) => sum + b.feeOwed, 0),` **How it fails** A user stops a live bot with a 300-Shard fee due. `collect` gets 409 `idempotency_in_flight` (Pay is committing the debit), so the row stays `pending` and `bot.feeOwed` stays 300. Pay commits: the user's wallet is down 300. The bot is now `stopped`, so no sweep ever touches it and stop is a no-op. Forever after, `/billing` tells the user they owe 300 Shards that were already taken, `feePaid` understates by 300, and the settlement row reads `pending, collected: 0`. Only restarting that exact bot would ever reconcile it. **Suggested fix** Have `settlementSweep` also pick up bots that are not running but have unfinished business — `status <> 'running' AND (fee_owed > 0 OR EXISTS (pending settlement))` — and call `resolvePending` for them, or add a dedicated reconciliation pass over `fee_settlements WHERE status = 'pending'` keyed by bot. Until one exists, change routes/bots.ts:305 to say the settlement will NOT be retried, because that is the behaviour today. **Done when** - [ ] Have `settlementSweep` also pick up bots that are not running but have unfinished business — `status <> 'running' AND (fee_owed > 0 OR EXISTS (pending settlement))` — and call `resolvePending` for them, or add a dedicated reconciliation pass over `fee_settlements WHERE status = 'pending'` keyed by bot. - [ ] Until one exists, change routes/bots.ts:305 to say the settlement will NOT be retried, because that is the behaviour today. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: Two details are imprecise. (1) The log line at routes/bots.ts:305 is inside `.catch()`, so it only fires when `settle` *throws*; the headline in-flight case does not throw at all — `settle` returns `{ settlement, deferred }` normally and the stop path logs nothing. The stale "it will be retried" promise is real but it is not the path that produces the orphan in the stated scenario, and the orphan also has a third source the finding omits: `resolvePending`'s own unresolved branch (fees.ts:164-167) when Pay is merely unreachable. (2) "/billing is wrong in both directions" overstates it: only when the in-flight charge committed is the figure actually wrong (owed overstates by the fee, paid understates by it). If the charge did not commit, `feeOwed` is accurate — the defect there is that the debt is simply never collected again, not that the number lies. > Reviewer note: Broaden the scope and add the tail: it is not only a `pending` row that is orphaned — ANY residual `bot.feeOwed` at stop time is permanent, including the ordinary short-wallet case where `collect` cannot cover the fee and `settle` writes 'uncollectable' with feeOwed = due - collected (fees.ts:227-231). That path is far more likely than a crash window, and it is the one the UI lies about: Billing.tsx:45-56 tells the user the outstanding amount "will be collected on the next settlement pass" and to top up their wallet, which for a stopped bot can never happen. Also correct the mechanism in the scenario: Pay claims the idempotency key inside the same transaction as the debit (forge-pay store.ts:157-193), so a 409 idempotency_in_flight generally means the original rolled back rather than committed; the realistic "wallet debited, row still pending" window is a Crucible crash between the committed charge and `updateSettlement`. Finally, add the sharpest consequence: because Pay reaps idempotency keys after 30 days on the stated assumption that "nothing in this estate retries for thirty days", an orphaned pending row — whose retry horizon is unbounded, firing only if the user ever restarts that exact bot — can be replayed after the key is gone and debit the user a SECOND time for the same settlement. --- ### CF-20 — Fix README: deposit addresses are signable now, and the Ember chain-id gap is closed **P2** · inaccurate · `forge-keyvault/forge-keyvault` · effort S **What is wrong** The README's "What it will and won't sign" section is pre-sweep. It states twice that a deposit-purpose address can never be signed for and that /sign answers 403 `purpose_forbidden` — the code admits `deposit` to SIGNABLE_PURPOSES and gives it the `sweep` shape. It also carries a "Known gap" block claiming Hearth uses one chain id on both networks so a testnet Ember transaction is byte-valid on mainnet; that was fixed (7411/7412), and the same README contradicts itself about it 15 lines earlier. Neither the treasury pin nor `SWEEPABLE_FAMILIES` is mentioned anywhere. `git log -- README.md` shows the sweep commit 52dbe0a never touched it. **Evidence** - `repos/forge-keyvault/README.md:16` — `**The row.** Deposit-purpose addresses are **never** signable.` - `repos/forge-keyvault/README.md:26` — `| `deposit` | nothing, ever. 403 `purpose_forbidden`, before anything is decrypted |` - `repos/forge-keyvault/services/forge-keyvault/src/routes/vault.ts:56` — `const SIGNABLE_PURPOSES: ReadonlySet = new Set(['deployer', 'treasury', 'deposit'])` - `repos/forge-keyvault/README.md:83` — `> **Known gap, and not this service's to close.** Hearth declares one chain id > and uses it on both networks, so a testnet Ember transaction is by…` - `repos/forge-keyvault/services/forge-keyvault/src/chains.ts:52` — `export const EMBER_CHAIN_IDS = { mainnet: 7411, testnet: 7412 } as const` - `repos/forge-keyvault/README.md:68` — `EIP-155 transactions bound to chain id **7411** on mainnet and **7412** on testnet.` **How it fails** An operator or a reviewer reads the README to decide the blast radius of the shared service token and concludes that customer deposit keys are unreachable through /sign. They are not: a token holder can move every customer deposit into the treasury and then out of it. The same reader takes the "Known gap" paragraph as current and either re-does the per-network chain-id work or, worse, avoids using Ember testnet on the belief that its signatures are mainnet-valid, which stopped being true at commit 54960d3. **Suggested fix** Rewrite the row/purpose table to the three real shapes (deployer→creation, treasury→transfer, deposit→sweep-to-pin), document `SWEEPABLE_FAMILIES` and the admin-only treasury pin as the property the sweep rests on, and delete the "Known gap" block outright. **Done when** - [ ] Rewrite the row/purpose table to the three real shapes (deployer→creation, treasury→transfer, deposit→sweep-to-pin), document `SWEEPABLE_FAMILIES` and the admin-only treasury pin as the property the sweep rests on, and delete the "Known gap" block outright. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. - [ ] Every document asserting the old behaviour is corrected in the same change. > Reviewer note: Two claims need tightening. First, "Neither the treasury pin nor SWEEPABLE_FAMILIES is mentioned anywhere" is true of the README but false of the repo: MAP.md documents both at length (the per-family sweepability table at line 218, the SWEEPABLE_FAMILIES rationale at 244-251, and the pin in section 4.1a), including the sweep-then-drain chain at line 366. The defect is that README and MAP.md disagree, not that the change is undocumented. Second, the failure scenario's "a token holder can move every customer deposit into the treasury" overstates what a bare stolen service token buys: /sign still requires the caller to restate orderId via bindingMatches, and GET /addresses/:address was narrowed specifically to stop serving it, so the drain needs a compromised peer service (forge-pay holds the bindings) or database read, not the token alone. The README sentence is still false as written. > Reviewer note: Two line-level imprecisions: the "Known gap" block begins at README.md:82, not :83, and the README contradicts itself about the Ember chain ids in two places (line 40 in the family/binding table and line 68 in the Ember keys section), not only once 15 lines earlier. Also worth noting the sweep is documented correctly in the sibling MAP.md (lines 218, 226, 244, 261), so the defect is README-only staleness rather than an estate-wide documentation gap. --- ### CF-21 — Let a reverted or dropped deploy be retried: a set `txHash` bricks the order forever **P2** · broken · `forge-mint/forge-mint` · effort M · user-visible **What is wrong** `onBroadcast` writes `txHash` the instant a transaction is on the wire, and `/deploy` refuses any order that has a `txHash` and no `contractAddress` (tokens.ts:214). The only writer that can clear that condition is `settleEvmDeploy`, and it clears it *only on success* — a revert returns `{status:'failed'}` with no `contractAddress`, and a transaction that was dropped from the mempool returns `null` forever. Nothing in the service ever sets `txHash` back to null; `claimDeploy` additionally requires `isNull(tokenOrders.txHash)`. So `'failed'` being listed in `DEPLOYABLE` is meaningless for exactly the failures that matter, and there is no admin route in this service to unstick it. MAP.md:197-201 claims the status settler is "the exit from the deploy_in_flight 409"; for a revert or a dropped transaction it is not. **Evidence** - `repos/forge-mint/services/forge-mint/src/routes/tokens.ts:214` — `if (order.txHash && !order.contractAddress) {` - `repos/forge-mint/services/forge-mint/src/chain/evm.ts:40` — `if (receipt.status !== 1 || !receipt.contractAddress) return { status: 'failed' }` - `repos/forge-mint/services/forge-mint/src/chain/evm.ts:38` — `const receipt = await provider.getTransactionReceipt(txHash)` - `repos/forge-mint/services/forge-mint/src/store.ts:112` — `isNull(tokenOrders.txHash),` - `repos/forge-mint/MAP.md:199` — ``deploy_in_flight` 409 — without it, an order whose transaction mined after the` **How it fails** A customer deploys on Ethereum mainnet during a fee spike. `getFeeData()` returns a `maxFeePerGas` that is stale by the time the tx is submitted; the transaction sits in the mempool, is evicted, and never mines. `sent.wait(1, 180_000)` throws, the order becomes `failed` with `txHash` set. `GET /tokens/:id/status` calls `getTransactionReceipt` forever and gets `null`, so the order never advances. `POST /tokens/:id/deploy` answers 409 `deploy_in_flight` on every retry. The customer has spent Shards and sent real ETH to the deployer, holds no token, and has no route — user or operator — to broadcast a replacement. **Suggested fix** Make `settleEvmDeploy` return a resolution for the two dead cases and act on it: on `receipt.status !== 1` clear `txHash` (recording it as `lastFailedTxHash`) and set `failed`, so a retry can re-broadcast at a fresh nonce; and when `getTransactionReceipt` returns `null` past a bounded age (e.g. `deployStartedAt` older than the lease) and `getTransactionCount(deployer,'latest')` shows the nonce unused, clear `txHash` the same way. Correct MAP.md:197-201 to state which outcomes the settler actually exits. **Done when** - [ ] Make `settleEvmDeploy` return a resolution for the two dead cases and act on it: on `receipt.status !== 1` clear `txHash` (recording it as `lastFailedTxHash`) and set `failed`, so a retry can re-broadcast at a fresh nonce; and when `getTransactionReceipt` returns `null` past a bounded age (e.g. - [ ] `deployStartedAt` older than the lease) and `getTransactionCount(deployer,'latest')` shows the nonce unused, clear `txHash` the same way. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: The bricking is real and permanent, but the money-losing form described (Ethereum mainnet, real ETH) is not reachable in any configuration present in the repo: `FORGE_MINT_MAINNET_ENABLED` defaults to false and is set nowhere. The reachable harm is on testnet — the customer's spent Shards (real, crypto-funded value) and a permanently 409'd order requiring a second full-price order to work around. Also, while there is no admin API route in the service, an operator can unstick a row with a single `UPDATE token_orders SET tx_hash = NULL`, so the state is recoverable by hand rather than unrecoverable. > Reviewer note: The mechanism stands, but three details are overstated. (1) The failure scenario's "Ethereum mainnet, real ETH" framing needs an operator-enabled deployment: `mainnet.ts` requires `FORGE_MINT_MAINNET_ENABLED` plus allowlist membership or the `admin` role, and is closed by default — the reachable-today version of this bug bricks a testnet order and the Shards paid for it. (2) The revert half is largely pre-empted: `evm.ts:88-113` now refuses to broadcast when `estimateGas` throws, so a reverting constructor never reaches the wire; the live case is the dropped/never-mined transaction, where `getTransactionReceipt` returns `null` forever. (3) "no route — user or operator" is true of the API only; a single SQL `UPDATE token_orders SET tx_hash = NULL` unsticks it, which is what the comment at tokens.ts:352-354 ("this needs a human, not a retry") is pointing at. Also, `'failed'` in `DEPLOYABLE` is not "meaningless" — it still readmits every pre-broadcast failure (gas-estimate refusal, `VaultRefused`, RPC outage), which is most of them; it is meaningless only once `txHash` is set. --- ### CF-22 — Expose deposit addresses and their ids — the only remedy for a frozen address is keyed by an id nothing shows **P2** · unexposed · `forge-pay/pay` · effort L · found independently 2 times **What is wrong** When an address's confirmed balance drops below its high-water mark without a recorded sweep, the watcher freezes crediting for that address and logs the exact remedy: `POST /admin/deposit-addresses/:id/sweeps`. That route is depth-gated, idempotent and correct — but nothing exposes it. There is no Nimbus relay, no console screen, and, decisively, no admin route anywhere that lists deposit addresses: `GET /deposits` is `requireAuth` and scoped to the calling user. The `:id` the remedy needs exists only in Postgres. Found independently a second time: The manual-sweep route is the only remedy for a frozen deposit address and, with `PAY_SWEEP_ENABLED` off by default, the only way the treasury ever gets funded. It is keyed by `deposit_addresses.id`, a random UUID. There is no `GET /admin/deposit-addresses` in forge-pay, no relay for one in nimbus's pay proxy, and no admin-console page. The alarm that instructs the operator to use the route logs `coin` and `address` but not `row.id`, so the operator is told to call an endpoint whose only parameter they have no way to obtain except by querying Postgres directly. `POST /admin/withdrawals/:id/abandon` is in the same position — reachable only by hand-crafted request. **Evidence** - `repos/forge-pay/services/pay/src/watcher.ts:67` — `'accounted for with POST /admin/deposit-addresses/:id/sweeps',` - `repos/forge-pay/services/pay/src/routes/admin.ts:221` — `app.post<{ Params: { id: string } }>('/admin/deposit-addresses/:id/sweeps', async (request, reply) => {` - `repos/forge-pay/services/pay/src/routes/deposits.ts:90` — `app.get('/deposits', { preHandler: requireAuth }, async (request) => {` - `repos/platform/services/nimbus/src/routes/pay.ts:41` — `* withdrawal or records a sweep. Those move or unmove real money and have no UI` - `repos/forge-pay/services/pay/src/watcher.ts:64` — `{ coin: row.coin, address: row.address, missing: scan.missing },` - `repos/platform/services/nimbus/src/routes/pay.ts:40` — `Deliberately narrow: the price write, and three reads. Nothing here abandons a` **How it fails** An operator moves EMBER out of a customer deposit address by hand. The next watcher tick logs `regression` and stops crediting that address permanently. The user deposits again and is never credited. To clear it the operator must SSH to the host, run a SQL query against `deposit_addresses` to find the row's uuid, and craft an authenticated POST by hand — there is no route, in any service, that will tell them that id. **Suggested fix** Add `GET /admin/deposit-addresses` to forge-pay (address, coin, network, userId, swept, high-water mark, and the last scan outcome including `regression`/`settling`), relay it plus the existing sweeps POST through Nimbus, and add a Deposits panel to the console that lists frozen addresses and offers the record-a-sweep form. Also: Add `GET /admin/deposit-addresses` (filterable by coin/address, returning id, user, address, last_seen, swept, pending and any unfinished sweep), relay it plus the sweep-record and abandon POSTs through nimbus's pay proxy, and include `addressId: row.id` in the `regression`, `settling`-stalled and `duplicate` watcher log lines. **Done when** - [ ] Add `GET /admin/deposit-addresses` to forge-pay (address, coin, network, userId, swept, high-water mark, and the last scan outcome including `regression`/`settling`), relay it plus the existing sweeps POST through Nimbus, and add a Deposits panel to the console that lists frozen addresses and offers the record-a-sweep form. - [ ] Also: Add `GET /admin/deposit-addresses` (filterable by coin/address, returning id, user, address, last_seen, swept, pending and any unfinished sweep), relay it plus the sweep-record and abandon POSTs through nimbus's pay proxy, and include `addressId: row.id` in the `regression`, `settling`-stalled and `duplicate` watcher log lines. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: Two corrections. (1) The failure scenario understates reachability: it blames an operator moving EMBER by hand, but the common path needs no human — the service's own sweeper marks a sweep `stuck` when its ~60-minute deadline beats EMBER's 61-block maturity (sweeper.ts:453), `stuck` leaves both UNMATURED_SWEEP_STATUSES and UNFINISHED_SWEEP_STATUSES, so the address reports `regression` and no sweeper tick will ever retry it. (2) It overstates the remedy's inaccessibility: the operator does not need to "craft" anything exotic — the id is one indexed SQL lookup on the address that the regression log already prints, and the cheapest fix is adding `addressId: row.id` to the log line at watcher.ts:63 where the value is already in scope, not building an admin listing route. > Reviewer note: Two claims are overstated. (1) "The :id exists only in Postgres" is wrong: GET /deposits returns it (toDepositAddress, store.ts:740 emits `id`), just scoped to the address's owner — the accurate statement is that no route exposes it to an operator for someone else's address. (2) "stops crediting that address permanently … never credited" is too strong: crediting resumes on its own once the confirmed balance climbs back above the high-water mark, so a later deposit larger than the moved amount credits the excess; the loss is the moved amount, not every future deposit. Also worth noting the cheapest fix is not a new route at all — the watcher already holds row.id (watcher.ts:56) and simply omits it from the regression/settling log lines that state the remedy; adding `addressId: row.id` there puts the id in the exact log the operator is reading. --- ### CF-23 — Give the deposit sweeper and withdrawal worker a Beacon signal — today they alert nowhere **P2** · missing · `forge-pay/pay` · effort L **What is wrong** The sweeper and the withdrawal worker are the only two loops in the estate that move real chain funds, and neither has an observability path that reaches a human. Beacon's pay probes cover `/health`, `/coins/rates` and `/deposit-coins`; its four pay journeys cover wallet reads, the Shard ledger, the convert round trip and the storefront. None touches a sweep, a treasury balance or a withdrawal. The only output of a stuck sweep is a pino line that Lantern groups into an issue — and Lantern has no notification path of any kind (no notify module, no webhook config, nothing in `infra/lantern/src`). Beacon's single webhook fires on incidents only, and `BEACON_WEBHOOK_URL` is blank in .env.example and in compose, so on a stock deployment there is no outbound alert channel at all. **Evidence** - `repos/forge-pay/services/pay/src/sweeper.ts:480` — `log.error({ sweepId: row.id, coin, txid: row.txid }, 'sweep stuck: never seen on chain')` - `repos/forge-pay/services/pay/src/withdrawer.ts:213` — `log.error({ withdrawalId: row.id, coin, txid: row.txid }, 'withdrawal stuck: never seen on chain')` - `infra/beacon/src/targets.js:171` — `name: 'pay.catalog',` - `infra/beacon/src/journeys/pay.js:27` — `covers: ['pay', 'forge-keyvault', 'nimbus', 'postgres'],` - `infra/beacon/src/notify.js:18` — `if (!env.webhookUrl) return false` - `.env.example:227` — `BEACON_WEBHOOK_URL=` **How it fails** forge-keyvault starts refusing to sign EMBER sweeps (a chain-id binding mismatch, say). Every sweep hits `log.error(... 'sweep refused before signing')` on each poll for hours. Every probe stays green, every journey stays green, `beacon_target_up` is 1 everywhere, and no webhook fires. User deposits keep landing on unswept addresses and nobody learns about it until somebody opens Lantern's issues tab for an unrelated reason. **Suggested fix** Two steps, cheapest first. (1) Add a Beacon probe against an existing pay operator read (e.g. `GET /admin/treasury` behind the service token) that fails when any sweep row has been in `stuck`/`failed` longer than N minutes, or when a treasury's spendable balance has fallen more than X% between reads — the mitigation DEPOSIT-SWEEP.md §9 already records as needed and out of scope. (2) Ship `BEACON_WEBHOOK_URL` set in the deploy path, and either give Lantern a webhook on new `fatal` issues or make Beacon poll Lantern's `/api/issues` so a `log.error` from the sweeper has some route to a person. **Done when** - [ ] Two steps, cheapest first. (1) Add a Beacon probe against an existing pay operator read (e.g. - [ ] `GET /admin/treasury` behind the service token) that fails when any sweep row has been in `stuck`/`failed` longer than N minutes, or when a treasury's spendable balance has fallen more than X% between reads — the mitigation DEPOSIT-SWEEP.md §9 already records as needed and out of scope. (2) Ship `BEACON_WEBHOOK_URL` set in the deploy path, and either give Lantern a webhook on new `fatal` issues or make Beacon poll Lantern's `/api/issues` so a `log.error` from the sweeper has some route to a person. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: Reframe around the always-on loop and drop the deposit-credit claim. The withdrawal worker runs by default (`env.ts:79`); the sweeper is opt-in (`PAY_SWEEP_ENABLED` defaults false). The sharpest statement of the defect is that `markWithdrawalStuck`/`markSweepStuck` are, by the workers' own comments, "a request for a human", and a signed-and-broadcast stuck withdrawal has no automatic exit — only `POST /admin/withdrawals/:id/abandon` — yet no Beacon probe, no Beacon journey and no Lantern notification path exists to tell a human it happened. A stuck sweep does NOT cost users their deposit credit (the watcher credits independently of sweeps); its cost is treasury starvation, which shows up as queued withdrawals being refunded after 60 minutes. Note also that the operator does have pull surfaces (`GET /admin/withdrawals?status=stuck`, `GET /admin/treasury`) and the user can see their own stuck row, so the current detection mechanism is a support ticket rather than nothing at all. Drop `BEACON_WEBHOOK_URL=` being blank in `.env.example` from the evidence — a per-deployment webhook URL cannot ship in an example file; the point is that no probe watches these loops, so a configured webhook would still be silent. > Reviewer note: Three claims need softening. (a) The failure scenario's "Every sweep hits log.error(...) on each poll for hours" is wrong: ChainBlocks (sweeper.ts:170-205) blocks the whole chain on first refusal and shouldLog throttles to at most once per hour per chain ("a persistent refusal is said and not shouted"), so the real signal is far quieter than described — which strengthens rather than weakens the finding. (b) PAY_SWEEP_ENABLED defaults to false (env.ts:111, documented at .env.example:77), so a stock deployment opens no sweeps at all; the default-on money mover is the withdrawal worker (env.ts:79), and that is where the coverage gap actually bites out of the box. (c) It is not true that there is no observability path — GET /admin/treasury, GET /admin/withdrawals?status=stuck and POST /admin/withdrawals/:id/abandon are deliberate, documented operator surfaces (admin.ts:157, 319, 335). The defect is that nothing pushes: no probe, no journey, no incident, so a human must already suspect a problem before those surfaces help. Also, BEACON_WEBHOOK_URL being blank in .env.example is a per-deployment secret placeholder, not itself a defect. --- ### CF-24 — Mark a build-failing `pending` withdrawal stuck — today it retries forever and blocks the chain **P2** · broken · `forge-pay/pay` · effort S · user-visible **What is wrong** `stuckDeadlinePassed` is consulted for a `pending` withdrawal only inside the `InsufficientTreasuryError` arm of `startWithdrawal`. Every other build failure falls through to the generic `will retry` line, and `advanceWithdrawal` (which owns the other stuck-marking paths) is only called for rows whose status is not `pending`. So a `pending` withdrawal that can never be built is never refunded, never marked `stuck`, and therefore never appears in `GET /admin/withdrawals?status=stuck` — the queue the admin surface describes as the one waiting on a human. Because the chain loop only ever starts the oldest `pending` row per tick, that row also blocks every other queued withdrawal on the same chain. **Evidence** - `repos/forge-pay/services/pay/src/withdrawer.ts:94` — `if (stuckDeadlinePassed(row)) {` - `repos/forge-pay/services/pay/src/withdrawer.ts:122` — `log.error({ err, coin, network, withdrawalId: row.id }, 'withdrawal build failed — will retry')` - `repos/forge-pay/services/pay/src/withdrawer.ts:246` — `if (row.status !== 'pending') await advanceWithdrawal(row, log)` - `repos/forge-pay/services/pay/src/withdrawer.ts:250` — `const next = group.find((r) => r.status === 'pending')` - `repos/forge-pay/services/pay/src/routes/admin.ts:318` — `one an operator actually wants: it is the queue of payments waiting on a human.` **How it fails** A withdrawal row's locked `fee` is not a whole multiple of `TRANSFER_GAS` (a corrupted row, or one written before the constant existed): `buildAndSignEvm` throws a plain `Error`, which is not `InsufficientTreasuryError`, `UnsupportedDestinationError`, `SignRefusedError` or `UnsupportedChainError`. Every 20 seconds the worker logs `withdrawal build failed — will retry` and moves on. The user's coin balance stays debited indefinitely with status `pending`, the operator's stuck queue is empty, and because `group.find(r => r.status === 'pending')` always returns this same oldest row, no other EMBER withdrawal on the platform is ever started. **Suggested fix** Apply `stuckDeadlinePassed` to every non-classified build failure in `startWithdrawal` — mark the row `stuck` (nothing was signed, so refunding is also safe) — and skip past a `pending` row that has already exceeded the deadline so it cannot head-of-line-block the rest of the chain's queue. **Done when** - [ ] Apply `stuckDeadlinePassed` to every non-classified build failure in `startWithdrawal` — mark the row `stuck` (nothing was signed, so refunding is also safe) — and skip past a `pending` row that has already exceeded the deadline so it cannot head-of-line-block the rest of the chain's queue. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: The defect is real but should be restated as: the generic build-failure arm of `startWithdrawal` has no bounded retry, unlike the `InsufficientTreasuryError` arm directly above it, so a `pending` withdrawal whose build keeps throwing an untyped `Error` retries forever and head-of-line blocks its chain. Two corrections to the write-up: (1) the cited trigger (a `fee` that is not a multiple of `TRANSFER_GAS`) is unreachable in practice — `estimateFee` locks `gasPriceBid(quoted) * TRANSFER_GAS` and the legacy ember1 rows were retired; the realistic trigger is an XRP treasury account that does not yet exist (`xrp account_info` error at outbound.ts:441) or a persistent `ensureTreasury`/keyvault failure. (2) "never marked `stuck`" describes the wrong fix: `markWithdrawalStuck` is deliberately restricted to `signed`/`broadcast` rows, because `stuck` means "a signed payment needs a human". A `pending` row has signed nothing, so the missing behaviour is a `stuckDeadlinePassed` → `refundWithdrawal(..., ['pending'])` in the generic arm, matching :94. Also note the row is not invisible to operators: it shows under `?status=pending`, logs an error every tick, and can be cleared with the existing abandon route. > Reviewer note: Two corrections. (1) The remedy in the title is wrong as written: `markWithdrawalStuck` (store.ts:1606) is gated on `inArray(status, ['signed','broadcast'])`, so calling it for a `pending` row is a silent no-op and the chain stays blocked. The fix consistent with this file's own state machine is a deadline REFUND from `['pending']`, exactly as the InsufficientTreasuryError, UnsupportedDestinationError and SignRefusedError arms already do — safe because nothing has been signed. (2) "Never appears in the queue waiting on a human" overstates the operator's position: the row is visible in unfiltered `GET /admin/withdrawals` and is clearable via `POST /admin/withdrawals/:id/abandon`, whose `from` list includes 'pending'. What is missing is the signal in the queue designed to carry it, not the tool to fix it. Also worth adding to the failure scenario: the most likely real trigger is not a corrupted fee but a treasury address the EVM node cannot parse (the documented legacy `ember1…` row), which fails on eth_getBalance before the SignRefusedError that would have refunded it. --- ### CF-25 — Stop minting deposit addresses on a network the deployment does not run **P2** · broken · `forge-pay/pay` · effort S · user-visible **What is wrong** `POST /deposits` takes `network` from the request body and never compares it to `PAY_DEPOSIT_NETWORK`. The shared schema defaults it to the literal string `'testnet'`, not to the server's configured network, and the product's only client sends `{ coin }` with no network at all. Meanwhile `walletSnapshot`/`getCoinBalances` only ever read balances for `env.depositNetwork`, and every conversion and withdrawal route defaults to `env.depositNetwork`. So an address minted on the other network is watched against the other network's RPC and credits into a balance the wallet can never display. **Evidence** - `repos/shared-libs/packages/shared/src/deposits.ts:225` — `network: z.enum(['testnet', 'mainnet']).default('testnet'),` - `repos/forge-pay/services/pay/src/routes/deposits.ts:40` — `const { coin, network } = parsed.data` - `repos/ninety-days-after/apps/game/src/components/funding.tsx:96` — `body: { coin: selected },` - `repos/forge-pay/services/pay/src/store.ts:396` — `.where(and(eq(coinBalances.userId, userId), eq(coinBalances.network, env.depositNetwork)))` - `repos/forge-pay/services/pay/src/chains.ts:146` — `const mainnet = network === 'mainnet'` **How it fails** Set `PAY_DEPOSIT_NETWORK=mainnet` (the documented production setting; `.env.example` currently ships `testnet`). The game's funding panel POSTs `{coin:'ETH'}`; pay mints a `network: 'testnet'` ETH deposit address and returns it as the user's deposit address. An EVM address is identical on both networks, so the user pastes it into their wallet and sends real mainnet ETH. The watcher probes that row at `ETH_TESTNET_RPC` (Sepolia), sees nothing, and never credits; `GET /wallet` shows 0 because it filters to `mainnet`. The funds sit at an address whose key only keyvault holds, with no route that will ever find them. **Suggested fix** Ignore the request's `network` on `POST /deposits` and use `env.depositNetwork`, or reject a body whose `network` differs from it with a 400. Apply the same rule to `POST /coins/convert`, `/coins/convert-to-ember`, `/withdrawals` and `/internal/trade`, and have the deposit watcher skip (and warn once about) rows whose network is not the configured one. **Done when** - [ ] Ignore the request's `network` on `POST /deposits` and use `env.depositNetwork`, or reject a body whose `network` differs from it with a 400. - [ ] Apply the same rule to `POST /coins/convert`, `/coins/convert-to-ember`, `/withdrawals` and `/internal/trade`, and have the deposit watcher skip (and warn once about) rows whose network is not the configured one. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: Two corrections to the mechanism. First, "credits into a balance the wallet can never display" overstates it — the balance is invisible to `GET /wallet` and the game UI, but it is not unreachable: `/coins/convert`, `/coins/convert-to-ember`, `/withdrawals` and `/internal/trade` all accept an explicit `network`, so a mismatched balance is fully spendable by anyone crafting the call. What is unrecoverable is the un-watched *address*, not the balance. Second, the stated failure scenario (user pastes the address and sends real mainnet ETH) is weaker than claimed: `funding.tsx:151-163` renders the network as a tag and prints "Send only ETH on testnet", so the user is told. The certain, un-mitigated consequences are instead (a) no mainnet deposit can be credited at all, since every product-minted address is watched at a testnet RPC, and (b) a faucet-funded testnet balance converts to Shards at real market rates, because `pricing.ts:414 currentRate(coin)` is network-blind. > Reviewer note: Three corrections. (1) The chains.ts quote is at line 154, not 146 (146 is comment prose). (2) The silent-loss framing overstates the harm: funding.tsx:155/162 renders the returned network as a tag next to the address and says "Send only {coin} on {network}", so with PAY_DEPOSIT_NETWORK=mainnet the card would read "testnet" — the certain, unavoidable consequence is that deposits become impossible for every user (addresses minted on testnet, credits filtered out of the wallet), and fund loss requires the user to ignore the network shown on the same card. Likewise "no route that will ever find them" is too strong: keyvault holds that key under a deterministic re-derivable orderId, so recovery is an operator/keyvault task rather than impossible. (3) Nothing is mis-minting today — no deploy file or compose file sets PAY_DEPOSIT_NETWORK, and env.ts:59 plus .env.example:56 are both 'testnet', so the default currently matches the server. The variant that is live today is the mirror: any authenticated client can POST {coin, network:'mainnet'} and get a real mainnet address whose credits land in a coin_balances row walletSnapshot hides (convert/withdraw would still accept it via the optional network param). The defect is a go-live footgun in the mainnet switch, not an active outage. --- ### CF-26 — Rewrite the status paragraphs in five docs — they all say no chain exists, and the code produces blocks **P2** · inaccurate · `hearth/docs` · effort M · user-visible **What is wrong** `node/src/evmnode.js` builds the chain, the miner and the JSON-RPC server and mounts it on 8545; `node/test/evm-p2p-fork.js` passes 33/33 including two real nodes partitioning, reorging, agreeing on state roots byte for byte, and a restarted node replaying to the same tip; `docker-compose.testnet.yml` runs three nodes with `HEARTH_EVM=1`. Meanwhile network-config.md, quickstart.md, roadmap.md, listing-checklist.md and testing.md all still state, in bold, that nothing produces blocks and nothing mounts the RPC. The code wins; the docs are the defect. **Evidence** - `repos/hearth/docs/network-config.md:16` — `**Phase 5 — consensus on the new state model — has not landed**, so nothing produces blocks and nothing serves these methods over a real chain yet.` - `repos/hearth/docs/network-config.md:160` — `### What is still true: nothing serves it` - `repos/hearth/docs/quickstart.md:16` — `but **phase 5 — consensus on the account model — has not landed.** Nothing produces blocks. There is no endpoint to point at.` - `repos/hearth/docs/roadmap.md:22` — `| **5. Consensus** | block production and validation on the new state model | testnet produces and reorgs | 🟡 **being built. No block has ever been…` - `repos/hearth/docs/testing.md:133` — `- **No block has ever been produced on the account model.** Consensus is under construction.` - `repos/hearth/node/src/evmnode.js:186` — `this.listenJsonRpc(this.opts.jsonRpcPort === undefined ? P.DEFAULT_JSONRPC_PORT : this.opts.jsonRpcPort);` **How it fails** Phase 8's own gate is 'a stranger can deploy unaided'. A stranger opens quickstart.md, reads 'there is no endpoint to point at', and marks every step `[WAITING]` — the testnet is unusable by exactly the audience it exists for. Worse, a diligence reader comparing the repo's own listing-checklist ('EMBER is not ready... no block has ever been produced') against a live endpoint concludes the project does not know its own state. **Suggested fix** Do the status rewrite as part of the testnet launch checklist, not after: mark phase 5/6 done, replace the ⬜ RPC URL rows with the real testnet URL and chain id 7412, and state precisely what the p2p fork suite proves and what it does not (no long-range reorg, no load, no production PoW). Re-derive every count in testing.md from a fresh `npm test` run. **Done when** - [ ] Do the status rewrite as part of the testnet launch checklist, not after: mark phase 5/6 done, replace the ⬜ RPC URL rows with the real testnet URL and chain id 7412, and state precisely what the p2p fork suite proves and what it does not (no long-range reorg, no load, no production PoW). - [ ] Re-derive every count in testing.md from a fresh `npm test` run. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. - [ ] Every document asserting the old behaviour is corrected in the same change. > Reviewer note: Two citations are wrong, though neither weakens the finding. (1) There is no `docker-compose.testnet.yml`; the three EVM nodes are in the estate-root `/Users/savvaniss/dev/personal/stack/docker-compose.yml` (HEARTH_EVM at line 109, ports at 462/488/510). (2) The network-config.md quote sits at lines 13-15, not 16. Also worth adding to the fix list: network-config.md line 44 still marks the RPC URL "does not exist yet", line 46 says the explorer "has nothing to read", and line 165 points tooling at `tools/rpc-probe/stub.js` as "the thing to point your tooling at" — all superseded by the real endpoint. Conversely, one sentence in each doc remains true and should survive the rewrite: there is still no *public* Hearth endpoint. The correct edit is to split "no public network" (true) from "no chain exists / nothing produces blocks / nothing mounts the RPC" (false), and to document `hearthd --evm` as the [RUN] path in quickstart.md. > Reviewer note: The finding's "the code wins; the docs are the defect" is right in direction but over-corrects in two places, and a wholesale flip to "phase 5 is done" would introduce new inaccuracies: 1. NO PUBLIC ENDPOINT — still true. `docker-compose.yml:462,488,510` bind `127.0.0.1:8545/8547/8549`, and no infra/ or deploy/ entry serves hearth. network-config.md's "There is no public Hearth endpoint" and quickstart.md's "There is no live Hearth network" should be RETAINED. What must change is the causal clause: the reason is that nothing has been published, not that "nothing produces blocks." 2. roadmap.md:22 IS ONLY HALF STALE. Its "blocked on StateDB re-rooting both tries per mutation" is STILL ACCURATE — `node/src/state/statedb.js:358-368` `setStorage` still calls `a.storageRoot = st.root()` followed by `this._write(hex, a)` on every mutation, exactly the pattern robustness-review.md §1 measures at 443 MB / 65 s for one 30M-gas transaction against a 15 s block time. Only the "No block has ever been produced" clause is false. The honest rewrite is: blocks are produced and reorgs are proven under test and in compose, but throughput on contract-heavy load remains blocked on the same defect. So the accurate status is: consensus lands, the chain produces and reorgs blocks, the JSON-RPC surface is mounted on 8545 and a downstream service already consumes it — while (a) no endpoint is published and (b) the StateDB re-rooting blocker is unresolved. The five docs need that three-part statement, not a blanket "it works now." --- ### CF-27 — Put the genesis hash in the p2p handshake — a genesis mismatch is currently silent **P2** · missing · `hearth/node` · effort S · user-visible **What is wrong** The handshake gates only on `net` (the network name string). It carries no genesis hash and no chain id. `genesis.loadOrCreate` pins the genesis config to the data directory the first time a node starts, so a node with an existing volume keeps its old genesis while a freshly-created peer computes a new one from current `params.js`. Both still say `net: 'hearth-testnet'`, so they connect happily and then reject each other's blocks forever as `unknown parent`, which p2p treats as ordinary out-of-order sync traffic and does not even count against the peer. **Evidence** - `repos/hearth/node/src/p2p.js:182` — `return { t: 'hello', net: P.NETWORK, height: chain.height, tip: chain.tipId };` - `repos/hearth/node/src/p2p.js:355` — `if (msg.net && msg.net !== P.NETWORK) {` - `repos/hearth/node/src/chain/genesis.js:213` — `if (fs.existsSync(file)) { const onDisk = JSON.parse(fs.readFileSync(file, 'utf8'));` - `repos/hearth/node/src/chain/blockchain.js:403` — `if (!parent) return { ok: false, err: 'unknown parent' };` - `repos/hearth/node/src/p2p.js:250` — `else if (r.err === 'unknown parent') this._orphan(b);` **How it fails** Anyone retunes `GENESIS_TARGET`, `EVM_BLOCK_GAS_LIMIT` or sets `HEARTH_COMMONS_ADDRESS` and redeploys. Operators who wiped their volumes get genesis A; operators who did not get genesis B. Every node reports peers connected and a healthy `/info`, the beacon `hearth.peering` probe is green, and the two halves of the network simply never converge. The only symptom is a height that stops moving, with no log line naming the cause. **Suggested fix** Add `genesis` (and `chainId`) to `_hello()` and drop any peer whose genesis hash differs, with a log line naming both hashes. Test it: start two nodes with deliberately different `commonsAddress`, assert the connection is refused with a genesis-mismatch reason rather than sitting at height 0. Publish the testnet genesis hash and state root in TESTNET.md as the artifact operators check against. **Done when** - [ ] Add `genesis` (and `chainId`) to `_hello()` and drop any peer whose genesis hash differs, with a log line naming both hashes. - [ ] Test it: start two nodes with deliberately different `commonsAddress`, assert the connection is refused with a genesis-mismatch reason rather than sitting at height 0. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: Two claims in the writeup are wrong. (1) `GENESIS_TARGET` (params.js:130) and `EVM_BLOCK_GAS_LIMIT` (params.js:295) are plain constants, not env vars — retuning them takes a code edit and a rebuild; only `HEARTH_COMMONS_ADDRESS` (params.js:305) is env-settable, and that line already documents "changing it afterwards forks." (2) The split is not undetected estate-wide: `hearth.consensus` in infra/beacon/src/targets.js:394 is `critical: true` and goes DOWN on "both at height N with different tips" or a >10 block spread. `hearth.peering` stays green, but consensus does not. Relatedly, "a height that stops moving" is wrong — both halves keep mining and both heights keep climbing; the symptom is divergent tips, and the real gap is that no signal names *genesis* as the cause. Note `/info` already returns `genesis` (evmnode.js:288), so the cheap fix is two-sided: add `genesis` to `_hello()` and drop mismatched peers, and add `genesis` to the beacon's per-node assert. > Reviewer note: The mechanism stands, but three claims in the write-up should be corrected: (a) `GENESIS_TARGET` and `EVM_BLOCK_GAS_LIMIT` are hardcoded params.js constants, not env vars, so the trigger is a partial volume wipe (or adding a fresh node to a live network), not any redeploy; (b) the estate is not blind — `hearth.consensus` (infra/beacon/src/targets.js:394, critical) and the `chain.propagation` journey both report a same-height/different-tip fork as DOWN, and `hearth.peering` being green is beside the point; (c) the genesis hash IS already observable — it is a structured field on the node's startup `ready` log (evmnode.js:195) and on `/info` (evmnode.js:288). The real cost is time-to-diagnose on an internal, loopback-only testnet, and the real future exposure is external node operators once p2p is routed — which is also the point at which this becomes P1. --- ### CF-28 — Refuse a genesis.json whose chainId disagrees with HEARTH_NETWORK — it silently re-enables cross-net replay **P2** · security · `hearth/node` · effort S · user-visible **What is wrong** `params.js` goes out of its way to make a chain id un-guessable: an unregistered network is a hard error, `HEARTH_CHAIN_ID` must be explicit. But `Blockchain` does not use `params.CHAIN_ID` at all — it takes `chainId` from the persisted `genesis.json`, and `genesis.loadOrCreate` returns whatever is on disk with no comparison against the node's network. `eth_chainId`, the mempool's `TX.validate` and `_formatTx` all follow the file. So the one guard the spec calls mandatory can be bypassed by a stale or hand-edited volume, silently and with no log line. **Evidence** - `repos/hearth/node/src/chain/blockchain.js:78` — `const cfg = dataDir ? genesis.loadOrCreate(dataDir, config) : genesis.normalizeConfig(config || {});` - `repos/hearth/node/src/chain/blockchain.js:80` — `this.chainId = g.config.chainId;` - `repos/hearth/node/src/chain/genesis.js:215` — `return normalizeConfig(overrides ? { ...onDisk, ...overrides } : onDisk);` - `repos/hearth/node/src/params.js:69` — `CHAIN_ID: resolveChainId(NETWORK),` - `repos/hearth/docs/evm-spec.md:80` — `derived from `HEARTH_NETWORK` in `node/src/params.js` and read from there by` **How it fails** Verified live: start `HEARTH_NETWORK=hearth-test hearthd --evm --data DIR` with a `DIR/genesis.json` containing `"chainId": 9999`; the node logs `ready · height 0 · chain 9999` and answers `eth_chainId` → `0x270f`. The realistic instance is a testnet volume created before the 7411/7412 split: `hearth-testnet-seed` then advertises **7411** while forge-keyvault and hardhat resolve **7412**. Either every keyvault signature is rejected with `INVALID_CHAINID` and no obvious cause, or — worse — a wallet that trusts `eth_chainId` signs a 7411 transaction on the testnet that is byte-identical and valid on mainnet, which is precisely the replay the spec says must not be possible. **Suggested fix** In `Blockchain`'s constructor, compare `cfg.chainId` against `params.CHAIN_ID` and throw naming both values and the data directory, unless an explicit override was passed. Same treatment for `extraData`, which carries the network name for the same reason. **Done when** - [ ] In `Blockchain`'s constructor, compare `cfg.chainId` against `params.CHAIN_ID` and throw naming both values and the data directory, unless an explicit override was passed. - [ ] Same treatment for `extraData`, which carries the network name for the same reason. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: Two imprecisions. (1) "silently and with no log line" overstates it — evmnode.js:194 logs `ready · height N · chain ` and /info echoes both P.NETWORK and the chain id, so the wrong id is printed at startup; what is missing is the comparison and the refusal, not the value. (2) The exploit needs an operator-side precondition: a fresh data dir is written by genesis.loadOrCreate with the network-derived id, and an explicit config/--genesis override wins over disk (genesis.js:215 spreads overrides last), so only the default path over a stale or hand-edited genesis.json is affected. Worth adding a mitigating detail the finding does not mention: a stale pre-split volume also has a different genesis hash, so that node orphans every block from its peers and sits at height 0 — the mismatch is likely to surface as a stuck node before it surfaces as a replayed signature, though the wrongly-signed 7411 transaction is still produced and is still valid on mainnet. > Reviewer note: Refuse or loudly warn when a persisted genesis.json chainId disagrees with the node's HEARTH_NETWORK-derived id — the guard params.js enforces is not applied to the file it is actually read from. Corrections to the finding's narrative: (a) the "volume created before the 7411/7412 split" instance does not exist — chain ids were introduced together with the 7411/7412 split in the same commit (de322b6), so no volume predates it; the realistic drift is a data dir created with HEARTH_NETWORK unset (default 'hearth' -> 7411) and later relabelled testnet; (b) a stale dir also carries stale `extraData`, so its genesis hash differs and the node stalls at height 0 with "unknown parent" rather than serving a working testnet on the wrong id — the silent wrong-id-but-syncing case needs chainId alone to be edited; (c) forge-pay/keyvault cannot be made to sign on the wrong network by this: forge-pay reads eth_chainId and keyvault binds the id independently, so the outcome is a 403 refusal (stalled sweeps, no obvious cause), not a wrong-network signature; (d) the mainnet-replay outcome requires a mainnet, which does not exist yet. --- ### CF-29 — Fix the failing explorer-api suite — the Developer kit CI job is red on every push **P2** · broken · `hearth/tools` · effort M **What is wrong** `node tools/explorer-api/test/explorer-api.test.js` exits 1 today (verified by running it), throwing on a missing `logIndex` while indexing a block. CI runs exactly that command in the `tools` job, so that job has been failing continuously. The real chain adapter does number `logIndex` per block (`rpcadapter.js:232`), so the defect is in the test's fake chain, not in the shim — but nobody has established that, and a permanently-red job trains everyone to ignore the whole workflow. **Evidence** - `repos/hearth/.github/workflows/ci.yml:221` — `run: node tools/explorer-api/test/explorer-api.test.js` - `repos/hearth/tools/explorer-api/src/rpc.js:106` — `RpcError: internal error: receipt.logs[0].logIndex is missing — the chain must number logs across the block, and this layer cannot derive it from o…` - `repos/hearth/node/src/chain/rpcadapter.js:232` — `logIndex: logIndex++, // per BLOCK; see the header note` - `repos/hearth/docs/listing-checklist.md:148` — ``tools/verify` passes **116/116**. `tools/explorer-api` **does not pass**` **How it fails** A genuine regression lands in `tools/faucet` or `tools/verify` — both of which run in the same job, after the failing step or before it. The job is already red, so no one looks, and the faucet ships with a broken payout limiter to the first public testnet. **Suggested fix** Fix the test fixture to number `logIndex` across the block exactly as `rpcadapter.js` does, or fix the shim if the shim is wrong; then run the explorer-api service against the live compose testnet on 8545 and assert `module=account&action=balance` and `module=logs&action=getLogs` agree with `eth_getBalance`/`eth_getLogs`. That run is the actual gate for the aggregator/listing milestone, not the unit suite. **Done when** - [ ] Fix the test fixture to number `logIndex` across the block exactly as `rpcadapter.js` does, or fix the shim if the shim is wrong; then run the explorer-api service against the live compose testnet on 8545 and assert `module=account&action=balance` and `module=logs&action=getLogs` agree with `eth_getBalance`/`eth_getLogs`. - [ ] That run is the actual gate for the aggregator/listing milestone, not the unit suite. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: The consequence is not "a faucet regression ships unnoticed" — the faucet step runs before the failing one and passes on every red run. The consequence is that GitHub Actions fail-fast SKIPS the three steps after it: Contract verification (the reject test that gates the exchange listing), Templates and probe parse, and the probe boot check. Those have never executed in CI. Also, the failure is not unestablished: docs/listing-checklist.md:141-153 documents it verbatim, including the shim-vs-fixture question, and blocks the /api listing row on it. And the origin is node/src/jsonrpc/methods.js:448, not tools/explorer-api/src/rpc.js:106. Finally, the DeFi contracts job is red in the same run, so this is not the only thing keeping main red. > Reviewer note: Two imprecisions. (1) The quoted error text is not at tools/explorer-api/src/rpc.js:106 — that line is only the generic rethrow `throw new RpcError(payload.error.code, payload.error.message, payload.error.data)` in `Rpc.call`, i.e. the stack frame. The message and the guard that raises it live at node/src/jsonrpc/methods.js:450, in a deliberate hard-fail whose comment explains that a single receipt cannot derive a block-wide ordinal. So the shim is not the thing demanding logIndex; the node's own JSON-RPC receipt formatter is, and it is right to. (2) The failure scenario names the wrong victim: `tools/faucet` runs BEFORE the failing step, so a faucet regression still fails the job at its own step with its own visible output. `tools/verify` runs AFTER it and is therefore never executed in CI at all — its 116 checks have never gated a push. Restate the scenario around tools/verify (and the "Templates and probe parse" / probe-boot steps that follow), which are mechanically dead, rather than around the faucet, which is only at risk through the softer "nobody reads a red job" argument. Also note the redness is already tracked in MAP.md §12 and docs/listing-checklist.md; what has not been established there is which side owns the bug — and this review establishes it: the test fixture, not the shim. --- ### CF-30 — Stop hardcoding chain id 7411 in the browser wallet/explorer while compose runs testnet 7412 **P2** · drift · `hearth/web + stack/docker-compose` · effort M · user-visible **What is wrong** The shipped wallet pins EIP-155 chain id 7411 as a module constant and signs every transaction with it; the explorer hardcodes the same literal in its banner check. The only chain this stack runs is `HEARTH_NETWORK=hearth-testnet`, which params.js resolves to 7412. Nothing in docker-compose.yml or the image can change the wallet's constant — it is baked into a static file with no build step and no meta-tag override. **Evidence** - `repos/hearth/web/assets/wallet/transaction.js:34` — `export const CHAIN_ID = 7411;` - `repos/hearth/web/assets/explorer/app.js:217` — `if (chainId !== 7411) {` - `docker-compose.yml:434` — `HEARTH_NETWORK: "hearth-testnet"` - `repos/hearth/node/src/params.js:38` — `'hearth-testnet': 7412,` - `repos/hearth/web/assets/wallet/app.js:136` — `if (id !== CHAIN_ID) {` **How it fails** With the RPC upstream fixed to :8545, the wallet connects, reads chain id 7412, banners "This is not chain 7411", and any transaction the user signs carries v derived from 7411 — the node rejects it under EIP-155. The wallet is unusable against the only chain the stack ships. **Suggested fix** Resolve the chain id at runtime from `eth_chainId` (or a `` the container templates like HEARTH_RPC_UPSTREAM) and pass it into signAndCheck, keeping the mismatch banner as a warning rather than a hard-coded expectation. **Done when** - [ ] Resolve the chain id at runtime from `eth_chainId` (or a `` the container templates like HEARTH_RPC_UPSTREAM) and pass it into signAndCheck, keeping the mismatch banner as a warning rather than a hard-coded expectation. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. - [ ] Every document asserting the old behaviour is corrected in the same change. > Reviewer note: Two refinements. (1) The failure is latent today behind a second defect the finding half-acknowledges: web/Dockerfile pins HEARTH_RPC_UPSTREAM=hearth-testnet-seed:8645 (the legacy REST port), and compose's hearth-web service sets no environment, so the wallet's same-origin /rpc/ currently hits node/src/rpc.js's getinfo handler and dies with MalformedResponse before it ever reads a chain id. The 7411/7412 break is what you hit immediately after fixing the upstream to :8545. (2) The blast radius is larger than the two cited lines — 7411 is also baked into web/assets/wallet/keystore.js:103 (every keystore file records chainId: 7411), explorer/views.js:196 ("hearth mainnet" vs "not 7411"), explorer/fixtures.js:34, wallet.html:286, and assets/wallet-selftest.js:211/217/365, so the fix is a bundle-wide parameterisation (read eth_chainId, or a meta tag alongside hearth-eth-rpc), not a two-line edit. Also worth noting the wallet does degrade loudly (a "wrong chain — 7412" mode plus a banner telling the user to repoint with ?rpc=), and the rejected transactions cost nothing — though signing mainnet-id 7411 bytes with a testnet key is precisely the cross-network replay hazard evm-spec.md 75-96 exists to prevent. > Reviewer note: Two refinements. (1) The failure scenario is gated as stated ("with the RPC upstream fixed to :8545") for the same-origin path, but it is ALREADY live via the documented ?rpc=http://localhost:8545 loopback override, which CSP and the published 127.0.0.1:8545 port both permit. (2) The blast radius is one line wider than the evidence: explorer/views.js:196 hardcodes 7411 a second time in the chain-id stat tile, and wallet.html:286 plus wallet-selftest.js:211-229,365 and explorer/fixtures.js:34 also pin 7411 and must move together. Also note two constraints on the fix: it must NOT be "adopt whatever eth_chainId the node reports" (a hostile ?rpc= node could then choose what the user signs for — the pin exists partly for that), and it must NOT be "make the testnet 7411" (params.js:17-35 forbids that explicitly to prevent testnet/mainnet replay). The right shape is a deployment-configurable override, e.g. a meta tag read the way rpc.js already reads meta[name=hearth-eth-rpc]. --- ### CF-31 — Grant the commune join stipend once per player, not once per join **P2** · inaccurate · `ninety-days-after/game` · effort S · user-visible **What is wrong** shared-libs documents the commune credit as a one-time goodwill grant and states the invariant that a member cannot draw more over a season than they deposited plus the stipend; MAP.md:196 restates it. The join handler sets `communeCredit` to the flat stipend (not adding to it, not checking whether this player has ever been granted it), and the leave handler zeroes it. Nothing records that a stipend was already paid, so the grant is per-join. The daily counter (`withdrawDay`/`withdrawnToday`) is not reset by leaving, which caps the abuse at the stipend per day rather than stopping it. **Evidence** - `repos/ninety-days-after/services/game/src/routes/communes.ts:182` — `.set({ communeId: row.id, communeCredit: COMMUNE_JOIN_STIPEND })` - `repos/ninety-days-after/services/game/src/routes/communes.ts:380` — `.set({ communeId: null, communeCredit: 0 })` - `repos/shared-libs/packages/shared/src/game.ts:186` — `* refills, so over a season a member cannot take out more than they put in plus` - `repos/shared-libs/packages/shared/src/game.ts:180` — `export const COMMUNE_JOIN_STIPEND = 3` - `repos/ninety-days-after/MAP.md:196` — `more than they put in plus the stipend.` **How it fails** A player joins a stocked commune, withdraws 3 goods (cap = min(10, credit=3)), and leaves. The next in-game day they rejoin the same commune: `communeCredit` is set to 3 again, `withdrawnToday` no longer matches the current day, and they withdraw another 3. Repeating this for a 90-day season extracts ~270 goods from other members' deposits while depositing nothing — against a documented ceiling of 3. **Suggested fix** Track the grant on the player row (e.g. `stipend_granted boolean`, or keep `commune_credit` non-negative across leaves instead of zeroing it) and make join a no-op for credit when the player has already been paid the stipend in this world. Either that, or amend the shared-libs comment and MAP.md so they stop asserting an invariant the code does not hold. **Done when** - [ ] Track the grant on the player row (e.g. - [ ] `stipend_granted boolean`, or keep `commune_credit` non-negative across leaves instead of zeroing it) and make join a no-op for credit when the player has already been paid the stipend in this world. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. - [ ] Every document asserting the old behaviour is corrected in the same change. > Reviewer note: Accurate as written; two clarifications worth carrying into the report. (1) The abuse rate is one stipend per *in-game* day, and worlds default to `tickIntervalMinutes: 1440` (shared-libs `game.ts:241`), so the ~270-good figure is 90 real days of manual join/withdraw/leave, not a fast drain. (2) The rejoin loop only pays a player who deposits nothing — rejoining *sets* `communeCredit` to 3, so any member with real banked credit loses by cycling. The stipend is also collectable once per fresh account with no loop at all, and the join handler enforces no invite, approval, or member cap, so the same hole scales with open registration (`platform/services/nimbus/src/routes/auth.ts:111`) to N accounts x 3 goods/day against a publicly listed stockpile. > Reviewer note: Two refinements. (1) The founding path at communes.ts:155 regrants the stipend identically, so "leave and found a new commune" is a second entrance to the same hole — though a solo founder nets nothing because disbanding scatters the stockpile, so the join path remains the only profitable one. (2) "not adding to it" is not itself a defect: the join branch only fires when the player has no commune, and leave has already zeroed the credit, so set-vs-add can never lower a banked balance. The defect is solely the unconditional regrant. --- ### CF-32 — Make POST /worlds/:id/join atomic — unique (world_id, user_id) and a conditional tile claim **P2** · broken · `ninety-days-after/game` · effort M · user-visible **What is wrong** Join is a read-then-write with no transaction, no lock and no database constraint behind it. The idempotency check reads `players` for (worldId, userId) and, if absent, inserts — but the `players` table carries only non-unique indexes, so two concurrent joins from one account both pass the check and both insert. Separately, `assignHomestead` selects every unowned tile, picks the first claimable one with a deterministic `find`, and issues an unconditional `UPDATE … WHERE id = candidate.id`: two callers running concurrently select the same candidate and both write it, the second silently winning. **Evidence** - `repos/ninety-days-after/services/game/src/routes/worlds.ts:77` — `if (existing) return toPlayerState(existing)` - `repos/ninety-days-after/services/game/src/db/migrate.ts:50` — ``CREATE INDEX IF NOT EXISTS players_world_idx ON players (world_id)`,` - `repos/ninety-days-after/services/game/src/world/generate.ts:91` — `free.find((t) => CLAIMABLE.includes(t.terrain)) ??` - `repos/ninety-days-after/services/game/src/world/generate.ts:105` — `.set({ terrain: 'homestead', ownerId })` **How it fails** Two tabs (or a client retry after a slow response) fire POST /worlds/:id/join for the same account within the same second: two `players` rows exist for one user in one world, each with its own starting bag (12 food / 12 water / 8 materials / …), its own 6 AP per day and its own progress row, and the roster and `playerCount` show the account twice. Separately, two different users joining concurrently both receive the same homestead coordinates; the tile's `owner_id` ends as the second user, so the first player's homestead is drawn on a tile the map says belongs to someone else. **Suggested fix** Add `CREATE UNIQUE INDEX IF NOT EXISTS players_world_user_idx ON players (world_id, user_id) WHERE user_id IS NOT NULL` and run the join body in one transaction with `ON CONFLICT DO NOTHING` plus a re-read. In `assignHomestead`, make the claim conditional — `UPDATE tiles SET … WHERE id = $1 AND owner_id IS NULL RETURNING *` — and retry with the next candidate when it returns no row. **Done when** - [ ] Add `CREATE UNIQUE INDEX IF NOT EXISTS players_world_user_idx ON players (world_id, user_id) WHERE user_id IS NOT NULL` and run the join body in one transaction with `ON CONFLICT DO NOTHING` plus a re-read. - [ ] In `assignHomestead`, make the claim conditional — `UPDATE tiles SET … WHERE id = $1 AND owner_id IS NULL RETURNING *` — and retry with the next candidate when it returns no row. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: Two refinements. (1) "No transaction" is not the defect by itself — wrapping the read-then-write in a transaction at READ COMMITTED would not prevent either race; the actual fixes are the ones the title names: a unique index on (world_id, user_id) with an insert ... on conflict do nothing, and a conditional claim (WHERE id = ? AND owner_id IS NULL) with retry to the next candidate. (2) The duplicate-row fallout is worse than stated: getPlayerByUser (util.ts:233) has no ORDER BY and destructures the first row, so with two players rows the account's identity is nondeterministic per request — the same user can flip between two characters with different homesteads, bags and AP across calls — and releaseHomestead matches on ownerId, so a tile lost to the second writer is never released for the first owner. > Reviewer note: Two corrections to the framing, both about the fix rather than the bug. (1) The severity is carried almost entirely by the duplicate `players` row; the homestead collision is a cosmetic map inconsistency — tile `ownerId` is read only by GET /worlds/:id/map and `releaseHomestead`, never by the day resolver — so do not let it justify the rating. (2) The obvious fix has two sharp edges. `migrate.ts` runs its statements in a bare loop and `step('migrate')` exits the process on throw, so adding `CREATE UNIQUE INDEX ... ON players (world_id, user_id)` will crash the service at boot if any duplicate pair already exists in a deployed database — the migration needs a dedupe (or tolerant index creation) ahead of the constraint. And `assignHomestead` claims the tile at worlds.ts:80 *before* the players insert at line 84, so once the unique constraint starts rejecting the losing racer, that request leaks a tile permanently set to `terrain='homestead'` with a dangling `owner_id` pointing at a player that was never created; the claim and the insert have to share one transaction, or the claim has to be released on insert failure. --- ### CF-33 — Fetch withdrawals by status instead of filtering the newest 200 client-side — "Nothing is stuck" can be false **P2** · inaccurate · `platform/admin` · effort S **What is wrong** The console calls `listWithdrawals()` with no status, so pay returns the 200 newest rows across all statuses, and the status chips only filter that already-truncated array in the browser. The `?status=` parameter is implemented in pay, plumbed through the Nimbus relay and typed in the console's api client — and never passed. The stuck count on the header line is computed from the same truncated array. **Evidence** - `repos/platform/apps/admin/src/pages/Prices.tsx:419` — `.listWithdrawals()` - `repos/platform/apps/admin/src/pages/Prices.tsx:452` — `const stuck = (withdrawals ?? []).filter((w) => w.status === 'stuck').length` - `repos/forge-pay/services/pay/src/store.ts:1485` — `limit = 200,` - `repos/platform/apps/admin/src/lib/api.ts:577` — `listWithdrawals(status?: WithdrawalStatus): Promise<{ withdrawals: AdminWithdrawal[] }> {` - `repos/platform/services/nimbus/src/routes/pay.ts:151` — `app.get<{ Querystring: { status?: string } }>('/admin/pay/withdrawals', async (req, reply) => {` **How it fails** A withdrawal goes stuck, then 200 more withdrawals are created and confirm. The stuck row falls out of the newest-200 window. The Prices page header says "Nothing is stuck." and clicking the `stuck` chip shows "No stuck withdrawals" — while forge-pay would answer `?status=stuck` with the row. The one panel whose job is the human-intervention queue reports an empty queue that is not empty. **Suggested fix** Pass the selected status through to the API (`api.listWithdrawals(statusFilter === 'all' ? undefined : statusFilter)`) and refetch on chip change, and compute the stuck badge from a dedicated `listWithdrawals('stuck')` call rather than from the truncated all-statuses page. **Done when** - [ ] Pass the selected status through to the API (`api.listWithdrawals(statusFilter === 'all' ? undefined : statusFilter)`) and refetch on chip change, and compute the stuck badge from a dedicated `listWithdrawals('stuck')` call rather than from the truncated all-statuses page. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. - [ ] Every document asserting the old behaviour is corrected in the same change. > Reviewer note: One precision on the fix: passing `statusFilter` into `listWithdrawals()` repairs the chips, but NOT the header count. When the filter is `all` the request still returns the newest 200 across all statuses, so `const stuck = ...filter(w => w.status === 'stuck').length` (Prices.tsx:452) stays computed from a truncated array. The header needs its own `listWithdrawals('stuck')` call independent of the chip selection. Also worth noting the UI does disclose the truncation for the table itself ("The 200 most recent, newest first."), so the defect is specifically the two unqualified absolute claims sitting next to that caveat — "Nothing is stuck." and "No stuck withdrawals." — not the table being capped. > Reviewer note: The panel does disclose its window in prose ("The 200 most recent, newest first.", Prices.tsx:589), so the table itself is not misrepresenting its scope. The defect is narrower than stated: the unqualified "Nothing is stuck." sentence and the `stuck` chip are derived from that truncated window instead of from a `?status=stuck` fetch, so the human-intervention claim — not the table — is what can be false. --- ### CF-34 — Give the console a way to abandon a stuck withdrawal — it reports the queue and offers no action **P2** · unexposed · `platform/admin` · effort M · user-visible **What is wrong** forge-pay implements the whole remediation path for a stuck payment — it asks the chain first, refuses to refund anything the network can still see, refunds the user's balance and writes an attributed audit line. Nothing calls it. Nimbus's pay proxy deliberately relays only four routes, and the console has no `abandonWithdrawal` in its API surface and no button anywhere. The Prices page counts stuck rows and tells the operator they are "waiting on a human", then gives that human nothing to click. **Evidence** - `repos/forge-pay/services/pay/src/routes/admin.ts:335` — `app.post<{ Params: { id: string } }>('/admin/withdrawals/:id/abandon', async (request, reply) => {` - `repos/platform/apps/admin/src/pages/Prices.tsx:592` — `{stuck} {stuck === 1 ? 'is' : 'are'} stuck and waiting on a human.` - `repos/platform/services/nimbus/src/routes/pay.ts:40` — `* Deliberately narrow: the price write, and three reads. Nothing here abandons a` - `repos/platform/apps/admin/src/lib/api.ts:577` — `listWithdrawals(status?: WithdrawalStatus): Promise<{ withdrawals: AdminWithdrawal[] }> {` **How it fails** A user withdraws 0.5 ETH, the transaction is never mined and the withdrawal goes to `stuck`. Their Shards were already debited. The console shows "1 is stuck and waiting on a human" on the Prices page. The only way to return the money is a shell on the deployment host issuing a hand-rolled bearer POST to forge-pay, because nothing in the console or through Nimbus can reach `/admin/withdrawals/:id/abandon`. **Suggested fix** Relay `POST /admin/pay/withdrawals/:id/abandon` in services/nimbus/src/routes/pay.ts (same forwarded-bearer relay as the other four), add `abandonWithdrawal(id)` to apps/admin/src/lib/api.ts, and put a type-to-confirm "Abandon and refund" action on stuck/pending/broadcast rows in the Prices withdrawals table that shows pay's 409 `withdrawal_on_chain` refusal verbatim. **Done when** - [ ] Relay `POST /admin/pay/withdrawals/:id/abandon` in services/nimbus/src/routes/pay.ts (same forwarded-bearer relay as the other four), add `abandonWithdrawal(id)` to apps/admin/src/lib/api.ts, and put a type-to-confirm "Abandon and refund" action on stuck/pending/broadcast rows in the Prices withdrawals table that shows pay's 409 `withdrawal_on_chain` refusal verbatim. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: Two corrections. (a) The failure scenario says "Their Shards were already debited" — it is the custodial COIN balance, not Shards. POST /withdrawals debits the coin balance and refundWithdrawal (store.ts:1621) restores it via moveCoinBalance, writing a ledger row with delta 0 precisely because no Shards move. In an estate this careful about the Shards/coin boundary the distinction matters. (b) The finding should carry a prerequisite: wiring abandon into Nimbus and the console must not happen until admin.ts:335 makes the EVM nonce check (money-review.md §5). Exposing the route as written trades a stranded-funds inconvenience for a double-payment loss on the exact chains (EMBER, ETH) where the button would be pressed. > Reviewer note: Two corrections. (1) The finding treats the route as ready to wire up ("refuses to refund anything the network can still see"), but repos/forge-pay/docs/money-review.md:224 documents that for EVM, `unknown` means only "no receipt" (outbound.ts:364) — the signed bytes stay valid with an unconsumed nonce and no expiry, so abandoning an ETH/EMBER withdrawal can refund a payment that lands later. XRP is protected by LastLedgerSequence; EVM is not. Adding a console button without first adding the nonce-consumed check (eth_getTransactionCount at `latest` against the nonce in raw_tx) turns a curl-only hazard into a one-click double-pay. The fix is "add the nonce check, then expose it", not "add a button". (2) "a shell on the deployment host" is overstated: pay is routed at the edge as pay., admin. is on its CORS allowlist and POST is permitted, so any admin with a Nimbus token can curl the route from anywhere. Also, the gap is already inventoried in repos/forge-pay/MAP.md:149 ("nothing — no UI exists; curl only") and MAP.md:621, alongside the same gap for POST /admin/deposit-addresses/:id/sweeps. --- ### CF-35 — Stop swallowing treasury/withdrawal load failures into empty arrays — an outage renders as reassuring text **P2** · inaccurate · `platform/admin` · effort S **What is wrong** Both secondary panels on the Prices page convert any failure into an empty result set. The empty-state copy for those arrays is not neutral — it asserts facts. A 502 `pay_unreachable` from the Nimbus relay, a 403, or a timeout all render as "No treasury addresses configured." and "Nothing is stuck." The intent (don't take the price panel down with it) is right; the execution replaces an error with a confident lie. Every other panel in this console renders `err.message`, including the vault pin panel next door. **Evidence** - `repos/platform/apps/admin/src/pages/Prices.tsx:417` — `.catch(() => setTreasuries([]))` - `repos/platform/apps/admin/src/pages/Prices.tsx:421` — `.catch(() => setWithdrawals([]))` - `repos/platform/apps/admin/src/pages/Prices.tsx:551` — `No treasury addresses configured.` - `repos/platform/apps/admin/src/pages/Prices.tsx:595` — `'Nothing is stuck.'` **How it fails** forge-pay is down or `PAY_API_URL` is misconfigured. Nimbus answers 502 with `pay_unreachable`. An operator opens /prices to check the withdrawal queue during exactly that incident and reads "Nothing is stuck." and "No treasury addresses configured." — concluding the treasury was never set up and no payments need attention, when in fact nothing was read at all. **Suggested fix** Keep the panels non-fatal but store the error: `.catch((e) => setTreasuriesError(msg(e)))` and render an inline error block in place of the empty state, mirroring the `pinsError` handling in apps/admin/src/pages/Vault.tsx. **Done when** - [ ] Keep the panels non-fatal but store the error: `.catch((e) => setTreasuriesError(msg(e)))` and render an inline error block in place of the empty state, mirroring the `pinsError` handling in apps/admin/src/pages/Vault.tsx. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. - [ ] Every document asserting the old behaviour is corrected in the same change. > Reviewer note: The failure scenario should read: with forge-pay fully down, listAdministeredPrices() fails first and load() returns early, so the page shows a red pay_unreachable banner and both secondary panels stay as loading skeletons — the treasury empty-state does not render. The empty-array lie is reached when prices succeeds but a secondary read fails independently (edge/gateway timeout on the chain-balance-heavy /admin/treasury returning HTML -> non_json ApiError, or a 500 from /admin/withdrawals). Additionally, "Nothing is stuck." at line 595 is not gated on withdrawals !== null at all: it renders on first paint before any data has loaded and in the total-outage case too, which is a stronger defect than the cited catch. A third uncited assertive string, "No withdrawals yet." in the table body, has the same problem. > Reviewer note: The finding understates the blast radius on the withdrawals side: `stuck` is derived from `(withdrawals ?? [])` at Prices.tsx:452 and the header containing "Nothing is stuck." (lines 585-597) renders unconditionally, outside the `withdrawals === null` skeleton guard at line 617. So that sentence asserts a fact during the normal initial load as well, not only after a failed fetch — the treasury panel at least gates its empty-state copy on `treasuries !== null`. Also worth naming: both catches swallow the error object entirely, so the failure leaves no trace in the browser console either, which is what makes it undiagnosable rather than merely mis-rendered. --- ### CF-36 — Give refresh rotation a reuse grace window: two tabs refreshing at once burn the whole family **P2** · broken · `platform/nimbus` · effort M · user-visible **What is wrong** `rotateRefreshToken` treats any presentation of an already-revoked, unexpired token as theft and revokes every live token in the family. It has no grace window and no notion of "this is the same token I rotated 200ms ago". The clients guard against this only *within one tab*: the admin console has `refreshOnce()` single-flight and the game client has `inflightRefresh`, but both are module-level variables, and the portal's own `refreshPortal()` (portal.ts:515) and `/account`'s `refreshTokens()` have no guard at all. Refresh tokens live in `localStorage`, which is shared across every tab of an origin, so two tabs of the same product are two independent refreshers holding the same token. **Evidence** - `repos/platform/services/nimbus/src/tokens.ts:94` — `await db .update(refreshTokens) .set({ revoked: true }) .where(and(eq(refreshTokens.familyId, spent.familyId), eq(refreshTokens.revoked, false)))` - `repos/platform/services/nimbus/src/tokens.ts:92` — `if (!spent || spent.expiresAt.getTime() < Date.now()) return { status: 'invalid' }` - `repos/platform/services/nimbus/src/routes/portal.ts:515` — `async function refreshPortal() {` - `repos/platform/services/nimbus/src/routes/auth.ts:196` — `req.log.warn({ userId: rotated.userId }, 'refresh token reuse detected — family revoked')` - `repos/platform/apps/admin/src/lib/api.ts:337` — `function refreshOnce(): Promise {` **How it fails** A user has two tabs open on admin.. Both sit idle past the 15-minute access TTL. They come back, both tabs poll, both get 401, both read the same `cf.refreshToken` from localStorage and POST /auth/refresh. One rotates successfully; the other's token is now revoked-but-unexpired, so nimbus burns the entire family — including the successor the first tab just stored. Both tabs are signed out mid-work, and the operator log records `refresh token reuse detected — family revoked`, sending whoever reads it hunting a token thief that does not exist. **Suggested fix** Store the successor's id on the row being revoked (or keep a short `rotated_at`), and on re-presentation of a token revoked within a small window (5–10s) return the same successor instead of burning the family. Reserve the family burn for reuse outside that window — which is the case that actually indicates theft. Add a cross-tab lock (BroadcastChannel or a localStorage mutex) in the shared client helper as defence in depth. **Done when** - [ ] Store the successor's id on the row being revoked (or keep a short `rotated_at`), and on re-presentation of a token revoked within a small window (5–10s) return the same successor instead of burning the family. - [ ] Reserve the family burn for reuse outside that window — which is the case that actually indicates theft. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: Two details are wrong. (1) The storage key is not `cf.refreshToken`; admin uses `cf.admin.refresh` (apps/admin/src/lib/api.ts:204) and the portal uses `cf.portal.refresh` (portal.ts:513). (2) The stated trigger — "both sit idle past the 15-minute TTL, both tabs poll" — does not apply to the admin console: there is no setInterval anywhere under apps/admin, so admin only refreshes on a user-driven request's 401. And because `tokens.refresh` is a live localStorage getter rather than an in-memory snapshot, a user who clicks in tab A and then tab B seconds later is safe — tab B reads the already-rotated successor. The real race window is one refresh round-trip (a few hundred ms), not the whole idle period. The realistic triggers are therefore: simultaneous page loads (browser session restore, or both tabs reloaded together) where both mount and fire their first authenticated call at once; and the two pages in the estate that actually poll on a timer — forge-mint/apps/forge-mint/src/pages/Order.tsx and ninety-days-after/apps/game/src/pages/Wallet.tsx — where two tabs of the same product will collide on schedule. Both those apps' inflightRefresh guards are per-document and do not help. The consequence the finding describes (whole family burned, both tabs signed out, a false "refresh token reuse detected — family revoked" in the operator log) is accurate. > Reviewer note: Two details in the finding are imprecise. (a) The admin console does not poll and does not use `cf.refreshToken` — it stores `cf.admin.access`/`cf.admin.refresh` (apps/admin/src/lib/api.ts:203-204) and refreshes reactively on a 401 from a user action or route load; `cf.refreshToken` is the key used by the game, Crucible, Forge Mint, the site and Hearth. The per-origin-shared-key mechanism is unchanged. (b) Crucible (crucible/src/lib/api.ts:93) and Forge Mint (forge-mint/src/lib/api.ts:93) do have `inflightRefresh` guards; the genuinely unguarded refreshers are the portal's `refreshPortal()` (portal.ts:515), `/account`'s `refreshTokens()` (portal.ts:963), hearth/web/assets/cloudsforge.js:182 and hearth/site/src/lib/auth.tsx:88 — and all of the guards are per-tab module state regardless, which is the finding's actual point. Two things the finding understates and should be added: the winning tab dies up to 15 minutes later with a second false theft alert, and the loser's redirect to the portal can burn the portal's own family, escalating a silent SSO re-handoff into a password prompt. --- ### CF-37 — Correct the public roadmap: EMBER now credits at 60 confirmations and cosmetics are server-side **P2** · inaccurate · `platform/site` · effort S · user-visible **What is wrong** Three public claims on cloudsforge.online are no longer true of the code. Hearth is an account-model EVM chain and forge-pay reads `eth_getBalance` at `head - 60`; the tip reading has its own variable and is only ever written to `pending`, never credited. Yet the site's hero caveat and its "Now / In progress" roadmap entry both state EMBER credits at the tip and call it "the single thing standing between the mine-to-spend loop and a headline". Separately, cosmetics are persisted in a `player_cosmetics` row, entitlement-checked against Pay on equip, and fanned out to every `players.cosmeticStyle` so rosters render them — the roadmap still says they live in your own browser where nobody else sees them. **Evidence** - `repos/platform/apps/site/src/lib/site.ts:204` — `'Hearth is on testnet, and EMBER is the one coin Forge Pay still credits at the chain tip instead of waiting for confirmations.` - `repos/platform/apps/site/src/lib/site.ts:362` — `EMBER does not — Hearth’s API exposes only the balance at the tip, so a deposit counts the moment it is seen.` - `repos/forge-pay/services/pay/src/chains.ts:21` — `* EMBER — TRUE (eth_getBalance at `latest - confirmations`, same as ETH)` - `repos/shared-libs/packages/shared/src/deposits.ts:102` — `confirmations: 60,` - `repos/platform/apps/site/src/lib/site.ts:376` — `Cosmetics are stored in your own browser, so nobody else ever sees what you bought — which removes the entire reason to buy one.` - `repos/ninety-days-after/services/game/src/routes/cosmetics.ts:117` — `.set({ cosmeticStyle: serialiseEquipped(merged) })` **How it fails** A prospective user or partner reads /roadmap and the home page today and concludes that EMBER deposits are credited with zero confirmation depth and that game cosmetics are purely local. Both statements are refuted by the shipped code (60 confirmations in the shared deposit registry; a database-backed wardrobe fanned out to rosters). The site's own promise — "nothing claimed here that you cannot go and use" — is inverted: it is understating what runs. **Suggested fix** In apps/site/src/lib/site.ts, move the "EMBER credited honestly" entry to Shipped and rewrite it against `confirmations: 60` (rewriting or deleting LOOP_CAVEAT to match), and cut the cosmetics sentence from the "Deliver everything already for sale" entry, keeping the private-worlds half — `POST /private-worlds/rent` still provisions nothing. **Done when** - [ ] In apps/site/src/lib/site.ts, move the "EMBER credited honestly" entry to Shipped and rewrite it against `confirmations: 60` (rewriting or deleting LOOP_CAVEAT to match), and cut the cosmetics sentence from the "Deliver everything already for sale" entry, keeping the private-worlds half — `POST /private-worlds/rent` still provisions nothing. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. - [ ] Every document asserting the old behaviour is corrected in the same change. > Reviewer note: The two stale claims are confirmed, but the repair is narrower than "correct the roadmap": (a) in the "Deliver everything already for sale" entry, strike only the cosmetics sentence — the rented private world is still sold and still provisions nothing (SKU in shared/pay.ts:156, no provisioning anywhere in the game service), so the entry stays; (b) LOOP_CAVEAT and the "EMBER credited honestly" entry should be rewritten, not removed — EMBER now waits 60 blocks, but the economic controls that deposits.ts says "live in forge-pay" (per-user caps, halt on any reorg past ~5 blocks) are not implemented there, so a caveat about not trusting the loop with size remains honest for that reason instead. > Reviewer note: Two imprecisions. (1) The roadmap entry at site.ts:376 is only half stale: I found no private-world provisioning anywhere in repos/ninety-days-after/services/game/src, so 'a rented private world charges Shards and provisions nothing' still holds — only the cosmetics sentence needs rewriting, the entry should not be deleted. (2) The finding scopes to platform/site, but the same stale EMBER claim lives at the estate root in /Users/savvaniss/dev/personal/stack/ROADMAP.md:995-998 ('The Hearth REST API exposes only a tip balance with no per-UTXO height, so EMBER records confirmations: 0'); fixing only the site leaves the repo docs contradicting the code. --- ### CF-38 — Delete the invoice/fiat contract from shared-libs and CONTRACTS.md — the endpoints 404 **P2** · drift · `shared-libs/shared` · effort S **What is wrong** forge-pay's invoice path was deleted when the ecosystem went crypto-native: Shards are funded by on-chain deposit only, and pricing.ts:166 records that even the last consumer of `SHARD_PACKAGES` was replaced by a flat constant. But `@cloudsforge/shared@0.4.0` still publishes `SHARD_PACKAGES`, `Invoice`, `InvoiceStatus` and `createInvoiceSchema` — including a `provider: 'mock' | 'nowpayments'` field naming a mock payment provider that no longer exists — and the package's own CONTRACTS.md still tabulates `GET /packages`, `POST /invoices`, `GET /invoices/:id` and `POST /invoices/:id/mock-pay` as live Bearer endpoints with a `MockProvider` toggled by `PAY_PROVIDER=mock`. **Evidence** - `repos/shared-libs/packages/shared/src/pay.ts:29` — `provider: 'mock' | 'nowpayments'` - `repos/shared-libs/packages/shared/CONTRACTS.md:129` — `| POST | `/invoices` | Bearer | `createInvoiceSchema` | 201 `Invoice` |` - `repos/shared-libs/packages/shared/CONTRACTS.md:147` — ``MockProvider` (dev, `PAY_PROVIDER=mock`) returns a fake `payUrl` and enables `/mock-pay`.` - `repos/ninety-days-after/apps/game/src/pages/Store.tsx:19` — `* (`GET /packages`, `POST /invoices`, `POST /invoices/:id/mock-pay`) now 404 —` - `repos/forge-pay/services/pay/src/pricing.ts:166` — `* This used to be derived from the worst package in `SHARD_PACKAGES` — the retail` **How it fails** A new consumer (or a new front-end surface) is written against the published contract: it imports `createInvoiceSchema`, follows CONTRACTS.md, POSTs to /invoices to top a user up, and gets a 404 from forge-pay with no hint that the whole payment model changed. Nothing in the type system or the doc says the path is dead — only a comment inside one game page (Store.tsx:19) does. **Suggested fix** Remove `SHARD_PACKAGES`, `ShardPackage`, `Invoice`, `InvoiceStatus` and `createInvoiceSchema` from packages/shared/src/pay.ts and cut the invoice/provider rows from CONTRACTS.md, releasing as 0.5.0 (a removal is breaking under caret-on-0.x). If CONTRACTS.md cannot be kept honest by hand, generate it from the route tables or delete it — MAP.md:376-380 already says not to trust it, which is an argument for removing it rather than shipping it. **Done when** - [ ] Remove `SHARD_PACKAGES`, `ShardPackage`, `Invoice`, `InvoiceStatus` and `createInvoiceSchema` from packages/shared/src/pay.ts and cut the invoice/provider rows from CONTRACTS.md, releasing as 0.5.0 (a removal is breaking under caret-on-0.x). - [ ] If CONTRACTS.md cannot be kept honest by hand, generate it from the route tables or delete it — MAP.md:376-380 already says not to trust it, which is an argument for removing it rather than shipping it. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. - [ ] Every document asserting the old behaviour is corrected in the same change. > Reviewer note: Accurate, but understates the doc drift in two ways. (1) CONTRACTS.md:1-5 self-declares "Single source of truth for every HTTP endpoint", yet the live funding path is entirely missing — `grep -n deposits repos/shared-libs/packages/shared/CONTRACTS.md` returns nothing, so the Pay table documents four dead invoice endpoints while omitting the /deposits endpoints that Store.tsx actually calls. The fix is therefore delete-and-replace, not just delete. (2) There is a second stale reference outside shared-libs: repos/crucible/packages/contracts/src/index.ts:15 still tells readers "Shards are USD-anchored by SHARD_PACKAGES — see shardsPerUsd() in the pay service", which is now the flat 100/USD constant at forge-pay pricing.ts:163-178. Also worth noting for prioritisation: SHARD_PACKAGES, Invoice and createInvoiceSchema currently have zero importers anywhere in the estate, so this is purely a future-reader hazard with no live breakage. > Reviewer note: Two imprecisions in the finding, both of which understate it: (a) "Nothing in the type system or the doc says the path is dead — only a comment inside one game page (Store.tsx:19)" is wrong. Three other places record the deletion explicitly: `repos/forge-pay/MAP.md` §8 "Retired, and why" (:634-655), `repos/shared-libs/MAP.md` §1.4 "**Dead weight.**" (:134-141, which already names these exact four symbols), and `repos/forge-pay/services/pay/src/pricing.ts:166-181`. The defect is not that the record is missing — it is that the record lives in every file EXCEPT the one PLAN.md:97 tells a consumer to read. (b) The failure scenario should be the reconstruction hazard, not the 404. CONTRACTS.md:147 advertises `MockProvider` / `PAY_PROVIDER=mock` / `/mock-pay` as a supported dev toggle; forge-pay/MAP.md:642-646 records that this exact toggle defaulted to `mock` and let any authenticated user mint unlimited free Shards on the public host. Restoring the documented contract restores the hole. The fix must therefore delete the CONTRACTS.md rows AND leave a one-line "retired, crypto-native only, do not restore" note in their place — not just drop them silently, or the next contract-vs-code diff reads as an accidental omission to be filled back in. --- ### CF-39 — Monitor the 8545 JSON-RPC surface: beacon probes only REST 8645 and nothing asserts chain id 7412 **P2** · unexposed · `stack/beacon` · effort S **What is wrong** Every hearth probe in beacon — `hearth.seed/miner1/miner2`, `hearth.work`, `hearth.supply`, `hearth.liveness`, `hearth.consensus`, `hearth.peering`, `hearth.mempool` — targets the REST port 8645. A recursive grep of `infra/beacon/src` for `8545`, `eth_chainId`, `eth_blockNumber` or `jsonrpc` returns nothing. The 8545 surface is the one MetaMask, Hardhat, the explorer and Forge Pay actually consume, and the hex-vs-decimal `eth_chainId`/`net_version` contract that network-config.md calls the thing that breaks MetaMask is asserted only in a unit test against a stub, never against the running testnet. **Evidence** - `infra/beacon/src/env.js:114` — `hearthSeed: process.env.BEACON_HEARTH_SEED_URL ?? 'http://hearth-testnet-seed:8645',` - `infra/beacon/src/targets.js:274` — `path: '/info',` - `infra/beacon/src/targets.js:275` — `proves: 'the node answers RPC, and reports its height, tip, peers and mempool',` - `docker-compose.yml:462` — `- "127.0.0.1:8545:8545" # Ethereum JSON-RPC — MetaMask's default guess` - `repos/hearth/docs/network-config.md:68` — `**Get it backwards and MetaMask refuses the network outright.**` **How it fails** The JSON-RPC listener fails to bind (port taken, `HEARTH_JSONRPC` typo) or the node comes up on the wrong chain id after a config change. Beacon stays entirely green because `/info` on 8645 still answers with a climbing height. The first person to learn is a user whose MetaMask refuses the network, or Forge Pay silently reading balances from a chain it did not intend. **Suggested fix** Add beacon targets that POST to `http://hearth-testnet-seed:8545/` and assert `eth_chainId === '0x1cf4'`, `net_version === '7412'` (decimal string, not hex), and that `eth_blockNumber` advances. Add a `hearth.consensus`-style probe comparing `eth_getBlockByNumber` state roots across the three nodes, not just heights. **Done when** - [ ] Add beacon targets that POST to `http://hearth-testnet-seed:8545/` and assert `eth_chainId === '0x1cf4'`, `net_version === '7412'` (decimal string, not hex), and that `eth_blockNumber` advances. - [ ] Add a `hearth.consensus`-style probe comparing `eth_getBlockByNumber` state roots across the three nodes, not just heights. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: Two parts of the writeup are imprecise. (a) "asserted only in a unit test against a stub" undersells existing coverage. `repos/hearth/.github/workflows/ci.yml:242-254` has a dedicated step, "The probe boots and answers the chain id in both encodings", that POSTs `eth_chainId` and `net_version` over real HTTP and requires `0x1cf3` and `7411` — that one is against `tools/rpc-probe/stub.js`, but `node/test/evm-rpc.js:80-81` and `node/test/jsonrpc.js:339-340` assert the same contract against a real listener. The accurate gap is not "only a stub" but "never against a deployed node, and never for the testnet's own chain id 7412". (b) The MetaMask half of the failure scenario cannot occur today. 8545/8547/8549 are bound loopback-only (`docker-compose.yml:462, 488, 510`) and the cloudflared RPC ingress is still commented out (`deploy/cloudflared/config.example.yml:98-103`), so no external wallet can reach the endpoint at all. The live consequence is internal and narrower: forge-pay reads EMBER balances over `hearth-testnet-seed:8545`, and a bind failure or chain-id drift there is invisible to every beacon probe. > Reviewer note: The bind-failure half of the finding is fully confirmed and unmitigated, but the chain-id half is overstated in two respects. First, the dominant drift vector is already handled: params.js:54 throws a hard error on an unknown network name rather than defaulting, so a HEARTH_NETWORK typo crashes the node at startup and beacon DOES catch it as node-down; only an explicitly set, valid-but-wrong HEARTH_CHAIN_ID passes validation silently (params.js:47-49). Second, 'Forge Pay silently reading balances from a chain it did not intend' misdescribes the mechanism — a wrong chain id does not redirect RPC to a different chain, so eth_getBalance still returns correct data. Per params.js:22-24 the real harm of a chain-id collision is EIP-155 replay ('every testnet transaction is replayable on mainnet and back... a faucet becomes a way to drain the account it funds') plus withdrawal signatures rejected by ForgeKeyvault as 403 binding_mismatch. The finding should lead with the soft-fail evidence at evmnode.js:211 — combined with the 8645-only docker healthcheck, that is what makes a dead JSON-RPC surface invisible on a path carrying deposit credits and withdrawals. --- ### CF-40 — Correct .env.example and MAP.md: keyvault signs sweeps now, so the withdrawal advice is wrong **P2** · inaccurate · `stack/env + stack/docs` · effort S **What is wrong** Both operator-facing configuration documents state that the sweeper "cannot run until ForgeKeyvault grows a signable 'treasury' purpose (it refuses to sign for the 'deposit' addresses customers pay into)", and conclude that the operator should fund the treasury by hand or turn withdrawals off. That is no longer true: keyvault has a `sweep` EVM shape gated on a pinned treasury, and forge-pay's own env.ts says so explicitly — sweeping is now a decision, not a limitation. Neither file mentions `PAY_TREASURY_TARGET_` or the treasury pin at all, so the two knobs that actually make withdrawals work are undiscoverable from configuration. **Evidence** - `.env.example:77` — `# PAY_SWEEP_ENABLED default false — the sweeper that funds the treasury` - `.env.example:79` — `# 'treasury' purpose (it refuses to sign for the` - `.env.example:88` — `# fund the treasury by hand or set PAY_WITHDRAWALS_ENABLED=false so the refusal` - `MAP.md:194` — `**true** while `PAY_SWEEP_ENABLED` defaults to **false**. ForgeKeyvault refuses to sign for` - `repos/forge-pay/services/pay/src/env.ts:99` — `* The reason used to be that forge-keyvault refused to sign for `purpose: 'deposit'`` - `repos/forge-keyvault/services/forge-keyvault/src/signing.ts:138` — `export type EvmShape = 'creation' | 'transfer' | 'sweep'` **How it fails** An operator sets up a fresh deployment, reads .env.example line 77-89 and MAP.md §7, concludes withdrawals cannot be funded, and sets PAY_WITHDRAWALS_ENABLED=false. Withdrawals are refused for the life of the deployment while the sweeper that would have funded them sits behind an undocumented PAY_SWEEP_ENABLED=true plus a treasury pin the docs never name. **Suggested fix** Rewrite the block in .env.example and MAP.md §7 to describe the current rule: pin a treasury via the admin console (PUT /admin/vault/treasuries), set PAY_SWEEP_ENABLED=true, and optionally PAY_TREASURY_TARGET_EMBER to hold a float. State the blast-radius trade env.ts:120-135 already documents rather than the retired keyvault limitation. **Done when** - [ ] Rewrite the block in .env.example and MAP.md §7 to describe the current rule: pin a treasury via the admin console (PUT /admin/vault/treasuries), set PAY_SWEEP_ENABLED=true, and optionally PAY_TREASURY_TARGET_EMBER to hold a float. - [ ] State the blast-radius trade env.ts:120-135 already documents rather than the retired keyvault limitation. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. - [ ] Every document asserting the old behaviour is corrected in the same change. > Reviewer note: Two corrections. (1) "keyvault signs sweeps now" is too broad: vault.ts:102 gates purpose 'deposit' on SWEEPABLE_FAMILIES = {'evm','ember','xrp'}, and Bitcoin and Solana deposit addresses are still refused pre-decryption because neither has an output policy built. The doc fix must say sweeping works for EVM/Ember/XRP and remains blocked for BTC/SOL, not that the limitation is simply gone. (2) "the two knobs ... are undiscoverable from configuration" is overstated and undercounts the setup. .env.example already points the reader at repos/forge-pay/services/pay/src/env.ts for the rest of Pay's knobs, and that file documents PAY_TREASURY_TARGET_ at length; the stack root's own DEPOSIT-SWEEP.md:509 names it as well. And turning sweeping on takes three things, not two: PAY_SWEEP_ENABLED=true, an optional per-coin PAY_TREASURY_TARGET_, and a treasury pin written into keyvault via PUT /admin/treasuries/:chain/:network under the Nimbus admin credential — which is not an env var at all and so cannot be a ".env.example knob" in the first place. > Reviewer note: Three refinements. (1) Scope is wider than the two cited files: the same false claim appears at BLOCKCHAIN.md:165-166, findings.md:40 and findings.md:72, and DEPOSIT-SWEEP.md:55 states it in present tense — so the design doc a curious operator consults second also misleads, and env.ts:99-105 is the only place the truth is written. All five should be corrected together. (2) The rest of the cited .env.example paragraph is still accurate and should be kept: I verified withdrawer.ts:89-100 that an unfunded withdrawal really is auto-refunded at the stuck deadline because nothing was signed. Only the "cannot run until keyvault grows a signable treasury purpose" clause and the "fund by hand or disable withdrawals" conclusion are wrong. (3) The fix must not become "set PAY_SWEEP_ENABLED=true". env.ts:120-131 holds a deliberate property — sweeping stays off by default and treasuryTargets defaults to zero, because every coin in the treasury is inside the service token's blast radius while a coin left in a deposit address is outside it. The rewrite should document three steps (pin the treasury via the Nimbus admin PUT, set PAY_SWEEP_ENABLED=true, optionally set PAY_TREASURY_TARGET_ to trade custody exposure for first-withdrawal latency) and present it as the custody decision it is. --- ### CF-41 — Key Lantern's ingest quota on the forwarded client IP, not the tunnel's socket address **P2** · broken · `stack/lantern` · effort S **What is wrong** The quota is keyed on `req.socket.remoteAddress`, which for every public request is the cloudflared connector, not the browser. Lantern reads no `x-forwarded-for` anywhere (grep over infra/lantern returns nothing). So the documented 'per-IP, per-minute' limit is in practice one global bucket of 120 requests per minute shared by every browser on the internet, and the browser client is capped at 40 distinct reports per session precisely so it can afford to report. **Evidence** - `infra/lantern/src/server.js:57` — `// A misbehaving page must not be able to fill the store. Per-IP, per-minute.` - `infra/lantern/src/server.js:74` — `const ip = req.socket.remoteAddress ?? 'unknown'` - `infra/lantern/src/server.js:68` — `return slot.count > 120` - `deploy/cloudflared/config.example.yml:70` — `service: http://localhost:4010` **How it fails** A bad release makes the admin console throw on boot for 60 concurrent users. Each browser sends two or three reports; the shared bucket is exhausted in seconds and Lantern 429s the rest. The one incident the client-side reporting exists to capture is the one whose reports get dropped — and because `report()` swallows failures by design (web-obs.tsx:111-114), nothing anywhere records that they were dropped. **Suggested fix** Read the leftmost entry of `x-forwarded-for` (or `cf-connecting-ip`) when the socket peer is a trusted proxy address, and fall back to `req.socket.remoteAddress` otherwise. Add a counter for quota rejections to `/health` so the drop is visible rather than silent. **Done when** - [ ] Read the leftmost entry of `x-forwarded-for` (or `cf-connecting-ip`) when the socket peer is a trusted proxy address, and fall back to `req.socket.remoteAddress` otherwise. - [ ] Add a counter for quota rejections to `/health` so the drop is visible rather than silent. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: Two refinements. First, the scenario understates the reach: the bucket is not only exhaustible by accident during an incident, it is exhaustible on purpose by anyone on the internet at ~2 requests/second against an unauthenticated endpoint, and it stays exhausted for as long as they keep going. Second, the "nothing records that they were dropped" claim is stronger than stated — Beacon does have a 429 assertion on this exact endpoint, but it ingests from the compose network (`http://lantern:4010`), so it lands in a different bucket and structurally cannot observe the public one being saturated. Minor imprecision in the finding: the browser's 40-report cap is documented as self-restraint against DDoSing the log service, not as an accommodation of the server quota. > Reviewer note: Two fixes. (1) The `MAX_REPORTS = 40` cap is not there "so it can afford to report" against the shared bucket — the comment at web-obs.tsx:75-77 says it exists so one broken render firing hundreds of times a second cannot DDoS the log service; the auditor's causal link is invented and should be dropped. (2) The finding understates the impact by framing it only as an incident-storm scenario. `/ingest/client` is routed at server.js:182 *before* `authorise()`, so the shared bucket is also exhaustible by any unauthenticated caller on the internet: a script doing 120 POSTs/min silently disables browser error reporting for the entire estate for as long as it runs. That, not the 60-concurrent-users case, is the sharpest form of the bug. --- ### CF-42 — Expose a way to mint a second treasury address, or the rotation UI can never be used **P3** · unexposed · `forge-keyvault/forge-keyvault` · effort M **What is wrong** Rotation is fully built on the keyvault side (previous/current in `pinTreasury`, a `treasury_rotated` audit line, `supersededAddress` in the response) and on the console side (a candidate ``, a ROTATE confirmation modal). It requires a second `treasury`-purpose address on the same chain and network, because `pinTreasury` refuses anything else. Nothing operator-facing can produce one: keyvault's only minting route is `POST /addresses` under the service token, Nimbus's vault proxy relays no such route, and forge-pay's `planTreasury` mints only when it holds no row at all. So the candidate list always has exactly one entry, `unchanged` is always true, and the Rotate button is permanently disabled. **Evidence** - `repos/forge-keyvault/services/forge-keyvault/src/routes/admin.ts:137` — `const superseded = result.previous` - `repos/platform/apps/admin/src/pages/Vault.tsx:452` — `const unchanged = choice === row.pin?.address` - `repos/platform/apps/admin/src/pages/Vault.tsx:430` — `No treasury addresses exist yet. Forge Pay mints one per chain the first time it needs` - `repos/forge-pay/services/pay/src/treasury.ts:312` — `if (row) return { action: 'use-row', row }` - `repos/forge-keyvault/services/forge-keyvault/src/store.ts:129` — `if (!row) return { refusal: 'address_unknown' } if (row.purpose !== 'treasury') return { refusal: 'address_not_treasury' }` **How it fails** A treasury key is suspected compromised, or an operator wants to move custody off a key that was revealed through `/admin/keys/:address/reveal`. They open the Vault page, find the chain's row, and the only selectable address is the one already pinned — Rotate is greyed out and the modal is unreachable. The documented remedy (mint a new treasury, move the balance, re-pin, delete the old forge-pay row) cannot start from any UI; it needs a raw curl to loopback:4005 with the shared service token plus hand-editing forge-pay's Postgres row. Meanwhile sweeps keep consolidating customer deposits into the address they are trying to abandon. **Suggested fix** Add an admin-credentialed mint on keyvault (`POST /admin/treasuries/:chain/:network/mint`, purpose fixed to 'treasury', chain/network from the path, orderId synthesized) and relay it through the Nimbus vault proxy alongside the pin write. That gives the rotation flow its missing first step under the same credential that already owns the pin, without widening what the service token can create. **Done when** - [ ] Add an admin-credentialed mint on keyvault (`POST /admin/treasuries/:chain/:network/mint`, purpose fixed to 'treasury', chain/network from the path, orderId synthesized) and relay it through the Nimbus vault proxy alongside the pin write. - [ ] That gives the rotation flow its missing first step under the same credential that already owns the pin, without widening what the service token can create. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: Two imprecisions. (1) "forge-pay's planTreasury mints only when it holds no row at all" understates the closure: planTreasury returns `adopt` whenever a pin exists (treasury.ts:311-314), so once a chain is pinned forge-pay will never mint there again even if its treasury_addresses row is deleted by hand. (2) "the rotation UI can never be used" / "permanently disabled" is too strong — the select and modal light up the moment a second treasury-purpose address exists on that (chain, network), and creating one is a single curl to loopback:4005 on the same host session a rotation already requires. forge-pay/services/pay/src/treasury.ts:370-408 is an explicit, argued decision that the balance-move step deliberately has NO route in this estate and is a hand-built POST /sign plus eth_sendRawTransaction; so the operator is already at a shell for the step that actually costs something, and the missing mint route is the cheapest link in a procedure that is CLI-bound by design. The real defect is narrower: the console offers a rotation affordance whose precondition it gives the operator no way to reach and no text explaining where a second candidate comes from. > Reviewer note: Two sharpenings. (1) The finding says forge-pay "mints only when it holds no row at all", which understates the blockage: with no row but a pin present, `planTreasury` returns `adopt`, not `mint` (treasury.ts:310-313). So deleting forge-pay's Postgres row does not coax a second address out of it either — it re-adopts the already-pinned one. There is no sequence of operator actions through any product surface that produces a second candidate. (2) The manual workaround is costlier than "a raw curl plus hand-editing a row": the hand-minted address must carry `orderId: treasury::` and `userId: cloudsforge:treasury` exactly, or forge-pay's `adopt` path records a binding keyvault refuses at signing time, and every subsequent withdrawal fails with 403 `binding_mismatch` (warned about in treasury.ts's adopt branch). --- ### CF-43 — Remove or wire up EVM_RPC_URL / SOLANA_RPC_URL — they cannot take effect **P3** · drift · `forge-mint/forge-mint` · effort S **What is wrong** `resolveNetwork`'s precedence is `RPC__` → `baked` → generic fallback, but every entry in `SUPPORTED_CHAINS` has a non-empty `mainnetRpc` and `testnetRpc`, so `baked` is always truthy and the `genericFallback` arm is dead code. `EVM_RPC_URL` and `SOLANA_RPC_URL` therefore have no effect on any deploy or balance read, yet they are the only RPC knobs present in the stack's `.env.example` and are listed as live configuration in MAP.md:405. The knob that does work — `RPC__` — appears in `.env.example` nowhere at all. **Evidence** - `repos/forge-mint/services/forge-mint/src/chain/networks.ts:30` — `process.env[overrideKey]?.trim() || baked || (isMainnet ? baked : genericFallback)` - `repos/forge-mint/services/forge-mint/src/chain/networks.ts:26` — `const baked = isMainnet ? chain.mainnetRpc : chain.testnetRpc` - `repos/shared-libs/packages/shared/src/forgemint.ts:47` — `testnetRpc: 'https://ethereum-sepolia-rpc.publicnode.com',` - `repos/forge-mint/services/forge-mint/src/env.ts:102` — `evmRpcUrl: process.env.EVM_RPC_URL ?? 'https://ethereum-sepolia-rpc.publicnode.com',` - `.env.example:131` — `EVM_RPC_URL=https://ethereum-sepolia-rpc.publicnode.com` - `repos/forge-mint/MAP.md:405` — ``NIMBUS_ISSUER`, `PAY_API_URL`, `KEYVAULT_URL`, `EVM_RPC_URL`, `SOLANA_RPC_URL`,` **How it fails** An operator enabling mainnet deploys sets `EVM_RPC_URL=https://mainnet.infura.io/v3/` in `.env`, as `.env.example:131` and MAP.md:405 invite. Every deploy silently continues to use `https://ethereum-rpc.publicnode.com`; under a public node's rate limit `estimateGas` fails and customers get `gas estimate failed` 409s that no configuration change appears to fix. **Suggested fix** Delete `evmRpcUrl`/`solanaRpcUrl` from `env.ts` and the dead third arm of the expression in `networks.ts:30`, drop the two vars from `.env.example` and MAP.md:405, and document `RPC_ETHEREUM_MAINNET` / `RPC_SOLANA_MAINNET` etc. in `.env.example` instead — with a note that they are usually keyed provider URLs, which is why `safeUrl` exists. **Done when** - [ ] Delete `evmRpcUrl`/`solanaRpcUrl` from `env.ts` and the dead third arm of the expression in `networks.ts:30`, drop the two vars from `.env.example` and MAP.md:405, and document `RPC_ETHEREUM_MAINNET` / `RPC_SOLANA_MAINNET` etc. in `.env.example` instead — with a note that they are usually keyed provider URLs, which is why `safeUrl` exists. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. - [ ] Every document asserting the old behaviour is corrected in the same change. > Reviewer note: Two framing fixes. (1) "Wire up" is not a valid resolution -- EVM_RPC_URL is a single value for five distinct EVM chains, so giving it precedence over `baked` would route a Polygon or BSC deploy through an Ethereum endpoint with real gas. The current shape (fallback only when `baked` is empty) is the safe one; only removal is correct, and the per-chain RPC__ knob is the right design and already functional. (2) The docs are less misleading than stated: MAP.md:407-408 names RPC__ immediately after the quoted line, and CONTRACTS.md:219 documents it too. The genuinely false artifact is .env.example:130's comment "EVM/Solana testnet RPCs (used for balance checks + testnet deploys)", which describes a use that does not exist, sitting next to the only two RPC knobs a reader of that file will ever see. Fix: delete evmRpcUrl/solanaRpcUrl from env.ts:102-103 and networks.ts:27,30, and replace the two .env.example lines with a commented RPC_ETHEREUM_MAINNET= example. > Reviewer note: The failure scenario is wrong and should be replaced. Mainnet was never meant to consume EVM_RPC_URL: the `isMainnet ? baked : genericFallback` ternary excludes it by design, networks.ts:21 says "fallback (testnet only)", CONTRACTS.md:219 says "EVM_RPC_URL / SOLANA_RPC_URL are testnet fallbacks", and .env.example:130 — the comment immediately above the cited line — reads "# EVM/Solana testnet RPCs (used for balance checks + testnet deploys)" with a Sepolia value. Neither .env.example nor MAP.md invites an operator to set a mainnet Infura URL there, and fixing the dead-code bug would not make that URL reach a mainnet deploy. The accurate defect: even on testnet, where the fallback IS intended to fire, it cannot, because every chain has a non-empty testnetRpc — so an operator hitting public-node rate limits on Sepolia/Devnet sets EVM_RPC_URL, sees no change, and the knob that does work (RPC_ETHEREUM_TESTNET) is documented only in MAP.md:407 and CONTRACTS.md:254, never in .env.example. The .env.example comment claiming these drive "balance checks + testnet deploys" is itself false and should be part of the fix. --- ### CF-44 — MAP.md §9 still says ETH cannot be sent, which the same file and outbound.ts contradict **P3** · drift · `forge-pay/pay` · effort S **What is wrong** `MAP.md` §9 — the section that exists specifically to be the authoritative list of what this service does not do — states that ETH cannot be sent and that custodied ETH is permanently inside the platform. `WITHDRAWABLE_COINS` includes ETH, and §7 of the same document (around line 596-600) already records that "ETH left that list when keyvault split its EVM signing rule by purpose". The document contradicts itself on the one question §9 exists to answer, and the stale half understates the platform's on-chain exposure. **Evidence** - `repos/forge-pay/MAP.md:664` — `- **It cannot send BTC, ETH or SOL.** Custodied balances in those three coins are, today,` - `repos/forge-pay/MAP.md:599` — `instead of failing at submit. A user holding custodial BTC still has no way to get it out of` - `repos/forge-pay/services/pay/src/outbound.ts:62` — `export const WITHDRAWABLE_COINS = (['EMBER', 'ETH', 'XRP'] as DepositCoin[]).filter(` **How it fails** An operator or reviewer reads §9 to establish the outbound blast radius before funding a treasury and concludes that a leaked `PAY_SERVICE_TOKEN` can only reach EMBER and XRP, when the ETH treasury is equally payable to any destination a user names. The correct sentence is three hundred lines earlier in the same file. **Suggested fix** Change §9 to "It cannot send BTC or SOL" and state, in the same bullet, that BTC and SOL deposits can also never be swept (forge-keyvault's `SWEEPABLE_FAMILIES` is `{evm, ember, xrp}`), so those balances are unreachable by the platform as well as by the user. **Done when** - [ ] Change §9 to "It cannot send BTC or SOL" and state, in the same bullet, that BTC and SOL deposits can also never be swept (forge-keyvault's `SWEEPABLE_FAMILIES` is `{evm, ember, xrp}`), so those balances are unreachable by the platform as well as by the user. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. - [ ] Every document asserting the old behaviour is corrected in the same change. > Reviewer note: The failure scenario overstates the reach of a leaked `PAY_SERVICE_TOKEN`. `/internal` exposes only `GET /internal/wallet/:userId`, `POST /internal/charge`, `POST /internal/credit` and `POST /internal/trade` (`services/pay/src/routes/internal.ts`) — there is no internal withdrawal route, and `POST /withdrawals` is behind `requireAuth` (user JWT). A token holder can mint unlimited Shards and trade them into an ETH coin balance with no reserve check, but still needs a user session to name a destination and pull the ETH out; the token alone does not reach the treasury. The accurate statement of the harm is narrower: §9 understates the set of coins that can leave the platform at all (it omits ETH, and its "EMBER … is the only exit" clause also omits XRP), so anyone using §9 as the outbound inventory sizes the on-chain blast radius short by the ETH and XRP treasuries. > Reviewer note: The §9 bullet is stale in one more way the finding did not catch: after the ETH claim it says the custodied balances "can be converted to Shards or to EMBER, and the EMBER can be withdrawn — which is the only exit," but XRP is also in WITHDRAWABLE_COINS, so EMBER is not the only exit. The corrected bullet should read that only BTC and SOL cannot be sent (with the PSBT and SPL-only reasons already given in outbound.ts:54-57), and that EMBER, ETH and XRP all leave the platform on chain. --- ### CF-45 — XRP mark rebase can collide the synthetic deposit txid and latch the address on `duplicate` **P3** · broken · `forge-pay/pay` · effort S · user-visible **What is wrong** The synthetic deposit txid is derived from `observed`, and its uniqueness rests entirely on `observed` being strictly greater than every previously credited value — true because `last_seen` is a high-water mark. The XRP `MARK_BASIS` restatement is the one mechanism that lowers `last_seen`, by exactly one base reserve. After it fires, a later `observed` can equal a value already recorded against that address on the old basis. The insert then conflicts, `recordDepositAndCredit` returns `duplicate` without crediting and without advancing `last_seen`, and every subsequent tick recomputes the identical `observed` and returns `duplicate` again. There is no route that clears it. **Evidence** - `repos/forge-pay/services/pay/src/store.ts:1157` — `const txid = `${row.coin}:${row.address}:${observed.toString()}`` - `repos/forge-pay/services/pay/src/store.ts:1108` — `...(restate ? { lastSeen: observed.toString(), pending: '0' } : {}),` - `repos/forge-pay/services/pay/src/store.ts:1180` — `if (inserted.length === 0) return { kind: 'duplicate' }` - `repos/forge-pay/services/pay/src/db/migrate.ts:108` — `CREATE UNIQUE INDEX IF NOT EXISTS deposit_payments_addr_txid_idx` **How it fails** An XRP address was credited 20 XRP before the reserve change (`last_seen = 20000000`, a `deposit_payments` row with txid `XRP:rAbc:20000000`). The rebase restates the mark to `10000000` (reserve 10 XRP). The user then deposits exactly 10 more XRP: balance 30, creditable 20, so `observed = 20000000` and the txid is `XRP:rAbc:20000000` — already present. The route returns `duplicate`, the 10 XRP is never credited, `last_seen` stays at 10000000, and the address logs `this total was already recorded but the high-water mark is behind it` at warn on every tick forever. **Suggested fix** Stop deriving the payment identity from `observed` alone — include the mark basis (or a monotonic per-address sequence) in the synthetic txid, e.g. `${coin}:${address}:${basis ?? 'v0'}:${observed}` — and treat a `duplicate` whose stored payment predates the current `mark_basis` as a credit rather than a refusal. **Done when** - [ ] Stop deriving the payment identity from `observed` alone — include the mark basis (or a monotonic per-address sequence) in the synthetic txid, e.g. - [ ] `${coin}:${address}:${basis ?? 'v0'}:${observed}` — and treat a `duplicate` whose stored payment predates the current `mark_basis` as a credit rather than a refusal. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: Two overstatements. (a) "There is no route that clears it" / "forever" is wrong: the latch is not permanent. It persists only while `observed` stays pinned on the collided value; the very next deposit (any amount) makes `observed` exceed every prior mark, mints a fresh txid, and credits `delta = observed - lastSeen`, which includes the previously stalled amount. Recording a manual sweep (`POST /admin/deposit-addresses/:id/sweeps`) also raises `swept` and therefore `observed`, unlatching it the same way. No funds are permanently lost — this is a stall in crediting one deposit, loudly logged at warn every tick, that self-heals on the next balance change. (b) Reachability is much narrower than "a later `observed` can equal a value already recorded": because the restatement lowers the mark by exactly one base reserve R, collision requires the next total to land *exactly* on a prior mark — i.e. a subsequent deposit of exactly R drops (1 XRP mainnet, 10 XRP testnet), or exactly R minus the gap to an intermediate mark within R of the top. Any other deposit size skips over the recorded value and credits normally. It also only applies inside the one-time migration window, to XRP addresses that were already credited on the old basis. > Reviewer note: Two precisions to the finding. (1) "There is no route that clears it" is true of the HTTP surface but the latch is not permanent: any subsequent deposit that moves `observed` past the collision credits the full delta from the restated mark, so the stalled amount is eventually paid — the damage is an indefinite stall plus swept-to-treasury-while-uncredited, not a loss. (2) The collision window is narrower than "a later `observed` can equal a value already recorded": because the mark restates to T_m - R, the only reachable old txids are totals within one reserve of the last one, so in practice there is a single collision point per address — a post-rebase net top-up of exactly the base reserve in drops. --- ### CF-46 — Correct the pay-proxy rationale in two files: forge-pay's CORS now allows PUT **P3** · drift · `platform/nimbus` · effort S **What is wrong** Both the Nimbus pay proxy and the admin API client justify their existence with a claim about forge-pay that is no longer true: that pay registers CORS with no `methods`, therefore answers preflights with `GET,HEAD,POST`, therefore cannot accept the price PUT from a browser, and that "widening that list is a change to a repo this one does not own." forge-pay's index.ts now states the method list explicitly and includes PUT, with its own comment describing the same bug being fixed on that side. The proxy still works and is still defensible on other grounds (one origin, one auth path); the stated reason is stale. **Evidence** - `repos/platform/services/nimbus/src/routes/pay.ts:20` — `* access-control-allow-methods: GET,HEAD,POST` - `repos/platform/apps/admin/src/lib/api.ts:550` — `// `access-control-allow-methods: GET,HEAD,POST`, so no browser will send it, and` - `repos/forge-pay/services/pay/src/index.ts:31` — `methods: ['GET', 'HEAD', 'OPTIONS', 'POST', 'PUT'],` **How it fails** An engineer needs to add the withdrawal-abandon action to the console. They read the comment in apps/admin/src/lib/api.ts, conclude that non-GET calls to pay cannot come from a browser and that forge-pay cannot be changed from here, and either add a second proxy layer or abandon the work — when a direct POST from the admin origin has been allowed by pay's CORS config all along. **Suggested fix** Rewrite the header comment in services/nimbus/src/routes/pay.ts and the note above `listAdministeredPrices` in apps/admin/src/lib/api.ts to cite the real current reason (single origin, single auth path, attribution on pay's side), and drop the assertion about the preflight method list. **Done when** - [ ] Rewrite the header comment in services/nimbus/src/routes/pay.ts and the note above `listAdministeredPrices` in apps/admin/src/lib/api.ts to cite the real current reason (single origin, single auth path, attribution on pay's side), and drop the assertion about the preflight method list. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. - [ ] Every document asserting the old behaviour is corrected in the same change. > Reviewer note: Four sites carry the stale claim, not two: add repos/platform/services/nimbus/src/env.ts:133-136 and repos/platform/MAP.md:323-327 to the cited pay.ts and api.ts. The failure scenario should be restated: an engineer adding withdrawal-abandon is not misdirected, because pay.ts's closing paragraph names this proxy as the destination. The accurate harm is that the proxy's stated justification is false in four decision records, so someone auditing whether it should exist may delete it and lose the single-origin/single-auth-path property — and the accompanying "widening that list is a change to a repo this one does not own" is doubly wrong, since the list was already widened and forge-pay is checked out in this same working tree. > Reviewer note: Two details in the finding are off. (1) "when a direct POST from the admin origin has been allowed by pay's CORS config all along" is wrong — the PUT really was refused until forge-pay commit f3eb408 on 2026-07-28; the comments were accurate when written and only drifted a day ago. (2) The failure scenario's example is weak: withdrawal-abandon is POST /admin/withdrawals/:id/abandon (repos/forge-pay/services/pay/src/routes/admin.ts:335), and POST was allowed even under the old @fastify/cors default — the api.ts comment explicitly lists POST as permitted, so an engineer would not conclude that POST is blocked. The real harm is narrower: both comments state a fact about forge-pay that is no longer true and claim forge-pay cannot be changed from here, which could push someone toward a second proxy layer for a future PUT/DELETE instead of one line of config. Fix is a comment edit in both files. --- ### CF-47 — Derive the portal origin from the configured apex instead of hardcoding account.cloudsforge.online **P3** · drift · `platform/nimbus` · effort S **What is wrong** env.ts unconditionally appends the literal `https://account.cloudsforge.online` to the CORS allowlist. The apex is configurable everywhere else — compose builds every origin from `${CLOUDSFORGE_APEX:-cloudsforge.online}` — so on any deployment with a different apex this adds a foreign origin that the operator does not control to a credentialed CORS allowlist, while *not* adding the `account.` the .env.example promises is "always allowed". The compose comment at :57-61 makes the point explicitly: nimbus carrying its own copy of the origin list is exactly what drifted last time. **Evidence** - `repos/platform/services/nimbus/src/env.ts:31` — `if (!corsOrigins.includes('https://account.cloudsforge.online')) {` - `repos/platform/services/nimbus/src/env.ts:32` — `corsOrigins.push('https://account.cloudsforge.online')` - `repos/platform/services/nimbus/.env.example:26` — `# wildcard. account. is always allowed on top of whatever is set here.` - `docker-compose.yml:67` — `x-cors-origins: &cors-origins ${CORS_ORIGINS:-...https://account.${CLOUDSFORGE_APEX:-cloudsforge.online}}` **How it fails** An operator deploys with CLOUDSFORGE_APEX=forge.example and sets CORS_ORIGINS by hand without account.forge.example, trusting the .env.example line that says it is added automatically. It is not — the portal's own subdomain is missing from CORS — while https://account.cloudsforge.online, a host on somebody else's domain, is granted `credentials: true` cross-origin access to the identity API. **Suggested fix** Drop the hardcoded push and derive it: read `CLOUDSFORGE_APEX` and append `https://account.${apex}` when set, or drop the special case entirely since docker-compose.yml already puts `https://account.${CLOUDSFORGE_APEX}` in CORS_ORIGINS. Then fix .env.example:26 to describe whichever behaviour survives. **Done when** - [ ] Drop the hardcoded push and derive it: read `CLOUDSFORGE_APEX` and append `https://account.${apex}` when set, or drop the special case entirely since docker-compose.yml already puts `https://account.${CLOUDSFORGE_APEX}` in CORS_ORIGINS. - [ ] Then fix .env.example:26 to describe whichever behaviour survives. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. - [ ] Every document asserting the old behaviour is corrected in the same change. > Reviewer note: Two refinements. (1) The two halves have different blast radii: the stray foreign origin is added on EVERY deployment whose apex is not cloudsforge.online, but the missing account. only bites when the operator hand-sets CORS_ORIGINS — compose's default at :67 already interpolates https://account.${CLOUDSFORGE_APEX}, so the shipped path is covered. The failure scenario states this correctly; the problem paragraph overstates it as unconditional. (2) The title's remedy is heavier than needed: nimbus reads no CLOUDSFORGE_APEX today, so "derive from the configured apex" means plumbing a new variable into the service. The cheaper fix that matches docker-compose.yml:57-61's own argument is to delete the push entirely (compose already supplies account. in both allowlists) and correct .env.example:26 to stop promising it. > Reviewer note: Keep the finding but restate the impact. The harm is not "the portal loses CORS" (the portal is served by nimbus and uses same-origin relative fetches, so it needs no CORS entry) and not "credentialed access to the identity API" (nimbus issues no cookies; tokens are bearer values in per-origin localStorage, so credentials:true carries nothing for a foreign origin). The harm is that corsOrigins feeds portalAllowedOrigins by default (env.ts:39-41): on any run that does not set PORTAL_ALLOWED_ORIGINS — services/nimbus/.env.example:31 leaves it commented, though docker-compose.yml:151 always sets it — a foreign-apex deployment will accept /login?return=https://account.cloudsforge.online/... and hand a live single-use SSO exchange code to a domain the operator does not control. Today the append is an exact no-op in the only deployed configuration, because the compose default already contains that origin. --- ### CF-48 — Fix Beacon's incident rendering to read camelCase fields — timestamps vanish exactly when Postgres is down **P3** · broken · `stack/beacon` · effort S · user-visible **What is wrong** In-memory incidents are camelCase (`openedAt`, `closedAt`, `lastError`); Postgres rows are snake_case. `/api/status` always returns the in-memory objects, and `/api/incidents` falls back to them whenever `dbHealthy()` is false. The UI reads `inc.opened_at` and `inc.closed_at` in both the banner (app.js:421) and the incidents view (app.js:1550, 1563, 1572, 1577), so against the in-memory shape those are `undefined`: `since()` returns '—' and `fmtDateTime(undefined)` renders 'Invalid Date'. The sort at app.js:1550 also collapses, since both keys parse to NaN. **Evidence** - `infra/beacon/src/incidents.js:28` — `lastError: error ?? null, openedAt: at, closedAt: null,` - `infra/beacon/src/server.js:159` — `incidents: openIncidents(),` - `infra/beacon/src/server.js:456` — `: openIncidents()` - `infra/beacon/public/app.js:421` — ``${inc.subject ?? inc.scope ?? 'unknown'} · ${inc.cause ?? 'cause unrecorded'} · ${since(inc.opened_at)}`))` - `infra/beacon/public/app.js:1572` — `head.append(el('span', 'inc-when', `${since(inc.opened_at, inc.closed_at)}${open ? ' and counting' : ''}`))` - `infra/beacon/public/app.js:182` — `if (!Number.isFinite(a) || !Number.isFinite(b)) return '—'` **How it fails** Postgres goes down. Six services fall over, Beacon opens six incidents in memory, and the operator opens the incidents tab — the view whose whole justification is that it works with the database gone. Every row reads '— and counting' with 'opened Invalid Date', in arbitrary order, so there is no way to tell which service went first. That ordering is the single most useful fact in a cascade. **Suggested fix** Normalise at one boundary. Either have `openIncidents()` emit snake_case keys matching the `incidents` table, or map DB rows to camelCase in `db.js` and change the six UI reads. One shape, chosen once — the same rule the rest of the file applies to targets. **Done when** - [ ] Normalise at one boundary. - [ ] Either have `openIncidents()` emit snake_case keys matching the `incidents` table, or map DB rows to camelCase in `db.js` and change the six UI reads. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: The in-memory/snake_case mismatch is real and, for the incident banner, is live in every deployment (not just when Postgres is down) because /api/status always returns the in-memory shape — including the public redacted view. However: `fmtDateTime(undefined)` returns '—', not 'Invalid Date' (app.js:157), so nothing renders corrupted; and the sort does not collapse to arbitrary order — a NaN comparator is a stable no-op in V8, so rows appear in Map insertion order, i.e. true chronological open order, merely inverted relative to the DB view's `opened_at DESC`. The cascade ordering is also independently available and correct on the Status tab via per-target `since` (store.js:132 → server.js:114 → app.js:547,598), which is likewise memory-backed and works with Postgres down. The finding additionally missed that `inc.last_error` (app.js:1578) is dropped for in-memory incidents. Net effect: missing durations/timestamps/error text on one view during a DB outage plus a silently reversed reading order, with a correct alternate source in the same UI. > Reviewer note: The defect is the camelCase/snake_case mismatch, but its effect is narrower and differently scoped than described. Corrected: the incident banner is broken at all times, not only during a Postgres outage, because /api/status (server.js:159) always returns the in-memory camelCase objects — since(inc.opened_at) at app.js:421 renders '—' unconditionally. During a Postgres outage the incidents view additionally loses the open duration ('— and counting'), the timestamps ('opened —', not 'Invalid Date' — fmtDateTime at app.js:157 returns '—' for undefined), and the error text (app.js:1578 reads inc.last_error against the in-memory lastError). Ordering is NOT lost: the comparator at app.js:1550 returns NaN, treated as +0 by SortCompare, so the stable sort preserves the Map insertion order from openIncidents(), which is the order the incidents were opened — the list is chronologically ascending rather than the intended descending. Severity, scope, subject and cause all still render correctly. --- ### CF-49 — Add Beacon to the shared surface registry so something actually links to the status page **P3** · unexposed · `stack/docker-compose + shared-libs` · effort S · user-visible **What is wrong** `https://beacon.` and `http://localhost:4011` are on both the CORS allowlist and the return-URL allowlist, whose stated purpose is "a page a user signs into ... linked from the product switcher on every front-end". But `beacon` appears nowhere in repos/shared-libs/packages/shared/src/products.ts — grep returns zero hits — so it is not in SURFACES, not in SWITCHER_SURFACES and not in KNOWN_SUBS. Lantern got a registry entry for exactly this reason; Beacon did not. **Evidence** - `docker-compose.yml:75` — `# Lantern (:4010) is on this list for the same reason ForgeMint and Crucible` - `docker-compose.yml:79` — `x-portal-allowed-origins: &portal-allowed-origins ${PORTAL_ALLOWED_ORIGINS:-http://localhost:3000,...,https://beacon.${CLOUDSFORGE_APEX:-cloudsforg…` - `repos/shared-libs/packages/shared/src/products.ts:302` — `export const SWITCHER_SURFACES: readonly CloudsForgeSurface[] = SURFACES.filter(` - `deploy/cloudflared/config.example.yml:81` — `- hostname: beacon.cloudsforge.online` **How it fails** An operator signs into the admin console during an incident and opens the product switcher looking for the status page. Beacon is not there, and no front-end anywhere in the estate links to it, so the monitor is reachable only by typing beacon. from memory. **Suggested fix** Add a `beacon` entry to SURFACES (subdomain 'beacon', devPort 4011, adminOnly, inSwitcher: true) alongside the existing lantern entry, republish @cloudsforge/shared and @cloudsforge/ui, and bump the consumers. **Done when** - [ ] Add a `beacon` entry to SURFACES (subdomain 'beacon', devPort 4011, adminOnly, inSwitcher: true) alongside the existing lantern entry, republish @cloudsforge/shared and @cloudsforge/ui, and bump the consumers. - [ ] The scenario under *How it fails* is reproduced, then shown not to occur. > Reviewer note: Two claims in the finding should be softened. (1) "no front-end anywhere links to it, so the monitor is reachable only by typing beacon. from memory" — the hostname is documented in MAP.md §3 (line 113) and throughout TESTING.md, and a bookmark is the normal path; the registry entry would also be `adminOnly`, so it would only appear for a viewer the switcher already knows is an admin. (2) The incident framing is the weakest part: Beacon is deliberately built to be readable when the estate is down (no depends_on, break-glass BEACON_TOKEN in infra/beacon/src/auth.js), whereas the switcher is served by the very front-ends and admin-role check that an outage can take out — so the switcher entry helps everyday navigation, not the outage case the scenario describes. The real content of the finding is registry/consistency drift (Beacon is the one hostname on both allowlists with no registry row), not lost reachability. > Reviewer note: The KNOWN_SUBS half of the claim is true but inert: infra/beacon/public/app.js derives no apex from window.location.hostname (it uses location.origin for API calls and a server-supplied config.accountUrl for sign-in, app.js:39 and :102-103), and no other front-end is served on beacon., so the missing KNOWN_SUBS entry has no runtime effect. The live defect is only the absent SURFACES/SWITCHER_SURFACES entry. Also note the fix is cross-repo: shared-libs is its own git checkout, so it needs a SurfaceKey addition, a publish, and consumer bumps. Evidence line 75 is off by one (the quoted comment starts at line 74). --- ## What was killed Twenty-one findings were raised and did not survive review — wrong, already fixed, or a deliberate decision. They are listed by title only, so the register reads as the narrow end of a wider survey rather than as everything anyone noticed: - `stack/lantern` — Authenticate or Origin-enforce /ingest/client — the CORS allowlist does not gate writes, only reads - `stack/beacon` — Make Beacon's headline verdict account for failing journeys, not just probe targets - `stack/observability` — Add a CI check that the vendored obs copies match the canonical — one copy has already drifted - `platform/admin` — Render `pinned`/`pinAgrees` on the console Treasury panel — the sweep-vs-payout mismatch is invisible - `stack/docker-compose` — Bind Lantern, Beacon and the other published ports to 127.0.0.1 — the tunnel already dials loopback - `stack/docker-compose` — Drop the pay dependency from crucible and forge-mint — a dead chain node currently stops them booting - `stack/env + platform/nimbus` — Document NIMBUS_VAULT_REVEAL_PROXY in .env.example — the console's Reveal button is otherwise permanently dead - `forge-keyvault/forge-keyvault` — Stop calling the /sign binding a check with entropy — every field of it is publicly derivable - `forge-keyvault/forge-keyvault` — Bind XRP signatures to a network, or stop recording `network` on XRP rows as if it did - `forge-keyvault/forge-keyvault` — Say that `purpose` picks a signing shape on EVM only — on bitcoin and xrp a deployer spends freely - `platform/nimbus` — An admin-issued reset link is an impersonation back door, and the comment above it says it is not - `forge-pay/pay` — Narrow trustProxy in the shared obs.ts: pay, crucible, forge-mint and keyvault still trust any X-Forwarded-For - `hearth/node` — Bound node memory and restart time before a public testnet: all state is an in-process Map rebuilt by full replay - `stack/compose` — Decide and build the public RPC ingress: no tunnel route, no TLS, no rate limit, no batch cap - `hearth/tools` — Deploy the faucet and fix its default endpoint — it defaults to the REST port and mainnet chain id - `hearth/node` — Decide mainnet key custody: the node writes a plaintext coinbase key and the Commons has no owner - `hearth/docs` — Settle the irreversible ethereum-lists/chains fields before filing: shortName, currency name, icon, explorer - `stack/deploy` — Publish rpc. — phase 6 shipped and the ingress rule is still commented out - `hearth/tools` — The Etherscan proxy shim advertises two eth_* actions the node returns -32601 for - `hearth/node` — Key the /mining/template candidate memo per requester with an LRU — one slot defeats its own DoS guard - `hearth/node` — Bound header extraData — it is unlimited and is hashed before the block-size check