[vibhavgo]

Summary

Closes the GG18 (ECDSA MPCv1) external-signer replay hole: an attacker who can reach the signer's tssshare port could replay one encryptedOShare with different signables or D-shares, producing two s values that solve for k/omicron and — with an attacker-supplied Paillier modulus — the user's key share.

Linear: WAL-2459

Changes

• adata binding (sdk-core EcdsaUtils): wShare/oShare envelopes created by the signer carry adata = <domain separator>:sha256(signable):derivationPath and later rounds validate it with the existing validateAdata before decryption — an envelope can only be replayed against the exact signable it was created for. Message signing is bound defensively.
• Counterparty Paillier-modulus validation: bitgoChallenge.n (K and MuDelta rounds) and aShareFromBitgo.n (MuDelta) must match the signer's own keygen-time bitgoNShare.n.
• One-time consumption registry (express): new consumedSharesRegistry.ts marks each MuDelta wShare / S-round oShare ciphertext consumed exactly once, before the share runs (fail-closed). File-backed so protection survives signer restarts; ENOENT = clean first run, read/parse failures fail closed; entries pruned on persist (30-day TTL); keys exclude attacker-controlled fields (e.g. walletId) so relabeled replays are rejected.
• Non-breaking: createOfflineMuDeltaShare / CustomMuDeltaShareGeneratingFunction accept the new fields optionally; legacy relay bodies (top-level txRequest, route-injected prv) still get full adata + modulus validation; legacy direct SDK callers keep unchanged behavior. Old relay + new signer and new relay + old signer interoperate.
• Orchestrator (signEcdsaTssUsingExternalSigner) MuDelta call mirrors the K/S param shape; relay generators unchanged (they spread full params).
• Tests: sdk-core offline-helper suite (adata mismatch, foreign challenge/A-share modulus, legacy shapes) and express end-to-end K→MuDelta→S with replay kills (replayed MuDelta, replayed S, same oShare/different dShare, fresh session still succeeds).

Test Plan

• yarn run unit-test --scope bitgo -- -- --grep "createOffline" — 14 passing
• yarn run unit-test --scope @bitgo/express -- -- --grep "externalSign" — full flow + replay rejections pass
• Root yarn build clean; tsc --noEmit and eslint clean on all touched packages
• Multi-round AI review of the diff (4 phases) converged with no actionable findings

Operational requirements & documented residuals

• Compat note: SigningMaterial.bitgoNShare is a required type field, so the K-round bitgoNShare assert breaks no supported caller — material lacking it cannot type-check and cannot complete GG18 signing (it is consumed by keyDerive).

• Registry location is configurable (--signerRegistryFilePath / BITGO_SIGNER_REGISTRY_FILE_PATH, default <signerFileSystemPath>.consumedShares.json). The signer process never assumes write access there: if the location is not writable, a warning is logged and replay protection degrades to in-memory only (lost on restart) — no startup or request failures. Deployments wanting full restart protection should point the flag at a writable location.

• One signer process per registry file is required. ConsumedShareRegistry.load() reads the file once per process and persist() rewrites it from the in-memory map, so sharing one registry file across replicas can lose records (cross-process locking is out of scope).

• Legacy direct SDK callers that pass neither txRequest nor prv skip Paillier-modulus validation (same as v1 behavior); the one-time consumption registry remains the replay defense on the signer route, which always has both (route-injected).

• A relay retry after a lost signer response restarts the session from K (fail-closed); registry entries and envelope adata share a 30-day TTL, so pruning never drops an entry that could still be accepted.

• Node 27-nightly WASM compatibility failures in the EdDSA suites are pre-existing and unrelated to this diff.

[linear-code]

WAL-2459

[github-actions]

⚠️ Compatibility checks are failing on Node.js 27-nightly (experimental, non-blocking). This does not block merge, but it signals an incompatibility to fix before this release becomes supported.

View run

[vibhavgo]

@claude review always

[vibhavgo]

@claude review

[vibhavgo]

@claude review

[claude]

Code review completed

Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.

[vibhavgo]

@claude review

[vibhavgo]

@claude review

[github-actions]

Claude finished @vibhavgo's task in 33s —— View job

---

Review of 3036ebc

I read the full diff of clientRoutes.ts, consumedSharesRegistry.ts, baseTypes.ts and ecdsa.ts. I didn't re-run the tests. I found no new blocking issues, and I checked that the earlier findings are fixed.

Earlier findings

• Registry poisoning: markConsumed now runs only after bitgo.decrypt has verified the AEAD tag, in both createOfflineMuDeltaShare and createOfflineSShare. Junk bodies are rejected with a 400 before any registry write.
• Check-then-set race: decrypt is awaited before the mark, but markConsumed itself (load, has, set) is synchronous. Two concurrent replays of the same envelope therefore can't both pass. The comment in the code says so.
• v1 envelope key: I compared the key logic against decryptV1.ts.
  • v1CcmNonceLength matches ccmNonceLength, and it uses ct.length - ts/8, the same ciphertext length decryption uses.
  • The fields left out of the key (salt, iter, ks, ts, adata) all feed the key derivation, tag split or AAD. Changing any of them makes decryption fail instead of producing a second valid decrypt of the same share.
• TTL: an entry is stamped when the share is consumed, which is never earlier than the envelope's creation time. So an entry lives at least as long as the envelope can still be accepted, and pruning can't re-open a replay.
• Other earlier findings:
  • The top-level txRequest is back in the orchestrator call.
  • The adata is decodeURIComponent'd for v1 envelopes.
  • The legacy no-txRequest flow carries the wShare binding over to the oShare.
  • persist writes atomically and rethrows on failure.

Minor notes (optional)

