diff --git a/docs/offline-audit-guide.md b/docs/offline-audit-guide.md index a4877dcc..b9ac197f 100644 --- a/docs/offline-audit-guide.md +++ b/docs/offline-audit-guide.md @@ -5,7 +5,7 @@ The online creation flow also detects proposals another owner already created an Initial verified-store mismatches show “Vault data isn’t up to date” with **Retry / Cancel**. Retry restarts online preparation or offline request export from current state and indexed stores; it never bypasses root validation or retries automatically. Network and unrelated errors are not classified as store mismatches. -**Pre-broadcast state checks:** signed uploads retain their existing binding and policy checks, then compare their actual app-state preconditions with current node state. Stale approval/execution files offer an eligibility-checked fresh export. Imported propose files require review of the original form. Requests and signed responses remain version 1. See [design and boundaries](transaction-coordination-design.md). +**Pre-broadcast state checks:** signed uploads retain their existing binding and policy checks, then compare their actual app-state preconditions with current node state. A CREATE_CHILD response that deploys into a bare child account (one a payment created, which the node reports with `zkappState: null`) is checked against all-zero state, as the chain checks it; an answer that omits the field blocks the broadcast. Stale approval/execution files offer an eligibility-checked fresh export. Imported propose files require review of the original form. Requests and signed responses remain version 1. See [design and boundaries](transaction-coordination-design.md). This document describes the **air-gapped signing path**: the bundle export/import UI inside the web app (`ui/lib/offline-signing.ts`, @@ -97,7 +97,10 @@ Propose bundles add the form's `NewProposalInput` and a freshly re-fetched `configNonce`; execute bundles add per-receiver existence (`receiverAccountExists`) and, for child actions, the child's address and event history. Field-by-field details are in the bundle format reference -below. +below. The export fails when the node answers an account query with an error, +or with an account that lacks `nonce`, `balance` or `zkappState`: the CLI +decides funding and deploy targets from these snapshots, so a guess here would +produce a transaction that fails on chain. After building, the UI surfaces **pre-transfer warnings** from data already in the bundle (missing fee-payer account, balance under 1 MINA, account-creation @@ -293,6 +296,7 @@ the offline CLI restores the resulting leaves without replaying events. | `input.childMultiSigEnable` | `boolean` | enableChildMultiSig | | `input.createChildConfigHash` | `string` | createChild: Poseidon(ownersCommitment, threshold, numOwners) | | `input.childPrivateKey` | `string` | createChild only — signs the child's deploy update (see threat model) | +| `accounts[input.childAccount]` | `FetchedAccount?` | createChild: present when the child's bare account already exists; the CLI then deploys into it without `fundNewAccount` (`deployTargetFromSnapshot`). A snapshot with a verification key or app state is refused | | `input.childOwners` / `input.childThreshold` | `string[]` / `number` | createChild config | | `input.expirySlot` | `number` | UInt32 inclusion deadline for propose, approve, and execute; 0 = no expiry | | `input.memo` | `string` | Plaintext memo — hashed into the proposal **and** attached as the broadcast memo | diff --git a/docs/security-audit-guide.md b/docs/security-audit-guide.md index 08b5d576..104431fb 100644 --- a/docs/security-audit-guide.md +++ b/docs/security-audit-guide.md @@ -190,6 +190,7 @@ attest an independently built deployment artifact. | 7 | **F-2026-19133: a root vault has no key-loss recovery path.** If fewer independently held owner keys remain available than the threshold, no proposal can lower the threshold, replace an owner, or move funds. Operators must choose an owner count above the threshold by a redundancy margin they accept, keep those keys independently controlled and recoverable, and understand that losing too many keys can permanently strand the root vault's funds. A child vault has parent-authorized reclaim/destroy paths; a root vault does not. | Accepted product design; no emergency recovery key or timelocked bypass is provided | | 8 | **Vault events are unauthenticated.** Events need no permission, so any account update can attach events to a vault. The indexer keeps only events whose emitting account update is `Proof`-authorized (the archive API's `authorizationKind`): those come from the vault's own methods, proved against its verification key, which admission already checks is canonical. Undecodable events from its own updates, and events that fail to apply, are recorded with `applyError` and no state change, so none of these can halt indexing or vanish silently. The signature-authorized `deployed` event is not indexed; nothing reads it. Residual: the filter relies on the network verifying proofs (a dev chain with proof verification disabled would accept a dummy proof), and an archive without `authorizationKind` (older than Archive-Node-API v0.0.8) makes indexing fail closed. Clients still check rebuilt stores against the chain before proving. | Mitigated; residual accepted | | 9 | **The verification-key pin is scoped to a transaction version.** `setVerificationKey` is `impossibleDuringCurrentVersion()`: an `Impossible` permission paired with the transaction version o1js stamps at deploy (`4`). Mina RFC 0052 reads `Impossible` and `Proof` verification-key permissions as `Signature` once the network's transaction version is higher, so a zkApp whose proofs no longer verify can be given a new key by its account key. Two consequences for a vault. First, the supported creation flow generates that account key in the browser and discards it, so after a proof-system fork such a vault cannot be migrated by anyone and its proof-only `send` and `editState` leave the funds unreachable; a creator who kept the key out of band is then the one party able to migrate it, or to hijack it. Second, `setPermissions: impossible()` carries no version, so the pin cannot be re-stamped in-protocol; only a hard-fork migration can restore it. The backend compares the stored VK hash and `txnVersion` with the canonical values at admission, and the online UI on each vault load, so a swapped key on an already-tracked vault is caught by the UI, not the backend. | Accepted; a fork that raises the transaction version needs a coordinated migration (new o1js, new canonical VK, re-pinning in the fork). The existing UI and backend builds compare a vault's stored `txnVersion` with their own build-time constant, not with the network's current version, so they do not notice the bump and keep accepting the old pin; builds updated to expect the new version reject every pre-fork pin | +| 10 | **A deployment can be griefed once per address.** The vault address is public once its deploy transaction reaches the mempool, and anyone can create the bare account first by paying into it; a deployment that declares a new account then fails with `Invalid_fee_excess`, costing the deployer the fee and the proving time (for an air-gapped CREATE_CHILD, a rebuilt bundle too). The attacker gains no control and pays the creation fee. Both clients deploy into an existing bare account without the creation fee, so the same address can be retried while its key is still at hand (the offline bundle keeps it; the web wizard keeps it only until the page is left), and refuse an address that already carries a verification key or app state; a failed SubVault creation lifts the browser's vault lock after 20 minutes instead of a day. | Accepted; the first attempt stays exposed. Creating the account in a separate, confirmed transaction before deploying would remove it | Operational and UI-level quirks (state staleness windows, preview-cache behavior) are tracked in [`KNOWN_ISSUES.md`](../KNOWN_ISSUES.md). Infrastructure risks and their mitigations live in the diff --git a/docs/transaction-coordination-design.md b/docs/transaction-coordination-design.md index d1c28a9f..d95dd4c5 100644 --- a/docs/transaction-coordination-design.md +++ b/docs/transaction-coordination-design.md @@ -20,7 +20,11 @@ state fields actually constrained by the transaction, including parent/child accounts and token IDs. Null preconditions are ignored. Repeated account updates are processed in order, applying their known state writes before checking later updates. An unconstrained deployment update can initialize a missing account before -setup in the same transaction. +setup in the same transaction. The node reports an account that exists without +zkApp state (a plain payment created it) with `zkappState: null`; the check reads +it as all zeros, as the chain does, so a deployment into it is checked like any +other, and only an `isNew` precondition on it is stale. An answer that omits the +field is treated as unavailable. - Current: continue sending, without an extra confirmation. - Stale: do not send. Refresh eligibility and offer an explicit recovery action. diff --git a/docs/ui-audit-guide.md b/docs/ui-audit-guide.md index 68d7eaf5..0cdac9ab 100644 --- a/docs/ui-audit-guide.md +++ b/docs/ui-audit-guide.md @@ -5,7 +5,7 @@ Online creation checks whether the exact proposal already exists before requesti Initial verified-store mismatches show “Vault data isn’t up to date” with **Retry / Cancel**. Retry restarts online preparation or offline request export from current state and indexed stores; it never bypasses root validation or retries automatically. Network and unrelated errors are not classified as store mismatches. -**Pre-broadcast state checks:** the actual transaction app-state preconditions are compared with fresh node state before sending (before Auro wallet handoff). Stale transactions require explicit, eligibility-checked recovery. Node failures block broadcast; a successful check does not guarantee inclusion. See [design and boundaries](transaction-coordination-design.md). Permission-loading notices on vault and proposal pages are delayed and neutral; action gates remain fail-closed. +**Pre-broadcast state checks:** the actual transaction app-state preconditions are compared with fresh node state before sending (before Auro wallet handoff). Stale transactions require explicit, eligibility-checked recovery. A deploy target that exists without zkApp state (the node answers `zkappState: null` for an account a payment created) is read as all zeros, as the chain reads it, so a deployment into it is checked like any other; an answer that omits the field blocks the broadcast. Node failures block broadcast; a successful check does not guarantee inclusion. See [design and boundaries](transaction-coordination-design.md). Permission-loading notices on vault and proposal pages are delayed and neutral; action gates remain fail-closed. Online progress follows the submission boundary: Auro shows “Checking latest vault state…” after proving, then “Waiting for wallet confirmation…” only after the check passes. Ledger requests wallet confirmation first, checks the signed transaction, then shows “Broadcasting transaction…”. Early store/root mismatches offer explicit Retry / Cancel; the post-proof check uses the recovery panel. @@ -268,7 +268,12 @@ A guard that is deployed but not yet configured could be controlled by whoever calls `setup()` first. - Top-level vaults use the atomic `deployAndSetupContract` — one tx doing `fundNewAccount` + `deploy` + `setup`; the worker exposes no separate - deploy-only or setup-only API. + deploy-only or setup-only API. The creation fee is paid only when the node + has no account at the address (`classifyDeployTarget`, `lib/deploy-target.ts`): + anyone can create the bare account first by paying into it, which fails a + deployment that declares a new account, so a bare account is deployed into + as it is, and an address that already carries a verification key or app + state is refused. The same rule applies to the child in CREATE_CHILD. - CREATE_CHILD spans two transactions by design: the propose tx does `deploy(child)` + `reserveForParent(child)` + `propose(parent)` atomically (`worker.ts:979-1003`); the later `executeSetupChild` is bound on-chain to @@ -279,7 +284,9 @@ calls `setup()` first. - Account-creation fees on execute are counted from the hash-bound `proposalStruct.receivers`, never the raw backend array (`worker.ts:1142-1153`), so indexer rows beyond `MAX_RECEIVERS` can't - inflate the executor-signed fee. + inflate the executor-signed fee. A receiver counts as new only when the + node answers "no such account" (`receiverExists`); any other node error + stops the build, since a wrong guess fails the transaction after proving. **3. Indexer-supplied data feeding into signed transactions.** The backend is untrusted (see threat model), yet its data rebuilds the Merkle @@ -383,7 +390,10 @@ never reports the earlier tx's failure. An accepted report marks its record `recorded`; reconciliation then marks it `untracked` as soon as the backend's hash for that action is not this record's (`backendNoLongerTracks`). A report the backend never answered within two minutes, say after a reload mid-report, -is marked the same way (`reportUnanswered`). +is marked the same way (`reportUnanswered`). SubVault creation records (`kind: 'create'` with a `createChild` summary) use +the same 20-minute window: a failed creation emits no proposal, and the +best-chain probe covers only recent blocks, so nothing else would ever clear +them. **5. Ephemeral zkApp key lifecycle & local storage.** The only private key the UI holds is the in-browser zkApp deploy key diff --git a/e2e/unit/offline-checkpoint-export.test.ts b/e2e/unit/offline-checkpoint-export.test.ts index d7d5c362..5582ba22 100644 --- a/e2e/unit/offline-checkpoint-export.test.ts +++ b/e2e/unit/offline-checkpoint-export.test.ts @@ -36,7 +36,7 @@ describe('offline checkpoint export', () => { if (String(input).includes('/events')) throw new Error('Unexpected full history request'); const address = JSON.parse(String(init?.body)).variables.publicKey; calls.push(`account:${address}`); - return Response.json({ data: { account: { publicKey: address, zkappState: [] } } }); + return Response.json({ data: { account: { publicKey: address, nonce: '0', balance: { total: '1000000000' }, zkappState: [] } } }); }) as typeof fetch; const params = { contractAddress: 'vault', feePayerAddress: 'owner' }; const builders = [ diff --git a/offline-cli/src/build-tx.ts b/offline-cli/src/build-tx.ts index 9787a67e..2f56d503 100644 --- a/offline-cli/src/build-tx.ts +++ b/offline-cli/src/build-tx.ts @@ -367,6 +367,23 @@ export function countNewReceiverAccounts( return count; } +/** + * Decides how to deploy a child to an address from the bundle's snapshot of + * it. Anyone can create the bare Mina account first by paying into the + * address, and a deployment that declares a new account then fails. With a + * snapshot of a bare account the child is deployed into it without the + * creation fee. A snapshot that carries a verification key or app state is + * refused: only the address's own key could have put them there. + */ +export function deployTargetFromSnapshot(snapshot: BundleAccount | undefined): 'new' | 'existing' { + if (!snapshot) return 'new'; + const hasState = (snapshot.zkappState ?? []).some((value) => String(value) !== '0'); + if (snapshot.verificationKey || hasState) { + throw new Error('The child address already holds a zkApp. Create the SubVault with a fresh address.'); + } + return 'existing'; +} + function buildReceiversForProposal(input: NewProposalInput): InstanceType[] { if (input.txType === 'transfer' || input.txType === 'allocateChild') { return buildTransferReceivers(input.receivers ?? []); @@ -817,8 +834,17 @@ export async function handlePropose( let childKey: InstanceType | null = null; let childOwnerStore: InstanceType | null = null; let childPaddedOwners: InstanceType[] | null = null; + // A snapshot of the child address means someone created its bare account + // already; the deploy then must not fund it again. The address comes from + // the key that signs the deploy, and must be the one the proposal names. + let childTarget: 'new' | 'existing' | null = null; if (isCreateChild) { childKey = PrivateKey.fromBase58(input.childPrivateKey!); + const childAddress = childKey.toPublicKey().toBase58(); + if (childAddress !== input.childAccount) { + throw new Error('The createChild key does not match input.childAccount'); + } + childTarget = deployTargetFromSnapshot(bundle.accounts[childAddress]); childOwnerStore = new OwnerStore(); for (const addr of input.childOwners!) childOwnerStore.addSorted(PublicKey.fromBase58(addr)); childPaddedOwners = [...childOwnerStore.owners]; @@ -835,7 +861,7 @@ export async function handlePropose( if (isCreateChild && childKey && childOwnerStore && childPaddedOwners) { const childAddress = childKey.toPublicKey(); const childZkApp = new MinaGuard(childAddress); - AccountUpdate.fundNewAccount(proposer); + if (childTarget === 'new') AccountUpdate.fundNewAccount(proposer); await childZkApp.deploy(); await childZkApp.reserveForParent( contractAddress, diff --git a/offline-cli/src/summary.ts b/offline-cli/src/summary.ts index a92c059b..f25278a8 100644 --- a/offline-cli/src/summary.ts +++ b/offline-cli/src/summary.ts @@ -134,6 +134,8 @@ function renderBody( childAddress?: string | null; childOwners?: string[] | null; childThreshold?: string | number | null; + /** createChild propose: whether the bundle snapshots the child account, so the CLI skips the creation fee. */ + childAccountExists?: boolean | null; reclaimAmount?: string | null; enableMultiSig?: boolean | null; }, @@ -159,6 +161,9 @@ function renderBody( for (const o of src.childOwners) out.push(` ${o}`); } out.push(line('Threshold', src.childThreshold ?? '(unknown)')); + if (src.childAccountExists != null) { + out.push(line('Child account', src.childAccountExists ? 'exists, no creation fee' : 'new, 1 MINA creation fee')); + } return out; } case 'reclaimChild': @@ -265,6 +270,9 @@ export function renderBundleSummary(bundle: OfflineBundle): string { childAddress: input.childAccount ?? null, childOwners: input.childOwners ?? null, childThreshold: input.childThreshold ?? null, + childAccountExists: input.txType === 'createChild' && input.childAccount + ? Boolean(b.accounts?.[input.childAccount]) + : null, reclaimAmount: input.reclaimAmount ?? null, enableMultiSig: input.childMultiSigEnable ?? null, }), diff --git a/offline-cli/src/tests/offline-cli-e2e.test.ts b/offline-cli/src/tests/offline-cli-e2e.test.ts index 6a38e9be..563e6601 100644 --- a/offline-cli/src/tests/offline-cli-e2e.test.ts +++ b/offline-cli/src/tests/offline-cli-e2e.test.ts @@ -162,6 +162,8 @@ function snapshotAccount(addr: PublicKey): unknown { let owners: Array<{ key: PrivateKey; pub: PublicKey }>; let zkAppAddress: PublicKey; +// Kept for the test that deploys into the parent's own address. +let zkAppKey: InstanceType; let zkApp: MinaGuard; let deployer: { key: PrivateKey; pub: PublicKey }; @@ -188,7 +190,7 @@ describe('offline-cli e2e', () => { })); owners.sort((a, b) => (a.pub.toBase58() > b.pub.toBase58() ? 1 : -1)); - const zkAppKey = PrivateKey.random(); + zkAppKey = PrivateKey.random(); zkAppAddress = zkAppKey.toPublicKey(); zkApp = new MinaGuard(zkAppAddress); @@ -1353,6 +1355,104 @@ describe('offline-cli e2e', () => { expect(result.code).not.toBe(0); expect(result.stderr).toContain('config mismatch'); }, 120_000); + + it('propose createChild into an account someone created first, without funding it again', async () => { + const proposer = owners[0]; + const preKey = PrivateKey.random(); + const preAddr = preKey.toPublicKey(); + // Anyone can create the bare account by paying into the address. + const payer = owners[1]; + const pay = await Mina.transaction(payer.pub, async () => { + AccountUpdate.fundNewAccount(payer.pub); + AccountUpdate.createSigned(payer.pub).send({ to: preAddr, amount: 1_000_000_000 }); + }); + await pay.sign([payer.key]).send(); + expect(Mina.hasAccount(preAddr)).toBe(true); + + const bundle = { + version: 1, + action: 'propose', + minaNetwork: 'testnet', + contractAddress: zkAppAddress.toBase58(), + feePayerAddress: proposer.pub.toBase58(), + accounts: { ...accountsSnapshotForCreate(), [preAddr.toBase58()]: snapshotAccount(preAddr) }, + events: await parentEvents(), + input: { + txType: 'createChild', + nonce: 0, + childAccount: preAddr.toBase58(), + createChildConfigHash: configHash(), + childPrivateKey: preKey.toBase58(), + childOwners: childOwnerAddrs(), + childThreshold: 2, + }, + configNonce: 0, + networkId: '1', + }; + const bundlePath = join(tmpDir, 'create-child-existing-account.json'); + writeBundle(bundlePath, bundle); + + const result = await runCLI(bundlePath, proposer.key.toBase58(), 600_000, skipEnv); + if (result.code !== 0) console.log('[e2e] CLI stderr:', result.stderr, '\nstdout:', result.stdout); + expect(result.code).toBe(0); + + const output = JSON.parse(result.stdout); + // No account-creation update for the proposer: the account already exists. + const proposerUpdates = output.transaction.accountUpdates.filter( + (au: any) => au.body.publicKey === proposer.pub.toBase58(), + ); + expect(proposerUpdates).toHaveLength(0); + // The ledger accepts a deployment into the existing bare account: no + // creation fee, and the address's own key signs the deploy update. + const childZkApp = new MinaGuard(preAddr); + const deployTx = await Mina.transaction(proposer.pub, async () => { + await childZkApp.deploy(); + }); + await deployTx.sign([proposer.key, preKey]).send(); + expect(Mina.getAccount(preAddr).zkapp?.verificationKey).toBeDefined(); + }, 120_000); + + it('refuses to propose createChild into an address that already holds a zkApp, or with a key for another address', async () => { + const proposer = owners[0]; + const bundleWithKey = async (childPrivateKey: string) => ({ + version: 1, + action: 'propose', + minaNetwork: 'testnet', + contractAddress: zkAppAddress.toBase58(), + feePayerAddress: proposer.pub.toBase58(), + accounts: accountsSnapshotForCreate(), + events: await parentEvents(), + input: { + txType: 'createChild', + nonce: 0, + // The parent vault itself: an address with a verification key. + childAccount: zkAppAddress.toBase58(), + createChildConfigHash: configHash(), + childPrivateKey, + childOwners: childOwnerAddrs(), + childThreshold: 2, + }, + configNonce: 0, + networkId: '1', + }); + + // A key for some other address is refused before anything else. + const mismatchPath = join(tmpDir, 'create-child-key-mismatch.json'); + writeBundle(mismatchPath, await bundleWithKey(PrivateKey.random().toBase58())); + const mismatch = await runCLI(mismatchPath, proposer.key.toBase58(), 600_000, skipEnv); + expect(mismatch.code).not.toBe(0); + expect(mismatch.stderr).toContain('does not match input.childAccount'); + expect(mismatch.stdout).toBe(''); + + // With the address's own key, the snapshot shows it already holds a zkApp. + const takenPath = join(tmpDir, 'create-child-taken-address.json'); + writeBundle(takenPath, await bundleWithKey(zkAppKey.toBase58())); + const taken = await runCLI(takenPath, proposer.key.toBase58(), 600_000, skipEnv); + console.log('[e2e] CLI stderr:', taken.stderr); + expect(taken.code).not.toBe(0); + expect(taken.stderr).toContain('already holds a zkApp'); + expect(taken.stdout).toBe(''); + }, 240_000); }); }); diff --git a/offline-cli/src/tests/offline-cli.test.ts b/offline-cli/src/tests/offline-cli.test.ts index 31ca9b85..a8a0aac0 100644 --- a/offline-cli/src/tests/offline-cli.test.ts +++ b/offline-cli/src/tests/offline-cli.test.ts @@ -30,7 +30,7 @@ import { TxType, } from 'contracts'; -import { signFeePayer, decodeTxMemo, countNewReceiverAccounts, buildTransferReceivers, EMPTY_PUBKEY_B58, assertBundleNetwork, assertExecutableAddOwnerData, requireTxType, canonicalizeBundleTxType } from '../build-tx.ts'; +import { signFeePayer, decodeTxMemo, countNewReceiverAccounts, buildTransferReceivers, EMPTY_PUBKEY_B58, assertBundleNetwork, assertExecutableAddOwnerData, requireTxType, canonicalizeBundleTxType, deployTargetFromSnapshot } from '../build-tx.ts'; import { escapeTerminalText } from '../terminal-safe.ts'; import { renderBundleSummary } from '../summary.ts'; @@ -127,6 +127,33 @@ describe('offline-cli', () => { expect(result.stdout).toBe(''); }, 30_000); + describe('deployTargetFromSnapshot', () => { + const bare = { publicKey: 'B62qchild', nonce: '0', verificationKey: null, zkappState: null } as any; + + it('funds a child the bundle has no snapshot for, and deploys into a bare account without funding', () => { + expect(deployTargetFromSnapshot(undefined)).toBe('new'); + expect(deployTargetFromSnapshot(bare)).toBe('existing'); + expect(deployTargetFromSnapshot({ ...bare, zkappState: ['0', '0', '0'] })).toBe('existing'); + }); + + it('refuses a child address that already holds a zkApp', () => { + expect(() => deployTargetFromSnapshot({ ...bare, verificationKey: { verificationKey: 'vk', hash: '1' } })) + .toThrow('already holds a zkApp'); + expect(() => deployTargetFromSnapshot({ ...bare, zkappState: ['0', '5'] })).toThrow('already holds a zkApp'); + }); + }); + + it('tells the signer whether a createChild propose pays the child creation fee', () => { + const CHILD = 'B62qkYgXmsk3R65YGNG41Zqu61hf9X1qBktDPzZkkthkSnukbXLPCAY'; + const propose = (accounts: Record) => ({ + version: 1, action: 'propose', minaNetwork: 'testnet', contractAddress: EMPTY_PUBKEY_B58, + feePayerAddress: EMPTY_PUBKEY_B58, accounts, events: [], configNonce: 0, + input: { txType: 'createChild', nonce: 0, childAccount: CHILD, childOwners: [CHILD], childThreshold: 1 }, + }) as any; + expect(renderBundleSummary(propose({}))).toContain('new, 1 MINA creation fee'); + expect(renderBundleSummary(propose({ [CHILD]: { publicKey: CHILD } }))).toContain('exists, no creation fee'); + }); + it('accepts only the ten transaction types, by name or number', () => { expect(requireTxType('transfer')).toBe('transfer'); expect(requireTxType('9')).toBe('enableChildMultiSig'); diff --git a/ui/lib/deploy-target.ts b/ui/lib/deploy-target.ts new file mode 100644 index 00000000..6e191866 --- /dev/null +++ b/ui/lib/deploy-target.ts @@ -0,0 +1,48 @@ +/** What the node knows about a vault address before a deployment. */ +export interface DeployTargetAccount { + zkapp?: { appState?: ReadonlyArray | null; verificationKey?: unknown } | null; +} + +/** The result of fetching the address from the node, as o1js's `fetchAccount` reports it. */ +export interface DeployTargetFetch { + account?: DeployTargetAccount | null; + error?: { statusCode?: number; statusText?: string } | null; +} + +/** + * Decides how to deploy a vault to an address. Anyone can create the bare Mina + * account first by paying into the address, and a deployment that declares a + * new account then fails. An existing bare account is deployed into without + * the creation fee. An account that already carries a verification key or app + * state is refused: only the address's own key could have put them there. A + * node error other than "no such account" is refused too, since guessing + * would reproduce the failure this check exists to prevent. + */ +export function classifyDeployTarget(fetched: DeployTargetFetch): 'new' | 'existing' { + if (fetched.error) { + if (fetched.error.statusCode === 404) return 'new'; + throw new Error(`Could not check the vault address: ${fetched.error.statusText ?? 'node request failed'}`); + } + const account = fetched.account; + if (!account) return 'new'; + const zkapp = account.zkapp; + const hasState = (zkapp?.appState ?? []).some((value) => String(value) !== '0'); + if (zkapp?.verificationKey || hasState) { + throw new Error('This address already holds a zkApp. Create the vault with a fresh address.'); + } + return 'existing'; +} + +/** + * Whether a receiver already has an account, from the same fetch. Only "no such + * account" means no. Any other node error is refused: a wrong guess either funds + * an account that exists or leaves a new one unfunded, and the transaction fails + * either way after proving. + */ +export function receiverExists(fetched: DeployTargetFetch): boolean { + if (fetched.error) { + if (fetched.error.statusCode === 404) return false; + throw new Error(`Could not check a receiver address: ${fetched.error.statusText ?? 'node request failed'}`); + } + return !!fetched.account; +} diff --git a/ui/lib/multisigClient.worker.ts b/ui/lib/multisigClient.worker.ts index 2f43e4ce..001b8aba 100644 --- a/ui/lib/multisigClient.worker.ts +++ b/ui/lib/multisigClient.worker.ts @@ -3,6 +3,7 @@ import './disable-wasm-finalizers'; import type { PreflightContext, RetryEligibility } from './preflight-flow'; +import { classifyDeployTarget, receiverExists } from './deploy-target'; import { requireUnregisteredProposal } from './proposal-preparation'; import * as Comlink from 'comlink'; @@ -298,6 +299,7 @@ async function fetchContractState( } } + async function signProposalAuthorization( hashAsFieldString: string, action: 'propose' | 'approve', @@ -895,15 +897,21 @@ const workerApi = { signFeePayerFn?: SignFeePayerFn ): Promise { console.log('[MultisigWorker] deployAndSetupContract entered'); + const feePayer = PublicKey.fromBase58(params.feePayerAddress); + const zkAppKey = PrivateKey.fromBase58(params.zkAppPrivateKeyBase58); + const zkAppAddress = zkAppKey.toPublicKey(); + + // Check the address before the compile: someone may have created the + // bare account first, or the address may already be in use. + await configureNetwork(); + progressFn('Checking the vault address...'); + const vaultTarget = classifyDeployTarget(await fetchAccount({ publicKey: zkAppAddress })); + progressFn('Compiling contract...'); const ok = await compileContract(); if (!ok) return null; - await configureNetwork(); progressFn('Building transaction...'); - const feePayer = PublicKey.fromBase58(params.feePayerAddress); - const zkAppKey = PrivateKey.fromBase58(params.zkAppPrivateKeyBase58); - const zkAppAddress = zkAppKey.toPublicKey(); const zkApp = new MinaGuard(zkAppAddress); const ownerStore = new OwnerStore(); @@ -918,7 +926,7 @@ const workerApi = { await fetchAccount({ publicKey: feePayer }); clearStaleTransaction(); const tx = await Mina.transaction(txSender(feePayer), async () => { - AccountUpdate.fundNewAccount(feePayer); + if (vaultTarget === 'new') AccountUpdate.fundNewAccount(feePayer); await zkApp.deploy(); await zkApp.setup( Field(params.threshold), @@ -990,6 +998,17 @@ const workerApi = { requireUnregisteredProposal(hashStr, approvalStore.getCount(proposalHash).toBigInt()); progressFn(testPrivateKey ? 'Signing proposal authorization...' : 'Awaiting wallet signature...'); + // Check the child address before asking for a signature: someone may have + // created its bare account first, or the address may already be in use. + let childTarget: 'new' | 'existing' | null = null; + if (isCreateChild) { + if (!params.childPrivateKey) throw new Error('createChild proposal requires childPrivateKey'); + const childAddress = PrivateKey.fromBase58(params.childPrivateKey).toPublicKey(); + if (!childAddress.equals(proposal.childAccount).toBoolean()) { + throw new Error('The createChild key does not match the proposal child address'); + } + childTarget = classifyDeployTarget(await fetchAccount({ publicKey: childAddress })); + } const signature = await signProposalAuthorization(hashStr, 'propose', signFn); if (!signature) return null; @@ -1049,7 +1068,7 @@ const workerApi = { if (isCreateChild && childKey && childOwnerStore && childPaddedOwners) { const childAddress = childKey.toPublicKey(); const childZkApp = new MinaGuard(childAddress); - AccountUpdate.fundNewAccount(proposer); + if (childTarget === 'new') AccountUpdate.fundNewAccount(proposer); await childZkApp.deploy(); await childZkApp.reserveForParent( contractAddress, @@ -1238,11 +1257,11 @@ const workerApi = { if (txType === 'transfer' || txType === 'allocateChild') { for (const r of proposalStruct.receivers) { if (r.address.isEmpty().toBoolean()) continue; - const { account } = await fetchAccount({ publicKey: r.address }); - if (!account && txType === 'allocateChild') { + const exists = receiverExists(await fetchAccount({ publicKey: r.address })); + if (!exists && txType === 'allocateChild') { throw new Error('Allocation recipients must be initialized children of this vault'); } - if (!account) newAccountCount += 1; + if (!exists) newAccountCount += 1; } } diff --git a/ui/lib/offline-signing.ts b/ui/lib/offline-signing.ts index 4909f0eb..4ebf0d3e 100644 --- a/ui/lib/offline-signing.ts +++ b/ui/lib/offline-signing.ts @@ -168,7 +168,17 @@ async function fetchGraphQLAccount(address: string): Promise { body: JSON.stringify({ query, variables: { publicKey: address } }), }); const json = await res.json(); - return json.data?.account ?? null; + // A node error is not "no such account": guessing would make the CLI fund + // an account that exists, or skip funding one that does not. + if (!res.ok || json.errors || !json.data) { + throw new Error(`Could not fetch account ${address} from the Mina node`); + } + const account = json.data.account ?? null; + // The CLI decides from the snapshot's fields; an answer without them is no snapshot. + if (account !== null && !['nonce', 'balance', 'zkappState'].every((field) => typeof account === 'object' && field in account)) { + throw new Error(`Incomplete account snapshot for ${address} from the Mina node`); + } + return account; } async function checkAccountExists(address: string): Promise { diff --git a/ui/lib/storage.ts b/ui/lib/storage.ts index c678dcca..e2cb4f77 100644 --- a/ui/lib/storage.ts +++ b/ui/lib/storage.ts @@ -102,9 +102,12 @@ export interface PendingTx { const PENDING_TXS_KEY = getKey('pending-txs'); /** 24h prune window — survives long-lived sessions. */ const PENDING_TX_TTL_MS = 24 * 60 * 60 * 1000; -/** Prune window for a record the backend refused to track: without it a failed - * or dropped transaction would lock the vault in this tab for a day. It - * matches the backend's 20-minute wait before it calls a transaction dropped. */ +/** Prune window for a record nothing will resolve if its transaction fails: an + * approve/execute the backend refused to track, or a SubVault creation, whose + * failed transaction emits no proposal and leaves the best-chain probe empty + * once it ages past the probe's window. Without it such a transaction would + * lock the vault in this tab for a day. It matches the backend's 20-minute + * wait before it calls a transaction dropped. */ export const UNTRACKED_PENDING_TX_TTL_MS = 20 * 60 * 1000; /** Custom event dispatched on save/clear so banners can refresh in the same tab. @@ -121,7 +124,8 @@ function pruneStale(records: PendingTx[]): PendingTx[] { return records.filter((r) => { const ts = new Date(r.createdAt).getTime(); if (!Number.isFinite(ts)) return false; - return now - ts < (r.untracked ? UNTRACKED_PENDING_TX_TTL_MS : PENDING_TX_TTL_MS); + const shortLived = r.untracked || (r.kind === 'create' && r.summary?.txType === 'createChild'); + return now - ts < (shortLived ? UNTRACKED_PENDING_TX_TTL_MS : PENDING_TX_TTL_MS); }); } diff --git a/ui/lib/transaction-preflight.ts b/ui/lib/transaction-preflight.ts index d8876d5d..14bf06c2 100644 --- a/ui/lib/transaction-preflight.ts +++ b/ui/lib/transaction-preflight.ts @@ -1,6 +1,8 @@ /** Public transaction data only. This check never edits or signs the transaction. */ export type PreflightResult = { status: 'current' | 'stale' | 'unavailable'; message?: string }; -export type AccountState = string[] | null; +/** The node's view of one account: its app state, `'bare'` for an account that + * exists without zkApp state (a plain payment created it), or `null` for none. */ +export type AccountState = string[] | 'bare' | null; export type ReadAccount = (publicKey: string, tokenId: string) => Promise; const decimal = (x: unknown): x is string => typeof x === 'string' && /^(0|[1-9][0-9]*)$/.test(x); @@ -9,6 +11,8 @@ const decimal = (x: unknown): x is string => typeof x === 'string' && /^(0|[1-9] * Repeated updates are evaluated in transaction order, including in-tx writes. * An absent account can be initialized by an unconstrained deployment update; * otherwise a missing account requires an explicit isNew precondition. + * A bare account reads as all-zero state, which is what a deployment into it + * initializes; an isNew precondition on it is stale, since the account exists. */ export async function checkTransactionState(txJson: string, read: ReadAccount): Promise { try { @@ -27,8 +31,9 @@ export async function checkTransactionState(txJson: string, read: ReadAccount): const key = JSON.stringify([body.publicKey, body.tokenId]); if (!states.has(key)) states.set(key, await read(body.publicKey, body.tokenId)); let actual = states.get(key)!; - if (actual === null) { - if (body.preconditions.account.isNew !== true && expected.some((value: unknown) => value !== null)) return { status: 'stale' }; + if (actual === null || actual === 'bare') { + const isNew = body.preconditions.account.isNew === true; + if (actual === null ? !isNew && expected.some((value: unknown) => value !== null) : isNew) return { status: 'stale' }; actual = Array(expected.length).fill('0'); } if (actual.length !== expected.length || !actual.every(decimal)) throw new Error('Incomplete account state'); @@ -57,8 +62,12 @@ export function nodeAccountReader(endpoint: string): ReadAccount { if (!response.ok) throw new Error('Node unavailable'); const json = await response.json(); if (json.errors?.length || !json.data || !('account' in json.data)) throw new Error('Incomplete node response'); - if (json.data.account === null) return null; - const state = json.data.account.zkappState; + const account = json.data.account; + if (account === null) return null; + // The field must be answered: `null` is a bare account, an array its state. + if (typeof account !== 'object' || !('zkappState' in account)) throw new Error('Missing app state'); + const state = account.zkappState; + if (state === null) return 'bare'; if (!Array.isArray(state)) throw new Error('Missing app state'); return state; }; diff --git a/ui/tests/deploy-target.test.ts b/ui/tests/deploy-target.test.ts new file mode 100644 index 00000000..5f5eb185 --- /dev/null +++ b/ui/tests/deploy-target.test.ts @@ -0,0 +1,43 @@ +import { describe, expect, it } from 'bun:test'; +import { classifyDeployTarget, receiverExists } from '../lib/deploy-target'; + +describe('classifyDeployTarget', () => { + it('funds an account the node does not have', () => { + expect(classifyDeployTarget({ error: { statusCode: 404, statusText: 'does not exist' } })).toBe('new'); + expect(classifyDeployTarget({ account: null })).toBe('new'); + expect(classifyDeployTarget({})).toBe('new'); + }); + + it('deploys into a bare account someone created first, without funding it again', () => { + expect(classifyDeployTarget({ account: {} })).toBe('existing'); + expect(classifyDeployTarget({ account: { zkapp: null } })).toBe('existing'); + expect(classifyDeployTarget({ account: { zkapp: { appState: ['0', '0'], verificationKey: undefined } } })).toBe('existing'); + }); + + it('refuses an address that already carries a verification key or app state', () => { + expect(() => classifyDeployTarget({ account: { zkapp: { appState: [], verificationKey: { hash: 'vk' } } } })) + .toThrow('already holds a zkApp'); + expect(() => classifyDeployTarget({ account: { zkapp: { appState: ['0', '7'] } } })).toThrow('already holds a zkApp'); + }); + + it('refuses to guess when the node fails for any other reason', () => { + expect(() => classifyDeployTarget({ error: { statusCode: 500, statusText: 'upstream timeout' } })) + .toThrow('Could not check the vault address: upstream timeout'); + expect(() => classifyDeployTarget({ error: {} })).toThrow('Could not check the vault address'); + }); +}); + +describe('receiverExists', () => { + it('treats only "no such account" as a new receiver', () => { + expect(receiverExists({ error: { statusCode: 404, statusText: 'does not exist' } })).toBe(false); + expect(receiverExists({ account: {} })).toBe(true); + expect(receiverExists({ account: { zkapp: null } })).toBe(true); + expect(receiverExists({ account: { zkapp: { appState: ['1'], verificationKey: { hash: 'vk' } } } })).toBe(true); + }); + + it('refuses to guess when the node fails for any other reason', () => { + expect(() => receiverExists({ error: { statusCode: 500, statusText: 'upstream timeout' } })) + .toThrow('Could not check a receiver address: upstream timeout'); + expect(() => receiverExists({ error: {} })).toThrow('Could not check a receiver address'); + }); +}); diff --git a/ui/tests/offline-signing.test.ts b/ui/tests/offline-signing.test.ts index 34a27e01..88e6beb0 100644 --- a/ui/tests/offline-signing.test.ts +++ b/ui/tests/offline-signing.test.ts @@ -50,7 +50,7 @@ function configure(networkId: string | undefined) { fetchedUrls.push(String(url)); if (init?.method === 'POST') { const { variables } = JSON.parse(String(init.body)); - return Response.json({ data: { account: { publicKey: variables.publicKey } } }); + return Response.json({ data: { account: { publicKey: variables.publicKey, nonce: '0', balance: { total: '1000000000' }, zkappState: null } } }); } return Response.json([]); }) as typeof fetch; @@ -121,3 +121,23 @@ describe('offline allocation snapshots', () => { expect(bundle.receiverAccountExists).toEqual({ 'child-a': true, 'child-b': true }); }); }); + +describe('account snapshots', () => { + it('fails the export when the node answers with an error instead of an account', async () => { + configure('testnet'); + globalThis.fetch = (async (url, init) => { + if (init?.method === 'POST') return Response.json({ errors: [{ message: 'node busy' }] }); + return Response.json([]); + }) as typeof fetch; + await expect(buildOfflineApproveBundle({ ...common, proposal })).rejects.toThrow('Could not fetch account'); + }); + + it('fails the export when an account answer lacks the fields the CLI decides from', async () => { + configure('testnet'); + globalThis.fetch = (async (url, init) => { + if (init?.method === 'POST') return Response.json({ data: { account: { publicKey: 'vault' } } }); + return Response.json([]); + }) as typeof fetch; + await expect(buildOfflineApproveBundle({ ...common, proposal })).rejects.toThrow('Incomplete account snapshot'); + }); +}); diff --git a/ui/tests/pending-tx-expiry.test.ts b/ui/tests/pending-tx-expiry.test.ts index 73577548..86253aec 100644 --- a/ui/tests/pending-tx-expiry.test.ts +++ b/ui/tests/pending-tx-expiry.test.ts @@ -68,6 +68,17 @@ describe('pending records the backend refused to track', () => { expect(getPendingTxs().map((r) => r.txHash)).toEqual(['tracked-old', 'untracked-new']); }); + test('a SubVault creation expires after 20 minutes too; other creations keep the day', () => { + const create = (proposalHash: string, txHash: string, ageMs: number, txType: string): PendingTx => ({ + ...record(proposalHash, txHash, ageMs), kind: 'create', + summary: { txType } as PendingTx['summary'], + }); + savePendingTx(create('7', 'transfer-old', 25 * MINUTE, 'transfer')); + savePendingTx(create('8', 'create-new', 5 * MINUTE, 'createChild')); + savePendingTx(create('9', 'create-old', 25 * MINUTE, 'createChild')); + expect(getPendingTxs().map((r) => r.txHash)).toEqual(['transfer-old', 'create-new']); + }); + test('marking touches only the record for that transaction', () => { savePendingTx(record('1', 'tx-a', 0)); changes = 0; @@ -119,15 +130,19 @@ describe('reportSubmission', () => { }); describe('backendNoLongerTracks', () => { - const row = (hash: string | null) => ({ lastApproveTxHash: hash, lastExecuteTxHash: null }); + const row = (hash: string | null, error: string | null = null) => ({ + lastApproveTxHash: hash, lastApproveError: error, lastExecuteTxHash: null, lastExecuteError: null, + }); const mine = { ...record('1', 'tx-mine', 0), recorded: true }; test('is true once a recorded report is not the hash the backend tracks', () => { expect(backendNoLongerTracks(mine, row('tx-other'))).toBe(true); // replaced, still live - // The replacement already failed, or was applied and cleared before this - // poll: the backend will never report tx-mine's failure either way. + expect(backendNoLongerTracks(mine, row('tx-other', 'dropped'))).toBe(true); // replaced, already failed + // The replacement was applied and cleared before this poll. expect(backendNoLongerTracks(mine, row(null))).toBe(true); expect(backendNoLongerTracks(mine, row('tx-mine'))).toBe(false); + // Our own failure is the caller's normal cleanup, not a lost report. + expect(backendNoLongerTracks(mine, row('tx-mine', 'dropped'))).toBe(false); }); test('does not judge a report the backend has not answered', () => { diff --git a/ui/tests/transaction-preflight.test.ts b/ui/tests/transaction-preflight.test.ts index 9ef378c7..35d6bd27 100644 --- a/ui/tests/transaction-preflight.test.ts +++ b/ui/tests/transaction-preflight.test.ts @@ -1,6 +1,7 @@ import { describe, expect, test } from 'bun:test'; import { checkTransactionState, nodeAccountReader } from '../lib/transaction-preflight'; -import { AccountUpdate, Bool, Field, PrivateKey, TokenId } from 'o1js'; +import { classifyDeployTarget } from '../lib/deploy-target'; +import { AccountUpdate, Bool, Field, PrivateKey, TokenId, fetchAccount } from 'o1js'; function update(publicKey: string, state: Array, appState = state.map(() => null) as Array, tokenId = '1') { return { body: { publicKey, tokenId, preconditions: { account: { state, isNew: null as boolean | null } }, update: { appState } } }; } @@ -49,13 +50,55 @@ describe('pre-broadcast state check', () => { }); expect(result.status).toBe('current'); expect(json).toBe(tx(AccountUpdate.toJSON(au))); }); - test('node reader bypasses caches and rejects incomplete/error responses', async () => { + test('reads a bare account as zeros; only an isNew precondition on it is stale', async () => { + const bare = async () => 'bare' as const; + expect((await checkTransactionState(tx(update('vault', [null, null], ['0', '7']), update('vault', ['0', '7'], ['1', '8'])), bare)).status).toBe('current'); + expect((await checkTransactionState(tx(update('vault', ['0'], ['1'])), bare)).status).toBe('current'); + expect((await checkTransactionState(tx(update('vault', ['5'])), bare)).status).toBe('stale'); + const item = update('vault', [null], ['1']); + item.body.preconditions.account.isNew = true; + expect((await checkTransactionState(tx(item), bare)).status).toBe('stale'); + }); + test('node reader bypasses caches, tells absent from bare, and rejects incomplete/error responses', async () => { const original = globalThis.fetch; + const serve = (payload: unknown) => { + globalThis.fetch = (async (_url, init) => { expect(init?.cache).toBe('no-store'); return new Response(JSON.stringify(payload)); }) as typeof fetch; + }; try { - for (const payload of [{ errors: [{ message: 'unavailable' }] }, { data: {} }, { data: { account: { zkappState: null } } }]) { - globalThis.fetch = (async (_url, init) => { expect(init?.cache).toBe('no-store'); return new Response(JSON.stringify(payload)); }) as typeof fetch; + for (const payload of [{ errors: [{ message: 'unavailable' }] }, { data: {} }, { data: { account: {} } }, { data: { account: { zkappState: '7' } } }]) { + serve(payload); await expect(nodeAccountReader('http://node')('vault', '1')).rejects.toThrow(); } + serve({ data: { account: null } }); + expect(await nodeAccountReader('http://node')('vault', '1')).toBeNull(); + serve({ data: { account: { zkappState: null } } }); + expect(await nodeAccountReader('http://node')('vault', '1')).toBe('bare'); + serve({ data: { account: { zkappState: ['1', '2'] } } }); + expect(await nodeAccountReader('http://node')('vault', '1')).toEqual(['1', '2']); + } finally { globalThis.fetch = original; } + }); + test('a deployment into a bare account passes the target check and the state check from one node answer', async () => { + const address = PrivateKey.random().toPublicKey(); + // What the node answers for an account a plain payment created: every zkApp field is null. + const bareAccount = { + publicKey: address.toBase58(), token: TokenId.toBase58(TokenId.default), nonce: '0', balance: { total: '1000000000' }, + tokenSymbol: '', receiptChainHash: null, delegateAccount: null, votingFor: null, permissions: null, + timing: { initialMinimumBalance: null, cliffTime: null, cliffAmount: null, vestingPeriod: null, vestingIncrement: null }, + zkappState: null, verificationKey: null, actionState: null, provedState: null, zkappUri: null, + }; + const deploy = AccountUpdate.create(address); + deploy.body.update.appState[0] = { isSome: Bool(true), value: Field(7) }; + const setup = AccountUpdate.create(address); + setup.body.preconditions.account.state[0] = { isSome: Bool(true), value: Field(7) }; + setup.body.update.appState[0] = { isSome: Bool(true), value: Field(8) }; + const json = tx(AccountUpdate.toJSON(deploy), AccountUpdate.toJSON(setup)); + const original = globalThis.fetch; + try { + globalThis.fetch = (async () => new Response(JSON.stringify({ data: { account: bareAccount } }))) as unknown as typeof fetch; + expect(classifyDeployTarget(await fetchAccount({ publicKey: address }, 'http://node'))).toBe('existing'); + expect((await checkTransactionState(json, nodeAccountReader('http://node'))).status).toBe('current'); + globalThis.fetch = (async () => new Response(JSON.stringify({ data: { account: {} } }))) as unknown as typeof fetch; + expect((await checkTransactionState(json, nodeAccountReader('http://node'))).status).toBe('unavailable'); } finally { globalThis.fetch = original; } }); });