CLI: Disallow self-references for storage layout validations

packages/core/CHANGELOG.md

@@ -1,5 +1,9 @@
 # Changelog
 
+## Unreleased
+
+- **Breaking change**: CLI: Disallow self-references for storage layout validations. ([#1067](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/1067))
+
 ## 1.36.0 (2024-08-21)
 
 - Update dependency on Slang. ([#1059](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/1059))

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/src/cli/cli.test.ts

@@ -572,3 +572,45 @@ test('validate - contract must not have build info dir name - fully qualified',
     error?.message,
   );
 });
+
+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,
@@ -18,6 +17,7 @@ import { SourceContract } from './validations';
 import { LayoutCompatibilityReport } from '../../storage/report';
 import { indent } from '../../utils/indent';
 import { BuildInfoDictionary, SpecifiedContracts } from './validate-upgrade-safety';
+import { ValidateCommandError } from './error';
 
 /**
  * Report for an upgradeable contract.
@@ -29,7 +29,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);
@@ -103,7 +112,7 @@ export function getContractReports(
 function getUpgradeableContractReport(
   contract: SourceContract,
   referenceContract: SourceContract | undefined,
-  opts: ValidationOptions,
+  opts: Required<ValidationOptions>,
 ): UpgradeableContractReport | undefined {
   let version;
   try {
@@ -135,7 +144,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);
@@ -144,8 +153,8 @@ function getUpgradeableContractReport(
 function getStandaloneReport(
   data: ValidationData,
   version: Version,
-  opts: ValidationOptions,
+  opts: Required<ValidationOptions>,
 ): UpgradeableContractErrorReport {
-  const errors = getErrors(data, version, withValidationDefaults(opts));
+  const errors = getErrors(data, version, opts);
   return new UpgradeableContractErrorReport(errors);
 }