Repository navigation
Conversation
 [Critical impact] The PR reaches customer credentials and real payment execution through Fireblocks. The two remaining repository requirements should be satisfied before merging. FindingsSummaryAdds project-scoped Fireblocks BYOK setup and native SOL/USDC payments.
Reviews (9) · Last reviewed commit: "fix(web): preserve payment polling after..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
672be2c to
644893a
Compare
|
React Doctor found no new issues. 🎉 Reviewed by React Doctor for commit |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
keeganthomp
left a comment
keeganthomp
left a comment
There was a problem hiding this comment.
Requesting changes for fireblocks-transfer.ts:223 (ADR 0007 re-admission), :259 (429s wedge payments), :321 (swallowed settlement mismatch) and handlers.ts:953 (refusals return 200), plus the missing SDLC §7 tier, threat-model, dependency and AI-use lines; Greptile's eight items are still open.
| }); | ||
| if (!claimed) return currentRow(repo, row); | ||
| try { | ||
| await assertMoneyStartAdmitted( |
There was a problem hiding this comment.
ADR 0007 requires pending-approval signing to re-run decideMovement at execution. This admits once before the POST, so a key revoked or org suspended while Fireblocks awaits approval still pays out. Cancel open Fireblocks transactions on revocation, or amend ADR 0007 with security sign-off, which the value-movement allowlist entry needs too.
|
human translation: I propose we explicitly document and allow a race condition where someone can make a Fireblocks proposal to TRANSFER from within SDP, then, before the TRANSFER is approved in Fireblocks, you revoke the key that made the proposal in SDP... at this point, SDP doesn't do anything to attempt to prevent the transfer. I think that's okay and we should allow that behavior and document it. Detailed AI Slop for the Curious of Mind: Following up on the ADR 0007 re-admission concern: I propose retaining native Fireblocks The remaining behavior is:
We have documented this limitation and implemented safeguards around submission, tenant isolation, idempotency, and settlement verification. We have not eliminated the revocation race or implemented automatic cancellation on SDP revocation. The proposed boundary treats an accepted—or potentially accepted—Fireblocks request as an externally committed instruction. Revocation blocks new SDP submissions; incident response must separately address pending transfers in Fireblocks. Automatic cancellation could reduce exposure, but can race execution and would not guarantee compliance with the existing pre-sign authorization requirement. Requesting explicit, named security-owner approval for both the ADR 0007 exception and the This comment proposes a decision; it does not record security approval. Please keep the security thread open until that decision is made. If the exception is declined, we should disable native sends until an enforceable pre-sign authorization flow is available. |
Yeah I think this is a reasonable exception |
Windscar
left a comment
Windscar
left a comment
There was a problem hiding this comment.
Good work. The transfer recovery and settlement paths look sound, but credential rotation can leave imported vault wallets unusable.
Reviewed: Submission idempotency, reconciliation, provider responses, credential and database state transitions, tenant scope, release gates, dashboard flows, and relevant tests.
Required for approval:
- Verify every attached Fireblocks vault before credential rotation or rollback; details inline.
Non-blocking:
- Limit Fireblocks Pay choices to SOL and USDC; the wizard currently offers tokens the API rejects.
6405b37 to
f0d6845
Compare
Windscar
left a comment
Windscar
left a comment
There was a problem hiding this comment.
Good work. One recovery path still needs to handle credentials that become unavailable after Fireblocks accepts a transfer.
Reviewed: Prior review threads and fixes; BYOK credential lifecycle, payment admission and idempotency, settlement reconciliation, dashboard flows, and relevant tests.
Required for approval:
- Keep accepted Fireblocks transfers reconcilable after connection deactivation or a failed credential check; details inline.
Windscar
left a comment
Windscar
left a comment
There was a problem hiding this comment.
LGTM. The reconciliation gap is fixed.
Reviewed: Connection and failed-check recovery, submission gating, intent binding, and the new dashboard and cron tests.
| await assertMoneyStartAdmitted( | ||
| env, | ||
| { organizationId: row.organization_id, projectId: row.project_id }, | ||
| { surface: "http", movement: "payments.transfer", subjectId: row.id }, | ||
| { custodyWalletId: row.custody_wallet_id } | ||
| ); | ||
| const result = await createFireblocksTransfer(runtime, prepared.access, { |
There was a problem hiding this comment.
submitFireblocksTransfer uses credentials loaded by prepareFireblocksTransfer, before asset discovery, payment writes, and audit writes. If the wallet becomes inactive, the connection fails its health check, or credentials are retired during that work, assertMoneyStartAdmitted does not catch it. Its query checks organization and caller access, not custody state. The server can therefore submit a new payment after custody access was disabled.
Resolve custody access again immediately before the POST and check it against the saved intent. This happens before Fireblocks accepts the instruction, so it is separate from the documented post-acceptance exception.
How this was verified: The POST uses captured credentials, while its final access query does not read wallet, connection, or credential state.
Knowledge Base Used: Custody and transaction controls
| assert( | ||
| wallet.status === "active" && wallet.isRuntimeExecutionAllowed === true, | ||
| "The Fireblocks connection must be active and enabled" | ||
| ); |
There was a problem hiding this comment.
The runner requires an active, execution-enabled wallet even in status mode. After a connection fails its health check or is deactivated, this assertion stops the runner before its transfer GET. Operators then cannot use the documented recovery command, although the API supports checking accepted transfers through retained credentials.
Apply this execution check only to modes that start new work.
Generate dedicated organization/project RSA credentials and a downloadable CSR. Guide API-user registration, discover named vaults, and import selected wallets with encrypted BYOK credentials and tenant isolation. Support credential rotation, rollback, deactivation and secret cleanup. Use native Fireblocks TRANSFER for direct SOL and USDC payments. Resolve USDC by exact mint/network, persist one submission attempt with a stable external transaction ID, and reconcile approval/broadcast status without automatic resubmission. Show provider status, substatus and fee responsibility in the dashboard. Other signing paths retain solana-keychain RAW signing. Validate custody, policy, idempotency, concurrent submission, uncertain outcomes, tenant boundaries, status tracking, dashboard and live-test runner behavior. API/web typechecks, module boundaries, migration compatibility and secret serialization checks pass. Verify the real Fireblocks asset catalogue and local mainnet UI without submitting a native payment. Deploy migrations 0127 and 0128 before the BYOK runtime. Native payments require TRANSFER and destination-policy permission plus SOL for fees in the source vault. RAW permission is only required for separate custom signing and the optional signer check.
Validate provider boundaries and preserve credential identity through setup and rotation. Return definitive payment refusals, log reconciliation failures, bound unacknowledged recovery, and keep durable provider deduplication. Remove unused proposal schema and privileged policy reads, repair setup pending state and React Doctor findings, and harden the opt-in live runner. Retain native TRANSFER with a proposed ADR 0007 exception that still requires named security approval; no claim of approval or atomic revocation is made.
Preserve selected vault readiness after failed pagination and explicitly bind credential rotation requests to their provider. Reuse shared input styling, centralize transfer status mapping, and remove unchecked assertions and nested UI branching. Add regression tests for pagination failures and rotation provider mismatches, and verify live-runner configuration and receipt data boundaries.
…tion Keep COMPLETED create responses and detailed responses without a transaction signature processing until a detailed read verifies the payment intent, destination and exact settled amount. Make settlement verification an explicit input to the shared status mapping and cover eventual recovery without another transaction POST. Make the CSR retry regression test control action completion so it cannot race the parent transition when the next registration form mounts.
…ment Verify each attached vault through solana-keychain before rotation or rollback, compare its address with the stored wallet, and recheck the locked wallet set before committing. Preserve the current credential on failure and keep transient provider failures retryable. Each distinct vault requires one address GET, including the reused primary result; no Solana RPC calls are added. Limit both Pay layouts to SOL and canonical network USDC for Fireblocks BYOK, using the same supported-mint predicate as the API. Clear unsupported selections when switching wallets and reject stale selections before review. Validation: 224 API tests; 3,586 dashboard tests; all 28 workspace typecheck tasks; 119 changed-file Biome checks; module boundaries, value movement, SecretRef serialization, and React Doctor with no new findings. No live Fireblocks calls.
Keep main's recurring-payment expiry migration at 0152 and move the unchanged Fireblocks credential-constraint migration to unused number 0154. The CSR table does not depend on these constraints; the validation migration remains later. Document how existing local PR databases can rename the migration-ledger entry without replaying already-applied constraint changes or replacing payment data.
…vation Separate submission health checks from intent-bound recovery access. Before reading the retained active credential, verify the original connection, provider account, vault and source address. New payments still require an active healthy connection, and revoked credentials remain unavailable. Cover dashboard and cron settlement after deactivation and failed checks, blocked new submissions, mismatched intents, revoked credentials, and no further provider reads after settlement. No extra RPC or provider requests: each admitted poll still issues one Fireblocks GET and never resubmits. Validation: 95 focused API tests, API typecheck, Biome, module boundaries, value-movement and SecretRef serialization checks.
Resolve fresh custody access against the durable intent before the provider POST, fail definitive local refusals, and recover status without requiring an operational wallet. Cover custody transitions and receipt-bound read-only recovery.
Keep pending native payments refreshing while allowing ramps to use their existing live poller. Extract the refresh effect so the rebased transaction component stays within React Doctor complexity limits. Ramp reads use one polling path instead of both the ramp poller and a redundant 15-second page refresh. Native payment polling remains one visible-page refresh every 15 seconds. Cover settlement, missing transfer details, ramp exclusion, hidden tabs, and unmount cleanup.
b34da27 to
01502f7
Compare
|
|
||
| /** Recheck the exact approved message after slow custody signing; never silently re-sign it. */ | ||
| export async function signWithFreshBlockhash<T>(input: { | ||
| rpc: SolanaRpc; |
There was a problem hiding this comment.
signWithFreshBlockhash takes an RPC client from its callers. The “RPC and fee payment” section of the code standards requires new code to read that client from the open cluster scope rather than pass it between functions.
Create the client with createClusterRpc() inside this helper and remove the rpc argument from both callers. This repository requirement must be satisfied before merging.
File Used: docs/contributing/code-standards.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| // biome-ignore lint/security/noSecrets: Public Solana mainnet genesis hash. | ||
| const MAINNET_GENESIS = "5eykt4UsFv8P8NJdTREpY1vzqKqZKvdpKuc147dw2N9d"; | ||
| // biome-ignore lint/security/noSecrets: Public wrapped SOL mint address. | ||
| const SOL_MINT = "So11111111111111111111111111111111111111112"; |
There was a problem hiding this comment.
fireblocks-live.mjs adds a hardcoded SOL_MINT. The TypeScript section of the code standards says token mints must come from @sdp/types/well-known-tokens and explicitly requires scripts to import shared values.
Use that shared value instead of keeping another copy. This repository requirement must be satisfied before merging.
File Used: docs/contributing/code-standards.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
This branch was successfully deployed
4 participants
Summary
Add beta-only Fireblocks BYOK so organizations can connect their own Fireblocks workspace and send SOL or USDC payments through SDP.
Connection and payments
TRANSFERrequests. Fireblocks handles approval, signing, network fees, and broadcasting.Minimal integration scope
Validation
Operational notes
See
docs/ops/fireblocks-byok.mdfor setup instructions and the detailed security review.Security / SDLC §7
Risk tier: Critical — credential handling and fund movement.
Proposed ADR 0007 exception: retain native Fireblocks TRANSFER
I propose retaining Fireblocks native
TRANSFERand obtaining explicit, named security-owner approval for the exception described below.SDP checks the initiating principal’s live authorization, wallet scope, organization/project eligibility, and custody connection before submitting a transfer. Fireblocks subsequently controls approval, signing, and broadcasting. Our current integration has no execution callback that rechecks SDP authorization immediately before Fireblocks signs.
Consequently, a transfer submitted while access was valid can still execute after the initiating SDP key is revoked, the member loses access, the organization is suspended, or the connection is deactivated. Revocation prevents new SDP submissions, but does not withdraw an instruction already accepted by Fireblocks. An ambiguous submission outcome must also be treated as potentially accepted.
This differs from ADR 0007’s requirement to recheck authorization when an asynchronous signing request executes.
Proposed security boundary
Treat an accepted or potentially accepted Fireblocks transfer as an externally committed instruction:
Automatic cancellation on SDP revocation is not currently implemented. Adding it would be a useful mitigation, but would not guarantee that revocation wins a race with signing.
Threat model and controls
Relevant threats include cross-tenant credential or vault substitution, credential disclosure, duplicate payments after ambiguous responses, incorrect settlement attribution, and execution after SDP authorization is revoked.
Controls include tenant-scoped encrypted credentials, human-only connection management, beta gating, live authorization before submission, Fireblocks approval policies, durable external transaction IDs, no automatic resubmission after uncertainty, exact settlement verification, and sanitized logging.
The revocation race remains an explicit residual risk requiring approval.
Dependency assessment and AI use
This change adds
zod@4.6.5to the custody package for provider-response validation and@solana/keychain-core@1.4.0/@solana/keychain-fireblocks@1.4.0as API test dependencies. These versions already exist in the workspace dependency graph; the dependency and lockfile changes remain part of the security review.Codex assisted implementation, testing, and review remediation. Automated checks and AI-assisted review do not constitute independent human review or security approval.
Approval required before merge
Status: proposed; not yet security-approved.
Requesting named security-owner approval of both:
fireblocks-native-transferallowlist entry inscripts/check-value-movement.mjs.Record the reviewer, approval date, approval-comment link, and explicitly accepted exposure in the ADR. Until that approval is recorded, the security review item remains open.
If this boundary is not acceptable, native sends should remain disabled until we implement a flow that can enforce fresh SDP authorization before signing.