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/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> {

packages/core/src/validate/report.ts

@@ -72,6 +72,26 @@ const errorInfo: ErrorDescriptions<ValidationError> = {
       `     flag and ensure you always reassign internal functions in storage during upgrades`,
     link: 'https://zpl.in/upgrades/error-009',
   },
+  'missing-initializer': {
+    msg: () => `Contract is missing an initializer`,
+    hint: () => `Define an initializer function`, // TODO include instruction to call parent initializers, or use a separate error message
+    link: 'https://zpl.in/upgrades/error-010', // TODO define link
+  },
+  'missing-initializer-call': {
+    msg: () => `Contract is missing a call to a parent initializer`,
+    hint: () => `Call the parent initializer in your initializer function`,
+    link: 'https://zpl.in/upgrades/error-011', // TODO define link
+  },
+  'duplicate-initializer-call': {
+    msg: () => `Contract has multiple calls to a parent initializer`,
+    hint: () => `Only call each parent initializer once`,
+    link: 'https://zpl.in/upgrades/error-012', // TODO define link
+  },
+  'incorrect-initializer-order': {
+    msg: () => `Contract has an incorrect order of parent initializer calls`,
+    hint: () => `Call parent initializers in the order they are inherited`,
+    link: 'https://zpl.in/upgrades/error-013', // TODO define link
+  },
 };
 
 function describeError(e: ValidationError, color = true): string {