feat: merge-train/spartan-v5#23927
Merged
Merged
Conversation
…rs (A-818) (#23919) 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.
#23921) 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).
Collaborator
Author
|
🤖 Auto-merge enabled after 4 hours of inactivity. This PR will be merged automatically once all checks pass. |
Collaborator
Author
Flakey Tests🤖 says: This CI run detected 1 tests that failed, but were tolerated due to a .test_patterns.yml entry. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BEGIN_COMMIT_OVERRIDE
fix: guard formatViemError structuredClone against non-cloneable errors (A-818) (#23919)
fix: validate contract artifact schema in CLI deploy/call path (A-836) (#23921)
END_COMMIT_OVERRIDE