From b3c923408b7c4eaf6eb7605491c7d04680c075df Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 2 Jun 2022 15:58:05 -0300 Subject: [PATCH 01/11] simplify initializable --- contracts/proxy/utils/Initializable.sol | 53 +++++++++---------------- 1 file changed, 19 insertions(+), 34 deletions(-) diff --git a/contracts/proxy/utils/Initializable.sol b/contracts/proxy/utils/Initializable.sol index 9e8bb6e67f6..dd5f83bbf34 100644 --- a/contracts/proxy/utils/Initializable.sol +++ b/contracts/proxy/utils/Initializable.sol @@ -76,10 +76,14 @@ abstract contract Initializable { * `onlyInitializing` functions can be used to initialize parent contracts. Equivalent to `reinitializer(1)`. */ modifier initializer() { - bool isTopLevelCall = _setInitializedVersion(1); - if (isTopLevelCall) { - _initializing = true; - } + bool isTopLevelCall = !_initializing; + require( + (isTopLevelCall && _initialized < 1) + || (!Address.isContract(address(this)) && _initialized == 1), + "Initializable: contract is already initialized" + ); + _initialized = 1; + _initializing = true; _; if (isTopLevelCall) { _initializing = false; @@ -100,15 +104,15 @@ abstract contract Initializable { * a contract, executing them in the right order is up to the developer or operator. */ modifier reinitializer(uint8 version) { - bool isTopLevelCall = _setInitializedVersion(version); - if (isTopLevelCall) { - _initializing = true; - } + require( + !_initializing && _initialized < version, + "Initializable: contract is already initialized" + ); + _initialized = version; + _initializing = true; _; - if (isTopLevelCall) { - _initializing = false; - emit Initialized(version); - } + _initializing = false; + emit Initialized(version); } /** @@ -127,27 +131,8 @@ abstract contract Initializable { * through proxies. */ function _disableInitializers() internal virtual { - _setInitializedVersion(type(uint8).max); - } - - function _setInitializedVersion(uint8 version) private returns (bool) { - // If the contract is initializing we ignore whether _initialized is set in order to support multiple - // inheritance patterns, but we only do this in the context of a constructor, and for the lowest level - // of initializers, because in other contexts the contract may have been reentered. - - bool isTopLevelCall = !_initializing; // cache sload - uint8 currentVersion = _initialized; // cache sload - - require( - (isTopLevelCall && version > currentVersion) || // not nested with increasing version or - (!Address.isContract(address(this)) && (version == 1 || version == type(uint8).max)), // contract being constructed - "Initializable: contract is already initialized" - ); - - if (isTopLevelCall) { - _initialized = version; - } - - return isTopLevelCall; + require(!_initializing, "Initializable: contract is initializing"); + _initialized = type(uint8).max; + emit Initialized(type(uint8).max); } } From 12750aaa4ce4d444ab969b7e6f0087aed0cb1596 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 2 Jun 2022 16:07:29 -0300 Subject: [PATCH 02/11] lint --- contracts/proxy/utils/Initializable.sol | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/contracts/proxy/utils/Initializable.sol b/contracts/proxy/utils/Initializable.sol index dd5f83bbf34..1ac8b87e5cd 100644 --- a/contracts/proxy/utils/Initializable.sol +++ b/contracts/proxy/utils/Initializable.sol @@ -78,8 +78,7 @@ abstract contract Initializable { modifier initializer() { bool isTopLevelCall = !_initializing; require( - (isTopLevelCall && _initialized < 1) - || (!Address.isContract(address(this)) && _initialized == 1), + (isTopLevelCall && _initialized < 1) || (!Address.isContract(address(this)) && _initialized == 1), "Initializable: contract is already initialized" ); _initialized = 1; @@ -104,10 +103,7 @@ abstract contract Initializable { * a contract, executing them in the right order is up to the developer or operator. */ modifier reinitializer(uint8 version) { - require( - !_initializing && _initialized < version, - "Initializable: contract is already initialized" - ); + require(!_initializing && _initialized < version, "Initializable: contract is already initialized"); _initialized = version; _initializing = true; _; From 85a9bd386910c531bcf6ebc0f39b243d3577de82 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 2 Jun 2022 16:08:05 -0300 Subject: [PATCH 03/11] optimize unnecessary storage write --- contracts/proxy/utils/Initializable.sol | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/contracts/proxy/utils/Initializable.sol b/contracts/proxy/utils/Initializable.sol index 1ac8b87e5cd..26178106803 100644 --- a/contracts/proxy/utils/Initializable.sol +++ b/contracts/proxy/utils/Initializable.sol @@ -82,7 +82,9 @@ abstract contract Initializable { "Initializable: contract is already initialized" ); _initialized = 1; - _initializing = true; + if (isTopLevelCall) { + _initializing = true; + } _; if (isTopLevelCall) { _initializing = false; From 499b5ce9378d115768778ee8d5a578f3dbdb2116 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 2 Jun 2022 16:37:58 -0300 Subject: [PATCH 04/11] add tests --- contracts/mocks/InitializableMock.sol | 19 +++++++++++++++++++ test/proxy/utils/Initializable.test.js | 9 +++++++++ 2 files changed, 28 insertions(+) diff --git a/contracts/mocks/InitializableMock.sol b/contracts/mocks/InitializableMock.sol index 91ceabe91c0..679eb7a0f17 100644 --- a/contracts/mocks/InitializableMock.sol +++ b/contracts/mocks/InitializableMock.sol @@ -100,3 +100,22 @@ contract ReinitializerMock is Initializable { counter++; } } + +contract Disable1 is Initializable { + constructor() { + _disableInitializers(); + } +} + +contract Disable2 is Initializable { + constructor() initializer {} +} + +contract Disable3 is Disable1, Disable2 {} + +contract Disable4 is Initializable { + constructor() initializer { + _disableInitializers(); + } +} + diff --git a/test/proxy/utils/Initializable.test.js b/test/proxy/utils/Initializable.test.js index 28f272adca8..55f84bf64fc 100644 --- a/test/proxy/utils/Initializable.test.js +++ b/test/proxy/utils/Initializable.test.js @@ -6,6 +6,8 @@ const ConstructorInitializableMock = artifacts.require('ConstructorInitializable const ChildConstructorInitializableMock = artifacts.require('ChildConstructorInitializableMock'); const ReinitializerMock = artifacts.require('ReinitializerMock'); const SampleChild = artifacts.require('SampleChild'); +const Disable3 = artifacts.require('Disable3'); +const Disable4 = artifacts.require('Disable4'); contract('Initializable', function (accounts) { describe('basic testing without inheritance', function () { @@ -184,4 +186,11 @@ contract('Initializable', function (accounts) { expect(await this.contract.child()).to.be.bignumber.equal(child); }); }); + + describe('disabling initialization', function () { + it('old and new patterns dont interfere', async function () { + await expectRevert(Disable3.new(), 'Initializable: contract is already initialized'); + await expectRevert(Disable4.new(), 'Initializable: contract is initializing'); + }); + }); }); From 3b04ae3a236ba5a0f097a02da7029636260bd46d Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 2 Jun 2022 16:38:26 -0300 Subject: [PATCH 05/11] update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6fe15e5bccc..00c7a8eb05a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ * `ERC721`, `ERC1155`: simplified revert reasons. ([#3254](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3254), ([#3438](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3438))) * `ERC721`: removed redundant require statement. ([#3434](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3434)) * `PaymentSplitter`: add `releasable` getters. ([#3350](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3350)) + * `Initializable`: refactored implementation of modifiers for ease of understanding. ([#3450](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3450)) ## 4.6.0 (2022-04-26) From 5a4742d8f39e3672b1f6bc3aa7492fcf18f0b74c Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 2 Jun 2022 16:40:40 -0300 Subject: [PATCH 06/11] lint --- contracts/mocks/InitializableMock.sol | 1 - 1 file changed, 1 deletion(-) diff --git a/contracts/mocks/InitializableMock.sol b/contracts/mocks/InitializableMock.sol index 679eb7a0f17..a43dc12eb9c 100644 --- a/contracts/mocks/InitializableMock.sol +++ b/contracts/mocks/InitializableMock.sol @@ -118,4 +118,3 @@ contract Disable4 is Initializable { _disableInitializers(); } } - From b7e7c9401d9de0a244d33bfe1a4c4735fe0e3d74 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 2 Jun 2022 16:41:37 -0300 Subject: [PATCH 07/11] add breaking change in changelog --- CHANGELOG.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 00c7a8eb05a..f30f4174300 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,7 +17,11 @@ * `ERC721`, `ERC1155`: simplified revert reasons. ([#3254](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3254), ([#3438](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3438))) * `ERC721`: removed redundant require statement. ([#3434](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3434)) * `PaymentSplitter`: add `releasable` getters. ([#3350](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3350)) - * `Initializable`: refactored implementation of modifiers for ease of understanding. ([#3450](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3450)) + * `Initializable`: refactored implementation of modifiers for easier understanding. ([#3450](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3450)) + +### Breaking changes + + * `Initializable`: functions decorated with the modifier `reinitializer(1)` may no longer invoke each other. ## 4.6.0 (2022-04-26) From 37d4e6f221fbe7c05b7e2109d9505cabd7ce1fc7 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Fri, 3 Jun 2022 12:14:39 -0300 Subject: [PATCH 08/11] Update contracts/proxy/utils/Initializable.sol Co-authored-by: Hadrien Croubois --- contracts/proxy/utils/Initializable.sol | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/contracts/proxy/utils/Initializable.sol b/contracts/proxy/utils/Initializable.sol index 26178106803..2a56119aa12 100644 --- a/contracts/proxy/utils/Initializable.sol +++ b/contracts/proxy/utils/Initializable.sol @@ -130,7 +130,9 @@ abstract contract Initializable { */ function _disableInitializers() internal virtual { require(!_initializing, "Initializable: contract is initializing"); - _initialized = type(uint8).max; - emit Initialized(type(uint8).max); + if (_initialized < type(uint8).max) { + _initialized = type(uint8).max; + emit Initialized(type(uint8).max); + } } } From 325aa9f9b938d7afe86e6531881afd0c390656f0 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Fri, 3 Jun 2022 15:46:19 -0300 Subject: [PATCH 09/11] improve test and test description --- contracts/mocks/InitializableMock.sol | 10 ++++++---- test/proxy/utils/Initializable.test.js | 17 ++++++++++++----- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/contracts/mocks/InitializableMock.sol b/contracts/mocks/InitializableMock.sol index a43dc12eb9c..b24db08911c 100644 --- a/contracts/mocks/InitializableMock.sol +++ b/contracts/mocks/InitializableMock.sol @@ -101,20 +101,22 @@ contract ReinitializerMock is Initializable { } } -contract Disable1 is Initializable { +contract DisableNew is Initializable { constructor() { _disableInitializers(); } } -contract Disable2 is Initializable { +contract DisableOld is Initializable { constructor() initializer {} } -contract Disable3 is Disable1, Disable2 {} +contract DisableBad1 is DisableNew, DisableOld {} -contract Disable4 is Initializable { +contract DisableBad2 is Initializable { constructor() initializer { _disableInitializers(); } } + +contract DisableOk is DisableOld, DisableNew {} diff --git a/test/proxy/utils/Initializable.test.js b/test/proxy/utils/Initializable.test.js index 55f84bf64fc..664bd899d24 100644 --- a/test/proxy/utils/Initializable.test.js +++ b/test/proxy/utils/Initializable.test.js @@ -6,8 +6,9 @@ const ConstructorInitializableMock = artifacts.require('ConstructorInitializable const ChildConstructorInitializableMock = artifacts.require('ChildConstructorInitializableMock'); const ReinitializerMock = artifacts.require('ReinitializerMock'); const SampleChild = artifacts.require('SampleChild'); -const Disable3 = artifacts.require('Disable3'); -const Disable4 = artifacts.require('Disable4'); +const DisableBad1 = artifacts.require('DisableBad1'); +const DisableBad2 = artifacts.require('DisableBad2'); +const DisableOk = artifacts.require('DisableOk'); contract('Initializable', function (accounts) { describe('basic testing without inheritance', function () { @@ -188,9 +189,15 @@ contract('Initializable', function (accounts) { }); describe('disabling initialization', function () { - it('old and new patterns dont interfere', async function () { - await expectRevert(Disable3.new(), 'Initializable: contract is already initialized'); - await expectRevert(Disable4.new(), 'Initializable: contract is initializing'); + it('old and new patterns in bad sequence', async function () { + await expectRevert(DisableBad1.new(), 'Initializable: contract is already initialized'); + await expectRevert(DisableBad2.new(), 'Initializable: contract is initializing'); + }); + + it('old and new patterns in good sequence', async function () { + const ok = await DisableOk.new(); + await expectEvent.inConstruction(ok, 'Initialized', { version: '1' }); + await expectEvent.inConstruction(ok, 'Initialized', { version: '255' }); }); }); }); From 3c68c521594ef000d170b71dc7a46e3d072d114c Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Fri, 3 Jun 2022 16:21:01 -0300 Subject: [PATCH 10/11] ignore mocks from inheritance ordering check --- scripts/checks/inheritanceOrdering.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/scripts/checks/inheritanceOrdering.js b/scripts/checks/inheritanceOrdering.js index 9d332cba330..3ade7409a7a 100755 --- a/scripts/checks/inheritanceOrdering.js +++ b/scripts/checks/inheritanceOrdering.js @@ -13,6 +13,10 @@ for (const artifact of artifacts) { const linearized = []; for (const source in solcOutput.contracts) { + if (source.includes('/mocks/')) { + continue; + } + for (const contractDef of findAll('ContractDefinition', solcOutput.sources[source].ast)) { names[contractDef.id] = contractDef.name; linearized.push(contractDef.linearizedBaseContracts); From 35a11a9cecc4a389e09dce16d5b355458d8dd362 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Fri, 3 Jun 2022 16:21:29 -0300 Subject: [PATCH 11/11] rename inheritanceOrdering -> inheritance-ordering --- package.json | 2 +- .../checks/{inheritanceOrdering.js => inheritance-ordering.js} | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename scripts/checks/{inheritanceOrdering.js => inheritance-ordering.js} (100%) diff --git a/package.json b/package.json index 6074a428893..60509ecd027 100644 --- a/package.json +++ b/package.json @@ -29,7 +29,7 @@ "release": "scripts/release/release.sh", "version": "scripts/release/version.sh", "test": "hardhat test", - "test:inheritance": "scripts/checks/inheritanceOrdering.js artifacts/build-info/*", + "test:inheritance": "scripts/checks/inheritance-ordering.js artifacts/build-info/*", "test:generation": "scripts/checks/generation.sh", "gas-report": "env ENABLE_GAS_REPORT=true npm run test", "slither": "npm run clean && slither . --detect reentrancy-eth,reentrancy-no-eth,reentrancy-unlimited-gas" diff --git a/scripts/checks/inheritanceOrdering.js b/scripts/checks/inheritance-ordering.js similarity index 100% rename from scripts/checks/inheritanceOrdering.js rename to scripts/checks/inheritance-ordering.js