Detect issues in parent initializer calls

docs/modules/ROOT/pages/api-core.adoc

@@ -121,7 +121,7 @@ If any errors are found, the command will exit with a non-zero exit code and pri
 * `--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.
 

docs/modules/ROOT/pages/api-hardhat-upgrades.adoc

@@ -17,6 +17,10 @@ The following options are common to some functions.
 ** `"delegatecall"`, `"selfdestruct"`: Allow the use of these operations. Incorrect use of this option can put funds at risk of permanent loss. See xref:faq.adoc#delegatecall-selfdestruct[Can I safely use `delegatecall` and `selfdestruct`?]
 ** `"missing-public-upgradeto"`: Allow UUPS implementations that do not contain a public `upgradeTo` or `upgradeToAndCall` function. Enabling this option is likely to cause a revert due to the built-in UUPS safety mechanism.
 ** `"internal-function-storage"`: Allow internal functions in storage variables. Internal functions are code pointers which will no longer be valid after an upgrade, so they must be reassigned during upgrades. See xref:faq.adoc#internal-function-storage[How can I use internal functions in storage variables?]
+** `"missing-initializer"`: Allows implementations where an initializer function is not detected.
+** `"missing-initializer-call"`: Allows implementations where a parent initializer is not called from the child initializer.
+** `"duplicate-initializer-call"`: Allows implementations where a parent initializer is called more than once from the child initializer.
+** `"incorrect-initializer-order"`: Allows implementations where parent initializers are not called in the order that they are inherited.
 * `unsafeAllowRenames`: (`boolean`) Configure storage layout check to allow variable renaming.
 * `unsafeSkipStorageCheck`: (`boolean`) upgrades the proxy or beacon without first checking for storage layout compatibility errors. This is a dangerous option meant to be used as a last resort.
 * `constructorArgs`: (`unknown[]`) Provide arguments for the constructor of the implementation contract. Note that these are different from initializer arguments, and will be used in the deployment of the implementation contract itself. Can be used to initialize immutable variables.

packages/core/CHANGELOG.md

@@ -1,5 +1,9 @@
 # Changelog
 
+## Unreleased
+
+- Detect issues in parent initializer calls. ([#1095](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/1095))
+
 ## 1.41.0 (2024-11-25)
 
 - Update Slang dependency to 0.18.3. ([#1102](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/1102))

packages/core/contracts/test/ValidationsInitializer.sol

@@ -0,0 +1,243 @@
+// 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 internal {}
+}
+
+contract Parent_ReinitializerModifier is Initializable {
+  function parentReinit() reinitializer(2) internal {}
+}
+
+contract Parent__OnlyInitializingModifier is Initializable {
+  function __Parent_init() onlyInitializing() internal {}
+}
+
+contract Parent_InitializeName {
+  function initialize() internal virtual {}
+}
+
+contract Parent_InitializerName {
+  function initializer() internal {}
+}
+
+contract Parent_ReinitializeName {
+  function reinitialize(uint64 version) internal {}
+}
+
+contract Parent_ReinitializerName {
+  function reinitializer(uint64 version) internal {}
+}
+
+// ==== 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();
+  }
+}
+
+// ==== Initializer visibility ====
+
+contract Parent_PrivateInitializer {
+  function initialize() private {} // not considered an initializer because it's private
+}
+
+contract Parent_PublicInitializer {
+  function initialize() public {} // does not strictly need to be called by child
+}
+
+contract Child_Of_ParentPrivateInitializer_Ok is Parent_PrivateInitializer { // no initializer required since parent initializer is private
+}
+
+contract Child_Of_ParentPublicInitializer_Ok is Parent_PublicInitializer { // no initializer required since parent initializer is public
+}
+
+contract Child_Has_PrivateInitializer_Bad is Parent__OnlyInitializingModifier { // parent has internal initializer, but child has private
+  function initialize() private {}
+}

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/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.␊
     `