From 8f663a8af303b7cb12a88ce727eb59d1659787f6 Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Wed, 28 Aug 2024 16:28:31 -0400 Subject: [PATCH] CLI: Disallow self-references for storage layout validations (#1067) --- packages/core/CHANGELOG.md | 3 +- .../contracts/test/cli/SelfReferences.sol | 16 +++++++ packages/core/package.json | 2 +- packages/core/src/cli/cli.test.ts | 42 +++++++++++++++++++ .../core/src/cli/validate/contract-report.ts | 20 ++++++--- 5 files changed, 75 insertions(+), 8 deletions(-) create mode 100644 packages/core/contracts/test/cli/SelfReferences.sol diff --git a/packages/core/CHANGELOG.md b/packages/core/CHANGELOG.md index 55378041e..386aced96 100644 --- a/packages/core/CHANGELOG.md +++ b/packages/core/CHANGELOG.md @@ -1,7 +1,8 @@ # Changelog -## Unreleased +## 1.37.0 (2024-08-28) +- **Breaking change**: CLI: Disallow self-references for storage layout validations. ([#1067](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/1067)) - CLI: Support `--exclude` option. ([#1065](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/1065)) ## 1.36.0 (2024-08-21) diff --git a/packages/core/contracts/test/cli/SelfReferences.sol b/packages/core/contracts/test/cli/SelfReferences.sol new file mode 100644 index 000000000..4d2044f87 --- /dev/null +++ b/packages/core/contracts/test/cli/SelfReferences.sol @@ -0,0 +1,16 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.8; + +/// @custom:oz-upgrades-from SelfReference +contract SelfReference { + uint public x; +} + +/// @custom:oz-upgrades-from contracts/test/cli/SelfReferences.sol:SelfReferenceFullyQualified +contract SelfReferenceFullyQualified { + uint public x; +} + +contract NoAnnotation { + uint public x; +} \ No newline at end of file diff --git a/packages/core/package.json b/packages/core/package.json index 00b792aba..435da609e 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -1,6 +1,6 @@ { "name": "@openzeppelin/upgrades-core", - "version": "1.36.0", + "version": "1.37.0", "description": "", "repository": "https://github.com/OpenZeppelin/openzeppelin-upgrades/tree/master/packages/core", "license": "MIT", diff --git a/packages/core/src/cli/cli.test.ts b/packages/core/src/cli/cli.test.ts index 2af1a71c0..421e9b3ab 100644 --- a/packages/core/src/cli/cli.test.ts +++ b/packages/core/src/cli/cli.test.ts @@ -670,3 +670,45 @@ test('validate - excludes UpgradeableBeacon and its parents by default, and excl const expectation: string[] = [`Stdout: ${(error as any).stdout}`, `Stderr: ${(error as any).stderr}`]; t.snapshot(expectation.join('\n')); }); + +test('validate - self reference by annotation', async t => { + const temp = await getTempDir(t); + const buildInfo = await artifacts.getBuildInfo(`contracts/test/cli/SelfReferences.sol:SelfReference`); + await fs.writeFile(path.join(temp, 'validate.json'), JSON.stringify(buildInfo)); + + const error = await t.throwsAsync(execAsync(`${CLI} validate ${temp} --contract SelfReference`)); + t.assert(error?.message.includes('must not use itself as a reference'), error?.message); +}); + +test('validate - self reference by fully qualified annotation', async t => { + const temp = await getTempDir(t); + const buildInfo = await artifacts.getBuildInfo(`contracts/test/cli/SelfReferences.sol:SelfReference`); + await fs.writeFile(path.join(temp, 'validate.json'), JSON.stringify(buildInfo)); + + const error = await t.throwsAsync(execAsync(`${CLI} validate ${temp} --contract SelfReferenceFullyQualified`)); + t.assert(error?.message.includes('must not use itself as a reference'), error?.message); +}); + +test('validate - self reference by option', async t => { + const temp = await getTempDir(t); + const buildInfo = await artifacts.getBuildInfo(`contracts/test/cli/SelfReferences.sol:SelfReference`); + await fs.writeFile(path.join(temp, 'validate.json'), JSON.stringify(buildInfo)); + + const error = await t.throwsAsync( + execAsync(`${CLI} validate ${temp} --contract NoAnnotation --reference NoAnnotation`), + ); + t.assert(error?.message.includes('must not use itself as a reference'), error?.message); +}); + +test('validate - self reference by fully qualified option', async t => { + const temp = await getTempDir(t); + const buildInfo = await artifacts.getBuildInfo(`contracts/test/cli/SelfReferences.sol:SelfReference`); + await fs.writeFile(path.join(temp, 'validate.json'), JSON.stringify(buildInfo)); + + const error = await t.throwsAsync( + execAsync( + `${CLI} validate ${temp} --contract NoAnnotation --reference contracts/test/cli/SelfReferences.sol:NoAnnotation`, + ), + ); + t.assert(error?.message.includes('must not use itself as a reference'), error?.message); +}); diff --git a/packages/core/src/cli/validate/contract-report.ts b/packages/core/src/cli/validate/contract-report.ts index 18379b1a2..fa46b321f 100644 --- a/packages/core/src/cli/validate/contract-report.ts +++ b/packages/core/src/cli/validate/contract-report.ts @@ -4,7 +4,6 @@ import { getContractVersion, getStorageLayout, ValidationOptions, - withValidationDefaults, Version, ValidationData, ValidateUpgradeSafetyOptions, @@ -32,7 +31,16 @@ export class UpgradeableContractReport implements Report { readonly reference: string | undefined, readonly standaloneReport: UpgradeableContractErrorReport, readonly storageLayoutReport: LayoutCompatibilityReport | undefined, - ) {} + ) { + if (reference === contract) { + throw new ValidateCommandError( + `The contract ${contract} must not use itself as a reference for storage layout comparisons.`, + () => `\ +If this is the first version of your contract, do not specify a reference. +If this is a subsequent version, keep the previous version of the contract in another file and specify that as the reference, or specify a reference from another build info directory containing the previous version. If you do not have the previous version available, you can skip the storage layout check using the \`unsafeSkipStorageCheck\` option, which is a dangerous option meant to be used as a last resort.`, + ); + } + } get ok(): boolean { return this.standaloneReport.ok && (this.storageLayoutReport === undefined || this.storageLayoutReport.ok); @@ -123,7 +131,7 @@ export function getContractReports( function getUpgradeableContractReport( contract: SourceContract, referenceContract: SourceContract | undefined, - opts: ValidationOptions, + opts: Required, exclude?: string[], ): UpgradeableContractReport | undefined { const excludeWithDefaults = defaultExclude.concat(exclude ?? []); @@ -163,7 +171,7 @@ function getUpgradeableContractReport( } else { reference = referenceContract.fullyQualifiedName; } - storageLayoutReport = getStorageUpgradeReport(referenceLayout, layout, withValidationDefaults(opts)); + storageLayoutReport = getStorageUpgradeReport(referenceLayout, layout, opts); } return new UpgradeableContractReport(contract.fullyQualifiedName, reference, standaloneReport, storageLayoutReport); @@ -172,10 +180,10 @@ function getUpgradeableContractReport( function getStandaloneReport( data: ValidationData, version: Version, - opts: ValidationOptions, + opts: Required, excludeWithDefaults: string[], ): UpgradeableContractErrorReport { - const allErrors = getErrors(data, version, withValidationDefaults(opts)); + const allErrors = getErrors(data, version, opts); const includeErrors = allErrors.filter(e => { const shouldExclude = excludeWithDefaults.some(glob => minimatch(getPath(e.src), glob));