CLI: Disallow self-references for storage layout validations (#1067)

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)

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;
+}

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",

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);
+});

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<ValidationOptions>,
   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<ValidationOptions>,
   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));