diff --git a/docs/modules/ROOT/pages/api-core.adoc b/docs/modules/ROOT/pages/api-core.adoc index df1e6dc02..fcdaa6b53 100644 --- a/docs/modules/ROOT/pages/api-core.adoc +++ b/docs/modules/ROOT/pages/api-core.adoc @@ -121,7 +121,9 @@ 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 "[,...]"` - 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 `:` before the contract name or fully qualified name in the `--reference` option or `@custom:oz-upgrades-from` annotation, where `` 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 "" [--exclude ""...]` - 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 "[,...]"` - 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 "[,...]"` - Selectively disable one or more validation errors or warnings. Comma-separated list with one or more of the following: +** Errors: `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` +** Warnings: `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. diff --git a/docs/modules/ROOT/pages/api-hardhat-upgrades.adoc b/docs/modules/ROOT/pages/api-hardhat-upgrades.adoc index ff581d73d..34994ee6b 100644 --- a/docs/modules/ROOT/pages/api-hardhat-upgrades.adoc +++ b/docs/modules/ROOT/pages/api-hardhat-upgrades.adoc @@ -8,7 +8,7 @@ Both `deployProxy` and `upgradeProxy` functions will return instances of https:/ The following options are common to some functions. * `kind`: (`"uups" | "transparent" | "beacon"`) The kind of proxy to deploy, upgrade or import, or the kind of proxy that the implementation will be used with. `deployProxy()` and `upgradeProxy()` only support the values `"uups" | "transparent"`. Defaults to `"transparent"`. See xref:contracts:api:proxy.adoc#transparent-vs-uups[Transparent vs UUPS]. -* `unsafeAllow`: (`ValidationError[]`) Selectively disable one or more validation errors: +* `unsafeAllow`: (`ValidationError[]`) Selectively disable one or more validation errors or warnings: ** `"external-library-linking"`: Allows a deployment with external libraries linked to the implementation contract. (External libraries are otherwise xref:faq.adoc#why-cant-i-use-external-libraries[not yet supported].) ** `"struct-definition"`, `"enum-definition"`: Used to be necessary to deploy a contract with structs or enums. No longer necessary. ** `"state-variable-assignment"`: Allows assigning state variables in a contract even though they will be stored in the implementation. @@ -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 linearized order. *Note*: This condition shows a warning by default, and setting this option will silence the warning. * `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. diff --git a/packages/core/CHANGELOG.md b/packages/core/CHANGELOG.md index 4f28e04d5..84fec474d 100644 --- a/packages/core/CHANGELOG.md +++ b/packages/core/CHANGELOG.md @@ -1,8 +1,9 @@ # Changelog -## Unreleased +## 1.42.0 (2025-01-23) - Update dependencies. ([#1096](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/1096)) +- Detect issues in parent initializer calls. ([#1095](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/1095)) ## 1.41.0 (2024-11-25) diff --git a/packages/core/contracts/test/ValidationsInitializer.sol b/packages/core/contracts/test/ValidationsInitializer.sol new file mode 100644 index 000000000..f03ea3a59 --- /dev/null +++ b/packages/core/contracts/test/ValidationsInitializer.sol @@ -0,0 +1,610 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; +import {ERC20Upgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/ERC20Upgradeable.sol"; +import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; +import {Ownable2StepUpgradeable} from "@openzeppelin/contracts-upgradeable/access/Ownable2StepUpgradeable.sol"; + +import {ERC20BurnableUpgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC20BurnableUpgradeable.sol"; +import {ERC20FlashMintUpgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC20FlashMintUpgradeable.sol"; +import {ERC20PausableUpgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC20PausableUpgradeable.sol"; +import {ERC20PermitUpgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC20PermitUpgradeable.sol"; +import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; +import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.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 { + uint8 x; + function parentFn() internal { + x = 1; + } +} + +contract Parent_InitializerModifier is Initializable { + uint8 x; + function parentInit() initializer internal { + x = 1; + } +} + +contract Parent__OnlyInitializingModifier is Initializable { + uint8 x; + function __Parent_init() onlyInitializing() internal { + x = 1; + } +} + +contract Parent_InitializeName { + uint8 x; + function initialize() internal virtual { + x = 1; + } +} + +contract Parent_InitializerName { + uint8 x; + function initializer() internal { + x = 1; + } +} + +// ==== Child contracts ==== + +contract Child_Of_NoInitializer_Ok is Parent_NoInitializer { + uint y; + function childFn() public { + y = 2; + } +} + +contract Child_Of_InitializerModifier_Ok is Parent_InitializerModifier { + uint y; + function initialize() public { + parentInit(); + y = 2; + } +} + +contract Child_Of_InitializerModifier_UsesSuper_Ok is Parent_InitializerModifier { + uint y; + function initialize() public { + super.parentInit(); + y = 2; + } +} + +contract Child_Of_InitializerModifier_Bad is Parent_InitializerModifier { + uint y; + function initialize() public { + y = 2; + } +} + +contract Child_Of_OnlyInitializingModifier_Ok is Parent__OnlyInitializingModifier { + uint y; + function initialize() public { + __Parent_init(); + y = 2; + } +} + +contract Child_Of_OnlyInitializingModifier_Bad is Parent__OnlyInitializingModifier { + uint y; + function initialize() public { + y = 2; + } +} + +// This is considered to have a missing initializer because the `regularFn` function is not inferred as an intializer +contract MissingInitializer_Bad is Parent_InitializerModifier { + uint y; + function regularFn() public { + parentInit(); + y = 2; + } +} + +/// @custom:oz-upgrades-unsafe-allow missing-initializer +contract MissingInitializer_UnsafeAllow_Contract is Parent_InitializerModifier { + uint y; + function regularFn() public { + parentInit(); + y = 2; + } +} + +contract A is Initializable { + uint a; + function __A_init() onlyInitializing internal { + a = 2; + } +} + +contract B is Initializable { + uint b; + function __B_init() onlyInitializing internal { + b = 2; + } +} + +contract C is Initializable { + uint c; + function __C_init() onlyInitializing internal { + c = 2; + } +} + +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(); + } +} + +/// @custom:oz-upgrades-unsafe-allow duplicate-initializer-call +contract InitializationOrder_UnsafeAllowDuplicate_But_WrongOrder is A, B, C, Parent_NoInitializer { + function initialize() public { + __A_init(); + __C_init(); + __B_init(); + __B_init(); + } +} + +// ==== Initializer visibility ==== + +abstract contract Parent_Private is Initializable { + uint a; + function privateInit() initializer private { // not considered an initializer because it's private + a = 1; + } +} + +abstract contract Parent_Public is Initializable { + uint b; + function publicInit() initializer public { // does not strictly need to be called by child + b = 1; + } +} + +abstract contract Parent_External is Initializable { + uint c; + function externalInit() initializer external { // ignored since it cannot be called by child + c = 1; + } +} + +abstract contract Parent_Internal is Initializable { + uint d; + function internalInit() initializer internal { // require + d = 1; + } +} + +contract Child_Of_Private_Ok is Parent_Private { // no initializer required since parent initializer is private +} + +contract Child_Of_Public_Ok is Parent_Public { // no initializer required since parent initializer is public +} + +contract Child_Of_Public_MissingCall_Bad is Parent_Public { + function initialize() initializer public { + // missing call + } +} + +contract Child_Of_External_Ok is Parent_External { // no initializer required since parent initializer is external +} + +contract Child_Of_Internal_Bad is Parent_Internal { +} + +contract Child_Of_Internal_Has_Private_Bad is Parent_Internal { // private function is not considered an initializer + function initialize() initializer private { + internalInit(); + } +} + +contract Child_Of_Internal_Has_Public_Ok is Parent_Internal { + function initialize() initializer public { + internalInit(); + } +} + +contract Child_Of_Internal_Has_Internal_Ok is Parent_Internal { + function initialize() initializer internal { + internalInit(); + } +} + +contract Child_Of_Internal_Has_External_Ok is Parent_Internal { + function initialize() initializer external { + internalInit(); + } +} + +contract Child_Of_PrivatePublicExternal_Ok is Parent_Private, Parent_Public, Parent_External { +} + +contract Child_Of_AllVisibilities_Bad is Parent_Private, Parent_Public, Parent_External, Parent_Internal { // internal causes missing initializer error +} + +contract Child_Of_AllVisibilities_EmptyInitializer_Bad is Parent_Private, Parent_Public, Parent_External, Parent_Internal { // both public and internal parent initializers need to be called + function initialize() initializer public { + } +} + +abstract contract Parent_Public_2 is Initializable { + uint b2; + function public2Init() initializer public { // does not strictly need to be called by child + b2 = 1; + } +} + +contract Child_Of_MultiplePublic_MissingInitializer_Bad is Parent_Public, Parent_Public_2 { // both public parent initializers need to be called +} + +contract Child_Of_MultiplePublic_MissingCall_Bad is Parent_Public, Parent_Public_2 { + function initialize() initializer public { + publicInit(); + // missing call to public2Init + } +} + +contract Child_Of_MultiplePublic_Ok is Parent_Public, Parent_Public_2 { + function initialize() initializer public { + publicInit(); + public2Init(); + } +} + +// ==== Transitive initialization ==== + +abstract contract TransitiveGrandparent1 is Initializable { + uint x; + function __TransitiveGrandparent1_init(uint a) onlyInitializing internal { + x = a; + } +} + +abstract contract TransitiveGrandparent2 is Initializable { + uint y; + function __TransitiveGrandparent2_init(uint b) onlyInitializing internal { + y = b; + } +} + +contract TransitiveParent_Ok is TransitiveGrandparent1, TransitiveGrandparent2 { + function initializeParent() initializer public { + __TransitiveGrandparent1_init(1); + __TransitiveGrandparent2_init(2); + } +} + +contract TransitiveParent_Bad is TransitiveGrandparent1, TransitiveGrandparent2 { + function initializeParent() initializer public { + __TransitiveGrandparent1_init(1); + // Does not call __TransitiveGrandparent2_init, and this contract is not abstract, so it is required + } +} + +contract TransitiveChild_Bad_Parent is TransitiveParent_Bad { // this contract is ok but the parent is not + function initialize() initializer public { + initializeParent(); + } +} + +contract TransitiveChild_Bad_Order is TransitiveParent_Bad { + function initialize() initializer public { + initializeParent(); + __TransitiveGrandparent2_init(2); // grandparent should be initialized first + } +} + +contract TransitiveChild_Good_Order_Bad_Parent is TransitiveParent_Bad { // this contract is ok but the parent is not + function initialize() initializer public { + __TransitiveGrandparent2_init(2); + initializeParent(); + } +} + +contract TransitiveDuplicate_Bad is TransitiveGrandparent1, TransitiveParent_Ok { + function initialize() initializer public { + __TransitiveGrandparent1_init(1000); // duplicate + initializeParent(); + } +} + +contract Ownable_Ok is Initializable, ERC20Upgradeable, OwnableUpgradeable { + /// @custom:oz-upgrades-unsafe-allow constructor + constructor() { + _disableInitializers(); + } + + function initialize(address initialOwner) initializer public { + __ERC20_init("MyToken", "MTK"); + __Ownable_init(initialOwner); + } +} + +contract Ownable2Step_Ok is Initializable, ERC20Upgradeable, Ownable2StepUpgradeable { + /// @custom:oz-upgrades-unsafe-allow constructor + constructor() { + _disableInitializers(); + } + + function initialize(address initialOwner) initializer public { + __ERC20_init("MyToken", "MTK"); + __Ownable_init(initialOwner); // Transitive dependency that needs to be initialized + __Ownable2Step_init(); + } +} + +contract Ownable2Step_Bad is Initializable, ERC20Upgradeable, Ownable2StepUpgradeable { + /// @custom:oz-upgrades-unsafe-allow constructor + constructor() { + _disableInitializers(); + } + + function initialize() initializer public { + __ERC20_init("MyToken", "MTK"); + // Missing Ownable, which is a transitive dependency that needs to be initialized + __Ownable2Step_init(); + } +} + +// ==== Detection of duplicates from different contracts ==== + +abstract contract Grandparent is Initializable { + uint64 x; + function __Grandparent_init() onlyInitializing internal { + x = 1; + } +} + +abstract contract Parent is Grandparent { + function __Parent_init() onlyInitializing internal { + __Grandparent_init(); + } +} + +contract SkipsParent_Bad is Parent { + function initialize() initializer public { + // missing call to __Parent_init + __Grandparent_init(); // should not be duplicate since __Parent_init was not called + } +} + +contract DuplicateInHelpers_Bad is Grandparent { + function initialize() initializer public { + helper1(); + helper2(); + } + + function helper1() initializer private { + __Grandparent_init(); + } + + function helper2() initializer private { + __Grandparent_init(); + } +} + +contract Recursive_Bad is Parent { + function initialize() initializer public { + recurse(); + } + + function recurse() private { + // missing call to __Parent_init + initialize(); // recursion should be ignored for validations + } +} + +contract Recursive_Ok is Parent { + function initialize() initializer public { + recurse(); + } + + function recurse() private { + __Parent_init(); + initialize(); // recursion should be ignored for validations + } +} + +// ==== Complex ERC20 examples ==== + +contract ERC20_Ok is Initializable, ERC20Upgradeable, ERC20BurnableUpgradeable, ERC20PausableUpgradeable, OwnableUpgradeable, ERC20PermitUpgradeable, ERC20FlashMintUpgradeable, UUPSUpgradeable { + /// @custom:oz-upgrades-unsafe-allow constructor + constructor() { + _disableInitializers(); + } + + function initialize(address initialOwner) initializer public { + __ERC20_init("MyToken", "MTK"); + __ERC20Burnable_init(); + __ERC20Pausable_init(); + __Ownable_init(initialOwner); + __ERC20Permit_init("MyToken"); + __ERC20FlashMint_init(); + __UUPSUpgradeable_init(); + } + + function pause() public onlyOwner { + _pause(); + } + + function unpause() public onlyOwner { + _unpause(); + } + + function mint(address to, uint256 amount) public onlyOwner { + _mint(to, amount); + } + + function _authorizeUpgrade(address newImplementation) + internal + onlyOwner + override + {} + + // The following functions are overrides required by Solidity. + + function _update(address from, address to, uint256 value) + internal + override(ERC20Upgradeable, ERC20PausableUpgradeable) + { + super._update(from, to, value); + } +} + + +contract ERC20_Bad is Initializable, ERC20Upgradeable, ERC20BurnableUpgradeable, ERC20PausableUpgradeable, OwnableUpgradeable, ERC20PermitUpgradeable, ERC20FlashMintUpgradeable, UUPSUpgradeable { + /// @custom:oz-upgrades-unsafe-allow constructor + constructor() { + _disableInitializers(); + } + + function initialize(address initialOwner) initializer public { + // missing calls + } + + function pause() public onlyOwner { + _pause(); + } + + function unpause() public onlyOwner { + _unpause(); + } + + function mint(address to, uint256 amount) public onlyOwner { + _mint(to, amount); + } + + function _authorizeUpgrade(address newImplementation) + internal + onlyOwner + override + {} + + // The following functions are overrides required by Solidity. + + function _update(address from, address to, uint256 value) + internal + override(ERC20Upgradeable, ERC20PausableUpgradeable) + { + super._update(from, to, value); + } +} diff --git a/packages/core/contracts/test/cli/excludes/AbstractUUPS.sol b/packages/core/contracts/test/cli/excludes/AbstractUUPS.sol index 85005b80b..8e4a3e554 100644 --- a/packages/core/contracts/test/cli/excludes/AbstractUUPS.sol +++ b/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(); + } } \ No newline at end of file diff --git a/packages/core/package.json b/packages/core/package.json index e244f348b..33c98c387 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -1,6 +1,6 @@ { "name": "@openzeppelin/upgrades-core", - "version": "1.41.0", + "version": "1.42.0", "description": "", "repository": "https://github.com/OpenZeppelin/openzeppelin-upgrades/tree/master/packages/core", "license": "MIT", diff --git a/packages/core/src/cli/cli.test.ts.md b/packages/core/src/cli/cli.test.ts.md index bcc365e62..50755618d 100644 --- a/packages/core/src/cli/cli.test.ts.md +++ b/packages/core/src/cli/cli.test.ts.md @@ -21,7 +21,9 @@ 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 "[,...]" 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 ':' before the contract name or fully qualified name in the --reference option or @custom:oz-upgrades-from annotation, where 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 "" [--exclude ""...] 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 "[,...]" 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 "[,...]" Selectively disable one or more validation errors or warnings. Comma-separated list with one or more of the following:␊ + Errors: 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␊ + Warnings: 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 +45,9 @@ 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 "[,...]" 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 ':' before the contract name or fully qualified name in the --reference option or @custom:oz-upgrades-from annotation, where 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 "" [--exclude ""...] 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 "[,...]" 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 "[,...]" Selectively disable one or more validation errors or warnings. Comma-separated list with one or more of the following:␊ + Errors: 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␊ + Warnings: 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.␊ ` diff --git a/packages/core/src/cli/cli.test.ts.snap b/packages/core/src/cli/cli.test.ts.snap index c8b362076..0692b6855 100644 Binary files a/packages/core/src/cli/cli.test.ts.snap and b/packages/core/src/cli/cli.test.ts.snap differ diff --git a/packages/core/src/cli/validate.ts b/packages/core/src/cli/validate.ts index 8d7032e53..c309a2e01 100644 --- a/packages/core/src/cli/validate.ts +++ b/packages/core/src/cli/validate.ts @@ -1,7 +1,7 @@ import minimist from 'minimist'; import { ValidateUpgradeSafetyOptions, validateUpgradeSafety } from '.'; -import { ValidationError, errorKinds } from '../validate/run'; +import { ValidationError, convertToWarnings, errorKinds } from '../validate/run'; import debug from '../utils/debug'; import { withCliDefaults } from './validate/validate-upgrade-safety'; @@ -18,9 +18,9 @@ Options: --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 "[,...]" 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 ':' before the contract name or fully qualified name in the --reference option or @custom:oz-upgrades-from annotation, where 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 "" [--exclude ""...] 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 "[,...]" Selectively disable one or more validation errors. Comma-separated list with one or more of the following: ${errorKinds.join( - ', ', - )} + --unsafeAllow "[,...]" Selectively disable one or more validation errors or warnings. Comma-separated list with one or more of the following: + Errors: ${errorKinds.filter(kind => !convertToWarnings.includes(kind)).join(', ')} + Warnings: ${convertToWarnings.join(', ')} --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.`; diff --git a/packages/core/src/validate-initializers.test.ts b/packages/core/src/validate-initializers.test.ts new file mode 100644 index 000000000..c0f8b4be5 --- /dev/null +++ b/packages/core/src/validate-initializers.test.ts @@ -0,0 +1,218 @@ +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; + +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 testAccepts(name: string, kind: ValidationOptions['kind']) { + testOverride(name, kind, {}, undefined); +} + +interface ExpectedErrors { + contains: string[]; + count: number; +} + +function testRejects(name: string, kind: ValidationOptions['kind'], expectedErrors?: ExpectedErrors) { + testOverride(name, kind, {}, expectedErrors); +} + +function testOverride( + name: string, + kind: ValidationOptions['kind'], + opts: ValidationOptions, + expectedErrors?: ExpectedErrors, +) { + const expectValid = expectedErrors === undefined; + + const optKeys = Object.keys(opts); + const describeOpts = optKeys.length > 0 ? '(' + optKeys.join(', ') + ')' : ''; + const testName = [expectValid ? '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 (expectValid) { + t.notThrows(assertUpgSafe); + } else { + const error = t.throws(assertUpgSafe) as ValidationErrors; + t.true( + error.errors.length === expectedErrors.count, + `Expected ${expectedErrors.count} errors, got ${error.errors.length}:\n${error.message}`, + ); + t.true( + expectedErrors?.contains.every(c => error.message.includes(c)), + error.message, + ); + } + }); +} + +testAccepts('Child_Of_NoInitializer_Ok', 'transparent'); + +testAccepts('Child_Of_InitializerModifier_Ok', 'transparent'); +testRejects('Child_Of_InitializerModifier_Bad', 'transparent', { + contains: ['Missing initializer calls for one or more parent contracts: `Parent_InitializerModifier`'], + count: 1, +}); +testAccepts('Child_Of_InitializerModifier_UsesSuper_Ok', 'transparent'); + +testAccepts('Child_Of_OnlyInitializingModifier_Ok', 'transparent'); +testRejects('Child_Of_OnlyInitializingModifier_Bad', 'transparent', { + contains: ['Missing initializer calls for one or more parent contracts: `Parent__OnlyInitializingModifier`'], + count: 1, +}); + +testRejects('MissingInitializer_Bad', 'transparent', { + contains: ['Missing initializer'], + count: 1, +}); +testAccepts('MissingInitializer_UnsafeAllow_Contract', 'transparent'); +testOverride('MissingInitializer_Bad', 'transparent', { unsafeAllow: ['missing-initializer'] }); + +testAccepts('InitializationOrder_Ok', 'transparent'); +testAccepts('InitializationOrder_Ok_2', 'transparent'); + +testAccepts('InitializationOrder_WrongOrder_Bad', 'transparent'); // warn 'Expected initializers to be called for parent contracts in the following order: A, B, C' +testAccepts('InitializationOrder_WrongOrder_UnsafeAllow_Contract', 'transparent'); +testAccepts('InitializationOrder_WrongOrder_UnsafeAllow_Function', 'transparent'); +testOverride('InitializationOrder_WrongOrder_Bad', 'transparent', { unsafeAllow: ['incorrect-initializer-order'] }); // skips the warning + +testRejects('InitializationOrder_MissingCall_Bad', 'transparent', { + contains: ['Missing initializer calls for one or more parent contracts: `C`'], + count: 1, +}); +testAccepts('InitializationOrder_MissingCall_UnsafeAllow_Contract', 'transparent'); +testAccepts('InitializationOrder_MissingCall_UnsafeAllow_Function', 'transparent'); +testOverride('InitializationOrder_MissingCall_Bad', 'transparent', { unsafeAllow: ['missing-initializer-call'] }); + +testRejects('InitializationOrder_Duplicate_Bad', 'transparent', { + contains: ['Duplicate calls found to initializer `__B_init` for contract `B`'], + count: 1, +}); +testAccepts('InitializationOrder_Duplicate_UnsafeAllow_Contract', 'transparent'); +testAccepts('InitializationOrder_Duplicate_UnsafeAllow_Function', 'transparent'); +testAccepts('InitializationOrder_Duplicate_UnsafeAllow_Call', 'transparent'); +testOverride('InitializationOrder_Duplicate_Bad', 'transparent', { unsafeAllow: ['duplicate-initializer-call'] }); +testAccepts('InitializationOrder_UnsafeAllowDuplicate_But_WrongOrder', 'transparent'); // warn 'Expected initializers to be called for parent contracts in the following order: A, B, C' + +testAccepts('Child_Of_Private_Ok', 'transparent'); +testAccepts('Child_Of_Public_Ok', 'transparent'); +testRejects('Child_Of_Public_MissingCall_Bad', 'transparent', { + contains: ['Missing initializer calls for one or more parent contracts: `Parent_Public`'], + count: 1, +}); +testAccepts('Child_Of_External_Ok', 'transparent'); +testRejects('Child_Of_Internal_Bad', 'transparent', { + contains: ['Missing initializer'], + count: 1, +}); +testRejects('Child_Of_Internal_Has_Private_Bad', 'transparent', { + contains: ['Missing initializer'], + count: 1, +}); +testAccepts('Child_Of_Internal_Has_Public_Ok', 'transparent'); +testAccepts('Child_Of_Internal_Has_Internal_Ok', 'transparent'); +testAccepts('Child_Of_Internal_Has_External_Ok', 'transparent'); +testAccepts('Child_Of_PrivatePublicExternal_Ok', 'transparent'); +testRejects('Child_Of_AllVisibilities_Bad', 'transparent', { + contains: ['Missing initializer'], + count: 1, +}); +testRejects('Child_Of_AllVisibilities_EmptyInitializer_Bad', 'transparent', { + contains: ['Missing initializer calls for one or more parent contracts: `Parent_Public, Parent_Internal`'], + count: 1, +}); + +testRejects('Child_Of_MultiplePublic_MissingInitializer_Bad', 'transparent', { + contains: ['Missing initializer'], + count: 1, +}); +testRejects('Child_Of_MultiplePublic_MissingCall_Bad', 'transparent', { + contains: ['Missing initializer calls for one or more parent contracts: `Parent_Public_2`'], + count: 1, +}); +testAccepts('Child_Of_MultiplePublic_Ok', 'transparent'); + +testAccepts('TransitiveParent_Ok', 'transparent'); +testRejects('TransitiveParent_Bad', 'transparent', { + contains: ['Missing initializer calls for one or more parent contracts: `TransitiveGrandparent2`'], + count: 1, +}); +testRejects('TransitiveChild_Bad_Parent', 'transparent', { + contains: [ + 'Missing initializer calls for one or more parent contracts: `TransitiveGrandparent2`', // occurs twice: missing initializer for child, missing initializer for parent + ], + count: 2, +}); // warn 'Expected initializers to be called for parent contracts in the following order: TransitiveGrandparent2, TransitiveParent_Bad' +testRejects('TransitiveChild_Bad_Order', 'transparent', { + contains: ['Missing initializer calls for one or more parent contracts: `TransitiveGrandparent2`'], + count: 1, +}); // warn 'Expected initializers to be called for parent contracts in the following order: TransitiveGrandparent2, TransitiveParent_Bad' +testRejects('TransitiveChild_Good_Order_Bad_Parent', 'transparent', { + contains: ['Missing initializer calls for one or more parent contracts: `TransitiveGrandparent2`'], + count: 1, +}); +testRejects('TransitiveDuplicate_Bad', 'transparent', { + contains: [ + 'Duplicate calls found to initializer `__TransitiveGrandparent1_init` for contract `TransitiveGrandparent1`', + ], + count: 1, +}); + +testAccepts('Ownable_Ok', 'transparent'); +testAccepts('Ownable2Step_Ok', 'transparent'); +testRejects('Ownable2Step_Bad', 'transparent', { + contains: ['Missing initializer calls for one or more parent contracts: `OwnableUpgradeable`'], + count: 1, +}); + +testRejects('SkipsParent_Bad', 'transparent', { + contains: ['Missing initializer calls for one or more parent contracts: `Parent`'], + count: 1, +}); +testRejects('DuplicateInHelpers_Bad', 'transparent', { + contains: ['Duplicate calls found to initializer `__Grandparent_init` for contract `Grandparent`'], + count: 1, +}); +testRejects('Recursive_Bad', 'transparent', { + contains: ['Missing initializer calls for one or more parent contracts: `Parent`'], + count: 1, +}); +testAccepts('Recursive_Ok', 'transparent'); + +testAccepts('ERC20_Ok', 'uups'); +testRejects('ERC20_Bad', 'uups', { + contains: [ + 'Missing initializer calls for one or more parent contracts: `ERC20Upgradeable, ERC20PausableUpgradeable, OwnableUpgradeable, ERC20PermitUpgradeable`', + ], + count: 1, +}); diff --git a/packages/core/src/validate/overrides.ts b/packages/core/src/validate/overrides.ts index 3c0d1a508..1e3986de3 100644 --- a/packages/core/src/validate/overrides.ts +++ b/packages/core/src/validate/overrides.ts @@ -1,6 +1,7 @@ -import { ValidationError } from './run'; +import { ValidationError, convertToWarnings } from './run'; import { ProxyDeployment } from '../manifest'; import { logWarning } from '../utils/log'; +import { describeError } from './report'; // Backwards compatibility export { silenceWarnings } from '../utils/log'; @@ -30,7 +31,7 @@ export interface StandaloneValidationOptions extends ProxyKindOption { unsafeAllowLinkedLibraries?: boolean; /** - * Selectively disable one or more validation errors. + * Selectively disable one or more validation errors or warnings. */ unsafeAllow?: ValidationError['kind'][]; } @@ -50,7 +51,11 @@ export interface ValidationOptions extends StandaloneValidationOptions { unsafeSkipStorageCheck?: boolean; } -export const ValidationErrorUnsafeMessages: Record = { +/** + * Warnings to display when a validation error occurs but is allowed. + * `null` indicates that the original message should be displayed. + */ +export const ValidationErrorUnsafeMessages: Record = { 'state-variable-assignment': [ `You are using the \`unsafeAllow.state-variable-assignment\` flag.`, `The value will be stored in the implementation and not the proxy.`, @@ -81,6 +86,19 @@ export const ValidationErrorUnsafeMessages: Record { @@ -97,6 +115,7 @@ export function withValidationDefaults(opts: ValidationOptions): Required error.kind !== 'missing-public-upgradeto'); } - for (const [errorType, errorDescription] of Object.entries(ValidationErrorUnsafeMessages)) { - if (unsafeAllow.includes(errorType as ValidationError['kind'])) { - let exceptionsFound = false; - - errors = errors.filter(error => { - const isException = errorType === error.kind; - exceptionsFound = exceptionsFound || isException; - return !isException; - }); - - if (exceptionsFound && errorDescription) { - logWarning(`Potentially unsafe deployment of ${contractName}`, errorDescription); + for (const [key, errorDescription] of Object.entries(ValidationErrorUnsafeMessages)) { + const errorType = key as ValidationError['kind']; + const skip = unsafeAllow.includes(errorType); + const warn = convertToWarnings.includes(errorType); + + if (skip || warn) { + const errorsWithType = errors.filter(error => error.kind === errorType); + errors = errors.filter(error => !errorsWithType.includes(error)); + + // Display message about the exception, unless it is a warning that the user has chosen to skip + if (errorsWithType.length > 0 && !(skip && warn)) { + if (errorDescription !== null) { + logWarning(`Potentially unsafe deployment of ${contractName}`, errorDescription); + } else { + for (const error of errorsWithType) { + logWarning(`Potentially unsafe deployment of ${contractName}`, [describeError(error)]); + } + } } } } diff --git a/packages/core/src/validate/report.ts b/packages/core/src/validate/report.ts index 44b42cf9a..ebbda3401 100644 --- a/packages/core/src/validate/report.ts +++ b/packages/core/src/validate/report.ts @@ -72,11 +72,33 @@ const errorInfo: ErrorDescriptions = { ` flag and ensure you always reassign internal functions in storage during upgrades`, link: 'https://zpl.in/upgrades/error-009', }, + 'missing-initializer': { + msg: () => `Missing initializer`, + hint: () => `Define an initializer function and use it to call the initializers of parent contracts`, + link: 'https://zpl.in/upgrades/error-001', + }, + 'missing-initializer-call': { + msg: e => `Missing initializer calls for one or more parent contracts: \`${e.parentContracts.join(', ')}\``, + hint: () => `Call the parent initializers in your initializer function`, + link: 'https://zpl.in/upgrades/error-001', + }, + 'duplicate-initializer-call': { + msg: e => `Duplicate calls found to initializer \`${e.parentInitializer}\` for contract \`${e.parentContract}\``, + hint: () => `Only call each parent initializer once`, + link: 'https://zpl.in/upgrades/error-001', + }, + 'incorrect-initializer-order': { + msg: e => + `Incorrect order of parent initializer calls. +- Found initializer calls to parent contracts in the following order: ${e.foundOrder.join(', ')} +- Expected: ${e.expectedLinearization.join(', ')}`, + hint: () => `Call parent initializers in linearized order`, + }, }; -function describeError(e: ValidationError, color = true): string { +export function describeError(e: ValidationError, color = true): string { const chalk = new _chalk.Instance({ level: color && _chalk.supportsColor ? _chalk.supportsColor.level : 0 }); - const info = errorInfo[e.kind]; + const info: any = errorInfo[e.kind]; // union type is too complex for TypeScript to represent, so we use `any` const log = [chalk.bold(e.src) + ': ' + info.msg(e as any)]; const hint = info.hint?.(e as any); if (hint) { diff --git a/packages/core/src/validate/run.ts b/packages/core/src/validate/run.ts index bc6ff266e..a567476da 100644 --- a/packages/core/src/validate/run.ts +++ b/packages/core/src/validate/run.ts @@ -24,6 +24,7 @@ import { getStorageLocationAnnotation, isNamespaceSupported } from '../storage/n import { UpgradesError } from '../error'; import { assertUnreachable } from '../utils/assert'; import { logWarning } from '../utils/log'; +import { getInitializerExceptions } from './run/initializer'; export type ValidationRunData = Record; @@ -50,13 +51,20 @@ export const errorKinds = [ 'selfdestruct', 'missing-public-upgradeto', 'internal-function-storage', + 'missing-initializer', + 'missing-initializer-call', + 'duplicate-initializer-call', + 'incorrect-initializer-order', ] as const; +export const convertToWarnings: (typeof errorKinds)[number][] = ['incorrect-initializer-order'] as const; + export type ValidationError = | ValidationErrorConstructor | ValidationErrorOpcode | ValidationErrorWithName - | ValidationErrorUpgradeability; + | ValidationErrorUpgradeability + | ValidationExceptionInitializer; interface ValidationErrorBase { src: string; @@ -74,6 +82,33 @@ interface ValidationErrorWithName extends ValidationErrorBase { | 'internal-function-storage'; } +interface ValidationErrorMissingInitializer extends ValidationErrorBase { + kind: 'missing-initializer'; +} + +interface ValidationErrorMissingInitializerCall extends ValidationErrorBase { + kind: 'missing-initializer-call'; + parentContracts: string[]; +} + +interface ValidationErrorDuplicateInitializerCall extends ValidationErrorBase { + kind: 'duplicate-initializer-call'; + parentInitializer: string; + parentContract: string; +} + +interface ValidationWarningIncorrectInitializerOrder extends ValidationErrorBase { + kind: 'incorrect-initializer-order'; + expectedLinearization: string[]; + foundOrder: string[]; +} + +export type ValidationExceptionInitializer = + | ValidationErrorMissingInitializer + | ValidationErrorMissingInitializerCall + | ValidationErrorDuplicateInitializerCall + | ValidationWarningIncorrectInitializerOrder; + interface ValidationErrorConstructor extends ValidationErrorBase { kind: 'constructor'; contract: string; @@ -125,7 +160,7 @@ function skipCheckReachable(error: string, node: Node): boolean { return getAllowed(node, true).includes(error); } -function skipCheck(error: string, node: Node): boolean { +export function skipCheck(error: ValidationError['kind'], node: Node): boolean { // skip both allow and allow-reachable errors in the lexical scope return getAllowed(node, false).includes(error) || getAllowed(node, true).includes(error); } @@ -209,6 +244,7 @@ export function validate( // https://github.com/OpenZeppelin/openzeppelin-upgrades/issues/52 ...getLinkingErrors(contractDef, bytecode), ...getInternalFunctionStorageErrors(contractDef, deref, decodeSrc), + ...getInitializerExceptions(contractDef, deref, decodeSrc), ]; validation[key].layout = extractStorageLayout( diff --git a/packages/core/src/validate/run/initializer.ts b/packages/core/src/validate/run/initializer.ts new file mode 100644 index 000000000..213de2ef3 --- /dev/null +++ b/packages/core/src/validate/run/initializer.ts @@ -0,0 +1,316 @@ +import type { ContractDefinition, FunctionDefinition } from 'solidity-ast'; +import { ASTDereferencer, findAll } from 'solidity-ast/utils'; +import { SrcDecoder } from '../../src-decoder'; +import { ValidationExceptionInitializer, skipCheck } from '../run'; + +/** + * Reports if this contract is non-abstract and any of the following are true: + * - 1. Missing initializer: This contract does not appear to have an initializer, but parent contracts require initialization. + * - 2. Missing initializer call: This contract's initializer is missing a call to a parent initializer. + * - 3. Duplicate initializer call: This contract has duplicate calls to the same parent initializer function. + * - 4. Incorrect initializer order (warning): This contract does not call parent initializers in the correct order. + */ +export function* getInitializerExceptions( + contractDef: ContractDefinition, + deref: ASTDereferencer, + decodeSrc: SrcDecoder, +): Generator { + if (contractDef.abstract) { + return; + } + + const linearizedParentContracts = getLinearizedParentContracts(contractDef, deref); + const parentNameToInitializersMap = getParentNameToInitializersMap(linearizedParentContracts); + + const remainingParents = getParentsNotInitializedByOtherParents(parentNameToInitializersMap); + + if (remainingParents.length > 0) { + const contractInitializers = getPossibleInitializers(contractDef, false); + + // Report if there is no initializer but parents need initialization + if ( + checkNeedsInitialization(remainingParents, parentNameToInitializersMap) && + contractInitializers.length === 0 && + !skipCheck('missing-initializer', contractDef) + ) { + yield { + kind: 'missing-initializer', + src: decodeSrc(contractDef), + }; + } + + // Linearized list of parent contracts that have initializers + const expectedLinearized: string[] = [...parentNameToInitializersMap.keys()]; + + // The parent contracts that need to be directly initialized by this contract, which excludes parents that are initialized by other parents. + // Calling these will initialize all parent contracts so that the entire state is initialized in one transaction. + const expectedDirectCalls = remainingParents; + + for (const contractInitializer of contractInitializers) { + yield* getInitializerCallExceptions( + contractInitializer, + expectedLinearized, + expectedDirectCalls, + parentNameToInitializersMap, + contractDef, + deref, + decodeSrc, + ); + } + } +} + +function getLinearizedParentContracts(contractDef: ContractDefinition, deref: ASTDereferencer) { + const parents = contractDef.linearizedBaseContracts.map(base => deref('ContractDefinition', base)); + parents.reverse(); // use most derived first + parents.pop(); // remove self + return parents; +} + +/** + * Gets a map of parent contract names to their possible initializers. + * If a parent contract has no initializers, it is not included in the map. + */ +function getParentNameToInitializersMap(linearizedParentContracts: ContractDefinition[]) { + const map = new Map(); + for (const parent of linearizedParentContracts) { + const initializers = getPossibleInitializers(parent, true); + if (initializers.length > 0) { + map.set(parent.name, initializers); + } + } + return map; +} + +/** + * Returns true if this contract must have its own initializer to call parent initializers. + * + * If there are multiple parents with initializers, regardless of whether they are internal or public, + * this contract must have its own initializer to call them so that the state is initialized in one transaction. + * + * Otherwise, if there is only one parent with initializers, they only need to be called if they are internal, since public initializers can be called directly. + */ + +function checkNeedsInitialization( + remainingParents: string[], + parentNameToInitializersMap: Map, +) { + if (remainingParents.length > 1) { + return true; + } + const [parent] = remainingParents; + const parentInitializers = parentNameToInitializersMap.get(parent)!; + return parentInitializers.every(init => init.visibility === 'internal'); +} + +/** + * Returns parent contract names that are not initialized by other parent contracts, + * keeping the same order as they appear in the original linearization. + */ +function getParentsNotInitializedByOtherParents( + parentNameToInitializersMap: Map, +): string[] { + const remainingParents = [...parentNameToInitializersMap.keys()]; + + for (const parent of remainingParents) { + const parentInitializers = parentNameToInitializersMap.get(parent)!; + for (const initializer of parentInitializers) { + const expressionStatements = + initializer.body?.statements?.filter(stmt => stmt.nodeType === 'ExpressionStatement') ?? []; + for (const stmt of expressionStatements) { + const fnCall = stmt.expression; + if ( + fnCall.nodeType === 'FunctionCall' && + (fnCall.expression.nodeType === 'Identifier' || fnCall.expression.nodeType === 'MemberAccess') + ) { + const referencedFn = fnCall.expression.referencedDeclaration; + if (referencedFn) { + const earlierParents = remainingParents.slice(0, remainingParents.indexOf(parent)); + const callsEarlierParentInitializer = earlierParents.find(base => + parentNameToInitializersMap.get(base)!.some(init => init.id === referencedFn), + ); + if (callsEarlierParentInitializer) { + remainingParents.splice(remainingParents.indexOf(callsEarlierParentInitializer), 1); + } + } + } + } + } + } + + return remainingParents; +} + +/** + * Reports exceptions for missing initializer calls, duplicate initializer calls, and incorrect initializer order. + * + * @param contractInitializer An initializer function for the current contract + * @param expectedLinearized Linearized list of parent contracts that have initializers + * @param expectedDirectCalls The parent contracts that need to be directly initialized by this contract + * @param parentNameToInitializersMap Map of parent contract names to their possible initializers + * @param initializersCalledByParents List of parent initializers that have already been called by other parents + * @param contractDef The current contract + * @param deref AST dereferencer + * @param decodeSrc Source decoder + */ +function* getInitializerCallExceptions( + contractInitializer: FunctionDefinition, + expectedLinearized: string[], + expectedDirectCalls: string[], + parentNameToInitializersMap: Map, + contractDef: ContractDefinition, + deref: ASTDereferencer, + decodeSrc: SrcDecoder, +): Generator { + const foundParents: string[] = []; + const remainingLinearized: string[] = [...expectedLinearized]; + const remainingDirectCalls: string[] = [...expectedDirectCalls]; + const calledInitializerIds: number[] = []; + + const expressionStatements = + contractInitializer.body?.statements?.filter(stmt => stmt.nodeType === 'ExpressionStatement') ?? []; + for (const stmt of expressionStatements) { + const fnCall = stmt.expression; + if ( + fnCall.nodeType === 'FunctionCall' && + (fnCall.expression.nodeType === 'Identifier' || fnCall.expression.nodeType === 'MemberAccess') + ) { + let recursiveFunctionIds: number[] = []; + const referencedFn = fnCall.expression.referencedDeclaration; + if (referencedFn) { + recursiveFunctionIds = getRecursiveFunctionIds(referencedFn, deref); + } + + // For each recursively called function, if it is a parent initializer, then: + // - Check if it was already called (duplicate call) + // - Otherwise, check if the parent initializer is called in linearized order + for (const calledFn of recursiveFunctionIds) { + for (const parent of parentNameToInitializersMap.keys()) { + const parentInitializers = parentNameToInitializersMap.get(parent)!; + const callsParentInitializer = parentInitializers.find(init => init.id === calledFn); + + if (calledFn && callsParentInitializer) { + const duplicate = calledInitializerIds.includes(calledFn); + if ( + duplicate && + !skipCheck('duplicate-initializer-call', contractDef) && + !skipCheck('duplicate-initializer-call', contractInitializer) && + !skipCheck('duplicate-initializer-call', stmt) + ) { + yield { + kind: 'duplicate-initializer-call', + src: decodeSrc(fnCall), + parentInitializer: callsParentInitializer.name, + parentContract: parent, + }; + } + calledInitializerIds.push(calledFn); + + // Omit multiple initializer calls of the same parent via different functions e.g. `__X_init` which calls `__X_init_unchained` + if (!foundParents.includes(parent)) { + foundParents.push(parent); + + const indexLinearized = remainingLinearized.indexOf(parent); + if ( + // Omit duplicate calls to avoid treating them as out of order. Duplicates are either reported above or they were skipped. + !duplicate && + // If the parent is not the next expected linearized parent, report it as out of order + indexLinearized !== 0 && + !skipCheck('incorrect-initializer-order', contractDef) && + !skipCheck('incorrect-initializer-order', contractInitializer) + ) { + yield { + kind: 'incorrect-initializer-order', + src: decodeSrc(fnCall), + expectedLinearization: expectedDirectCalls, + foundOrder: foundParents, + }; + } + if (indexLinearized !== -1) { + remainingLinearized.splice(indexLinearized, 1); + } + + const indexDirect = remainingDirectCalls.indexOf(parent); + if (indexDirect !== -1) { + remainingDirectCalls.splice(indexDirect, 1); + } + } + } + } + } + } + } + + // Report any remaining parents that were not directly initialized + if ( + remainingDirectCalls.length > 0 && + !skipCheck('missing-initializer-call', contractDef) && + !skipCheck('missing-initializer-call', contractInitializer) + ) { + yield { + kind: 'missing-initializer-call', + src: decodeSrc(contractInitializer), + parentContracts: remainingDirectCalls, + }; + } +} + +/** + * Gets the IDs of all functions that are recursively called by the given function, including the given function itself at the end of the list. + * + * @param referencedFn The ID of the function to start from + * @param deref AST dereferencer + * @param visited Set of function IDs that have already been visited + * @returns The IDs of all functions that are recursively called by the given function, including the given function itself at the end of the list. + */ +function getRecursiveFunctionIds(referencedFn: number, deref: ASTDereferencer, visited?: Set): number[] { + const result: number[] = []; + + if (visited === undefined) { + visited = new Set(); + } + if (visited.has(referencedFn)) { + return result; + } else { + visited.add(referencedFn); + } + + const fn = deref('FunctionDefinition', referencedFn); + const expressionStatements = fn.body?.statements?.filter(stmt => stmt.nodeType === 'ExpressionStatement') ?? []; + for (const stmt of expressionStatements) { + const fnCall = stmt.expression; + if ( + fnCall.nodeType === 'FunctionCall' && + (fnCall.expression.nodeType === 'Identifier' || fnCall.expression.nodeType === 'MemberAccess') + ) { + const referencedId = fnCall.expression.referencedDeclaration; + if (referencedId) { + result.push(...getRecursiveFunctionIds(referencedId, deref, visited)); + } + } + } + result.push(referencedFn); + + return result; +} + +/** + * Get all functions that could be initializers. Does not include private functions. + * For parent contracts, only internal and public functions which contain statements are included. + */ +function getPossibleInitializers(contractDef: ContractDefinition, isParentContract: boolean) { + const fns = [...findAll('FunctionDefinition', contractDef)]; + return fns.filter( + fnDef => + (fnDef.modifiers.some(modifier => ['initializer', 'onlyInitializing'].includes(modifier.modifierName.name)) || + ['initialize', 'initializer'].includes(fnDef.name)) && + // Skip virtual functions without a body, since that indicates an abstract function and is not itself an initializer + !(fnDef.virtual && !fnDef.body) && + // Ignore private functions, since they cannot be called outside the contract + fnDef.visibility !== 'private' && + // For parent contracts, only internal and public functions which contain statements need to be called + (isParentContract + ? fnDef.body?.statements?.length && (fnDef.visibility === 'internal' || fnDef.visibility === 'public') + : true), + ); +}