diff --git a/docs/MULTISIG_SDK.md b/docs/MULTISIG_SDK.md index bc662812..9c555d5a 100644 --- a/docs/MULTISIG_SDK.md +++ b/docs/MULTISIG_SDK.md @@ -403,12 +403,18 @@ SDK's own built-in execution uses. Extending a transaction's advice map with it therefore does not collide with the transaction's ordinary inputs; the integration extends rather than replaces its advice map. -> **Security:** for first-party types the SDK reconstructs the transaction from -> metadata and checks it against the signed `tx_summary` commitment. For custom -> types there is no such reconstruction, so the SDK cannot verify that display -> metadata (e.g. `description`) matches what the transaction actually does. -> Cosigners must verify the raw `tx_summary` they are signing — not trust the -> label or description. +> **Security:** for first-party types the SDK verifies that the signed +> `tx_summary` matches the proposal's metadata before a cosigner signs it. It +> decodes the summary and asserts the transaction's effects — its output notes, +> its consumed input notes, and the account-storage slots it changes — are +> *exactly* what the metadata describes and nothing more, so a cosigner is shown +> what they actually sign. The check is deterministic (it never reads the +> block-dependent transaction fee, which is why it works across cosigners at +> different sync heights). Two residual gaps: it cannot see **non-fungible** +> assets (the SDK exposes only fungible note/vault assets), and for **custom** +> types there is no metadata recipe at all, so the SDK cannot verify that display +> metadata (e.g. `description`) matches what the transaction does. Cosigners must +> verify the raw `tx_summary` for `custom` proposals — not trust the label. ### Offline Workflow @@ -584,13 +590,15 @@ await multisig.exportNoteToFile(noteId); const importedNoteId = await multisig.importNoteFromBytes(noteFileBytes); ``` -> **Note:** every cosigner device that verifies or signs the consume-notes -> proposal needs the note in its local store with the on-chain inclusion -> proof — deliver the note file to each of them (import + sync), not just to -> the proposer. A cosigner whose store lacks the authenticated note rebuilds -> the transaction differently (the input-notes commitment distinguishes -> authenticated from unauthenticated consumption) and rejects the proposal -> with `metadata does not match tx_summary`. The sender's own device heals +> **Note:** the device that **executes** the consume-notes proposal needs the +> note in its local store with the on-chain inclusion proof — deliver the note +> file to each cosigner (import + sync), any of whom may execute, not just to +> the proposer. Verifying and signing no longer require the note: the SDK checks +> the proposal by comparing the signed summary's input-note **ids** to the +> metadata (note ids are proof-agnostic), so a cosigner can sign without the +> note. But the device that executes rebuilds the consumption from its own store, +> so a store lacking the authenticated note produces a transaction that no longer +> matches the signed summary and fails on-chain. The sender's own device heals > itself: it already knows the full note, so a post-commit sync is enough. #### Consume Notes (Claim Received Funds) diff --git a/packages/miden-multisig-client/package.json b/packages/miden-multisig-client/package.json index dde64048..16c0edf4 100644 --- a/packages/miden-multisig-client/package.json +++ b/packages/miden-multisig-client/package.json @@ -36,7 +36,7 @@ "test:watch": "npm run generate:masm && vitest" }, "dependencies": { - "@miden-sdk/miden-sdk": "^0.15.8", + "@miden-sdk/miden-sdk": "^0.15.10", "@noble/curves": "^1.9.7", "@noble/hashes": "^2.0.1", "@openzeppelin/guardian-client": "^0.16.2" diff --git a/packages/miden-multisig-client/src/multisig.test.ts b/packages/miden-multisig-client/src/multisig.test.ts index 7522dd56..449fbd4b 100644 --- a/packages/miden-multisig-client/src/multisig.test.ts +++ b/packages/miden-multisig-client/src/multisig.test.ts @@ -7,6 +7,7 @@ import { buildUpdateSignersTransactionRequest, executeForSummary, } from './transaction.js'; +import { assertMetadataMatchesSummary } from './multisig/summaryBinding.js'; const { mockRpcGetAccountDetails, mockAccountDeserialize, mockDetectConfig, mockNoteFileDeserialize } = vi.hoisted(() => ({ mockRpcGetAccountDetails: vi.fn(), @@ -139,6 +140,14 @@ vi.mock('./inspector.js', () => ({ }, })); +// The metadata<->summary binding is tested exhaustively in +// ./multisig/summaryBinding.test.ts. Here it is mocked to a no-op so these +// tests exercise the surrounding flows; individual tests override it to throw +// when they want to assert that a binding rejection propagates. +vi.mock('./multisig/summaryBinding.js', () => ({ + assertMetadataMatchesSummary: vi.fn(), +})); + // Mock fetch for GUARDIAN client const mockFetch = vi.fn(); vi.stubGlobal('fetch', mockFetch); @@ -1042,11 +1051,9 @@ describe('Multisig', () => { }), }); - vi.mocked(executeForSummary).mockResolvedValueOnce({ - toCommitment: () => ({ - toHex: () => '0x' + 'f'.repeat(64), - }), - } as any); + vi.mocked(assertMetadataMatchesSummary).mockImplementationOnce((id: string) => { + throw new Error(`Invalid proposal: metadata does not match tx_summary for ${id}`); + }); await expect(multisig.syncProposals()).rejects.toThrow( 'Invalid proposal: metadata does not match tx_summary' @@ -1289,11 +1296,9 @@ describe('Multisig', () => { }), }); - vi.mocked(executeForSummary).mockResolvedValueOnce({ - toCommitment: () => ({ - toHex: () => '0x' + 'f'.repeat(64), - }), - } as any); + vi.mocked(assertMetadataMatchesSummary).mockImplementationOnce((id: string) => { + throw new Error(`Invalid proposal: metadata does not match tx_summary for ${id}`); + }); await expect( multisig.createProposal(1, 'AQID', { @@ -2024,11 +2029,9 @@ describe('Multisig', () => { description: '', }); - vi.mocked(executeForSummary).mockResolvedValueOnce({ - toCommitment: () => ({ - toHex: () => '0x' + 'f'.repeat(64), - }), - } as any); + vi.mocked(assertMetadataMatchesSummary).mockImplementationOnce((id: string) => { + throw new Error(`Invalid proposal: metadata does not match tx_summary for ${id}`); + }); await expect(multisig.signProposal('0x' + 'c'.repeat(64))).rejects.toThrow( 'Invalid proposal: metadata does not match tx_summary' @@ -2091,11 +2094,9 @@ describe('Multisig', () => { const multisig = createTestMultisig(config); - vi.mocked(executeForSummary).mockResolvedValueOnce({ - toCommitment: () => ({ - toHex: () => '0x' + 'f'.repeat(64), - }), - } as any); + vi.mocked(assertMetadataMatchesSummary).mockImplementationOnce((id: string) => { + throw new Error(`Invalid proposal: metadata does not match tx_summary for ${id}`); + }); await expect( multisig.importProposal( @@ -2127,12 +2128,6 @@ describe('Multisig', () => { const multisig = createTestMultisig(config); - vi.mocked(executeForSummary).mockResolvedValueOnce({ - toCommitment: () => ({ - toHex: () => '0x' + 'c'.repeat(64), - }), - } as any); - const proposal = await multisig.importProposal( JSON.stringify({ accountId: '0x' + 'a'.repeat(30), @@ -2156,11 +2151,9 @@ describe('Multisig', () => { description: '', }; - vi.mocked(executeForSummary).mockResolvedValueOnce({ - toCommitment: () => ({ - toHex: () => '0x' + 'f'.repeat(64), - }), - } as any); + vi.mocked(assertMetadataMatchesSummary).mockImplementationOnce((id: string) => { + throw new Error(`Invalid proposal: metadata does not match tx_summary for ${id}`); + }); await expect(multisig.signProposalOffline(proposal.id)).rejects.toThrow( 'Invalid proposal: metadata does not match tx_summary' @@ -2486,17 +2479,13 @@ describe('Multisig', () => { const ackSignature = '0x' + '6'.repeat(130); const finalRequest = { kind: 'final-change-threshold-request' }; - vi.mocked(buildUpdateSignersTransactionRequest) - .mockResolvedValueOnce({ - request: { kind: 'verify-change-threshold-request' }, - salt: { toHex: () => '0x' + 'd'.repeat(64) }, - configHash: { toHex: () => '0x' + 'e'.repeat(64) }, - } as any) - .mockResolvedValueOnce({ - request: finalRequest, - salt: { toHex: () => '0x' + 'd'.repeat(64) }, - configHash: { toHex: () => '0x' + 'e'.repeat(64) }, - } as any); + // Only one reconstruction now: verifyProposalMetadataBinding no longer + // rebuilds the request, so it is built once for execution. + vi.mocked(buildUpdateSignersTransactionRequest).mockResolvedValueOnce({ + request: finalRequest, + salt: { toHex: () => '0x' + 'd'.repeat(64) }, + configHash: { toHex: () => '0x' + 'e'.repeat(64) }, + } as any); (multisig as any).proposals.set(cachedProposalId, { id: cachedProposalId, @@ -2723,11 +2712,9 @@ describe('Multisig', () => { const multisig = createTestMultisig(config); const proposalId = '0x' + 'c'.repeat(64); - vi.mocked(executeForSummary).mockResolvedValueOnce({ - toCommitment: () => ({ - toHex: () => '0x' + 'd'.repeat(64), - }), - } as any); + vi.mocked(assertMetadataMatchesSummary).mockImplementationOnce((id: string) => { + throw new Error(`Invalid proposal: metadata does not match tx_summary for ${id}`); + }); (multisig as any).proposals.set(proposalId, { id: proposalId, @@ -2881,17 +2868,13 @@ describe('Multisig', () => { const proposalId = '0x' + 'c'.repeat(64); const finalRequest = { kind: 'fresh-message-word-request' }; - vi.mocked(buildUpdateSignersTransactionRequest) - .mockResolvedValueOnce({ - request: { kind: 'verify-change-threshold-request' }, - salt: { toHex: () => '0x' + 'd'.repeat(64) }, - configHash: { toHex: () => '0x' + 'e'.repeat(64) }, - } as any) - .mockResolvedValueOnce({ - request: finalRequest, - salt: { toHex: () => '0x' + 'd'.repeat(64) }, - configHash: { toHex: () => '0x' + 'e'.repeat(64) }, - } as any); + // Only one reconstruction now: verifyProposalMetadataBinding no longer + // rebuilds the request, so it is built once for execution. + vi.mocked(buildUpdateSignersTransactionRequest).mockResolvedValueOnce({ + request: finalRequest, + salt: { toHex: () => '0x' + 'd'.repeat(64) }, + configHash: { toHex: () => '0x' + 'e'.repeat(64) }, + } as any); (multisig as any).proposals.set(proposalId, { id: proposalId, diff --git a/packages/miden-multisig-client/src/multisig.ts b/packages/miden-multisig-client/src/multisig.ts index 2cf17b67..88d860c0 100644 --- a/packages/miden-multisig-client/src/multisig.ts +++ b/packages/miden-multisig-client/src/multisig.ts @@ -74,6 +74,7 @@ import { } from './utils/signature.js'; import { computeCommitmentFromTxSummary, accountIdToHex } from './multisig/helpers.js'; import { buildGuardianSignatureFromSigner } from './multisig/signing.js'; +import { assertMetadataMatchesSummary } from './multisig/summaryBinding.js'; import { AccountInspector } from './inspector.js'; import { ProposalFactory } from './proposal/factory.js'; import { ProposalMetadataCodec } from './proposal/metadata.js'; @@ -1834,40 +1835,23 @@ export class Multisig { return txSummaryCommitment; } + /** + * Verify a proposal's integrity and return the tx_summary commitment cosigners + * sign. The id must equal the commitment of the stored tx_summary, and the + * human-readable metadata must match what the signed summary actually does. + * + * The metadata↔summary binding is done by decoding the signed summary and + * comparing its intent-bearing components (output/input notes, storage deltas) + * against the metadata — NOT by re-executing. Re-execution was block-height + * dependent (the fee is derived from the reference block and is part of the + * summary's account delta), so it falsely rejected honest proposals whenever a + * cosigner synced at a different height than the proposer. See + * {@link assertMetadataMatchesSummary}. + */ private async verifyProposalMetadataBinding(proposal: Proposal): Promise { const txSummaryCommitment = this.ensureProposalCommitmentMatchesSummary(proposal); - if (proposal.metadata.proposalType === 'custom') { - // Custom proposals (issue #266) have no per-type reconstruction recipe; - // the id ↔ tx_summary commitment match above is the only available - // integrity guarantee for an opaque proposal. - return txSummaryCommitment; - } - - if (proposal.metadata.proposalType === 'switch_guardian') { - // Exempt from binding re-execution (mirrors the `custom` exemption above). - // The WASM `executeForSummary` leaves the guardian-disabling side effect - // applied to the in-session account, so re-execution reconstructs a smaller - // delta and falsely rejects with "metadata does not match tx_summary". The - // native Rust client does not mutate, so this is an intentional divergence. - // The id ↔ tx_summary match above plus `verifyGuardianEndpointCommitment` - // at propose/execute time still bind the proposal. - return txSummaryCommitment; - } - const summary = TransactionSummary.deserialize(base64ToUint8Array(proposal.txSummary)); - const salt = proposal.metadata.saltHex - ? Word.fromHex(normalizeHexWord(proposal.metadata.saltHex)) - : summary.salt(); - - const request = await this.buildTransactionRequestFromMetadata(proposal.metadata, salt); - const webClient = await this.getRawClient(); - const reconstructed = await executeForSummary(webClient, this._accountId, request); - const reconstructedCommitment = normalizeHexWord(reconstructed.toCommitment().toHex()); - - if (reconstructedCommitment !== txSummaryCommitment) { - throw new Error(`Invalid proposal: metadata does not match tx_summary for ${proposal.id}`); - } - + assertMetadataMatchesSummary(proposal.id, proposal.metadata, summary); return txSummaryCommitment; } diff --git a/packages/miden-multisig-client/src/multisig/summaryBinding.test.ts b/packages/miden-multisig-client/src/multisig/summaryBinding.test.ts new file mode 100644 index 00000000..08521d0f --- /dev/null +++ b/packages/miden-multisig-client/src/multisig/summaryBinding.test.ts @@ -0,0 +1,482 @@ +import { describe, it, expect } from 'vitest'; +import { + AccountId, + Felt, + FeltArray, + NoteId, + NoteRecipient, + NoteScript, + NoteStorage, + Word, +} from '@miden-sdk/miden-sdk'; + +import { assertMetadataMatchesSummary } from './summaryBinding.js'; +import { deriveP2idSerialNumber } from '../transaction/p2id.js'; +import { getProcedureRoot } from '../procedures.js'; +import { normalizeHexWord } from '../utils/encoding.js'; +import type { ProposalMetadata } from '../types/proposal.js'; + +const PROPOSAL_ID = '0x' + 'c'.repeat(64); +const SALT = Word.fromHex('0x' + 'ab'.repeat(32)); + +// Slot names must match src/account/masm/auth.ts. +const THRESHOLD_CONFIG_SLOT = 'openzeppelin::multisig::threshold_config'; +const SIGNER_PUBLIC_KEYS_SLOT = 'openzeppelin::multisig::signer_public_keys'; +const PROCEDURE_THRESHOLDS_SLOT = 'openzeppelin::multisig::procedure_thresholds'; +const GUARDIAN_PUBLIC_KEY_SLOT = 'openzeppelin::guardian::public_key'; + +// Two known-valid Miden account ids (arbitrary hex has an invalid version byte). +const ACCOUNT_1 = '0x69817bcc6fb9f99127c2245f6979c5'; +const ACCOUNT_2 = '0x79817bcc6fb9f99127c2245f6979ef'; +const FAUCET = AccountId.fromHex(ACCOUNT_2); + +interface SummaryParts { + salt?: Word; + outputNotes?: unknown[]; + inputNotes?: unknown[]; + valueDeltas?: { slotName: string; valueHex: string }[]; + maps?: { slotName: string; entries: { keyHex: string; valueHex: string }[] }[]; +} + +function mockSummary(parts: SummaryParts): any { + return { + salt: () => parts.salt ?? SALT, + outputNotes: () => ({ + notes: () => parts.outputNotes ?? [], + numNotes: () => (parts.outputNotes ?? []).length, + }), + inputNotes: () => ({ + notes: () => parts.inputNotes ?? [], + numNotes: () => (parts.inputNotes ?? []).length, + }), + accountDelta: () => ({ + storage: () => ({ + valueDeltas: () => + (parts.valueDeltas ?? []).map((v) => ({ + slotName: v.slotName, + value: { toHex: () => v.valueHex }, + })), + maps: () => + (parts.maps ?? []).map((m) => ({ + slotName: m.slotName, + entries: () => + m.entries.map((e) => ({ + key: { toHex: () => e.keyHex }, + value: { toHex: () => e.valueHex }, + })), + })), + }), + }), + }; +} + +function wordHex(felts: bigint[]): string { + return normalizeHexWord(Word.newFromFelts(felts.map((f) => new Felt(f))).toHex()); +} + +function p2idRecipientDigest(recipientId: string, salt: Word): string { + const recipient = AccountId.fromHex(recipientId); + const serialNum = deriveP2idSerialNumber(salt); + const noteRecipient = new NoteRecipient( + serialNum, + NoteScript.p2id(), + new NoteStorage(new FeltArray([recipient.suffix(), recipient.prefix()])), + ); + return normalizeHexWord(noteRecipient.digest().toHex()); +} + +function p2idOutputNote(recipientDigestHex: string, faucet: AccountId, amount: bigint): unknown { + return { + recipientDigest: () => ({ toHex: () => recipientDigestHex }), + assets: () => ({ + fungibleAssets: () => [{ faucetId: () => faucet, amount: () => amount }], + }), + }; +} + +function inputNote(idHex: string): unknown { + return { id: () => ({ toString: () => idHex }) }; +} + +function expectReject(metadata: ProposalMetadata, summary: any): void { + expect(() => assertMetadataMatchesSummary(PROPOSAL_ID, metadata, summary)).toThrow( + `Invalid proposal: metadata does not match tx_summary for ${PROPOSAL_ID}`, + ); +} + +function expectPass(metadata: ProposalMetadata, summary: any): void { + expect(() => assertMetadataMatchesSummary(PROPOSAL_ID, metadata, summary)).not.toThrow(); +} + +describe('assertMetadataMatchesSummary', () => { + describe('p2id', () => { + const recipientId = ACCOUNT_1; + const faucetId = FAUCET.toString(); + const metadata: ProposalMetadata = { + proposalType: 'p2id', + recipientId, + faucetId, + amount: '1000', + description: '', + }; + const digest = p2idRecipientDigest(recipientId, SALT); + + it('passes when the output note matches recipient, faucet and amount', () => { + expectPass(metadata, mockSummary({ outputNotes: [p2idOutputNote(digest, FAUCET, 1000n)] })); + }); + + it('rejects a different recipient (mislabel)', () => { + const wrongDigest = p2idRecipientDigest(ACCOUNT_2, SALT); + expectReject(metadata, mockSummary({ outputNotes: [p2idOutputNote(wrongDigest, FAUCET, 1000n)] })); + }); + + it('rejects a different amount', () => { + expectReject(metadata, mockSummary({ outputNotes: [p2idOutputNote(digest, FAUCET, 9999n)] })); + }); + + it('rejects a different faucet', () => { + const otherFaucet = AccountId.fromHex(ACCOUNT_1); + expectReject(metadata, mockSummary({ outputNotes: [p2idOutputNote(digest, otherFaucet, 1000n)] })); + }); + + it('rejects an extra output note (unexpected value leaving)', () => { + const extra = p2idOutputNote(p2idRecipientDigest(ACCOUNT_2, SALT), FAUCET, 1n); + expectReject(metadata, mockSummary({ outputNotes: [p2idOutputNote(digest, FAUCET, 1000n), extra] })); + }); + + it('rejects when no output note is present', () => { + expectReject(metadata, mockSummary({ outputNotes: [] })); + }); + + it('rejects a note carrying an extra fungible asset beyond the declared one', () => { + // Correct recipient + declared asset, but an additional asset would leave + // the account beyond what the metadata describes. + const noteWithExtra = { + recipientDigest: () => ({ toHex: () => digest }), + assets: () => ({ + fungibleAssets: () => [ + { faucetId: () => FAUCET, amount: () => 1000n }, + { faucetId: () => AccountId.fromHex(ACCOUNT_1), amount: () => 500n }, + ], + }), + }; + expectReject(metadata, mockSummary({ outputNotes: [noteWithExtra] })); + }); + + it('rejects a piggybacked value-slot write (threshold takeover)', () => { + // Correct p2id note, but the summary ALSO rewrites the threshold config. + expectReject( + metadata, + mockSummary({ + outputNotes: [p2idOutputNote(digest, FAUCET, 1000n)], + valueDeltas: [{ slotName: THRESHOLD_CONFIG_SLOT, valueHex: wordHex([1n, 1n, 0n, 0n]) }], + }), + ); + }); + + it('rejects a piggybacked signer-map write (signer-set takeover)', () => { + // Correct p2id note, but the summary ALSO installs an attacker signer. + expectReject( + metadata, + mockSummary({ + outputNotes: [p2idOutputNote(digest, FAUCET, 1000n)], + maps: [ + { + slotName: SIGNER_PUBLIC_KEYS_SLOT, + entries: [{ keyHex: wordHex([0n, 0n, 0n, 0n]), valueHex: normalizeHexWord('0x' + '9'.repeat(64)) }], + }, + ], + }), + ); + }); + + it('rejects a piggybacked input note', () => { + expectReject( + metadata, + mockSummary({ + outputNotes: [p2idOutputNote(digest, FAUCET, 1000n)], + inputNotes: [inputNote(NoteId.fromHex('0x' + '44'.repeat(32)).toString())], + }), + ); + }); + }); + + describe('consume_notes (v1)', () => { + const idA = NoteId.fromHex('0x' + '11'.repeat(32)).toString(); + const idB = NoteId.fromHex('0x' + '22'.repeat(32)).toString(); + const metadata: ProposalMetadata = { + proposalType: 'consume_notes', + noteIds: ['0x' + '11'.repeat(32), '0x' + '22'.repeat(32)], + description: '', + }; + + it('passes on exact set equality', () => { + expectPass(metadata, mockSummary({ inputNotes: [inputNote(idA), inputNote(idB)] })); + }); + + it('rejects an extra consumed note', () => { + const idC = NoteId.fromHex('0x' + '33'.repeat(32)).toString(); + expectReject(metadata, mockSummary({ inputNotes: [inputNote(idA), inputNote(idB), inputNote(idC)] })); + }); + + it('rejects a missing declared note', () => { + expectReject(metadata, mockSummary({ inputNotes: [inputNote(idA)] })); + }); + + it('rejects a substituted note', () => { + const idC = NoteId.fromHex('0x' + '33'.repeat(32)).toString(); + expectReject(metadata, mockSummary({ inputNotes: [inputNote(idA), inputNote(idC)] })); + }); + + it('rejects a consume that also emits an output note (drain)', () => { + // Declared input set matches, but the summary ALSO sends value out. + const drain = p2idOutputNote(p2idRecipientDigest(ACCOUNT_2, SALT), FAUCET, 1_000_000n); + expectReject( + metadata, + mockSummary({ inputNotes: [inputNote(idA), inputNote(idB)], outputNotes: [drain] }), + ); + }); + + it('rejects a consume that also writes storage (takeover)', () => { + expectReject( + metadata, + mockSummary({ + inputNotes: [inputNote(idA), inputNote(idB)], + valueDeltas: [{ slotName: THRESHOLD_CONFIG_SLOT, valueHex: wordHex([1n, 1n, 0n, 0n]) }], + }), + ); + }); + }); + + describe('add/remove/change_signer', () => { + const signers = ['0x' + '1'.repeat(64), '0x' + '2'.repeat(64)]; + const metadata: ProposalMetadata = { + proposalType: 'add_signer', + targetThreshold: 2, + targetSignerCommitments: signers, + description: '', + }; + const configHex = wordHex([2n, 2n, 0n, 0n]); + + function pubkeyMap(values: string[]) { + return [ + { + slotName: SIGNER_PUBLIC_KEYS_SLOT, + entries: values.map((v, i) => ({ keyHex: wordHex([BigInt(i), 0n, 0n, 0n]), valueHex: normalizeHexWord(v) })), + }, + ]; + } + + it('passes on matching threshold/count and target signer set', () => { + expectPass( + metadata, + mockSummary({ + valueDeltas: [{ slotName: THRESHOLD_CONFIG_SLOT, valueHex: configHex }], + maps: pubkeyMap(signers), + }), + ); + }); + + it('tolerates a zero-value (removal) map entry', () => { + expectPass( + metadata, + mockSummary({ + valueDeltas: [{ slotName: THRESHOLD_CONFIG_SLOT, valueHex: configHex }], + maps: [ + { + slotName: SIGNER_PUBLIC_KEYS_SLOT, + entries: [ + { keyHex: wordHex([0n, 0n, 0n, 0n]), valueHex: normalizeHexWord(signers[0]) }, + { keyHex: wordHex([1n, 0n, 0n, 0n]), valueHex: normalizeHexWord(signers[1]) }, + { keyHex: wordHex([2n, 0n, 0n, 0n]), valueHex: normalizeHexWord('0x' + '00'.repeat(32)) }, + ], + }, + ], + }), + ); + }); + + it('rejects a changed threshold', () => { + expectReject( + metadata, + mockSummary({ + valueDeltas: [{ slotName: THRESHOLD_CONFIG_SLOT, valueHex: wordHex([1n, 2n, 0n, 0n]) }], + maps: pubkeyMap(signers), + }), + ); + }); + + it('rejects a changed signer count', () => { + expectReject( + metadata, + mockSummary({ + valueDeltas: [{ slotName: THRESHOLD_CONFIG_SLOT, valueHex: wordHex([2n, 3n, 0n, 0n]) }], + maps: pubkeyMap(signers), + }), + ); + }); + + it('rejects an unlisted (attacker) signer in the pubkey map', () => { + const attacker = '0x' + '9'.repeat(64); + expectReject( + metadata, + mockSummary({ + valueDeltas: [{ slotName: THRESHOLD_CONFIG_SLOT, valueHex: configHex }], + maps: pubkeyMap([signers[0], attacker]), + }), + ); + }); + + it('rejects when the threshold-config value slot is absent', () => { + expectReject(metadata, mockSummary({ valueDeltas: [], maps: pubkeyMap(signers) })); + }); + + it('rejects a duplicated declared signer with an omitted one', () => { + // Declared {A,B}; summary writes index 0->A and index 1->A (B omitted, A + // duplicated). Index 1's value (A) is not the declared signer at index 1 (B). + expectReject( + metadata, + mockSummary({ + valueDeltas: [{ slotName: THRESHOLD_CONFIG_SLOT, valueHex: configHex }], + maps: pubkeyMap([signers[0], signers[0]]), + }), + ); + }); + + it('rejects a signer proposal that also emits an output note (drain)', () => { + const drain = p2idOutputNote(p2idRecipientDigest(ACCOUNT_2, SALT), FAUCET, 1_000_000n); + expectReject( + metadata, + mockSummary({ + outputNotes: [drain], + valueDeltas: [{ slotName: THRESHOLD_CONFIG_SLOT, valueHex: configHex }], + maps: pubkeyMap(signers), + }), + ); + }); + + it('rejects a signer proposal that writes an unexpected extra slot', () => { + expectReject( + metadata, + mockSummary({ + valueDeltas: [ + { slotName: THRESHOLD_CONFIG_SLOT, valueHex: configHex }, + { slotName: PROCEDURE_THRESHOLDS_SLOT, valueHex: wordHex([9n, 0n, 0n, 0n]) }, + ], + maps: pubkeyMap(signers), + }), + ); + }); + }); + + describe('update_procedure_threshold', () => { + const metadata: ProposalMetadata = { + proposalType: 'update_procedure_threshold', + targetProcedure: 'send_asset', + targetThreshold: 3, + description: '', + }; + const procRoot = normalizeHexWord(getProcedureRoot('send_asset')); + const valueHex = wordHex([3n, 0n, 0n, 0n]); + + it('passes when the procedure root maps to the target threshold', () => { + expectPass( + metadata, + mockSummary({ maps: [{ slotName: PROCEDURE_THRESHOLDS_SLOT, entries: [{ keyHex: procRoot, valueHex }] }] }), + ); + }); + + it('rejects a different threshold value', () => { + expectReject( + metadata, + mockSummary({ + maps: [{ slotName: PROCEDURE_THRESHOLDS_SLOT, entries: [{ keyHex: procRoot, valueHex: wordHex([1n, 0n, 0n, 0n]) }] }], + }), + ); + }); + + it('rejects when the declared procedure is not in the delta', () => { + const otherRoot = normalizeHexWord(getProcedureRoot('receive_asset')); + expectReject( + metadata, + mockSummary({ maps: [{ slotName: PROCEDURE_THRESHOLDS_SLOT, entries: [{ keyHex: otherRoot, valueHex }] }] }), + ); + }); + + it('rejects a procedure-threshold proposal that also emits an output note', () => { + const drain = p2idOutputNote(p2idRecipientDigest(ACCOUNT_2, SALT), FAUCET, 1_000_000n); + expectReject( + metadata, + mockSummary({ + outputNotes: [drain], + maps: [{ slotName: PROCEDURE_THRESHOLDS_SLOT, entries: [{ keyHex: procRoot, valueHex }] }], + }), + ); + }); + }); + + describe('switch_guardian', () => { + const newGuardianPubkey = '0x' + '7'.repeat(64); + const metadata: ProposalMetadata = { + proposalType: 'switch_guardian', + newGuardianPubkey, + description: '', + }; + const guardianMap = (pubkey: string) => [ + { + slotName: GUARDIAN_PUBLIC_KEY_SLOT, + entries: [{ keyHex: wordHex([0n, 0n, 0n, 0n]), valueHex: normalizeHexWord(pubkey) }], + }, + ]; + + it('passes when the guardian public-key map is set to the declared key', () => { + expectPass(metadata, mockSummary({ maps: guardianMap(newGuardianPubkey) })); + }); + + it('rejects a different guardian public key in the summary', () => { + expectReject(metadata, mockSummary({ maps: guardianMap('0x' + '8'.repeat(64)) })); + }); + + it('rejects when no guardian public-key delta is present', () => { + expectReject(metadata, mockSummary({})); + }); + + it('rejects a switch_guardian that also emits a drain output note', () => { + const drain = p2idOutputNote(p2idRecipientDigest(ACCOUNT_2, SALT), FAUCET, 1_000_000n); + expectReject(metadata, mockSummary({ outputNotes: [drain], maps: guardianMap(newGuardianPubkey) })); + }); + + it('rejects a switch_guardian that piggybacks a signer-set write', () => { + expectReject( + metadata, + mockSummary({ + maps: [ + ...guardianMap(newGuardianPubkey), + { + slotName: SIGNER_PUBLIC_KEYS_SLOT, + entries: [{ keyHex: wordHex([0n, 0n, 0n, 0n]), valueHex: normalizeHexWord('0x' + '9'.repeat(64)) }], + }, + ], + }), + ); + }); + + it('rejects a switch_guardian that disables the guardian (selector off)', () => { + expectReject( + metadata, + mockSummary({ + maps: guardianMap(newGuardianPubkey), + valueDeltas: [ + { slotName: 'openzeppelin::guardian::selector', valueHex: wordHex([0n, 0n, 0n, 0n]) }, + ], + }), + ); + }); + }); + + describe('custom', () => { + it('is exempt (no reconstruction recipe; WYSIWYS does not hold)', () => { + expectPass({ proposalType: 'custom', rawProposalType: 'x', description: '' } as ProposalMetadata, mockSummary({})); + }); + }); +}); diff --git a/packages/miden-multisig-client/src/multisig/summaryBinding.ts b/packages/miden-multisig-client/src/multisig/summaryBinding.ts new file mode 100644 index 00000000..7717e8ba --- /dev/null +++ b/packages/miden-multisig-client/src/multisig/summaryBinding.ts @@ -0,0 +1,435 @@ +import { + AccountId, + Felt, + FeltArray, + Note, + NoteId, + NoteRecipient, + NoteScript, + NoteStorage, + Word, +} from '@miden-sdk/miden-sdk'; +import type { TransactionSummary } from '@miden-sdk/miden-sdk'; + +import { getProcedureRoot } from '../procedures.js'; +import { deriveP2idSerialNumber } from '../transaction/p2id.js'; +import { noteFromBase64, normalizeHexWord } from '../utils/encoding.js'; +import { + isConsumeNotesV2, + type ConsumeNotesProposalMetadata, + type P2IdProposalMetadata, + type ProposalMetadata, + type SwitchGuardianProposalMetadata, + type UpdateProcedureThresholdProposalMetadata, + type UpdateSignersProposalMetadata, +} from '../types/proposal.js'; + +// Storage slot names (see src/account/masm/auth.ts). +const THRESHOLD_CONFIG_SLOT = 'openzeppelin::multisig::threshold_config'; +const SIGNER_PUBLIC_KEYS_SLOT = 'openzeppelin::multisig::signer_public_keys'; +const SIGNER_SCHEME_IDS_SLOT = 'openzeppelin::multisig::signer_scheme_ids'; +const PROCEDURE_THRESHOLDS_SLOT = 'openzeppelin::multisig::procedure_thresholds'; +// Every multisig transaction writes this map (a per-tx replay marker), so it is +// always an allowed storage slot. +const EXECUTED_TXS_SLOT = 'openzeppelin::multisig::executed_transactions'; +const GUARDIAN_SELECTOR_SLOT = 'openzeppelin::guardian::selector'; +const GUARDIAN_PUBLIC_KEY_SLOT = 'openzeppelin::guardian::public_key'; +const GUARDIAN_SCHEME_ID_SLOT = 'openzeppelin::guardian::scheme_id'; + +const ZERO_WORD_HEX = normalizeHexWord(`0x${'00'.repeat(32)}`); + +function reject(proposalId: string): never { + throw new Error(`Invalid proposal: metadata does not match tx_summary for ${proposalId}`); +} + +function wordHexFromFelts(felts: bigint[]): string { + return normalizeHexWord(Word.newFromFelts(felts.map((f) => new Felt(f))).toHex()); +} + +// A shared view of the storage delta, plus the slot-scoping helpers used by every +// per-type check to prove the transaction touches ONLY the slots that type may. +type StorageDelta = ReturnType['storage']>; + +function storageOf(summary: TransactionSummary): StorageDelta { + return summary.accountDelta().storage(); +} + +/** No output notes: nothing leaves the account (beyond the fee, a vault delta). */ +function assertNoOutputNotes(proposalId: string, summary: TransactionSummary): void { + if (summary.outputNotes().numNotes() !== 0) { + reject(proposalId); + } +} + +/** No input notes: the transaction consumes nothing. */ +function assertNoInputNotes(proposalId: string, summary: TransactionSummary): void { + if (summary.inputNotes().numNotes() !== 0) { + reject(proposalId); + } +} + +/** + * Every storage slot the transaction changed (value slots and map slots) must be + * in `allowed`. This is the load-bearing exhaustiveness check: it stops an + * attacker from piggybacking an undeclared storage effect (e.g. rewriting the + * signer set inside a "p2id" proposal). + */ +function assertStorageSlotsWithin( + proposalId: string, + storage: StorageDelta, + allowed: ReadonlySet, +): void { + for (const v of storage.valueDeltas()) { + if (!allowed.has(v.slotName)) { + reject(proposalId); + } + } + for (const m of storage.maps()) { + if (!allowed.has(m.slotName)) { + reject(proposalId); + } + } +} + +function valueDeltaFor(storage: StorageDelta, slotName: string): string | undefined { + const delta = storage.valueDeltas().find((v) => v.slotName === slotName); + return delta ? normalizeHexWord(delta.value.toHex()) : undefined; +} + +function mapEntriesFor( + storage: StorageDelta, + slotName: string, +): Array<{ key: string; value: string }> { + const map = storage.maps().find((m) => m.slotName === slotName); + if (!map) { + return []; + } + return map.entries().map((e) => ({ + key: normalizeHexWord(e.key.toHex()), + value: normalizeHexWord(e.value.toHex()), + })); +} + +/** + * Assert that a proposal's human-readable `metadata` matches the transaction its + * cosigners actually sign (the `TransactionSummary`), WITHOUT re-executing. + * + * The previous implementation rebuilt the transaction from metadata and + * re-executed it (`executeForSummary`) to compare the resulting summary + * commitment. That is block-height dependent: execution charges a fee taken from + * the reference block's fee parameters, and the fee is part of the account delta + * the summary commitment covers. So the check only matched on the same client at + * the same sync height as the proposer, and a second signer at a later block hit + * "metadata does not match tx_summary" and could not load or sign. + * + * This instead decodes the signed summary and, for each type, asserts the + * transaction's effects are EXACTLY the declared effect plus the fee — across + * every intent-bearing dimension of the summary: its output notes, its input + * notes, and the set of account-storage slots it changes. It never reads the + * vault delta (the fee is a native-asset vault delta, block-dependent), which is + * safe because assets can only leave the account through an output note or the + * fee: binding the output/input notes and the storage slots exactly makes the + * vault delta a consequence. So the comparison is deterministic across clients + * and blocks yet still what-you-see-is-what-you-sign — a mislabeled proposal, or + * one that piggybacks an undeclared effect in any dimension, is rejected before a + * cosigner signs it. + * + * Residual limitations (see the type comments): a non-fungible asset attached to + * an otherwise-correct p2id note is not yet bound (the SDK exposes only fungible + * note assets) — closable via output-note-id equality before merge, see + * `assertP2idBinding`; and the signer check binds by storage-map + * index over the CHANGED entries (a real-summary integration test should confirm + * the delta carries every changed index). `custom` proposals carry no metadata + * recipe, so WYSIWYS does not hold for them — a cosigner must verify the raw + * `tx_summary`, never trust a `custom` proposal's `description`. + */ +export function assertMetadataMatchesSummary( + proposalId: string, + metadata: ProposalMetadata, + summary: TransactionSummary, +): void { + switch (metadata.proposalType) { + case 'custom': + // No reconstruction recipe; the id <-> tx_summary commitment check is the + // only guarantee. WYSIWYS does NOT hold for custom proposals. + return; + case 'p2id': + return assertP2idBinding(proposalId, metadata, summary); + case 'consume_notes': + return assertConsumeNotesBinding(proposalId, metadata, summary); + case 'add_signer': + case 'remove_signer': + case 'change_threshold': + return assertSignerBinding(proposalId, metadata, summary); + case 'update_procedure_threshold': + return assertProcedureThresholdBinding(proposalId, metadata, summary); + case 'switch_guardian': + return assertSwitchGuardianBinding(proposalId, metadata, summary); + default: { + // Fail closed: a proposal type without a binding recipe must not silently + // pass. The union is exhaustive, so this is also a compile-time guard that + // any new built-in type is given an explicit binding before it can be + // signed. + const _exhaustive: never = metadata; + void _exhaustive; + reject(proposalId); + } + } +} + +/** + * p2id: exactly one output note carrying the declared recipient and exactly the + * declared fungible asset; no input notes; no storage change other than the + * per-tx executed-transactions marker. The recipient is rebuilt from metadata + + * the SIGNED summary's salt (so a tampered `metadata.saltHex` cannot force a + * match); the asset is read off the note (not the vault), so the block-dependent + * fee never contaminates the comparison. + * + * KNOWN GAP (must close before this leaves draft): the recipient and the + * fungible asset are bound separately, so a NON-FUNGIBLE asset attached to the + * note is not bound (`NoteAssets` exposes only fungible assets) — an NFT could + * be drained under a fungible-transfer label. Closure: bind the whole output + * note by `outputNote.id()` equality (a NoteId commits to ALL assets incl. NFTs; + * the consume path already binds by note id, and `p2id.ts` has the note-ID + * reconstruction), sourcing the fungible asset OFF the note to keep the callback + * flag and block-independence — validated by the pre-merge integration test. + */ +function assertP2idBinding( + proposalId: string, + metadata: P2IdProposalMetadata, + summary: TransactionSummary, +): void { + const recipient = AccountId.fromHex(metadata.recipientId); + const serialNum = deriveP2idSerialNumber(summary.salt()); + const noteRecipient = new NoteRecipient( + serialNum, + NoteScript.p2id(), + new NoteStorage(new FeltArray([recipient.suffix(), recipient.prefix()])), + ); + const expectedRecipientDigest = normalizeHexWord(noteRecipient.digest().toHex()); + + const outputs = summary.outputNotes().notes(); + if (outputs.length !== 1) { + reject(proposalId); + } + const note = outputs[0]; + if (normalizeHexWord(note.recipientDigest().toHex()) !== expectedRecipientDigest) { + reject(proposalId); + } + + const expectedFaucet = AccountId.fromHex(metadata.faucetId).toString(); + const expectedAmount = BigInt(metadata.amount); + const assets = note.assets()?.fungibleAssets() ?? []; + if ( + assets.length !== 1 || + assets[0].faucetId().toString() !== expectedFaucet || + assets[0].amount() !== expectedAmount + ) { + reject(proposalId); + } + + assertNoInputNotes(proposalId, summary); + assertStorageSlotsWithin(proposalId, storageOf(summary), new Set([EXECUTED_TXS_SLOT])); +} + +/** + * consume_notes: input-note-id set exactly equals the declared set; NO output + * notes (a consume that also emitted an output note would drain value the + * metadata does not describe); no storage change other than the executed-tx + * marker. Note ids are content commitments, stable across authentication, so the + * comparison is proof-agnostic. + */ +function assertConsumeNotesBinding( + proposalId: string, + metadata: ConsumeNotesProposalMetadata, + summary: TransactionSummary, +): void { + const declared = new Set(declaredConsumeNoteIds(metadata)); + const actual = new Set( + summary + .inputNotes() + .notes() + .map((n) => n.id().toString()), + ); + if (declared.size !== actual.size) { + reject(proposalId); + } + for (const id of declared) { + if (!actual.has(id)) { + reject(proposalId); + } + } + + assertNoOutputNotes(proposalId, summary); + assertStorageSlotsWithin(proposalId, storageOf(summary), new Set([EXECUTED_TXS_SLOT])); +} + +function declaredConsumeNoteIds(metadata: ConsumeNotesProposalMetadata): string[] { + if (isConsumeNotesV2(metadata) && metadata.notes) { + return metadata.notes.map((b64) => noteFromBase64(b64, Note).id().toString()); + } + // Canonicalize v1 hex ids through NoteId so they compare equal to the + // summary's `NoteId.toString()` regardless of casing / 0x padding. + return metadata.noteIds.map((id) => NoteId.fromHex(id).toString()); +} + +/** + * add/remove/change_signer: no notes; the only value slot changed is + * `threshold_config` set to `[threshold, count, 0, 0]`; the only map slots + * changed are the signer public-keys / scheme-ids (and the executed-tx marker); + * and every changed public-keys entry binds by INDEX to the declared signer set. + * + * The MASM writes signer `j` to map key `[j,0,0,0]` (auth.ts + * `update_signers_and_threshold`), and `cleanup_pubkey_mapping` zeroes stale + * higher indices. So a changed entry at index `j` must equal the declared + * commitment at `j` (or be a zero-value removal at an index >= count). This is + * stronger than a subset check: it catches a duplicated signer (same key at two + * indices) or an omitted one. Residual: the storage delta carries only CHANGED + * indices, so an unchanged index is not re-verified here — a real-summary + * integration test should confirm the delta covers every changed index. + */ +function assertSignerBinding( + proposalId: string, + metadata: UpdateSignersProposalMetadata, + summary: TransactionSummary, +): void { + assertNoOutputNotes(proposalId, summary); + assertNoInputNotes(proposalId, summary); + + const storage = storageOf(summary); + assertStorageSlotsWithin( + proposalId, + storage, + new Set([THRESHOLD_CONFIG_SLOT, SIGNER_PUBLIC_KEYS_SLOT, SIGNER_SCHEME_IDS_SLOT, EXECUTED_TXS_SLOT]), + ); + assertSignerConfigAndKeys(proposalId, storage, metadata.targetThreshold, metadata.targetSignerCommitments); +} + +/** + * Shared signer-config check: the `threshold_config` value slot equals + * `[threshold, count, 0, 0]`, and each changed `signer_public_keys` entry binds + * by index to the declared commitment set. Used by the signer bindings and by a + * `switch_guardian` proposal that also rotates the signer set. + */ +function assertSignerConfigAndKeys( + proposalId: string, + storage: StorageDelta, + targetThreshold: number, + targetSignerCommitments: string[], +): void { + const expectedConfig = wordHexFromFelts([ + BigInt(targetThreshold), + BigInt(targetSignerCommitments.length), + 0n, + 0n, + ]); + if (valueDeltaFor(storage, THRESHOLD_CONFIG_SLOT) !== expectedConfig) { + reject(proposalId); + } + + // Expected value at each map index key [i,0,0,0]. + const expectedByKey = new Map(); + targetSignerCommitments.forEach((commitment, i) => { + expectedByKey.set(wordHexFromFelts([BigInt(i), 0n, 0n, 0n]), normalizeHexWord(commitment)); + }); + + for (const entry of mapEntriesFor(storage, SIGNER_PUBLIC_KEYS_SLOT)) { + const expected = expectedByKey.get(entry.key); + if (expected !== undefined) { + // A declared index: the written key must be exactly the declared signer. + if (entry.value !== expected) { + reject(proposalId); + } + } else if (entry.value !== ZERO_WORD_HEX) { + // An index outside the declared set may only be a zero-value removal. + reject(proposalId); + } + } +} + +/** + * update_procedure_threshold: no notes; no value-slot change; the only map slots + * changed are `procedure_thresholds` (and the executed-tx marker), and it must + * set the declared procedure's root to `[threshold, 0, 0, 0]`. + */ +function assertProcedureThresholdBinding( + proposalId: string, + metadata: UpdateProcedureThresholdProposalMetadata, + summary: TransactionSummary, +): void { + assertNoOutputNotes(proposalId, summary); + assertNoInputNotes(proposalId, summary); + + const storage = storageOf(summary); + assertStorageSlotsWithin( + proposalId, + storage, + new Set([PROCEDURE_THRESHOLDS_SLOT, EXECUTED_TXS_SLOT]), + ); + + const expectedKey = normalizeHexWord(getProcedureRoot(metadata.targetProcedure)); + const expectedValue = wordHexFromFelts([BigInt(metadata.targetThreshold), 0n, 0n, 0n]); + const entry = mapEntriesFor(storage, PROCEDURE_THRESHOLDS_SLOT).find((e) => e.key === expectedKey); + if (!entry || entry.value !== expectedValue) { + reject(proposalId); + } +} + +/** + * switch_guardian: no notes; the guardian public-key map is set to the declared + * new key (`[0,0,0,0] => newGuardianPubkey`, auth.ts `update_guardian_public_key`); + * and the changed storage slots are confined to the guardian component (plus the + * executed-tx marker) — and, if the proposal also rotates the signer set, the + * multisig signer slots with the same index-bound checks. `verifyGuardianEndpoint + * Commitment` (propose/execute) remains an additional check that the new key is + * live at the declared endpoint. + */ +function assertSwitchGuardianBinding( + proposalId: string, + metadata: SwitchGuardianProposalMetadata, + summary: TransactionSummary, +): void { + assertNoOutputNotes(proposalId, summary); + assertNoInputNotes(proposalId, summary); + + const storage = storageOf(summary); + const allowed = new Set([ + GUARDIAN_SELECTOR_SLOT, + GUARDIAN_PUBLIC_KEY_SLOT, + GUARDIAN_SCHEME_ID_SLOT, + EXECUTED_TXS_SLOT, + ]); + const rotatesSigners = + metadata.targetSignerCommitments !== undefined && metadata.targetThreshold !== undefined; + if (rotatesSigners) { + allowed.add(THRESHOLD_CONFIG_SLOT); + allowed.add(SIGNER_PUBLIC_KEYS_SLOT); + allowed.add(SIGNER_SCHEME_IDS_SLOT); + } + assertStorageSlotsWithin(proposalId, storage, allowed); + + // The guardian public-key map is keyed by the zero word. + const expectedPubkey = normalizeHexWord(metadata.newGuardianPubkey); + const entry = mapEntriesFor(storage, GUARDIAN_PUBLIC_KEY_SLOT).find((e) => e.key === ZERO_WORD_HEX); + if (!entry || entry.value !== expectedPubkey) { + reject(proposalId); + } + + // If the guardian selector value slot changed, it must be ON ([1,0,0,0]). An + // honest guardian switch leaves the guardian enabled (the MASM re-enables it), + // so a summary that disables the guardian is rejected here rather than relying + // on that MASM invariant. + const selector = valueDeltaFor(storage, GUARDIAN_SELECTOR_SLOT); + if (selector !== undefined && selector !== wordHexFromFelts([1n, 0n, 0n, 0n])) { + reject(proposalId); + } + + if (rotatesSigners) { + assertSignerConfigAndKeys( + proposalId, + storage, + metadata.targetThreshold as number, + metadata.targetSignerCommitments as string[], + ); + } +}