Security review fixes — one PR for internal-docs#1774 (HIGH 1, HIGH 2, MED 3, MED 4 landed)

[0xZKnw]

Single PR for the whole security review, as requested. The findings are written up in holonym-foundation/internal-docs#1774 — 2 HIGH / 3 MEDIUM / 5 LOW, re-verified against testnet on Jul 22 (none of them were already fixed there).

Fixes land as separate commits so each one can be read on its own. This table is the current state and is updated as commits land — it is not a promise that everything below is already in the diff.

#	Finding	State
HIGH 1	Deposit-cap / Travel Rule bypass via self-reported accounting	✅ landed — b0560c1
MED 3	POCH per-day cap not atomic (TOCTOU)	✅ landed — b0560c1
MED 4	SIWE domain pinning accepts a sibling environment; Host trusted	✅ landed — 4dcbc4c
MED 5	ALCHEMY_API_KEY reaches logs via the full AxiosError	✅ landed — 325cc6b
HIGH 2	Permanent address-binding lockout (L2 address never proven)	✅ landed — 25e82a6 + 36e48c3 (no migration needed after all)
LOW 7–11	Left-most XFF trusted for rate-limit + audit IP; mint-tokens recipient; faucet re-enable footgun; PATCH scoped by id alone; JWT 7d without revocation	✅ landed — 3c4613d

If you would rather review this as several smaller PRs, say so and I will split it — the commits are already separable.

---

HIGH 1 + MED 3 — compliance caps enforced against server-signed holds

Commit b0560c1. Same class as #27, different vector.

The cumulative $1,000 Travel Rule threshold and the rolling-24h Alpha cap were summed from client-authored bridgeActivity rows: amountL1, tokenDecimalsL1 and status are all client-supplied, and the only server-authoritative guard — the attestation reservation — expires in ≤30 min while the caps span 24h / lifetime. So a qualified user could request an attestation → deposit on-chain → make the row read as $0 (mark it failed, under-report the amount, or never write it) → wait for the reservation to lapse → repeat, driving cumulative volume past the AML threshold. No funds are stolen; it is an AML/compliance bypass on a live mainnet bridge.

The caps now count the attestation holds the server itself signs, never client deposit rows:

