Check for initializer errors

packages/core/contracts/test/ValidationsInitializer.sol

@@ -0,0 +1,223 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.0;
+
+import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
+
+// These contracts are for testing only. They are not safe for use in production, and do not represent best practices.
+
+// ==== Parent contracts ====
+
+contract Parent_NoInitializer {
+  function parentFn() internal {}
+}
+
+contract Parent_InitializerModifier is Initializable {
+  function parentInit() initializer public {}
+}
+
+contract Parent_ReinitializerModifier is Initializable {
+  function parentReinit() reinitializer(2) public {}
+}
+
+contract Parent__OnlyInitializingModifier is Initializable {
+  function __Parent_init() onlyInitializing() internal {}
+}
+
+contract Parent_InitializeName {
+  function initialize() public virtual {}
+}
+
+contract Parent_InitializerName {
+  function initializer() public {}
+}
+
+contract Parent_ReinitializeName {
+  function reinitialize(uint64 version) public {}
+}
+
+contract Parent_ReinitializerName {
+  function reinitializer(uint64 version) public {}
+}
+
+// ==== Child contracts ====
+
+contract Child_Of_NoInitializer_Ok is Parent_NoInitializer {
+  function childFn() public {}
+}
+
+contract Child_Of_InitializerModifier_Ok is Parent_InitializerModifier {
+  function initialize() public {
+    parentInit();
+  }
+}
+
+contract Child_Of_InitializerModifier_UsesSuper_Ok is Parent_InitializerModifier {
+  function initialize() public {
+    super.parentInit();
+  }
+}
+
+contract Child_Of_InitializerModifier_Bad is Parent_InitializerModifier {
+  function initialize() public {}
+}
+
+contract Child_Of_ReinitializerModifier_Ok is Parent_ReinitializerModifier {
+  function initialize() public {
+    parentReinit();
+  }
+}
+
+contract Child_Of_ReinitializerModifier_Bad is Parent_ReinitializerModifier {
+  function initialize() public {}
+}
+
+contract Child_Of_OnlyInitializingModifier_Ok is Parent__OnlyInitializingModifier {
+  function initialize() public {
+    __Parent_init();
+  }
+}
+
+contract Child_Of_OnlyInitializingModifier_Bad is Parent__OnlyInitializingModifier {
+  function initialize() public {}
+}
+
+// This is considered to have a missing initializer because the `regularFn` function is not inferred as an intializer
+contract MissingInitializer_Bad is Parent_InitializerModifier {
+  function regularFn() public {
+    parentInit();
+  }
+}
+
+/// @custom:oz-upgrades-unsafe-allow missing-initializer
+contract MissingInitializer_UnsafeAllow_Contract is Parent_InitializerModifier {
+  function regularFn() public {
+    parentInit();
+  }
+}
+
+contract A is Initializable {
+  function __A_init() onlyInitializing internal {}
+}
+
+contract B is Initializable {
+  function __B_init() onlyInitializing internal {}
+}
+
+contract C is Initializable {
+  function __C_init() onlyInitializing internal {}
+}
+
+contract InitializationOrder_Ok is A, B, C, Parent_NoInitializer {
+  function initialize() public {
+    __A_init();
+    __B_init();
+    __C_init();
+  }
+}
+
+contract InitializationOrder_Ok_2 is A, B, C, Parent_NoInitializer {
+  function initialize() public {
+    __A_init();
+    __B_init();
+    parentFn(); // this is not an initializer so we don't check its linearization order
+    __C_init();
+  }
+}
+
+contract InitializationOrder_WrongOrder_Bad is A, B, C, Parent_NoInitializer {
+  function initialize() public {
+    __A_init();
+    __C_init();
+    parentFn();
+    __B_init();
+  }
+}
+
+/// @custom:oz-upgrades-unsafe-allow incorrect-initializer-order
+contract InitializationOrder_WrongOrder_UnsafeAllow_Contract is A, B, C, Parent_NoInitializer {
+  function initialize() public {
+    __A_init();
+    __C_init();
+    parentFn();
+    __B_init();
+  }
+}
+
+contract InitializationOrder_WrongOrder_UnsafeAllow_Function is A, B, C, Parent_NoInitializer {
+  /// @custom:oz-upgrades-unsafe-allow incorrect-initializer-order
+  function initialize() public {
+    __A_init();
+    __C_init();
+    parentFn();
+    __B_init();
+  }
+}
+
+contract InitializationOrder_MissingCall_Bad is A, B, C, Parent_NoInitializer {
+  function initialize() public {
+    __A_init();
+    __B_init();
+    parentFn();
+  }
+}
+
+/// @custom:oz-upgrades-unsafe-allow missing-initializer-call
+contract InitializationOrder_MissingCall_UnsafeAllow_Contract is A, B, C, Parent_NoInitializer {
+  function initialize() public {
+    __A_init();
+    __B_init();
+    parentFn();
+  }
+}
+
+contract InitializationOrder_MissingCall_UnsafeAllow_Function is A, B, C, Parent_NoInitializer {
+  /// @custom:oz-upgrades-unsafe-allow missing-initializer-call
+  function initialize() public {
+    __A_init();
+    __B_init();
+    parentFn();
+  }
+}
+
+contract InitializationOrder_Duplicate_Bad is A, B, C, Parent_NoInitializer {
+  function initialize() public {
+    __A_init();
+    __B_init();
+    parentFn();
+    __B_init();
+    __C_init();
+  }
+}
+
+/// @custom:oz-upgrades-unsafe-allow duplicate-initializer-call
+contract InitializationOrder_Duplicate_UnsafeAllow_Contract is A, B, C, Parent_NoInitializer {
+  function initialize() public {
+    __A_init();
+    __B_init();
+    parentFn();
+    __B_init();
+    __C_init();
+  }
+}
+
+contract InitializationOrder_Duplicate_UnsafeAllow_Function is A, B, C, Parent_NoInitializer {
+  /// @custom:oz-upgrades-unsafe-allow duplicate-initializer-call
+  function initialize() public {
+    __A_init();
+    __B_init();
+    parentFn();
+    __B_init();
+    __C_init();
+  }
+}
+
+contract InitializationOrder_Duplicate_UnsafeAllow_Call is A, B, C, Parent_NoInitializer {
+  function initialize() public {
+    __A_init();
+    __B_init();
+    parentFn();
+    /// @custom:oz-upgrades-unsafe-allow duplicate-initializer-call
+    __B_init();
+    __C_init();
+  }
+}

