From e8533fecb27c390e46fbef26c86ba4b9ab2294e3 Mon Sep 17 00:00:00 2001 From: Rohit Solia Date: Tue, 20 Oct 2026 17:59:25 -0500 Subject: [PATCH 1/9] fix: strip terminal escapes from outside text --- CHANGELOG.md | 1 + src/cli.ts | 10 ++++----- src/errors.ts | 10 +++++++++ src/explorer.ts | 36 +++++++++++++++++++++++--------- src/reconstruct.ts | 4 ++-- test/analyze.test.ts | 10 +++++++++ test/cli.test.ts | 20 ++++++++++++++++++ test/explorer.test.ts | 48 +++++++++++++++++++++++++++++++++++++++++++ 8 files changed, 122 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index abf3bdf..aaa31fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ - `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. +- Text from an explorer or from verified source can no longer move the cursor or recolor the terminal: control characters are stripped from explorer messages and solc's errors, and verified source with control characters in its name, paths or text (tabs and line breaks aside) is refused (`LayoutUnavailableError`). - **Breaking:** a contract missing from the compiler input throws a plain `Error` (was `LayoutIntegrityError`), as a wrong argument; `NamespacePair.problem` adds `"dropped"`. ## 0.2.0-alpha.1 diff --git a/src/cli.ts b/src/cli.ts index 9b05522..1f700ec 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 { @@ -258,7 +258,8 @@ export async function run(argv: string[], io: CliIo): Promise { // 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]); } 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; } @@ -418,9 +419,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/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..b76b99b 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,9 @@ 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 +149,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 +160,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 +173,20 @@ 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 contents = Object.values(input.sources).map((s) => String(s.content ?? "")); + if ( + [contractName, ...Object.keys(input.sources)].some((s) => printable(s) !== 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/reconstruct.ts b/src/reconstruct.ts index 8651958..4091d8f 100644 --- a/src/reconstruct.ts +++ b/src/reconstruct.ts @@ -28,7 +28,7 @@ import { 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"; @@ -457,7 +457,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/test/analyze.test.ts b/test/analyze.test.ts index 57e783f..c5a66a1 100644 --- a/test/analyze.test.ts +++ b/test/analyze.test.ts @@ -100,6 +100,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); diff --git a/test/cli.test.ts b/test/cli.test.ts index 9fbced9..f564281 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); diff --git a/test/explorer.test.ts b/test/explorer.test.ts index 92e0086..fe2817a 100644 --- a/test/explorer.test.ts +++ b/test/explorer.test.ts @@ -129,6 +129,48 @@ 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("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: "" } } })), + ], + [ + "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 +272,12 @@ describe("explorer edge cases", () => { }, ); + 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({ From 14423626af2d047eee9ab5c221c0e62f0542c5bd Mon Sep 17 00:00:00 2001 From: Rohit Solia Date: Tue, 20 Oct 2026 17:59:57 -0500 Subject: [PATCH 2/9] fix: refuse an old address that is not a proxy --- CHANGELOG.md | 1 + README.md | 2 +- src/cli.ts | 32 +++++++++++++++++++++++++++----- test/cli.test.ts | 29 ++++++++++++++++++++++++++++- 4 files changed, 57 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index aaa31fc..d65684f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ - `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. +- CLI: an `` address that is not an ERC-1967 or beacon proxy is refused (exit 2) unless `--old-implementation` says it is an implementation. In 0.2.0 it 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. **Breaking** for an `` implementation address: add `--old-implementation`. - Text from an explorer or from verified source can no longer move the cursor or recolor the terminal: control characters are stripped from explorer messages and solc's errors, and verified source with control characters in its name, paths or text (tabs and line breaks aside) is refused (`LayoutUnavailableError`). - **Breaking:** a contract missing from the compiler input throws a plain `Error` (was `LayoutIntegrityError`), as a wrong argument; `NamespacePair.problem` adds `"dropped"`. diff --git a/README.md b/README.md index 848c6d0..7eb402a 100644 --- a/README.md +++ b/README.md @@ -49,7 +49,7 @@ 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. diff --git a/src/cli.ts b/src/cli.ts index 1f700ec..ef4c1a5 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -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,9 +276,9 @@ 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) { // Messages can quote an explorer or verified source. io.stderr(`Could not check: ${printable((e as Error).message)}`); diff --git a/test/cli.test.ts b/test/cli.test.ts index f564281..a486c8e 100644 --- a/test/cli.test.ts +++ b/test/cli.test.ts @@ -301,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( @@ -310,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); From c3bb5e004238766176fbe1dc30038ea70fe1309c Mon Sep 17 00:00:00 2001 From: Rohit Solia Date: Tue, 20 Oct 2026 18:07:08 -0500 Subject: [PATCH 3/9] docs: lead with checkUpgrade for a live proxy --- README.md | 32 +++++++++++++------------------- 1 file changed, 13 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index 7eb402a..7503cbe 100644 --- a/README.md +++ b/README.md @@ -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"; From 73d1fd02a843109582a2a2e11e5b6f5e22aa7c99 Mon Sep 17 00:00:00 2001 From: Rohit Solia Date: Tue, 20 Oct 2026 18:07:09 -0500 Subject: [PATCH 4/9] refactor!: simplify the API before 0.3 --- CHANGELOG.md | 3 ++- README.md | 3 +-- src/cli.ts | 2 +- src/erc7201.ts | 9 ++++--- src/index.ts | 1 - src/reconstruct.ts | 51 ++++++++++++++++++++-------------------- src/upgrade.ts | 2 +- test/analyze.test.ts | 12 +++++++++- test/erc7201.test.ts | 10 ++++---- test/live.test.ts | 4 ++-- test/resolve-e2e.test.ts | 8 +++---- 11 files changed, 59 insertions(+), 46 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d65684f..02033af 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,8 @@ - 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. - CLI: an `` address that is not an ERC-1967 or beacon proxy is refused (exit 2) unless `--old-implementation` says it is an implementation. In 0.2.0 it 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. **Breaking** for an `` implementation address: add `--old-implementation`. - Text from an explorer or from verified source can no longer move the cursor or recolor the terminal: control characters are stripped from explorer messages and solc's errors, and verified source with control characters in its name, paths or text (tabs and line breaks aside) is refused (`LayoutUnavailableError`). -- **Breaking:** a contract missing from the compiler input throws a plain `Error` (was `LayoutIntegrityError`), as a wrong argument; `NamespacePair.problem` adds `"dropped"`. +- **Breaking:** a contract missing from the compiler input throws a plain `Error` (was `LayoutIntegrityError`), as a wrong argument; `NamespacePair.problem` adds `"dropped"`; `NamespaceCheck.result` says `"unchecked"` (was `"unread"`), as `UpgradeCheck.result` does; `layoutFromSource` is removed (use `(await analyzeSource(input, solc, contract)).layout`, at the same cost). +- `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. ## 0.2.0-alpha.1 diff --git a/README.md b/README.md index 7503cbe..1ec0e9f 100644 --- a/README.md +++ b/README.md @@ -183,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 @@ -289,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` | diff --git a/src/cli.ts b/src/cli.ts index ef4c1a5..422cee7 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -322,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", diff --git a/src/erc7201.ts b/src/erc7201.ts index 38999d6..f238e9e 100644 --- a/src/erc7201.ts +++ b/src/erc7201.ts @@ -218,8 +218,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 +287,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/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 4091d8f..4534f59 100644 --- a/src/reconstruct.ts +++ b/src/reconstruct.ts @@ -24,8 +24,6 @@ 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, printable } from "./errors.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, 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 c5a66a1..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"; @@ -118,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/erc7201.test.ts b/test/erc7201.test.ts index b88293c..1d4726e 100644 --- a/test/erc7201.test.ts +++ b/test/erc7201.test.ts @@ -572,7 +572,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 +617,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 +627,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 +648,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 +697,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/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", From 7326463a3b5784a8e49c4161dc9fa166843dbf11 Mon Sep 17 00:00:00 2001 From: Rohit Solia Date: Tue, 20 Oct 2026 18:13:46 -0500 Subject: [PATCH 5/9] fix: harden solc lookup and release workflows --- .github/workflows/oz-latest.yml | 34 +++++++++++++++++++++++++++++++++ .github/workflows/publish.yml | 4 +++- CHANGELOG.md | 1 + README.md | 2 +- src/solc.ts | 4 ++++ test/solc.test.ts | 19 ++++++++++++++++++ 6 files changed, 62 insertions(+), 2 deletions(-) create mode 100644 .github/workflows/oz-latest.yml 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..fee563e 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -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 02033af..d7e0f8c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ - 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. - CLI: an `` address that is not an ERC-1967 or beacon proxy is refused (exit 2) unless `--old-implementation` says it is an implementation. In 0.2.0 it 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. **Breaking** for an `` implementation address: add `--old-implementation`. - Text from an explorer or from verified source can no longer move the cursor or recolor the terminal: control characters are stripped from explorer messages and solc's errors, and verified source with control characters in its name, paths or text (tabs and line breaks aside) is refused (`LayoutUnavailableError`). +- `getSolc` refuses a malformed entry in solc's build list before it becomes a path. The publish workflow installs dependencies without their install scripts; a weekly workflow tests against the newest OpenZeppelin upgrades-core 1.x. - **Breaking:** a contract missing from the compiler input throws a plain `Error` (was `LayoutIntegrityError`), as a wrong argument; `NamespacePair.problem` adds `"dropped"`; `NamespaceCheck.result` says `"unchecked"` (was `"unread"`), as `UpgradeCheck.result` does; `layoutFromSource` is removed (use `(await analyzeSource(input, solc, contract)).layout`, at the same cost). - `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. diff --git a/README.md b/README.md index 1ec0e9f..cc14782 100644 --- a/README.md +++ b/README.md @@ -51,7 +51,7 @@ 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 (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: diff --git a/src/solc.ts b/src/solc.ts index e14a3be..c53cbd5 100644 --- a/src/solc.ts +++ b/src/solc.ts @@ -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( diff --git a/test/solc.test.ts b/test/solc.test.ts index 8b596d2..84aa55d 100644 --- a/test/solc.test.ts +++ b/test/solc.test.ts @@ -182,6 +182,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, From ac699cbedca833234337b6642cb51ed9c3551e38 Mon Sep 17 00:00:00 2001 From: Rohit Solia Date: Tue, 20 Oct 2026 20:23:08 -0500 Subject: [PATCH 6/9] fix: sandbox solc and keep outside text on one line --- CHANGELOG.md | 3 ++- SECURITY.md | 6 +++--- src/explorer.ts | 19 ++++++++++++++++--- src/solc.ts | 20 ++++++++++++++++---- test/explorer.test.ts | 28 ++++++++++++++++++++++++++++ test/solc.test.ts | 24 +++++++++++++++++++++++- 6 files changed, 88 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d7e0f8c..a51fa2e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,8 @@ - `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. - CLI: an `` address that is not an ERC-1967 or beacon proxy is refused (exit 2) unless `--old-implementation` says it is an implementation. In 0.2.0 it 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. **Breaking** for an `` implementation address: add `--old-implementation`. -- Text from an explorer or from verified source can no longer move the cursor or recolor the terminal: control characters are stripped from explorer messages and solc's errors, and verified source with control characters in its name, paths or text (tabs and line breaks aside) is refused (`LayoutUnavailableError`). +- solc runs in a new empty directory. Since 0.1.0 it ran in the caller's, so verified source could `import` a file under it (an `.env`, say) and solc would quote its first line in the compile error. Verified source given by `urls` instead of inline `content` is refused. +- Text from an explorer or from verified source can no longer move the cursor, recolor the terminal or draw lines of its own: 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`). - `getSolc` refuses a malformed entry in solc's build list before it becomes a path. The publish workflow installs dependencies without their install scripts; a weekly workflow tests against the newest OpenZeppelin upgrades-core 1.x. - **Breaking:** a contract missing from the compiler input throws a plain `Error` (was `LayoutIntegrityError`), as a wrong argument; `NamespacePair.problem` adds `"dropped"`; `NamespaceCheck.result` says `"unchecked"` (was `"unread"`), as `UpgradeCheck.result` does; `layoutFromSource` is removed (use `(await analyzeSource(input, solc, contract)).layout`, at the same cost). - `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. 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/src/explorer.ts b/src/explorer.ts index b76b99b..afba42e 100644 --- a/src/explorer.ts +++ b/src/explorer.ts @@ -115,9 +115,12 @@ async function requestWithRetry(url: string, fetchImpl: FetchLike): Promise String(s.content ?? "")); + 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 ( - [contractName, ...Object.keys(input.sources)].some((s) => printable(s) !== s) || + // 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( diff --git a/src/solc.ts b/src/solc.ts index c53cbd5..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"; @@ -210,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/test/explorer.test.ts b/test/explorer.test.ts index fe2817a..e078d0f 100644 --- a/test/explorer.test.ts +++ b/test/explorer.test.ts @@ -132,6 +132,20 @@ describe("fetchVerifiedSource", () => { // 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( @@ -149,6 +163,13 @@ describe("fetchVerifiedSource", () => { "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( @@ -272,6 +293,13 @@ 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 } } }; diff --git a/test/solc.test.ts b/test/solc.test.ts index 84aa55d..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"; @@ -216,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); +}); From d22e0ad16596e17fbdb91821e971eb2743c2cf67 Mon Sep 17 00:00:00 2001 From: Rohit Solia Date: Tue, 20 Oct 2026 20:27:47 -0500 Subject: [PATCH 7/9] fix: refuse storage outside the namespaces --- CHANGELOG.md | 3 ++- README.md | 3 ++- src/erc7201.ts | 33 ++++++++++++++++--------- test/erc7201.test.ts | 59 +++++++++++++++++++++++++++++++++++--------- 4 files changed, 74 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a51fa2e..3edd8f4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,12 +6,13 @@ - `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`). +- 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), 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. - CLI: an `` address that is not an ERC-1967 or beacon proxy is refused (exit 2) unless `--old-implementation` says it is an implementation. In 0.2.0 it 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. **Breaking** for an `` implementation address: add `--old-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, the diamond and Solidstate pattern, or a struct with no annotation) stops the analysis with `LayoutUnavailableError` (exit 2). OpenZeppelin leaves such structs out of the layout with a warning, so in 0.2.0 a change to them passed. - solc runs in a new empty directory. Since 0.1.0 it ran in the caller's, so verified source could `import` a file under it (an `.env`, say) and solc would quote its first line in the compile error. Verified source given by `urls` instead of inline `content` is refused. - Text from an explorer or from verified source can no longer move the cursor, recolor the terminal or draw lines of its own: 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`). - `getSolc` refuses a malformed entry in solc's build list before it becomes a path. The publish workflow installs dependencies without their install scripts; a weekly workflow tests against the newest OpenZeppelin upgrades-core 1.x. diff --git a/README.md b/README.md index cc14782..15216cf 100644 --- a/README.md +++ b/README.md @@ -307,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/src/erc7201.ts b/src/erc7201.ts index f238e9e..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)); diff --git a/test/erc7201.test.ts b/test/erc7201.test.ts index 1d4726e..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"); From 5993bfeb98c3ba7f1145b01785b546ec9e4d6aeb Mon Sep 17 00:00:00 2001 From: Rohit Solia Date: Tue, 20 Oct 2026 20:28:19 -0500 Subject: [PATCH 8/9] docs: group the changelog by kind --- CHANGELOG.md | 43 +++++++++++++++++++++++++++++-------------- 1 file changed, 29 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3edd8f4..58e2a15 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,22 +2,37 @@ ## Unreleased -- `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`. +### 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 + - `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 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. -- CLI: an `` address that is not an ERC-1967 or beacon proxy is refused (exit 2) unless `--old-implementation` says it is an implementation. In 0.2.0 it 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. **Breaking** for an `` implementation address: add `--old-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, the diamond and Solidstate pattern, or a struct with no annotation) stops the analysis with `LayoutUnavailableError` (exit 2). OpenZeppelin leaves such structs out of the layout with a warning, so in 0.2.0 a change to them passed. -- solc runs in a new empty directory. Since 0.1.0 it ran in the caller's, so verified source could `import` a file under it (an `.env`, say) and solc would quote its first line in the compile error. Verified source given by `urls` instead of inline `content` is refused. -- Text from an explorer or from verified source can no longer move the cursor, recolor the terminal or draw lines of its own: 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`). -- `getSolc` refuses a malformed entry in solc's build list before it becomes a path. The publish workflow installs dependencies without their install scripts; a weekly workflow tests against the newest OpenZeppelin upgrades-core 1.x. -- **Breaking:** a contract missing from the compiler input throws a plain `Error` (was `LayoutIntegrityError`), as a wrong argument; `NamespacePair.problem` adds `"dropped"`; `NamespaceCheck.result` says `"unchecked"` (was `"unread"`), as `UpgradeCheck.result` does; `layoutFromSource` is removed (use `(await analyzeSource(input, solc, contract)).layout`, at the same cost). +- `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 From b16b053fea34852e3b4279d0df61dfed415806ab Mon Sep 17 00:00:00 2001 From: Rohit Solia Date: Tue, 20 Oct 2026 20:42:17 -0500 Subject: [PATCH 9/9] chore: release 0.3.0 --- .github/workflows/publish.yml | 2 +- CHANGELOG.md | 2 +- package.json | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index fee563e..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 diff --git a/CHANGELOG.md b/CHANGELOG.md index 58e2a15..eb236c1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ # Changelog -## Unreleased +## 0.3.0 ### Fixed 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)",