• A hold is valued at its signed ceiling using canonical on-chain decimals from the deployment registry — never the client's tokenDecimals. An inflated value would shrink the charged USD while the signed ceiling still authorises the full amount on-chain.
• A hold counts from the moment it is signed and keeps counting until the chain proves its nonce went unused past the contract-enforced deadline, at which point it is tombstoned. The resolver reads the portals' public passportNonces mapping; a passport signature is verified for the depositor on-chain (TokenPortal._validateAttestations, _amount <= maxAmount + deadline), so a hold read as unused past deadline provably can never settle.
• Fail-safe throughout: an unreadable RPC, an unknown nonce state, or any error keeps the hold counted. A real deposit is never dropped from a cap.
• No DB migration (I can't run migrations against the deployed DB): amountUsd = 0 is the "released" tombstone, and each row's charge state is synthesised from the existing (amountUsd, expiresAt) columns. Deploy is code-only.
• The POCH per-day cap becomes atomic under the same per-user advisory lock as the passport path (MED 3).

MED 4 — SIWE and key-derivation domains pinned to the deployment's own network

Commit 4dcbc4c. This turned out to be three defects rather than one.

The SIWE allow-list hard-coded both shield.human.tech and testnet.shield.human.tech, so a mainnet deployment honoured a signature the user had approved for testnet. The signed domain is the only thing a user sees before approving in their wallet, so accepting a sibling environment's host removes the one boundary that signature carries: a user can be shown a low-stakes testnet domain and have the resulting signature spend its meaning on mainnet.

• The accepted host now comes from the network the deployment actually serves — AZTEC_ENV, resolved from the deployments.json that is committed per branch (testnet here, mainnet on main). No deployment has to set an env var to stay reachable, so this cannot lock anyone out. I could not read the deployed env vars to confirm AUTH_EXPECTED_DOMAIN was set, so the fix was built not to depend on it.
• The Host header is no longer consulted in either route. The localhost dev exception used a prefix test on it and was not gated on NODE_ENV — so localhost.attacker.example matched in production and was then added to the server's own allow-list. Local addresses are now matched on the exact parsed hostname, outside production only.
• The same Host trust inside isAllowedKeyDerivationDomain is removed.
• The signed SIWE uri is attacker-chosen, so it is now checked against the allow-list instead of being used to widen it. This overlaps the HIGH 2 surface; it is included because it depends on no product decision, and leaving it open in the very file being hardened would be incoherent. Happy to split it out if you would rather keep HIGH 2 whole.
• AUTH_EXPECTED_DOMAIN becomes purely additive with an empty default, and .env.example no longer suggests listing both environments — that example is what would reopen the hole.

HIGH 2 — the binding is keyed on the half SIWE actually proves

Commits 25e82a6 and 36e48c3. I first wrote this up as needing a schema change; it doesn't.

SIWE proves the L1 address. The L2 address is read from caller-chosen SIWE resources and is never proven. enforceAddressBinding matched on both halves and wrote a permanent row with no unbind path — so a throwaway L1 paired with someone else's Aztec account locked that account out of the bridge for good. /api/attestation/status then handed the victim the attacker's L1 address, which the hook comment describes as privacy-safe on the grounds that "both sides are the user's own connected addresses" — not true once the L2 half was claimed by a stranger.

Two commits:

• 25e82a6 — the two /check pre-flights no longer bind. Their own doc comments promised no side effects and they were creating permanent rows; a bare JWT no longer reaches a write.
• 36e48c3 — a binding is now looked up by l1Address alone. An unproven L2 collision can neither block a user nor disclose anything about who claimed it. If the DB's l2Address @unique still rejects the insert, that is swallowed: the row simply isn't recorded, which is the safe outcome for the half we can't prove.

Nothing the product does is lost. The conflict this drives in the UI — "your EVM wallet is linked to Aztec account X, switch to it" (issues #98/#120/#124/#130) — is the L1-side conflict, and it is untouched: your own proven wallet being bound elsewhere still blocks, with the same copy. What disappears is only the L2-side conflict, which was the attack.

Why this no longer needs l2Address @unique relaxed. The constraint's job was to stop one L1 spawning several cap buckets. The caps no longer depend on it: after the change above they are counted per L1 address, summing every User row that shares it, with the advisory lock taken on the L1 address so concurrent requests from one L1 across different L2s still serialize. That re-keying is a strict tightening on its own — a User is @@unique([l1Address, l2Address]), so per-row counting was handing the same L1 a fresh allowance for each L2 it paired with.

The constraint stays in the schema, unused by the application logic. Dropping it is a tidy-up you can do whenever, not a prerequisite — no migration, deploy is code-only.

Still worth pulling when you have DB access, because it decides whether anyone is owed a fix rather than just a patch: do conflicting bindings already exist in production? An L2 address bound to an L1 that has never deposited is a victim, and those rows are now inert but still there.

MED 5 — the Alchemy key can no longer reach the log store

Commit 325cc6b.

The key is a path segment of every Alchemy request URL, so the AxiosError raised on failure carries it in config.url. Both proxy routes passed that object straight to console.error, and echoed the upstream error body back to the caller in a details field. Any timeout or upstream rejection therefore left the key one serialization away from wherever the logs land.

• Failures are now summarized through a single helper — upstream status, transport code, and the message with every occurrence of the key masked. The raw error never reaches a sink.
• details is gone from both 500 responses; they return the same generic shape the other routes already use. Nothing read the field.
• The key still travels in the URL. Alchemy's public docs don't confirm that the Data API accepts an Authorization header, and I have no key to test the alternative with, so I did not change the wire format of a live route on a guess. The repo already uses the header pattern for Passport (X-API-KEY) if you want that followed up.

---

LOW 7–11 — the remaining hardening

Commit 3c4613d. Small individually, so they land together.

• 7 — client IP. The nonce rate limit and the clientIp audit column both read the left-most x-forwarded-for entry. A proxy appends the peer it saw rather than replacing the list, so the left-most value is simply what the caller sent: the limit was evadable by rotating a header, and the recorded IP was attacker-chosen. One shared helper now reads the right-most entry — which assumes exactly one trusted proxy in front, stated in the comment — falls back to x-real-ip where no proxy sets the list, and drops anything longer than an address can be. The column was previously unbounded.

• 8 — mint-tokens. The recipient came from the request body, so a single account could mint the testnet supply into unlimited addresses. It now mints to the caller's own wallet, capped per user per minute.

• 9 — faucet. The handler body sat unreachable below its 503, unauthenticated and unthrottled: re-enabling it was one deleted line away from an open drain on the faucet wallet. Removed. The route stays disabled and git history keeps the implementation.

• 10 — operations PATCH. Ownership was verified in one statement and the write keyed on the primary key in another. Not exploitable today — the owner column is immutable — so this is defense in depth: the write now carries the condition it was checked against, and a non-match 404s.

• 11 — JWT. Signing refuses a secret under 32 characters; HS256 is only as strong as its key, and a short one is recoverable offline from a single issued token. I also shortened the default session from 7 days to 1 and then reverted it (e3eea8e) — with no revocation path the lifetime is the only bound on a stolen token, but how often people re-sign is a product call rather than a security requirement. JWT_EXPIRES_IN sets it per deployment if you want it lower.

  Before merging, check that the deployed JWT_SECRET is at least 32 characters. If it is shorter, signing throws and verification returns null, so nobody can log in. I can't read the deployed value. Rotating it is also a fine answer — it invalidates existing sessions, which is the revocation this finding says we don't have.

---

One thing that needs a decision from you

Fully closing the daily ($25k) POCH cap needs a contract change. The clean-hands attestation binds no on-chain amount and no deadline — TokenPortal._validateAttestations checks _amount <= maxAmount for the passport branch only. So for a POCH deposit the counted figure is the client's self-reported amount: this PR serialises POCH requests and counts honest usage, but a caller can still under-state a POCH deposit and slip under the cap. Options, in preference order:

1. Add a signed maxAmount + _amount <= maxAmount check to the clean-hands branch (mirror the passport branch). Smallest change — the hold ledger in this PR then closes POCH exactly like the passport path.
2. Enforce the cumulative cap on-chain directly.
3. Index on-chain deposit events as the source of truth — blocked today because the deposit path doesn't emit the depositor address.

---

Testing

• 18/18 pure-logic unit tests green for the hold ledger — valuation, canonical-decimals spoof-proofing, fail-safe hold resolution, window summing, and the two named security cases. Zero new dependencies:

  cd frontend && node --test --experimental-strip-types src/lib/deposit-ledger.test.ts

• 6/6 unit tests green for the MED 5 redaction, including a differential case asserting that serializing the whole AxiosError — what the old handler did — does contain the key:

  cd frontend && npx tsx src/utils/alchemy.utils.test.ts

• 8/8 unit tests green for the client-IP helper, including the case that pins the difference: the left-most entry the old sites read is the caller's forged value, not what the helper returns:

  cd frontend && npx tsx src/lib/validation.test.ts

• Full-project tsc --noEmit clean, and next build --webpack succeeds on this branch — 28 routes generated, no errors. So the red Vercel check is not coming from this code; the same check fails on Account chip: dynamic connect-state chip + dropdown (first pass) #72/Shield UX round 6: declutter fuel card into messages/tooltips + mini-bar status #67/Shield UX round 5: cleaner nav (no dividers), Activity 20/80 footer + more rows #66 while testnet itself deploys green.
• MED 4's isLocalDevHost was proven against the old implementation on 9 cases in an isolated harness — including that localhost.attacker.example was accepted before and is rejected now. It is not a committed test: the function lives in a module that imports @/config (path alias + JSON), which node --test cannot resolve. So it is a one-off proof, not a guarded regression, and I would rather flag that than let the testing section imply otherwise.
• 16/16 integration cases green against a throwaway Postgres (06c0a27), covering the decisions the pure suite can't reach because Prisma makes them: which identity a cap counts against, which half of the pair a binding matches on, and which rows a PATCH may write. It drives the exported surface and the real /api/attestation/status handler, JWT included, rather than restating the queries. Repro is in the file header.
  • Six cases are labelled REGRESSION and fail on the parent commit — the lockout, the pre-flight blocking on it, the claiming L1 disclosed back to the victim, and the three cap totals a second L2 address used to split. I ran the suite against 25e82a6 to confirm that: 8 passed / 6 failed there, and all 16 pass here.
  • The other ten pin down what must not move: an L1 bound to another L2 still blocks with the same message, the L1-side conflict still reaches the UI, binding stays idempotent, concurrent races resolve to one winner, and the PATCH owner can still write while a stranger cannot. Those pass on both commits, which is the point of them — the two PATCH cases are not labelled regressions, because the old check-then-act also refused a stranger.
  • It is not wired into any automated run — it needs a database and there's no runner for one in this repo. Flagging that rather than letting "14/14" imply CI coverage.
• Not exercised here: live-mainnet-RPC integration. The viem glue is type-checked; the on-chain resolver's decision table is unit-tested in isolation.

Notes for review

• deposit-ledger.ts is a new dependency-free module, so the security-critical valuation and release logic is auditable and testable without a DB or a chain. The glue in address-binding.ts is a thin layer over it.
• The on-chain resolver only ever frees a passport hold on a definitive "unused past deadline". POCH holds are never time-released — their signature carries no deadline — and instead age out of the rolling window.
• domainAllowlist.ts is now the single source of truth for which hosts this deployment answers on; the same allow-list was previously duplicated in two files that had drifted apart.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
aztec-bridge	Error		Jul 30, 2026 7:12am