From 2cd0ffe17ed011dbc1f68981b92d89f4beefba16 Mon Sep 17 00:00:00 2001 From: PhilWindle <60546371+PhilWindle@users.noreply.github.com> Date: Mon, 8 Jun 2026 09:29:50 +0100 Subject: [PATCH 1/2] fix: guard formatViemError structuredClone against non-cloneable errors (A-818) (#23919) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes A-818 (Audit #163). ## Problem `formatViemError` (`yarn-project/ethereum/src/utils.ts`) called `structuredClone(error)` unguarded, before the `instanceof Error` formatting fallback. `structuredClone` throws `DataCloneError` on values it cannot clone — and viem RPC/contract errors routinely attach plain-object context holding functions (e.g. transport request methods). When that happened, the formatter itself threw, and since several call sites do `throw formatViemError(...)` (`rollup.ts`, `l1_tx_utils.ts`), the original L1 revert/RPC error was replaced by a `DataCloneError` and lost — degrading operator diagnostics. ## Fix Wrap the clone (and the `stripAbis` call that depends on it) in a `try/catch`. On clone failure, fall back to formatting the original error untouched (skipping `stripAbis` so we don't mutate the caller's object). The `cause` still carries the original error in all paths. ## Test Added a `formatViemError` test in `utils.test.ts` covering an error whose `cause` holds a function-valued `request` field. It throws `DataCloneError` without the fix and returns a `FormattedViemError` (preserving the message and `cause`) with it. --- yarn-project/ethereum/src/utils.test.ts | 18 +++++++++++++++++- yarn-project/ethereum/src/utils.ts | 22 +++++++++++----------- 2 files changed, 28 insertions(+), 12 deletions(-) diff --git a/yarn-project/ethereum/src/utils.test.ts b/yarn-project/ethereum/src/utils.test.ts index d31c5a6a6c0d..723c740585a2 100644 --- a/yarn-project/ethereum/src/utils.test.ts +++ b/yarn-project/ethereum/src/utils.test.ts @@ -1,6 +1,6 @@ import type { Abi } from 'viem'; -import { mergeAbis } from './utils.js'; +import { FormattedViemError, formatViemError, mergeAbis } from './utils.js'; describe('mergeAbis', () => { it('dedupes identical function items', () => { @@ -59,3 +59,19 @@ describe('mergeAbis', () => { expect(merged).toHaveLength(2); }); }); + +describe('formatViemError', () => { + it('formats an error whose cause carries non-cloneable function-valued context', () => { + // viem RPC errors routinely attach plain-object context holding functions (e.g. transport + // request methods). structuredClone throws DataCloneError on these, so formatViemError must + // not let the clone failure mask the underlying error. + const error = new Error('rpc request failed'); + (error as any).cause = { code: -32000, request: { send: () => undefined } }; + + const formatted = formatViemError(error); + + expect(formatted).toBeInstanceOf(FormattedViemError); + expect(formatted.message).toContain('rpc request failed'); + expect(formatted.cause).toBe(error); + }); +}); diff --git a/yarn-project/ethereum/src/utils.ts b/yarn-project/ethereum/src/utils.ts index 543df2182bc9..5a05d806ab12 100644 --- a/yarn-project/ethereum/src/utils.ts +++ b/yarn-project/ethereum/src/utils.ts @@ -235,18 +235,18 @@ export function formatViemError(error: any, abi: Abi = ErrorsAbi): FormattedViem // If decoding fails, we fall back to the original formatting } - // Strip ABI from the error object before formatting + // Strip ABI from the error object before formatting. We clone first to avoid mutating the + // caller's error, but structuredClone throws DataCloneError on values it cannot clone (e.g. + // viem RPC errors carrying function-valued request context). If cloning fails, fall back to + // formatting the original error untouched rather than letting the clone failure mask it. if (error && typeof error === 'object') { - // Create a clone to avoid modifying the original - const errorClone = structuredClone(error); - - // Helper function to recursively remove ABI properties - - // Strip ABIs from the clone - stripAbis(errorClone); - - // Use the cleaned clone for further processing - error = errorClone; + try { + const errorClone = structuredClone(error); + stripAbis(errorClone); + error = errorClone; + } catch { + // Leave `error` as the original; we skip stripAbis to avoid mutating the caller's object. + } } // If it's a regular Error instance, return it with its message From a742e683cb838ab9460cd16556f98f5476e88454 Mon Sep 17 00:00:00 2001 From: PhilWindle <60546371+PhilWindle@users.noreply.github.com> Date: Mon, 8 Jun 2026 09:56:22 +0100 Subject: [PATCH 2/2] fix: validate contract artifact schema in CLI deploy/call path (A-836) (#23921) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes A-836 (Audit #188). ## Problem The CLI loaded contract artifacts via `loadContractArtifact` (`getContractArtifact` in `cli/src/utils/aztec.ts`). For an **already-processed** artifact, `loadContractArtifact` only runs the shallow `isContractArtifact` heuristic — its own JSDoc notes "The check is not exhaustive". It checks `name`/`functions`/`nonDispatchPublicFunctions` shape but never validates parameter ABIs, types, storage layout, outputs, etc. (Raw nargo output is already fully validated, since it flows through `generateContractArtifact` → `ContractArtifactSchema.parse`.) A malformed-but-superficially-shaped artifact therefore bypassed schema validation and surfaced as an opaque error later during deployment/arg-encoding rather than a clear validation failure. ## Fix Add `loadContractArtifactWithValidation` in stdlib: it runs the full `ContractArtifactSchema` over an already-processed artifact before returning, and otherwise defers to `loadContractArtifact` (raw nargo stays validated as before). The returned object is identical to `loadContractArtifact`'s — the schema parse is used purely as a validation gate. The CLI's `getContractArtifact` now uses it. Scoped deliberately to the CLI (the audit's concern). `loadContractArtifact` is called at 100+ module-load sites with wire-form JSON; adding a stricter schema gate to all of them would have a large blast radius and risk rejecting legacy artifacts, so the unguarded function is left unchanged. ## Test Added `loadContractArtifactWithValidation` tests in `contract_artifact.test.ts`: a valid already-processed artifact (wire form) loads, and one whose `functionType` is a non-enum string — which still passes the shallow `isContractArtifact` check — is rejected. The rejection test fails without the schema gate and passes with it. ## Note Severity is MEDIUM in the finding, but as the artifact is local input the deployer supplies (not an untrusted network boundary) and downstream code still throws on bad input, the practical impact is closer to LOW (a clearer error message, earlier). --- yarn-project/aztec.js/src/api/abi.ts | 1 + yarn-project/cli/src/utils/aztec.ts | 4 +-- .../stdlib/src/abi/contract_artifact.test.ts | 25 ++++++++++++++++++- .../stdlib/src/abi/contract_artifact.ts | 19 ++++++++++++++ 4 files changed, 46 insertions(+), 3 deletions(-) diff --git a/yarn-project/aztec.js/src/api/abi.ts b/yarn-project/aztec.js/src/api/abi.ts index 56ff6448708a..847b1708f7eb 100644 --- a/yarn-project/aztec.js/src/api/abi.ts +++ b/yarn-project/aztec.js/src/api/abi.ts @@ -17,6 +17,7 @@ export { isWrappedFieldStruct, isFunctionSelectorStruct, loadContractArtifact, + loadContractArtifactWithValidation, loadContractArtifactForPublic, getAllFunctionAbis, contractArtifactToBuffer, diff --git a/yarn-project/cli/src/utils/aztec.ts b/yarn-project/cli/src/utils/aztec.ts index 89502f5c47a4..4f157d08ae04 100644 --- a/yarn-project/cli/src/utils/aztec.ts +++ b/yarn-project/cli/src/utils/aztec.ts @@ -3,7 +3,7 @@ import { type FunctionAbi, FunctionType, getAllFunctionAbis, - loadContractArtifact, + loadContractArtifactWithValidation, } from '@aztec/aztec.js/abi'; import { EthAddress } from '@aztec/aztec.js/addresses'; import type { L1ContractsConfig } from '@aztec/ethereum/config'; @@ -132,7 +132,7 @@ export async function getContractArtifact(fileDir: string, log: LogFn) { } try { - return loadContractArtifact(JSON.parse(contents)); + return loadContractArtifactWithValidation(JSON.parse(contents)); } catch (err) { log('Invalid file used. Please try again.'); throw err; diff --git a/yarn-project/stdlib/src/abi/contract_artifact.test.ts b/yarn-project/stdlib/src/abi/contract_artifact.test.ts index 5413028e4a7a..4b41394fe44a 100644 --- a/yarn-project/stdlib/src/abi/contract_artifact.test.ts +++ b/yarn-project/stdlib/src/abi/contract_artifact.test.ts @@ -1,5 +1,9 @@ import { getBenchmarkContractArtifact } from '../tests/fixtures.js'; -import { contractArtifactFromBuffer, contractArtifactToBuffer } from './contract_artifact.js'; +import { + contractArtifactFromBuffer, + contractArtifactToBuffer, + loadContractArtifactWithValidation, +} from './contract_artifact.js'; describe('contract_artifact', () => { it('serializes and deserializes an instance', () => { @@ -8,4 +12,23 @@ describe('contract_artifact', () => { const deserialized = contractArtifactFromBuffer(serialized); expect(deserialized).toEqual(artifact); }); + + describe('loadContractArtifactWithValidation', () => { + // The wire form of an already-processed artifact (hex/base64 strings) is what reaches the + // loader from a JSON file, e.g. via the CLI deploy command. + const wireForm = () => JSON.parse(contractArtifactToBuffer(getBenchmarkContractArtifact()).toString('utf-8')); + + it('accepts a valid already-processed artifact', () => { + const loaded = loadContractArtifactWithValidation(wireForm()); + expect(loaded.name).toEqual(getBenchmarkContractArtifact().name); + }); + + it('rejects an artifact that passes the shallow shape check but violates the schema', () => { + const input = wireForm(); + // functionType stays a string, so the shallow isContractArtifact() heuristic still passes, + // but it is not a valid FunctionType enum value, so full schema validation must reject it. + input.functions[0].functionType = 'not-a-real-type'; + expect(() => loadContractArtifactWithValidation(input)).toThrow(); + }); + }); }); diff --git a/yarn-project/stdlib/src/abi/contract_artifact.ts b/yarn-project/stdlib/src/abi/contract_artifact.ts index b3cfc0e919d2..d04f3fea090f 100644 --- a/yarn-project/stdlib/src/abi/contract_artifact.ts +++ b/yarn-project/stdlib/src/abi/contract_artifact.ts @@ -61,6 +61,25 @@ export function loadContractArtifact(input: NoirCompiledContract): ContractArtif return generateContractArtifact(input); } +/** + * Like {@link loadContractArtifact}, but fully validates an already-processed artifact against the + * contract artifact schema before returning it. Use when loading an artifact from untrusted or + * external JSON (e.g. a file path passed to the CLI), so a malformed artifact is rejected up-front + * with a clear schema error instead of surfacing as an opaque failure later during deployment. + * + * `loadContractArtifact` only runs the shallow `isContractArtifact` shape check on already-processed + * artifacts; raw nargo output is validated via `generateContractArtifact` regardless. The returned + * object is identical to `loadContractArtifact`'s; the schema parse is used purely for validation. + * @param input - Input object as generated by nargo compile, or an already-processed artifact. + * @returns A valid contract artifact instance. + */ +export function loadContractArtifactWithValidation(input: NoirCompiledContract): ContractArtifact { + if (isContractArtifact(input)) { + ContractArtifactSchema.parse(input); + } + return loadContractArtifact(input); +} + /** * Gets nargo build output and returns a valid contract artifact instance. * Differs from loadContractArtifact() by retaining all bytecode.