packages/core/contracts/test/cli/excludes/AbstractUUPS.sol

@@ -11,4 +11,8 @@ abstract contract AbstractUUPS is UUPSUpgradeable, Abstract1, Abstract2 {
     constructor(uint256 _x, uint256 _y, uint256 _z) Abstract1(_x) Abstract2(_y) {
         z = _z;
     }
+
+    function initialize() initializer public {
+        __UUPSUpgradeable_init();
+    }
 }

packages/core/contracts/test/cli/excludes/UsesAbstractUUPS.sol

@@ -12,4 +12,8 @@ contract UsesAbstractUUPS is AbstractUUPS {
     function _authorizeUpgrade(address newImplementation) internal pure override {
         revert("Upgrade disabled");
     }
+
+    function initializeChild() initializer public {
+        super.initialize();
+    }
 }

packages/core/src/cli/cli.test.ts.md

@@ -21,7 +21,7 @@ Generated by [AVA](https://avajs.dev).
       --requireReference  Can only be used when the --contract option is also provided. Not compatible with --unsafeSkipStorageCheck. If specified, requires either the --reference option to be provided or the contract to have a @custom:oz-upgrades-from annotation.␊
       --referenceBuildInfoDirs "<BUILD_INFO_DIR>[,<BUILD_INFO_DIR>...]"  Optional paths of additional build info directories from previous versions of the project to use for storage layout comparisons. When using this option, refer to one of these directories using prefix '<dirName>:' before the contract name or fully qualified name in the --reference option or @custom:oz-upgrades-from annotation, where <dirName> is the directory short name. Each directory short name must be unique, including compared to the main build info directory. If passing in multiple directories, separate them with commas or call the option multiple times, once for each directory.␊
       --exclude "<GLOB_PATTERN>" [--exclude "<GLOB_PATTERN>"...]  Exclude validations for contracts in source file paths that match any of the given glob patterns. For example, --exclude "contracts/mocks/**/*.sol". Does not apply to reference contracts. If passing in multiple patterns, call the option multiple times, once for each pattern.␊
-      --unsafeAllow "<VALIDATION_ERROR>[,<VALIDATION_ERROR>...]"  Selectively disable one or more validation errors. Comma-separated list with one or more of the following: state-variable-assignment, state-variable-immutable, external-library-linking, struct-definition, enum-definition, constructor, delegatecall, selfdestruct, missing-public-upgradeto, internal-function-storage␊
+      --unsafeAllow "<VALIDATION_ERROR>[,<VALIDATION_ERROR>...]"  Selectively disable one or more validation errors. Comma-separated list with one or more of the following: state-variable-assignment, state-variable-immutable, external-library-linking, struct-definition, enum-definition, constructor, delegatecall, selfdestruct, missing-public-upgradeto, internal-function-storage, missing-initializer, missing-initializer-call, duplicate-initializer-call, incorrect-initializer-order␊
       --unsafeAllowRenames  Configure storage layout check to allow variable renaming.␊
       --unsafeSkipStorageCheck  Skips checking for storage layout compatibility errors. This is a dangerous option meant to be used as a last resort.␊
     `
@@ -43,7 +43,7 @@ Generated by [AVA](https://avajs.dev).
       --requireReference  Can only be used when the --contract option is also provided. Not compatible with --unsafeSkipStorageCheck. If specified, requires either the --reference option to be provided or the contract to have a @custom:oz-upgrades-from annotation.␊
       --referenceBuildInfoDirs "<BUILD_INFO_DIR>[,<BUILD_INFO_DIR>...]"  Optional paths of additional build info directories from previous versions of the project to use for storage layout comparisons. When using this option, refer to one of these directories using prefix '<dirName>:' before the contract name or fully qualified name in the --reference option or @custom:oz-upgrades-from annotation, where <dirName> is the directory short name. Each directory short name must be unique, including compared to the main build info directory. If passing in multiple directories, separate them with commas or call the option multiple times, once for each directory.␊
       --exclude "<GLOB_PATTERN>" [--exclude "<GLOB_PATTERN>"...]  Exclude validations for contracts in source file paths that match any of the given glob patterns. For example, --exclude "contracts/mocks/**/*.sol". Does not apply to reference contracts. If passing in multiple patterns, call the option multiple times, once for each pattern.␊
-      --unsafeAllow "<VALIDATION_ERROR>[,<VALIDATION_ERROR>...]"  Selectively disable one or more validation errors. Comma-separated list with one or more of the following: state-variable-assignment, state-variable-immutable, external-library-linking, struct-definition, enum-definition, constructor, delegatecall, selfdestruct, missing-public-upgradeto, internal-function-storage␊
+      --unsafeAllow "<VALIDATION_ERROR>[,<VALIDATION_ERROR>...]"  Selectively disable one or more validation errors. Comma-separated list with one or more of the following: state-variable-assignment, state-variable-immutable, external-library-linking, struct-definition, enum-definition, constructor, delegatecall, selfdestruct, missing-public-upgradeto, internal-function-storage, missing-initializer, missing-initializer-call, duplicate-initializer-call, incorrect-initializer-order␊
       --unsafeAllowRenames  Configure storage layout check to allow variable renaming.␊
       --unsafeSkipStorageCheck  Skips checking for storage layout compatibility errors. This is a dangerous option meant to be used as a last resort.␊
     `

packages/core/src/validate-initializers.test.ts

@@ -0,0 +1,106 @@
+import _test, { TestFn } from 'ava';
+import { artifacts } from 'hardhat';
+
+import {
+  validate,
+  getContractVersion,
+  assertUpgradeSafe,
+  ValidationOptions,
+  RunValidation,
+  ValidationErrors,
+} from './validate';
+import { solcInputOutputDecoder } from './src-decoder';
+
+interface Context {
+  validation: RunValidation;
+}
+
+const test = _test as TestFn<Context>;
+
+test.before(async t => {
+  const contracts = ['contracts/test/ValidationsInitializer.sol:Parent_NoInitializer'];
+
+  t.context.validation = {} as RunValidation;
+  for (const contract of contracts) {
+    const buildInfo = await artifacts.getBuildInfo(contract);
+    if (buildInfo === undefined) {
+      throw new Error(`Build info not found for contract ${contract}`);
+    }
+    const solcOutput = buildInfo.output;
+    const solcInput = buildInfo.input;
+    const decodeSrc = solcInputOutputDecoder(solcInput, solcOutput);
+    Object.assign(t.context.validation, validate(solcOutput, decodeSrc));
+  }
+});
+
+function testValid(name: string, kind: ValidationOptions['kind'], valid: boolean, numExpectedErrors?: number) {
+  testOverride(name, kind, {}, valid, numExpectedErrors);
+}
+
+function testOverride(
+  name: string,
+  kind: ValidationOptions['kind'],
+  opts: ValidationOptions,
+  valid: boolean,
+  numExpectedErrors?: number,
+) {
+  if (numExpectedErrors !== undefined && numExpectedErrors > 0 && valid) {
+    throw new Error('Cannot expect errors for a valid contract');
+  }
+
+  const optKeys = Object.keys(opts);
+  const describeOpts = optKeys.length > 0 ? '(' + optKeys.join(', ') + ')' : '';
+  const testName = [valid ? 'accepts' : 'rejects', kind, name, describeOpts].join(' ');
+  test(testName, t => {
+    const version = getContractVersion(t.context.validation, name);
+    const assertUpgSafe = () => assertUpgradeSafe([t.context.validation], version, { kind, ...opts });
+    if (valid) {
+      t.notThrows(assertUpgSafe);
+    } else {
+      const error = t.throws(assertUpgSafe) as ValidationErrors;
+      if (numExpectedErrors !== undefined) {
+        t.is(error.errors.length, numExpectedErrors);
+      }
+    }
+  });
+}
+
+testValid('Child_Of_NoInitializer_Ok', 'transparent', true);
+
+testValid('Child_Of_InitializerModifier_Ok', 'transparent', true);
+testValid('Child_Of_InitializerModifier_Bad', 'transparent', false, 1);
+testValid('Child_Of_InitializerModifier_UsesSuper_Ok', 'transparent', true);
+
+testValid('Child_Of_ReinitializerModifier_Ok', 'transparent', true);
+testValid('Child_Of_ReinitializerModifier_Bad', 'transparent', false, 1);
+
+testValid('Child_Of_OnlyInitializingModifier_Ok', 'transparent', true);
+testValid('Child_Of_OnlyInitializingModifier_Bad', 'transparent', false, 1);
+
+testValid('MissingInitializer_Bad', 'transparent', false, 1);
+testValid('MissingInitializer_UnsafeAllow_Contract', 'transparent', true);
+testOverride('MissingInitializer_Bad', 'transparent', { unsafeAllow: ['missing-initializer'] }, true);
+
+testValid('InitializationOrder_Ok', 'transparent', true);
+testValid('InitializationOrder_Ok_2', 'transparent', true);
+
+testValid('InitializationOrder_WrongOrder_Bad', 'transparent', false, 1);
+testValid('InitializationOrder_WrongOrder_UnsafeAllow_Contract', 'transparent', true);
+testValid('InitializationOrder_WrongOrder_UnsafeAllow_Function', 'transparent', true);
+testOverride(
+  'InitializationOrder_WrongOrder_Bad',
+  'transparent',
+  { unsafeAllow: ['incorrect-initializer-order'] },
+  true,
+);
+
+testValid('InitializationOrder_MissingCall_Bad', 'transparent', false, 1);
+testValid('InitializationOrder_MissingCall_UnsafeAllow_Contract', 'transparent', true);
+testValid('InitializationOrder_MissingCall_UnsafeAllow_Function', 'transparent', true);
+testOverride('InitializationOrder_MissingCall_Bad', 'transparent', { unsafeAllow: ['missing-initializer-call'] }, true);
+
+testValid('InitializationOrder_Duplicate_Bad', 'transparent', false, 1);
+testValid('InitializationOrder_Duplicate_UnsafeAllow_Contract', 'transparent', true);
+testValid('InitializationOrder_Duplicate_UnsafeAllow_Function', 'transparent', true);
+testValid('InitializationOrder_Duplicate_UnsafeAllow_Call', 'transparent', true);
+testOverride('InitializationOrder_Duplicate_Bad', 'transparent', { unsafeAllow: ['duplicate-initializer-call'] }, true);

packages/core/src/validate/overrides.ts

@@ -81,6 +81,22 @@ export const ValidationErrorUnsafeMessages: Record<ValidationError['kind'], stri
     `Internal functions are code pointers which will no longer be valid after an upgrade.`,
     `Make sure you reassign internal functions in storage variables during upgrades.`,
   ],
+  'missing-initializer': [
+    `You are using the \`unsafeAllow.missing-initializer\` flag.`,
+    `Make sure you have manually checked that the contract has an initializer and that it correct calls any parent initializers.`,
+  ],
+  'missing-initializer-call': [
+    `You are using the \`unsafeAllow.missing-initializer-call\` flag.`,
+    `Make sure you have manually checked that the contract has an initializer and that it correct calls any parent initializers.`,
+  ],
+  'duplicate-initializer-call': [
+    `You are using the \`unsafeAllow.duplicate-initializer-call\` flag.`,
+    `Make sure you have manually checked that the contract has an initializer and that it correct calls any parent initializers.`,
+  ],
+  'incorrect-initializer-order': [
+    `You are using the \`unsafeAllow.incorrect-initializer-order\` flag.`,
+    `Make sure you have manually checked that the contract has an initializer and that it correct calls any parent initializers.`,
+  ],
 };
 
 export function withValidationDefaults(opts: ValidationOptions): Required<ValidationOptions> {