diff --git a/.github/workflows/oz-latest.yml b/.github/workflows/oz-latest.yml new file mode 100644 index 0000000..21226dc --- /dev/null +++ b/.github/workflows/oz-latest.yml @@ -0,0 +1,34 @@ +name: OpenZeppelin latest + +# slotdiff imports a few OpenZeppelin upgrades-core files that are not its +# public API, and accepts any 1.x. A 1.x release that moves one would break +# every install; this finds out before users do. +on: + schedule: + # Mondays 16:30 UTC, after Scorecard. + - cron: "30 16 * * 1" + workflow_dispatch: + +permissions: + contents: read + +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + - uses: pnpm/action-setup@ea17c68df8912ef543352723c149a84f56e3d413 # v6.1.0 + - uses: actions/setup-node@949feb2413d6458794dcd2491c4babbbce0c15c1 # v7.1.0 + with: + node-version: "24" + cache: pnpm + - run: pnpm install --frozen-lockfile + # The newest version package.json allows, as a fresh install gets it. + - run: pnpm update @openzeppelin/upgrades-core + - run: node -p "'upgrades-core ' + require('@openzeppelin/upgrades-core/package.json').version" + - run: pnpm typecheck + - run: pnpm test + - run: pnpm build + - run: node -e "require('./dist/index.js')" diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index ab649fd..7ebf155 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -1,6 +1,6 @@ name: Publish -# Run by hand: `gh workflow run publish.yml -f dist-tag=alpha`. npm trusts +# Run by hand: `gh workflow run publish.yml -f dist-tag=latest`. npm trusts # this workflow through trusted publishing (OIDC), so no npm token is stored, # and the package gets a provenance attestation linking it to this repository # and commit. It only stages the release: nothing is public until a maintainer @@ -36,7 +36,9 @@ jobs: # publishing alone needs 11.5.1+). No registry-url: trusted publishing needs no .npmrc. node-version: "24" check-latest: true - - run: pnpm install --frozen-lockfile + # No dependency install scripts in the job that can mint the publish token + # (keccak and secp256k1 only build optional native addons). + - run: pnpm install --frozen-lockfile --ignore-scripts - name: Check npm supports staged publishing run: | npm --version diff --git a/CHANGELOG.md b/CHANGELOG.md index abf3bdf..eb236c1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,17 +1,38 @@ # Changelog -## Unreleased +## 0.3.0 + +### Fixed + +These were in 0.2.0 (the solc one since 0.1.0); upgrade if you rely on the CLI or check verified source. + +- CLI: an `` address that is not an ERC-1967 or beacon proxy was taken as an implementation, so a proxy of another kind (a custom slot, a diamond) was compared as its own, nearly empty, code and could pass. It is now refused (exit 2) unless `--old-implementation` says it is an implementation. +- Storage placed at a fixed slot through a struct that is not an ERC-7201 namespace of the contract or its bases (a namespace declared in a library, as in the diamond and Solidstate pattern, or a struct with no annotation) is left out of OpenZeppelin's layout with only a warning, so a change to it passed. It now stops the analysis with `LayoutUnavailableError` (exit 2). +- solc ran in the caller's directory, so verified source could `import` a file under it (an `.env`, say) and solc would quote its first line in the compile error. It now runs in a new empty directory, and verified source given by `urls` instead of inline `content` is refused. +- Text from an explorer or from verified source could move the cursor, recolor the terminal or draw lines of its own (a fake "Safe to upgrade." under the error). Control characters are stripped from solc's errors, explorer messages are kept on one line, and verified source with control characters in its name, paths or text (tabs and line breaks in text aside), or any whitespace but a space in its name and paths, is refused (`LayoutUnavailableError`). + +### Breaking + +- CLI: an `` implementation address needs `--old-implementation` (above). +- `layoutFromSource` is removed: use `(await analyzeSource(input, solc, contract)).layout`, at the same cost. +- `NamespaceCheck.result` says `"unchecked"` (was `"unread"`), as `UpgradeCheck.result` does. New union members: `NamespaceSlot.status` `"unused"` (such namespaces were `unread`), `NamespacePair.problem` `"dropped"`. +- `checkNamespaces`: `next` needs its `layout`. +- Plain `Error` (was `LayoutIntegrityError`), as wrong arguments: a contract missing from the compiler input, an input source without `content`. `safety()` throws on an unknown `unsafeAllow` kind (OpenZeppelin ignored it). + +### Added -- `analyzeBuild(input, output, compiler)`: `analyzeSource` for a build already compiled (a build tool's), with no second full compile; one namespaced pass serves every contract. Its `metadata` must prove the output was compiled from those sources by that compiler, so output from another build is refused (`LayoutIntegrityError`); incomplete output throws the new `IncompleteBuildError`. - `checkUpgrade(old, next, options?)`: layout, ERC-7201 slots and safety rules in one verdict (`pass`, `fail` or `unchecked`), as the CLI runs them. The proxy kind comes from `old`. -- Layout records keep the proxy `kind` and each namespace's slot (`namespaces`), so a saved record can stand in for `old`. Optional: older records still load. -- `checkNamespaces`: a namespace missing from `namespaces` (an older record) is unread, never matched by annotation alone; slots that do not fit the layout are refused. **Breaking:** `next` needs its `layout`. -- ERC-7201: a namespace with no `$.slot :=` assignment (kept reserved) is `unused` and compared by its annotation (unless the code places another struct at a known slot, or a mapping or an array pointer), instead of making every upgrade of such a contract exit 2. Dropping the last assignment of a namespace whose deployed annotation is wrong fails (`dropped`), while the real slot is still known. **Breaking:** `NamespaceSlot.status` adds `"unused"` (such namespaces were `unread`). -- `SafetyReport.explain(color?)` and `NamespaceCheck.explain(color?)` (the CLI's namespace grid); `safety()` refuses an unknown `unsafeAllow` kind; `resolveProxyLayout` returns the proxy's `kind`; `proveLocalBuild` takes `kind` and `namespaces` for the record. New types: `SolcOutput`, `UpgradeSide`, `UpgradeOptions`, `UpgradeCheck`, `BuildAnalysis`, `BuildCompiler`. -- `analyzeSource` checks its own compile as `analyzeBuild` does: it refuses an input source without `content` (such as `urls`), and a compiler whose version differs from the one the metadata names (`LayoutIntegrityError`). -- `parseRecord` refuses a namespace key not of the form `:`, and a malformed `kind` or `namespaces`. -- CLI: namespaces with no problem that the code does not assign are left out of the grid, and the summary counts them ("N not assigned in code, compared by annotation"); an unread slot says which side could not be read. -- **Breaking:** a contract missing from the compiler input throws a plain `Error` (was `LayoutIntegrityError`), as a wrong argument; `NamespacePair.problem` adds `"dropped"`. +- `analyzeBuild(input, output, compiler)`: `analyzeSource` for a build already compiled (a build tool's), with no second full compile; one namespaced pass serves every contract. Its `metadata` must prove the output was compiled from those sources by that compiler (`LayoutIntegrityError` otherwise); incomplete output throws the new `IncompleteBuildError`. +- Layout records keep the proxy `kind` and each namespace's slot (`namespaces`), so a saved record can stand in for `old`; `proveLocalBuild` takes both. Optional: older records still load, their namespaces unchecked. +- `resolveProxyLayout` returns the proxy's `kind`; `SafetyReport.explain(color?)` and `NamespaceCheck.explain(color?)` (the CLI's namespace grid). New types: `SolcOutput`, `UpgradeSide`, `UpgradeOptions`, `UpgradeCheck`, `BuildAnalysis`, `BuildCompiler`. + +### Changed + +- ERC-7201: a namespace with no `$.slot :=` assignment (kept reserved) is `unused` and compared by its annotation (unless the code places a mapping or an array pointer), so such upgrades exit 0 where 0.2.0 exited 2. Dropping the last assignment of a namespace whose deployed annotation is wrong fails (`dropped`), while the real slot is still known. +- CLI: unassigned namespaces with no problem are left out of the grid and counted in the summary ("N not assigned in code, compared by annotation"); an unread slot says which side could not be read. +- Stricter input: `analyzeSource` checks its own compile as `analyzeBuild` does (a compiler whose version differs from the one the metadata names is a `LayoutIntegrityError`); `parseRecord` refuses a namespace key not of the form `:` and a malformed `kind` or `namespaces`; `checkNamespaces` refuses slots that do not fit the layout (`LayoutIntegrityError`); `getSolc` refuses a malformed entry in solc's build list. +- `SAFETY_ERROR_KINDS` and `SafetyErrorKind` are slotdiff's own list (a test compares it with OpenZeppelin's), so the published types no longer import an OpenZeppelin internal file. +- Releases: the publish workflow installs dependencies without their install scripts; a weekly workflow tests against the newest OpenZeppelin upgrades-core 1.x. ## 0.2.0-alpha.1 diff --git a/README.md b/README.md index 848c6d0..15216cf 100644 --- a/README.md +++ b/README.md @@ -49,9 +49,9 @@ Node 22.13+. Works from ESM and CommonJS. slotdiff [--rpc ] ``` -Each side is a contract in your build info (`src/Vault.sol:Vault`, or just `Vault` when only one contract has that name) or an address: a proxy, read as the implementation it runs now, or an implementation, such as one waiting in a multisig queue. Any mix works. An address needs `--rpc` and `ETHERSCAN_API_KEY` (or `--explorer-url` for another explorer), since its source comes from the explorer. The RPC is a flag because it picks the chain for each run; the key lives in the environment because one Etherscan key covers every chain. It is sent only to Etherscan: with `--explorer-url`, put that explorer's own key in the URL's query if it needs one. +Each side is a contract in your build info (`src/Vault.sol:Vault`, or just `Vault` when only one contract has that name) or an address: a proxy (ERC-1967 or beacon), read as the implementation it runs now, or for `` an implementation, such as one waiting in a multisig queue. Any mix works. An `` address that is not such a proxy is refused, since a proxy of another kind (a diamond, a custom slot) would be compared as its own code: pass `--old-implementation` when `` is an implementation, or give such a proxy by its implementation's address. An address needs `--rpc` and `ETHERSCAN_API_KEY` (or `--explorer-url` for another explorer), since its source comes from the explorer. The RPC is a flag because it picks the chain for each run; the key lives in the environment because one Etherscan key covers every chain. It is sent only to Etherscan: with `--explorer-url`, put that explorer's own key in the URL's query if it needs one. -A local side is read from the build info your tool already wrote; slotdiff does not compile your project. Build first, from the project root or with `--root`. Foundry writes build info only with `build_info = true` in `foundry.toml` or `forge build --build-info`. A build older than the sources on disk is refused, never used. `--build-info ` names one build-info file to use as is, such as a CI artifact from another checkout; the output then says it was not compared with the sources on disk. +A local side is read from the build info your tool already wrote; slotdiff does not compile your project. Build first, from the project root or with `--root`. Foundry writes build info only with `build_info = true` in `foundry.toml` or `forge build --build-info`. A build older than the sources on disk is refused, never used (each file the contract imports is compared, except a dependency slotdiff cannot find from the root, such as one hoisted to a monorepo's top `node_modules`). `--build-info ` names one build-info file to use as is, such as a CI artifact from another checkout; the output then says it was not compared with the sources on disk. Three checks run: @@ -122,30 +122,24 @@ Colors and the banner appear only on an interactive terminal, never in a pipe, a ```ts import { JsonRpcProvider } from "ethers"; -import { - diffLayouts, - findBuildInfo, - getSolc, - layoutFromSource, - resolveProxyLayout, -} from "slotdiff"; +import { analyzeSource, checkUpgrade, findBuildInfo, getSolc, resolveProxyLayout } from "slotdiff"; // Any object with send(method, params): an ethers JsonRpcProvider or Hardhat's network provider. const provider = new JsonRpcProvider(process.env.RPC_URL); const apiKey = process.env.ETHERSCAN_API_KEY; -// 1. The layout of whatever the proxy runs now, proven against its deployed code. +// 1. Whatever the proxy runs now: its layout and ERC-7201 slots, proven against its deployed code. const live = await resolveProxyLayout(provider, "0xProxy", { explorer: { apiKey } }); -// 2. The layout of the new version, from your build info (Foundry, Hardhat 2 or 3). -const build = await findBuildInfo(process.cwd(), "src/Vault.sol:Vault"); -const solc = await getSolc(build.solcVersion); -const next = await layoutFromSource(build.input, solc, build.contract); +// 2. The new version, from your build info (Foundry, Hardhat 2 or 3). +const build = await findBuildInfo(process.cwd(), "src/VaultV2.sol:VaultV2"); +const next = await analyzeSource(build.input, await getSolc(build.solcVersion), build.contract); -// 3. Compare. -const report = diffLayouts(live.record.layout, next); -if (!report.ok) { - console.error(report.explain()); +// 3. The layout, the ERC-7201 slots and the safety rules, with the proxy's own kind. +const check = checkUpgrade({ ...live.record, kind: live.kind }, next); +if (check.result !== "pass") { + // "fail", or "unchecked" when an ERC-7201 slot could not be read: neither is a pass. + console.error(check.layout.explain(), check.namespaces.explain(), check.safety?.explain()); process.exit(1); } ``` @@ -160,7 +154,7 @@ if (!report.ok) { With `findBuildInfo`, Hardhat 3's `project/` prefix is optional. -`explain()` gives OpenZeppelin's report, for example: +`check.layout.explain()` gives OpenZeppelin's report, for example: ``` src/Vault.sol:7: Inserted `inserted` @@ -169,9 +163,9 @@ src/Vault.sol:7: Inserted `inserted` The explorer defaults to Etherscan's v2 API, where one key covers every chain it indexes. For another Etherscan-compatible explorer, such as Blockscout, pass `{ apiUrl }` (a key is optional there). -### Safety rules and ERC-7201 slots too +### Two local builds -`analyzeSource` is `layoutFromSource` plus OpenZeppelin's safety check and the slot each ERC-7201 namespace really uses, from one compile. `checkUpgrade` runs all three checks on two analyses and gives one verdict, as the command line does. +`analyzeSource` gives a contract's layout, the slot each ERC-7201 namespace really uses and OpenZeppelin's safety check, from one compile. `checkUpgrade` runs all three checks on two analyses, or a record and an analysis, and gives one verdict, as the command line does. ```ts import { analyzeSource, checkUpgrade, findBuildInfo, getSolc } from "slotdiff"; @@ -189,7 +183,7 @@ if (check.result !== "pass") { } ``` -The safety rules take the proxy kind from `old`, the proxy being upgraded, never from `next`: a UUPS implementation that lost its upgrade function would otherwise look transparent and pass. For a beacon proxy, pass `{ ...old, kind: "beacon" }`. `checkUpgrade` also takes `unsafeAllowRenames`, `unsafeAllow` (error kinds to accept, listed in `SAFETY_ERROR_KINDS`, on top of `@custom:oz-upgrades-unsafe-allow` in the source) and `skipSafety`. `unchecked` means nothing failed but an ERC-7201 slot could not be read (`check.namespaces.result` is `unread`). With `skipSafety`, `pass` covers the layout and slots only, and `check.safety` is absent. The pieces are there too: `checkNamespaces`, `diffLayouts` and the analysis's own `safety({ kind })`. +The safety rules take the proxy kind from `old`, the proxy being upgraded, never from `next`: a UUPS implementation that lost its upgrade function would otherwise look transparent and pass. For a beacon proxy, pass `{ ...old, kind: "beacon" }`. `checkUpgrade` also takes `unsafeAllowRenames`, `unsafeAllow` (error kinds to accept, listed in `SAFETY_ERROR_KINDS`, on top of `@custom:oz-upgrades-unsafe-allow` in the source) and `skipSafety`. `unchecked` means nothing failed but an ERC-7201 slot could not be read (`check.namespaces.result` is `unchecked` too, and that pair's `problem` is `unread`). With `skipSafety`, `pass` covers the layout and slots only, and `check.safety` is absent. The pieces are there too: `checkNamespaces`, `diffLayouts` and the analysis's own `safety({ kind })`. ### Reuse a build you already compiled @@ -295,7 +289,6 @@ Problems that are yours to fix throw a plain `Error`, or `IncompleteBuildError` | `proveLocalBuild(provider, address, build)` | A record from your own build, only when it is the deployed build | | `verifyRecord(provider, address, value)` | Load a saved record, checked against the chain | | `parseRecord(value, address?)` | Load a saved record, checked offline | -| `layoutFromSource(input, solc, "file.sol:Name")` | Layout from standard-json input, including ERC-7201 namespaces. No proof | | `analyzeSource(input, solc, "file.sol:Name")` | The layout, proxy `kind`, ERC-7201 `namespaces` and `safety({ kind })` | | `analyzeBuild(input, output, compiler)` | The same for every contract of a build already compiled, with no second full compile | | `checkUpgrade(old, next, options?)` | Layout, ERC-7201 slots and safety rules in one verdict: `pass`, `fail` or `unchecked` | @@ -314,8 +307,9 @@ Renames and retypes follow OpenZeppelin's NatSpec tags, `@custom:oz-renamed-from - Explorer source must be verified as standard-json. Flattened and multi-file verifications do not record every compiler setting, so they are refused. - Solidity only. +- Storage placed at a fixed slot through a struct that is not an ERC-7201 namespace of the contract or its bases (a namespace declared in a library, as in the diamond pattern, or a struct with no annotation) is not in OpenZeppelin's layout, so slotdiff cannot compare it and stops (exit 2) rather than pass it. - Struct members cannot be renamed or retyped: Solidity has no NatSpec on struct members, so OpenZeppelin's tags cannot reach them. -- An ERC-7201 slot is read only from a `$.slot := ...` assignment whose value is a constant (a hex literal, the ERC-7201 formula, or `erc7201("id")`), directly or through a local variable set once or a function with no arguments that returns one. Any other slot (computed at run time, a library's constant) is not read, and the upgrade is reported as not checked. A namespace with no `$.slot := ...` assignment is compared by its annotation alone, as with OpenZeppelin, unless the code places a struct without an annotation (a library's `Layout`, say) at a slot it can read, or a mapping or an array pointer anywhere: its data may live there, so the upgrade is reported as not checked. Data an even older version left at another slot, or data reached another way (raw `sload` and `sstore`, or a pointer whose slot is passed in, as OpenZeppelin's `StorageSlot` takes it), goes unnoticed. +- An ERC-7201 slot is read only from a `$.slot := ...` assignment whose value is a constant (a hex literal, the ERC-7201 formula, or `erc7201("id")`), directly or through a local variable set once or a function with no arguments that returns one. Any other slot (computed at run time, a library's constant) is not read, and the upgrade is reported as not checked. A namespace with no `$.slot := ...` assignment is compared by its annotation alone, as with OpenZeppelin, unless the code places a mapping or an array pointer anywhere: its data may live there, so the upgrade is reported as not checked. Data an even older version left at another slot, or data reached another way (raw `sload` and `sstore`, or a pointer whose slot is passed in, as OpenZeppelin's `StorageSlot` takes it), goes unnoticed. ## No guarantee diff --git a/SECURITY.md b/SECURITY.md index 5ef4d41..3c87f66 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -12,9 +12,9 @@ Include what you ran (the command or code, the contracts or their shape) and wha For anyone reviewing slotdiff, or the alerts a supply-chain scanner raises about it: -- **Network:** the explorer (Etherscan's v2 API, or `--explorer-url`) for verified source, the RPC endpoint you pass, and `binaries.soliditylang.org` for compilers, each checked against the published checksum. Nothing else; no telemetry. -- **Processes:** it runs the native solc binary it downloaded, found in Hardhat's cache, or that a library caller names with `solc.resolvePath`. -- **Files:** it reads your build info and sources. It writes only the compiler cache (`slotdiff/compilers` under your OS cache directory, or `cacheDir`). +- **Network:** the explorer (Etherscan's v2 API, or `--explorer-url`) for verified source, the RPC endpoint you pass, and `binaries.soliditylang.org` for compilers (each download checked against the published checksum). Nothing else; no telemetry. +- **Processes:** it runs the native solc binary it downloaded, found in Hardhat's cache, or that a library caller names with `solc.resolvePath`, each time in a new empty temporary directory, so verified source cannot import your files. +- **Files:** it reads your build info and sources. It writes only the compiler cache (`slotdiff/compilers` under your OS cache directory, or `cacheDir`) and solc's empty working directory, removed after each compile. - **Environment:** `ETHERSCAN_API_KEY` (sent only to Etherscan), `XDG_CACHE_HOME` or `LOCALAPPDATA` for the cache directory, and, through Node's own color detection, `NO_COLOR`, `FORCE_COLOR`, `NODE_DISABLE_COLORS`, `TERM` and `COLORTERM`. - **Install:** slotdiff has no install scripts, and one dependency, `@openzeppelin/upgrades-core`. Two of its transitive dependencies (`keccak` and `secp256k1`, through `ethereumjs-util`) build optional native code at install (`node-gyp-build || exit 0`). diff --git a/package.json b/package.json index 7d109e9..f18bf96 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "slotdiff", - "version": "0.2.0-alpha.1", + "version": "0.3.0", "description": "Storage layout diff for upgradeable Solidity contracts: compare a local build or a live deployment against another", "license": "MIT", "author": "Rohit Solia (https://github.com/rokso)", diff --git a/src/cli.ts b/src/cli.ts index 9b05522..422cee7 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -21,7 +21,7 @@ import { type Check, type Paint, } from "./report.js"; -import { errorCode, LayoutUnavailableError } from "./errors.js"; +import { errorCode, LayoutUnavailableError, printable } from "./errors.js"; import { canQueryExplorer } from "./explorer.js"; import { rebuild } from "./resolve.js"; import { @@ -42,7 +42,7 @@ OpenZeppelin's safety rules for (constructors, initializers, selfdestruct. Each side is either: a contract in this project's build info src/Vault.sol:Vault, or Vault when unique an address a proxy (the implementation it runs now), - or an implementation + or for , an implementation Options: --rpc JSON-RPC endpoint, required when a side is an address @@ -54,6 +54,8 @@ Options: --unsafe-allow Comma-separated safety errors to accept, e.g. constructor; prefer @custom:oz-upgrades-unsafe-allow in the source --no-safety Skip the safety rules + --old-implementation is an implementation address, not a proxy; also + for a proxy slotdiff cannot read (pass its implementation) -h, --help Show this help -v, --version Show the version @@ -117,6 +119,7 @@ export async function run(argv: string[], io: CliIo): Promise { "unsafe-allow-renames": { type: "boolean" }, "unsafe-allow": { type: "string" }, "no-safety": { type: "boolean" }, + "old-implementation": { type: "boolean" }, help: { type: "boolean", short: "h" }, version: { type: "boolean", short: "v" }, }, @@ -167,6 +170,12 @@ export async function run(argv: string[], io: CliIo): Promise { return 2; } + const oldImplementation = values["old-implementation"] === true; + if (oldImplementation && !ADDRESS.test(positionals[0])) { + io.stderr("--old-implementation needs to be an address."); + return 2; + } + const safety = !values["no-safety"]; if (!safety && values["unsafe-allow"] !== undefined) { io.stderr("--unsafe-allow does nothing with --no-safety."); @@ -212,10 +221,23 @@ export async function run(argv: string[], io: CliIo): Promise { if (io.tty) io.stdout(`${await banner()}\n`); - const readSide = async (spec: string): Promise => { + const readSide = async (spec: string, side: "old" | "new"): Promise => { if (ADDRESS.test(spec)) { - // A proxy stands for what it runs now; any other address is an implementation. + // A proxy stands for what it runs now. may be an implementation, but + // only when said so: a proxy of a kind slotdiff cannot read (a + // diamond, a custom slot) would otherwise be compared as its own code. const proxy = await readProxy(provider!, spec); + if (side === "old" && (proxy === undefined) !== oldImplementation) { + // An address with no code is reported as such. + if (proxy === undefined) await readCode(provider!, spec); + throw new Error( + proxy === undefined + ? `${spec.toLowerCase()} is not a proxy slotdiff can read (no ERC-1967 implementation ` + + "or beacon slot is set). If it is an implementation, or a proxy of another kind " + + "given by its implementation's address, pass --old-implementation." + : `${spec.toLowerCase()} is a proxy, not an implementation: drop --old-implementation.`, + ); + } const implementation = proxy?.implementation ?? spec.toLowerCase(); if (!canQueryExplorer(explorer)) { await readCode(provider!, implementation); // a wrong address is reported as such first @@ -254,11 +276,12 @@ export async function run(argv: string[], io: CliIo): Promise { let old: Side; let next: Side; try { - old = await readSide(positionals[0]); + old = await readSide(positionals[0], "old"); // The same side twice (e.g. a proxy against itself) is fetched and compiled once. - next = positionals[1] === positionals[0] ? old : await readSide(positionals[1]); + next = positionals[1] === positionals[0] ? old : await readSide(positionals[1], "new"); } catch (e) { - io.stderr(`Could not check: ${(e as Error).message}`); + // Messages can quote an explorer or verified source. + io.stderr(`Could not check: ${printable((e as Error).message)}`); return 2; } @@ -299,7 +322,7 @@ export async function run(argv: string[], io: CliIo): Promise { name: "ERC-7201 slots", ...(namespaces.result === "fail" ? { status: "fail", summary: "problems found in (see above)" } - : namespaces.result === "unread" + : namespaces.result === "unchecked" ? { status: "skip", summary: "not checked: some slots could not be read (see above)" } : { status: "pass", @@ -418,9 +441,8 @@ function jsonRpcProvider(url: URL, fetchImpl: FetchLike): EthProvider { } if (body.error !== undefined) { // The server's text, without control characters (terminal escapes) and capped. - const message = String(body.error.message ?? "unknown error") - // eslint-disable-next-line no-control-regex - .replace(/[\u0000-\u001f\u007f-\u009f]/g, " ") + const message = printable(String(body.error.message ?? "unknown error")) + .replace(/\s/g, " ") .slice(0, 300); throw new Error(`RPC ${method} failed: ${message}`); } diff --git a/src/erc7201.ts b/src/erc7201.ts index 38999d6..51d2b3e 100644 --- a/src/erc7201.ts +++ b/src/erc7201.ts @@ -12,7 +12,7 @@ import type { SolcOutput, StorageLayout } from "@openzeppelin/upgrades-core"; import { styleText } from "node:util"; -import { LayoutIntegrityError } from "./errors.js"; +import { LayoutIntegrityError, LayoutUnavailableError } from "./errors.js"; import { namespaceLines, plain } from "./report.js"; // Not exported from the package index; the package has no exports map, and // all exist since 1.46.0, the lowest version slotdiff accepts. @@ -113,13 +113,10 @@ export function namespaceSlots( // that is not a struct (a mapping or an array) is assigned one. const hidden = new Set(); let untyped = false; - // Whether a struct without an annotation is placed at a known slot: it may - // hold a namespace's data under another name (a library's Layout, say). const unannotated = (struct: number) => { const node = byId.get(struct); return node === undefined || getStorageLocationAnnotation(node as never) === undefined; }; - let stray = false; for (const assembly of setAside) { forEachSlotAssignment(assembly, byId, (struct) => { if (struct === undefined) untyped = true; @@ -154,31 +151,45 @@ export function namespaceSlots( ? normalize(value.value) : undefined : resolveDecl(ctx, byId.get(declarationAt(value.src))); - if (slot !== undefined && unannotated(struct)) stray = true; assigned.set(struct, [...(assigned.get(struct) ?? []), slot]); } }); } - return namespaces.map((namespace) => { - const expected = calculateERC7201StorageLocation(namespace.slice("erc7201:".length)); - const structs = [...byId.values()].filter( + const structsOf = (namespace: string) => + [...byId.values()].filter( (n) => n.nodeType === "StructDefinition" && bases.has(n.scope as number) && getStorageLocationAnnotation(n as never) === namespace, ); + // Storage placed through any other struct (a library's Layout, annotated or + // not, or a struct with no annotation) is in no layout OpenZeppelin builds, + // so a change to it would pass unseen. A slot passed in (StorageSlot) is not known. + const namespaced = new Set(namespaces.flatMap((n) => structsOf(n).map((s) => s.id!))); + for (const [id, slots] of assigned) { + if (namespaced.has(id) || slots.every((s) => s === undefined)) continue; + const struct = byId.get(id); + const scope = byId.get(struct?.scope as number)?.name; + throw new LayoutUnavailableError( + `${contract} keeps storage at a fixed slot through ${[scope, struct?.name].filter(Boolean).join(".")}, ` + + "which is not an ERC-7201 namespace of the contract or its bases (a @custom:storage-location " + + "struct declared there): its fields are in no layout slotdiff compares, so it cannot check them.", + ); + } + + return namespaces.map((namespace) => { + const expected = calculateERC7201StorageLocation(namespace.slice("erc7201:".length)); + const structs = structsOf(namespace); const slots = structs.flatMap((n) => assigned.get(n.id!) ?? []); const known = [...new Set(slots.filter((s) => s !== undefined))]; if (slots.length === 0) { // Unused only when nothing could be placing it: no assignment in - // set-aside code, none to a struct nested in it, none to an untyped - // pointer, none to a struct without an annotation at a known slot. + // set-aside code, none to a struct nested in it, none to an untyped pointer. const nested = new Set(structs.flatMap((n) => [...nestedStructs(n, byId)])); const own = new Set(structs.map((n) => n.id!)); const placed = untyped || - stray || [...hidden].some(unannotated) || [...own, ...nested].some((id) => hidden.has(id)) || [...nested].some((id) => !own.has(id) && assigned.has(id)); @@ -218,8 +229,11 @@ export interface NamespaceCheck { layout: StorageLayout; /** Every namespace of either side, old and new paired when they are the same storage. */ pairs: NamespacePair[]; - /** `fail` when any pair has a problem other than `unread`; `unread` when a slot could not be read. */ - result: "pass" | "fail" | "unread"; + /** + * `fail` when any pair has a problem other than `unread`; `unchecked` when a + * slot could not be read (a pair's problem is `unread`), as in `UpgradeCheck`. + */ + result: "pass" | "fail" | "unchecked"; /** The CLI's namespace grid, with what each problem means; empty when nothing is worth showing. */ explain(color?: boolean): string; } @@ -284,7 +298,7 @@ export function checkNamespaces( result: problems.some((p) => p !== undefined && p !== "unread") ? "fail" : problems.includes("unread") - ? "unread" + ? "unchecked" : "pass", // Colored by default, as OpenZeppelin's reports are. explain: (color = true) => diff --git a/src/errors.ts b/src/errors.ts index f995026..9c0c95b 100644 --- a/src/errors.ts +++ b/src/errors.ts @@ -35,6 +35,16 @@ export class IncompleteBuildError extends Error { } } +/** + * Text from outside (an explorer, an RPC endpoint, solc quoting verified + * source) without control characters, so it cannot move the cursor or recolor + * a terminal. Tabs and line breaks stay. + */ +export function printable(text: string): string { + // eslint-disable-next-line no-control-regex + return text.replace(/[\u0000-\u0008\u000b-\u001f\u007f-\u009f]/g, " "); +} + /** * A network error's code (ECONNREFUSED, ENOTFOUND), never its message: fetch * quotes the URL in some messages, and URLs carry API keys. diff --git a/src/explorer.ts b/src/explorer.ts index 641d9ee..afba42e 100644 --- a/src/explorer.ts +++ b/src/explorer.ts @@ -8,7 +8,7 @@ */ import type { SolcInput } from "@openzeppelin/upgrades-core"; -import { errorCode, LayoutUnavailableError } from "./errors.js"; +import { errorCode, LayoutUnavailableError, printable } from "./errors.js"; export const ETHERSCAN_V2_API_URL = "https://api.etherscan.io/v2/api"; @@ -115,8 +115,12 @@ async function requestWithRetry(url: string, fetchImpl: FetchLike): Promise { return new Promise((r) => setTimeout(r, 1000 * attempt)); } +// Control characters other than tabs and line breaks (\v, \f and \r included). +// eslint-disable-next-line no-control-regex +const CONTROL = /[\u0000-\u0008\u000e-\u001f\u007f-\u009f]/; + /** Exported for tests. */ export function parseSourceCodeEntry(entry: SourceCodeEntry, address: string): VerifiedSource { const raw = entry.SourceCode ?? ""; @@ -144,7 +152,8 @@ export function parseSourceCodeEntry(entry: SourceCodeEntry, address: string): V const compilerVersion = entry.CompilerVersion ?? ""; if (!compilerVersion.startsWith("v0.")) { throw new LayoutUnavailableError( - `Contract ${address} was verified with "${compilerVersion}"; only Solidity is supported.`, + `Contract ${address} was verified with "${printable(compilerVersion).slice(0, 80)}"; ` + + "only Solidity is supported.", ); } @@ -154,7 +163,8 @@ export function parseSourceCodeEntry(entry: SourceCodeEntry, address: string): V ) ) { throw new LayoutUnavailableError( - `Contract ${address} reports an unrecognized compiler version "${compilerVersion}".`, + `Contract ${address} reports an unrecognized compiler version ` + + `"${printable(compilerVersion).slice(0, 80)}".`, ); } @@ -166,11 +176,30 @@ export function parseSourceCodeEntry(entry: SourceCodeEntry, address: string): V ); } - return { - input, - solcLongVersion: compilerVersion.slice(1), - contractName: entry.ContractName ?? "", - }; + // Names, paths and source text (annotations, quoted by solc) reach the + // screen; real source holds no control characters but tabs and line breaks. + const contractName = entry.ContractName ?? ""; + const sources = Object.values(input.sources) as Array<{ content?: unknown }>; + // Every source inline: solc would fetch `urls` itself. + if (sources.some((s) => typeof s?.content !== "string")) { + throw new LayoutUnavailableError( + `Contract ${address} is verified with sources that are not inline (urls); only content is read.`, + ); + } + const contents = sources.map((s) => s.content as string); + if ( + // Names and paths are printed on one line: no whitespace but a space. + [contractName, ...Object.keys(input.sources)].some( + (s) => printable(s) !== s || /[^\S ]/.test(s), + ) || + contents.some((s) => CONTROL.test(s)) + ) { + throw new LayoutUnavailableError( + `Contract ${address} is verified with control characters in its name, paths or source.`, + ); + } + + return { input, solcLongVersion: compilerVersion.slice(1), contractName }; } // Etherscan wraps standard-json in an extra pair of braces (`{{...}}`); some diff --git a/src/index.ts b/src/index.ts index 4646e14..2348b30 100644 --- a/src/index.ts +++ b/src/index.ts @@ -38,7 +38,6 @@ export { type UpgradeSide, } from "./upgrade.js"; export { - layoutFromSource, analyzeSource, analyzeBuild, type BuildAnalysis, diff --git a/src/reconstruct.ts b/src/reconstruct.ts index 8651958..4534f59 100644 --- a/src/reconstruct.ts +++ b/src/reconstruct.ts @@ -24,11 +24,9 @@ import { type StorageLayout, type ValidationRunData, } from "@openzeppelin/upgrades-core"; -// Not exported from the package index; the package has no exports map. -import { errorKinds, type ValidationError } from "@openzeppelin/upgrades-core/dist/validate/run.js"; import { compareDeployedBytecode, isProvingMatch, type ProvingMatch } from "./bytecode.js"; import { namespaceSlots, type NamespaceSlot } from "./erc7201.js"; -import { IncompleteBuildError, LayoutIntegrityError } from "./errors.js"; +import { IncompleteBuildError, LayoutIntegrityError, printable } from "./errors.js"; import type { VerifiedSource } from "./explorer.js"; import type { SolcRunner } from "./solc.js"; @@ -81,33 +79,32 @@ export async function reconstructLayout( }; } -/** - * The layout of `contract` (`source.sol:Name`) in `input`, with no bytecode - * proof. Only for sources whose provenance is already known, such as a - * project's own build-info. - */ -export async function layoutFromSource( - input: SolcInput, - solc: SolcRunner, - contract: string, -): Promise { - return (await analyzeSource(input, solc, contract)).layout; -} - export type ProxyKind = ProxyDeployment["kind"]; +type ValidationError = ReturnType[number]; + /** * OpenZeppelin's safety error kinds, without the deprecated struct-definition - * and enum-definition, which the check never reports. + * and enum-definition, which the check never reports. Listed here, not taken + * from OpenZeppelin, so its releases cannot change this API unseen; a test + * compares the two. */ -export type SafetyErrorKind = Exclude< - ValidationError["kind"], - "struct-definition" | "enum-definition" ->; - -export const SAFETY_ERROR_KINDS: readonly SafetyErrorKind[] = errorKinds.filter( - (k): k is SafetyErrorKind => k !== "struct-definition" && k !== "enum-definition", -); +export const SAFETY_ERROR_KINDS = [ + "state-variable-assignment", + "state-variable-immutable", + "external-library-linking", + "constructor", + "delegatecall", + "selfdestruct", + "missing-public-upgradeto", + "internal-function-storage", + "missing-initializer", + "missing-initializer-call", + "duplicate-initializer-call", + "incorrect-initializer-order", +] as const; + +export type SafetyErrorKind = (typeof SAFETY_ERROR_KINDS)[number]; export interface SafetyError { /** OpenZeppelin's error kind, such as `constructor`. */ @@ -151,7 +148,11 @@ export interface SourceAnalysis { safety(options: SafetyOptions): SafetyReport; } -/** `layoutFromSource` plus OpenZeppelin's safety check, from one compile. */ +/** + * The layout of `contract` (`source.sol:Name`) in `input`, its ERC-7201 slots + * and OpenZeppelin's safety check, from one compile. No bytecode proof: only + * for sources whose provenance is already known, such as a project's own build info. + */ export async function analyzeSource( input: SolcInput, solc: BuildCompiler, @@ -457,7 +458,7 @@ async function compileOrThrow( const errors = (output.errors ?? []).filter((e) => e.severity === "error"); if (errors.length > 0) { throw new LayoutIntegrityError( - `solc ${solc.longVersion} failed to compile the ${what}: ${errors[0].formattedMessage}`, + `solc ${solc.longVersion} failed to compile the ${what}: ${printable(String(errors[0].formattedMessage))}`, ); } return output; diff --git a/src/solc.ts b/src/solc.ts index e14a3be..3c7293f 100644 --- a/src/solc.ts +++ b/src/solc.ts @@ -14,9 +14,9 @@ import { spawn } from "node:child_process"; import { createHash, randomUUID } from "node:crypto"; -import { access, chmod, mkdir, readdir, rename, writeFile } from "node:fs/promises"; +import { access, chmod, mkdir, mkdtemp, readdir, rename, rm, writeFile } from "node:fs/promises"; import { constants } from "node:fs"; -import { homedir } from "node:os"; +import { homedir, tmpdir } from "node:os"; import { join } from "node:path"; import type { SolcInput, SolcOutput } from "@openzeppelin/upgrades-core"; import { LayoutIntegrityError, LayoutUnavailableError } from "./errors.js"; @@ -169,6 +169,10 @@ async function findBuild( if (build === undefined) { throw new LayoutUnavailableError(`solc ${version} has no native ${platform} build.`); } + // Both become paths: a release is matched by prefix, so its long version comes from the list. + if (!SOLC_VERSION.test(build.longVersion) || !/^[\w.+-]+$/.test(build.path)) { + throw new LayoutUnavailableError(`The solc build list has a malformed entry for ${version}.`); + } // Old Windows releases (up to 0.7.1) ship as zip archives, not executables. if (build.path.endsWith(".zip")) { throw new LayoutUnavailableError( @@ -206,9 +210,21 @@ async function download( await rename(tmp, dest); } -function runSolc(path: string, input: SolcInput): Promise { +// solc reads imports it is not given from its working directory, so it runs in +// an empty one: verified source could otherwise import, and quote in an error, +// any file under the caller's (an .env, say). +async function runSolc(path: string, input: SolcInput): Promise { + const cwd = await mkdtemp(join(tmpdir(), "slotdiff-solc-")); + try { + return await runSolcIn(cwd, path, input); + } finally { + await rm(cwd, { recursive: true, force: true }); + } +} + +function runSolcIn(cwd: string, path: string, input: SolcInput): Promise { return new Promise((resolve, reject) => { - const child = spawn(path, ["--standard-json"], { stdio: ["pipe", "pipe", "pipe"] }); + const child = spawn(path, ["--standard-json"], { cwd, stdio: ["pipe", "pipe", "pipe"] }); const out: Buffer[] = []; const err: Buffer[] = []; child.stdout.on("data", (d: Buffer) => out.push(d)); diff --git a/src/upgrade.ts b/src/upgrade.ts index def7008..729ff33 100644 --- a/src/upgrade.ts +++ b/src/upgrade.ts @@ -84,7 +84,7 @@ export function checkUpgrade( const result = !layout.ok || namespaces.result === "fail" || safety?.ok === false ? "fail" - : namespaces.result === "unread" + : namespaces.result === "unchecked" ? "unchecked" : "pass"; return { layout, namespaces, ...(safety !== undefined ? { safety } : {}), result }; diff --git a/test/analyze.test.ts b/test/analyze.test.ts index 57e783f..7918f4f 100644 --- a/test/analyze.test.ts +++ b/test/analyze.test.ts @@ -5,7 +5,8 @@ import { describe, it, expect, beforeAll, vi } from "vitest"; import { UpgradeableContractErrorReport, type SolcInput } from "@openzeppelin/upgrades-core"; // From the public entry point: library users get the same check as the CLI. -import { analyzeSource, type SafetyErrorKind } from "../src/index.js"; +import { analyzeSource, SAFETY_ERROR_KINDS, type SafetyErrorKind } from "../src/index.js"; +import { errorKinds } from "@openzeppelin/upgrades-core/dist/validate/run.js"; import { getSolc, type SolcRunner } from "../src/solc.js"; import { LayoutIntegrityError } from "../src/errors.js"; @@ -100,6 +101,16 @@ describe("analyzeSource", () => { 60_000, ); + // Verified source is someone else's text, and solc quotes it in its errors. + it("strips terminal escapes from the source solc quotes", async () => { + const sources = { "src/C.sol": `${header}contract C { uint256 a = \u001b[2K; }\n` }; + const err = (await analyzeSource(input(sources), solc, "src/C.sol:C").catch( + (e: unknown) => e, + )) as Error; + expect(err.message).toContain("failed to compile"); + expect(err.message).not.toContain("\u001b"); + }, 60_000); + it("still calls out an abstract contract as such", async () => { const sources = { "src/C.sol": `${header}abstract contract C { uint256 a; }\n` }; const err = await analyzeSource(input(sources), solc, "src/C.sol:C").catch((e: unknown) => e); @@ -108,6 +119,15 @@ describe("analyzeSource", () => { expect((err as Error).message).toMatch(/compiled to no bytecode/); }, 60_000); + // Listed by slotdiff so an OpenZeppelin release cannot change the API unseen; + // this fails when one adds or drops a kind. + it("lists OpenZeppelin's error kinds, without the two deprecated ones", () => { + const deprecated = ["struct-definition", "enum-definition"]; + expect([...SAFETY_ERROR_KINDS].sort()).toEqual( + errorKinds.filter((k) => !deprecated.includes(k)).sort(), + ); + }); + it.each(["bogus", "struct-definition"])( "refuses an unsafeAllow kind it does not know (%s), which would accept nothing", async (kind) => { diff --git a/test/cli.test.ts b/test/cli.test.ts index 9fbced9..a486c8e 100644 --- a/test/cli.test.ts +++ b/test/cli.test.ts @@ -259,6 +259,26 @@ describe("slotdiff CLI", () => { expect(String(explorer.mock.calls[0][0])).toContain("apikey=KEY"); }, 60_000); + it("echoes no terminal escapes from its own arguments", async () => { + const r = await cli(["V1", "V2", "--build-info", "x\u001b[2K.json"]); + expect(r.code).toBe(2); + expect(r.stderr).toMatch(/^Could not check: Cannot read .*x \[2K\.json/); + expect(r.stderr).not.toContain("\u001b"); + }, 60_000); + + it("prints no terminal escapes an explorer sends", async () => { + explorer = vi.fn(async () => ({ + ok: true, + status: 200, + json: async () => ({ status: "0", result: "\u001b[1A\u001b[2K\u001b[32mSafe to upgrade." }), + })); + vi.stubGlobal("fetch", explorer); + const r = await cli([PROXY, "V2", "--rpc", RPC]); + expect(r.code).toBe(2); + expect(r.stderr).toContain("Could not check: Explorer could not answer:"); + expect(r.stderr).not.toContain("\u001b"); + }, 60_000); + it("never sends the Etherscan key to another explorer", async () => { const r = await cli([PROXY, "V2", "--rpc", RPC, "--explorer-url", "https://other.test/api"]); expect(r.code).toBe(0); @@ -281,8 +301,19 @@ describe("slotdiff CLI", () => { expect(r.code).toBe(1); }, 60_000); - it("takes an address that is not a proxy as an implementation", async () => { + // A proxy slotdiff cannot read (a diamond, a custom slot) would otherwise be + // compared as its own, nearly empty, code and pass. + it("refuses an address that is not a proxy unless told it is an implementation", async () => { const r = await cli([IMPL, "V2", "--rpc", RPC]); + expect(r.code).toBe(2); + expect(r.stdout).not.toContain("Safe to upgrade"); + expect(r.stderr).toContain(`Could not check: ${IMPL} is not a proxy slotdiff can read`); + expect(r.stderr).toContain("pass --old-implementation"); + expect(explorer).not.toHaveBeenCalled(); + }); + + it("takes an implementation address with --old-implementation", async () => { + const r = await cli([IMPL, "V2", "--rpc", RPC, "--old-implementation"]); expect(r.stderr).toBe(""); expect(r.code).toBe(0); expect(r.stdout).toContain( @@ -290,6 +321,22 @@ describe("slotdiff CLI", () => { ); }, 60_000); + it("refuses --old-implementation for a proxy, and for a local ", async () => { + const proxy = await cli([PROXY, "V2", "--rpc", RPC, "--old-implementation"]); + expect(proxy.code).toBe(2); + expect(proxy.stderr).toContain(`${PROXY} is a proxy, not an implementation`); + const local = await cli(["V1", "V2", "--old-implementation"]); + expect(local.code).toBe(2); + expect(local.stderr).toBe("--old-implementation needs to be an address."); + }); + + it("still takes a implementation address as it is", async () => { + const r = await cli(["V1", IMPL, "--rpc", RPC]); + expect(r.stderr).toBe(""); + expect(r.code).toBe(0); + expect(r.stdout).toContain(`new ${IMPL} (implementation)`); + }, 60_000); + it("exits 2 for an address with no code", async () => { const r = await cli([ADMIN, "V2", "--rpc", RPC]); expect(r.code).toBe(2); diff --git a/test/erc7201.test.ts b/test/erc7201.test.ts index b88293c..a0c5cdc 100644 --- a/test/erc7201.test.ts +++ b/test/erc7201.test.ts @@ -9,7 +9,7 @@ import { calculateERC7201StorageLocation } from "@openzeppelin/upgrades-core/dis import { analyzeSource } from "../src/reconstruct.js"; import { checkNamespaces, namespaceSlots, type NamespaceSlot } from "../src/erc7201.js"; import { diffLayouts } from "../src/diff.js"; -import { LayoutIntegrityError } from "../src/errors.js"; +import { LayoutIntegrityError, LayoutUnavailableError } from "../src/errors.js"; import type { StorageLayout } from "@openzeppelin/upgrades-core"; import { getSolc, type SolcRunner } from "../src/solc.js"; @@ -257,37 +257,73 @@ contract C is Base { ])( "is never claimed when code assigns a slot to %s", async (_, type) => { - const body = `function _p() internal pure returns (${type} storage $) { assembly { $.slot := 1 } }`; + // From a parameter: a known slot would stop the analysis (see below). + const body = `function _p(bytes32 s) internal pure returns (${type} storage $) { assembly { $.slot := s } }`; expect(await statusOf(body, "", false)).toBe("unread"); }, 60_000, ); - // Its data may live under another struct's name, which OpenZeppelin does not see either. + // OpenZeppelin's layout holds only the namespaces declared in the contract and + // its bases; storage placed through any other struct would change unseen. it.each([ [ - "a library's struct", + "a library's struct, next to an assigned namespace", `library L { struct Layout { uint256 a; } - bytes32 internal constant SLOT = ${slot("x.main")}; + bytes32 internal constant SLOT = ${slot("x.lib")}; function layout() internal pure returns (Layout storage $) { assembly { $.slot := SLOT } } }`, - "function a() external view returns (uint256) { return L.layout().a; }", + "function b() external view returns (uint256) { return L.layout().a; }", + true, + "L.Layout", ], [ "a struct without an annotation", "", `struct T { uint256 a; } function _t() internal pure returns (T storage $) { assembly { $.slot := ${slot("x.other")} } }`, + false, + "C.T", + ], + [ + "a struct nested in a namespace", + "", + `function _i() internal pure returns (Inner storage $) { assembly { $.slot := 1 } }`, + false, + "Base.Inner", ], ])( - "is never claimed when code places %s at a known slot", - async (_, extra, body) => { - expect(await statusOf(body, extra, false)).toBe("unread"); + "refuses storage placed through %s at a known slot", + async (_, extra, body, getter, name) => { + await expect(statusOf(body, extra, getter)).rejects.toThrow( + new RegExp( + `keeps storage at a fixed slot through ${name.replace(".", "\\.")}, which is not`, + ), + ); }, 60_000, ); + // The diamond and Solidstate pattern: OpenZeppelin skips such a struct with a warning. + it("refuses a namespace declared in a library, whose fields no layout holds", async () => { + const content = `// SPDX-License-Identifier: MIT +pragma solidity 0.8.24; +library LibFoo { + /// @custom:storage-location erc7201:foo.storage + struct Layout { uint256 a; address b; } + bytes32 internal constant SLOT = ${slot("foo.storage")}; + function layout() internal pure returns (Layout storage l) { assembly { l.slot := SLOT } } +} +contract C { + function setA(uint256 v) external { LibFoo.layout().a = v; } +} +`; + const err = await analyze(content).catch((e: unknown) => e); + expect(err).toBeInstanceOf(LayoutUnavailableError); + expect((err as Error).message).toContain("through LibFoo.Layout"); + }, 60_000); + it("is never claimed when an overridden function places a struct without an annotation", async () => { const content = `// SPDX-License-Identifier: MIT pragma solidity 0.8.24; @@ -443,8 +479,9 @@ contract Other { /// @custom:storage-location erc7201:x.main struct S { uint256 a; } } -// Free, so its assembly counts; it assigns Other's struct, not C's. -function otherS() pure returns (Other.S storage $) { assembly { $.slot := ${slot("y.main")} } } +// Free, so its assembly counts; it assigns Other's struct, not C's (from a +// parameter: a known slot would stop the analysis). +function otherS(bytes32 s) pure returns (Other.S storage $) { assembly { $.slot := s } } `; const { namespaces } = await analyze(content); expect(namespaces[0].status).toBe("match"); @@ -572,7 +609,7 @@ describe("checkNamespaces", () => { ])("cannot check a namespace when %s is not read", (_, oldSlots, nextSlots) => { const old = { layout: layoutWith({ "erc7201:a": "A" }), namespaces: oldSlots }; const checked = checkNamespaces(old, side(nextSlots)); - expect(checked.result).toBe("unread"); + expect(checked.result).toBe("unchecked"); expect(checked.pairs[0].problem).toBe("unread"); }); @@ -617,7 +654,7 @@ describe("checkNamespaces", () => { it("cannot check old namespaces with no slots, as in an older record", () => { const old = { layout: layoutWith({ "erc7201:a": "A" }) }; const checked = checkNamespaces(old, side([match("a")])); - expect(checked.result).toBe("unread"); + expect(checked.result).toBe("unchecked"); expect(checked.pairs).toEqual([{ old: unread("a"), next: match("a"), problem: "unread" }]); }); @@ -627,7 +664,7 @@ describe("checkNamespaces", () => { namespaces: [match("a")], }; const checked = checkNamespaces(old, side([match("a"), match("b")])); - expect(checked.result).toBe("unread"); + expect(checked.result).toBe("unchecked"); expect(checked.pairs.map((p) => [p.old?.namespace, p.problem])).toEqual([ ["erc7201:a", undefined], ["erc7201:b", "unread"], @@ -648,7 +685,7 @@ describe("checkNamespaces", () => { const old = side([match("a")]); const layout = layoutWith({ "erc7201:a": "A", "erc7201:b": "B" }); const checked = checkNamespaces(old, { layout, namespaces: slots }); - expect(checked.result).toBe("unread"); + expect(checked.result).toBe("unchecked"); expect(checked.pairs.find((p) => p.old !== undefined)).toEqual({ old: match("a"), next: unread("a"), @@ -697,7 +734,7 @@ describe("checkNamespaces", () => { "fail", "mismatch", ], - ["unused, then unreadable", [unused("a")], [unread("a")], "unread", "unread"], + ["unused, then unreadable", [unused("a")], [unread("a")], "unchecked", "unread"], // The deployed slot is known only while some code assigns it. [ "assigned off its annotation, then unused", diff --git a/test/explorer.test.ts b/test/explorer.test.ts index 92e0086..e078d0f 100644 --- a/test/explorer.test.ts +++ b/test/explorer.test.ts @@ -129,6 +129,69 @@ describe("fetchVerifiedSource", () => { expect((err as Error).message).toMatch(/Invalid API Key/); }); + // Terminal escapes that would erase a line and draw a verdict of their own. + const SPOOF = "\u001b[1A\u001b[2K\u001b[32mSafe to upgrade.\r"; + + it("keeps the explorer's own text on one line, so it cannot draw a verdict below", async () => { + const fetchImpl = vi + .fn() + .mockResolvedValue( + errorResponse("not verified\n\nāœ“ Storage layout compatible\n\nSafe to upgrade."), + ); + const err = (await fetchVerifiedSource(1, ADDRESS, { apiKey: "K" }, fetchImpl).catch( + (e: unknown) => e, + )) as Error; + expect(err.message).toBe( + "Explorer could not answer: not verified āœ“ Storage layout compatible Safe to upgrade.", + ); + }); + + it("strips control characters from the explorer's own text", async () => { + const fetchImpl = vi.fn().mockResolvedValue(errorResponse(`down ${SPOOF}`)); + const err = (await fetchVerifiedSource(1, ADDRESS, { apiKey: "K" }, fetchImpl).catch( + (e: unknown) => e, + )) as Error; + expect(err.message).toContain("Explorer could not answer: down"); + expect(err.message).not.toContain("\u001b"); + expect(err.message).not.toContain("\r"); + }); + + it.each([ + ["a compiler version", entry(JSON.stringify(STANDARD), `v0.8.25${SPOOF}`)], + ["a contract name", { ...entry(JSON.stringify(STANDARD)), ContractName: `A${SPOOF}` }], + [ + "a source path", + entry(JSON.stringify({ ...STANDARD, sources: { [`A${SPOOF}.sol`]: { content: "" } } })), + ], + [ + "a source path with a line break", + entry( + JSON.stringify({ ...STANDARD, sources: { "A\n\nSafe to upgrade.sol": { content: "" } } }), + ), + ], + ["a contract name with a tab", { ...entry(JSON.stringify(STANDARD)), ContractName: "A\tB" }], + [ + "source text, such as an annotation the grid prints", + entry( + JSON.stringify({ + ...STANDARD, + sources: { "A.sol": { content: `/// @custom:storage-location erc7201:x${SPOOF}` } }, + }), + ), + ], + ])("never passes control characters on from %s", (_, verified) => { + const err = (() => { + try { + return parseSourceCodeEntry(verified, ADDRESS); + } catch (e) { + return e; + } + })(); + expect(err).toBeInstanceOf(LayoutUnavailableError); + expect((err as Error).message).not.toContain("\u001b"); + expect((err as Error).message).not.toContain("\r"); + }); + it("retries HTTP 429", async () => { vi.useFakeTimers(); const fetchImpl = vi @@ -230,6 +293,19 @@ describe("explorer edge cases", () => { }, ); + it("refuses sources given by urls, which solc would fetch itself", () => { + const urls = { ...STANDARD, sources: { "A.sol": { urls: ["file:///etc/hosts"] } } }; + expect(() => parseSourceCodeEntry(entry(JSON.stringify(urls)), ADDRESS)).toThrow( + /not inline \(urls\)/, + ); + }); + + it("accepts source with tabs and Windows line breaks", () => { + const content = "contract A {\r\n\tuint256 a;\f\r\n}"; + const crlf = { ...STANDARD, sources: { "A.sol": { content } } }; + expect(parseSourceCodeEntry(entry(JSON.stringify(crlf)), ADDRESS).input).toEqual(crlf); + }); + it("accepts a pre-release compiler version", () => { const pre = "v0.8.31-pre.1+commit.b59566f6"; expect(parseSourceCodeEntry(entry(JSON.stringify(STANDARD), pre), ADDRESS)).toMatchObject({ diff --git a/test/live.test.ts b/test/live.test.ts index 05010b2..9785e87 100644 --- a/test/live.test.ts +++ b/test/live.test.ts @@ -19,7 +19,7 @@ import type { SolcInput } from "@openzeppelin/upgrades-core"; import { resolveProxyLayout, type ProxyLayout } from "../src/resolve.js"; import { fetchVerifiedSource, type ExplorerConfig } from "../src/explorer.js"; import { readChainId } from "../src/chain.js"; -import { layoutFromSource } from "../src/reconstruct.js"; +import { analyzeSource } from "../src/reconstruct.js"; import { getSolc } from "../src/solc.js"; import { diffLayouts } from "../src/diff.js"; @@ -87,7 +87,7 @@ describe.skipIf(!RPC || !PROXY || !(EXPLORER.apiKey || EXPLORER.apiUrl))( async function check(changed: SolcInput) { const solc = await getSolc(baseline.record.compiler!); - const layout = await layoutFromSource(changed, solc, baseline.record.contract); + const layout = (await analyzeSource(changed, solc, baseline.record.contract)).layout; return diffLayouts(baseline.record.layout, layout); } diff --git a/test/resolve-e2e.test.ts b/test/resolve-e2e.test.ts index b17ae36..dcbd7c8 100644 --- a/test/resolve-e2e.test.ts +++ b/test/resolve-e2e.test.ts @@ -13,7 +13,7 @@ import type { SolcInput } from "@openzeppelin/upgrades-core"; import { resolveProxyLayout } from "../src/resolve.js"; import { verifyRecord } from "../src/record.js"; import { getSolc, type SolcRunner } from "../src/solc.js"; -import { layoutFromSource } from "../src/reconstruct.js"; +import { analyzeSource } from "../src/reconstruct.js"; import { LayoutIntegrityError } from "../src/errors.js"; import { diffLayouts } from "../src/diff.js"; import { makeMockChain } from "./helpers/mock-chain.js"; @@ -111,7 +111,7 @@ function explorerServing(served: string | SolcInput) { // The "new" side: the same extraction pipeline, applied to the local source. async function localLayout(content: string) { - return layoutFromSource(input(content), solc, "contracts/Box.sol:Box"); + return (await analyzeSource(input(content), solc, "contracts/Box.sol:Box")).layout; } beforeAll(async () => { @@ -238,7 +238,7 @@ describe("resolveProxyLayout end to end", () => { basePath: "/project", includePaths: [], } as SolcInput; - const layout = await layoutFromSource(foundryInput, solc, "contracts/Box.sol:Box"); + const layout = (await analyzeSource(foundryInput, solc, "contracts/Box.sol:Box")).layout; expect(layout).toEqual(await localLayout(V1)); }, 120_000); @@ -333,7 +333,7 @@ contract Box { }, settings: { optimizer: { enabled: true, runs: 200 }, metadata: { bytecodeHash: "none" } }, } as unknown as SolcInput; - const layout = await layoutFromSource(both, solc, "contracts/Box.sol:Box"); + const layout = (await analyzeSource(both, solc, "contracts/Box.sol:Box")).layout; expect(layout.storage.map((s) => `${s.contract}.${s.label}:${s.type}`)).toEqual([ "Box.a:t_uint256", "Box.dep:t_uint256", diff --git a/test/solc.test.ts b/test/solc.test.ts index 8b596d2..261bbe8 100644 --- a/test/solc.test.ts +++ b/test/solc.test.ts @@ -1,8 +1,9 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { join } from "node:path"; import { chmod, mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; -import { createHash } from "node:crypto"; +import { createHash, randomUUID } from "node:crypto"; import { tmpdir } from "node:os"; +import type { SolcInput } from "@openzeppelin/upgrades-core"; import { getSolc } from "../src/solc.js"; import { LayoutIntegrityError, LayoutUnavailableError } from "../src/errors.js"; @@ -182,6 +183,25 @@ describe("getSolc input and concurrency", () => { expect(solc.path).toBe("/usr/bin/true"); }); + it.each([ + [ + "a long version that leaves the cache", + { longVersion: "0.0.1+commit.0/../../x", path: "solc" }, + ], + ["a path with a directory", { longVersion: "0.0.1+commit.22222222", path: "../solc" }], + ])("refuses a build list entry with %s, before any download", async (_, entry) => { + const fetchImpl = vi.fn().mockResolvedValue({ + ok: true, + status: 200, + json: async () => ({ builds: [{ ...entry, sha256: "0x00" }] }), + arrayBuffer: async () => new ArrayBuffer(0), + }); + await expect(getSolc("0.0.1", { cacheDir }, fetchImpl)).rejects.toThrow( + new LayoutUnavailableError("The solc build list has a malformed entry for 0.0.1."), + ); + expect(fetchImpl).toHaveBeenCalledTimes(1); + }); + it("refuses a build published as a zip archive, as unavailable", async () => { const fetchImpl = vi.fn().mockResolvedValue({ ok: true, @@ -197,3 +217,24 @@ describe("getSolc input and concurrency", () => { expect(fetchImpl).toHaveBeenCalledTimes(1); // never downloads it }); }); + +// Verified source is someone else's: it must not reach files of the caller's. +describe("getSolc sandbox", () => { + it("compiles in an empty directory, so an import cannot read the caller's files", async () => { + const decoy = `.slotdiff-decoy-${randomUUID()}`; + await writeFile(join(process.cwd(), decoy), "DECOY_SECRET=1\n"); + try { + const solc = await getSolc("0.8.24+commit.e11b9ed9"); + const output = await solc.compile({ + language: "Solidity", + sources: { "A.sol": { content: `import "${decoy}"; contract A {}` } }, + settings: { outputSelection: {} }, + } as SolcInput); + const text = JSON.stringify(output.errors); + expect(text).toContain("File not found"); + expect(text).not.toContain("DECOY_SECRET"); + } finally { + await rm(join(process.cwd(), decoy)); + } + }, 120_000); +});