• 🟡 createOfflineKShare now asserts bitgoNShare.n unconditionally. Signer material from an older keygen that lacks bitgoNShare will fail at K with "missing the BitGo Paillier modulus". The PR description says the change is non-breaking, so it may be worth a changelog or release note, or a check that all supported key formats include bitgoNShare.
• 🟡 If persist() fails, the share is recorded in memory but never computed, so the session dies. That is fail-closed and fine. Still, the error message could tell operators to check the registry file's disk space and permissions, since that's the likely cause.
• 🟡 In the legacy path (no txRequest/tssParams in the MuDelta body), the MuDelta round doesn't check the wShare against the signable. The S round still enforces the binding, and the consumed-share registry blocks a second use. The security argument therefore holds, but add a code comment saying the S round is what enforces the signable binding here.
• The one-process-per-registry-file requirement is documented in the PR. I'd also put a short note in the express README or signer docs, since operators won't read the PR description.

No changes requested from my side.

[claude]

Code review completed

Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.

[vibhavgo]

@claude review

[vibhavgo]

@claude review

[github-actions]

Claude finished @vibhavgo's task in 30s —— View job

---

Review of 1413b87

I read consumedSharesRegistry.ts and the express changes in clientRoutes.ts, expressApp.ts, args.ts, config.ts and the README. I didn't re-run tests, and the checkout is shallow (no merge base with origin/master), so I couldn't produce a full diff against master. I did not re-review the sdk-core changes in this pass.

Earlier findings

• Read-only key directory: fixed. The registry path is now configurable through --signerRegistryFilePath or BITGO_SIGNER_REGISTRY_FILE_PATH. Startup probes that the path is writable and fails with a clear ExternalSignerConfigError. The README documents this.
• Registry poisoning: the route rejects bodies that aren't JSON or lack string iv/ct with a 400. It passes the mark as a lazy closure, so nothing is recorded before AEAD authentication.
• Re-serialized replays: the key now hashes the decoded iv and ct, with the v1 iv truncated to the CCM nonce length. Whitespace, key order, base64 padding and appended iv bytes no longer change it.
• Persist failure: the write is atomic (temp file plus rename), and errors are rethrown with a message that points at disk space and permissions.
• load(): markConsumed calls load(), and the check-then-set is synchronous.

Optional notes (no change needed)

• 🟡 In consumedSharesRegistry.ts, computeConsumedShareKey asserts on an envelope without string iv/ct. The route already rejects those bodies, so this is defence in depth. Direct callers of the function get a bare AssertionError, though.
• 🟡 The startup probe runs whenever both signerMode and signerFileSystemPath are set. Any signerMode deployment that doesn't use GG18 external signing now also needs a writable directory or an explicit registry path. The error message says how to fix it, but the release notes should mention it.
• 🟡 persist() rewrites the whole registry on every consume. That's fine at signing volumes, and the 30-day prune bounds the size.
• 🟣 Still open from the earlier review and pre-existing: bitgoChallenge.ntilde/h1/h2 are not authenticated. Only n is pinned, so the range-proof parameters from the request still go into the wProof. This is out of scope for this PR but worth a follow-up ticket.

No blocking issues from my side.

[claude]

Findings marked 🟡 are optional suggestions and need no follow-up push.

[vibhavgo]

@claude review

[github-actions]

Claude finished @vibhavgo's task in 25s —— View job

---

Review of 19a7b55

I re-read consumedSharesRegistry.ts in full and the express diffs (clientRoutes.ts, expressApp.ts, args.ts, config.ts). I did not re-read the sdk-core ecdsa.ts changes or the tests, and I didn't run any tests or builds. The sdk-core part rests on the earlier rounds.

No new blocking issues in what I read.

• Registry poisoning: The route only does a cheap check for JSON with string iv/ct, and returns the mark as a lazy closure. Nothing is written until the helper has authenticated the envelope.
• Key canonicalisation: The key hashes the decoded iv and ct, with the v1 iv cut to the CCM nonce length. Re-serialized and appended-iv replays collide with the original.
• Check-then-set: load(), has and set are synchronous, so two concurrent replays can't both pass.
• Persist failures: The write goes to a temp file and is renamed over the registry. Errors are rethrown, and the in-memory entry stays, so it fails closed.
• Config: --signerRegistryFilePath and BITGO_SIGNER_REGISTRY_FILE_PATH are wired through args, env and merge. The registry path is built the same way in the route and in the startup probe.

Optional notes

• 🟡 Startup probe: The check in expressApp.ts runs for every signerMode process with a signerFileSystemPath. That includes EdDSA and MPCv2 signers that never use the registry. A read-only key directory, such as a mounted secret, now stops startup until the operator sets --signerRegistryFilePath. The error message says how to fix it. Please put this in the release notes, or downgrade it to a warning and fail only GG18 MuDelta/S requests.
• 🟡 Direct callers: computeConsumedShareKey throws a bare AssertionError on an envelope without string iv/ct. The route already prevents this, so it only matters for direct callers.
• 🟡 Cost per consume: persist() rewrites the whole map on every consume. That's fine at signing volume, and the 30-day prune bounds the size.
• 🟡 Fragile mirror: v1CcmNonceLength copies logic from sdk-api decryptV1.ts. Consider a test that fails if the two diverge, so a future change in sdk-api can't silently weaken the replay key.
• 🟣 Existing, out of scope: bitgoChallenge.ntilde/h1/h2 are still unauthenticated. Only n is pinned. This is tracked as WAL-2464.
• Docs: The one-signer-process-per-registry-file requirement should stay in the README, not only in the PR description.

No changes requested from my side on the express part.

[claude]

Code review completed

Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.