From aa0bb6f3024098958d7850cd415e51fd17ddb972 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Thu, 27 Jul 2023 15:53:54 -0600 Subject: [PATCH] Revert #3950 fix --- contracts/mocks/InitializableMock.sol | 28 ++++++++- contracts/proxy/utils/Initializable.sol | 77 ++++++++----------------- test/proxy/utils/Initializable.test.js | 27 +++++++-- 3 files changed, 72 insertions(+), 60 deletions(-) diff --git a/contracts/mocks/InitializableMock.sol b/contracts/mocks/InitializableMock.sol index 29fa56ebc8c..ab5ea031132 100644 --- a/contracts/mocks/InitializableMock.sol +++ b/contracts/mocks/InitializableMock.sol @@ -25,6 +25,10 @@ contract InitializableMock is Initializable { onlyInitializingRan = true; } + function initializerNested() public initializer { + initialize(); + } + function onlyInitializingNested() public initializer { initializeOnlyInitializing(); } @@ -60,6 +64,18 @@ contract ConstructorInitializableMock is Initializable { } } +contract ChildConstructorInitializableMock is ConstructorInitializableMock { + bool public childInitializerRan; + + constructor() initializer { + childInitialize(); + } + + function childInitialize() public initializer { + childInitializerRan = true; + } +} + contract ReinitializerMock is Initializable { uint256 public counter; @@ -93,14 +109,22 @@ contract ReinitializerMock is Initializable { } } -contract DisableOk is Initializable { +contract DisableNew is Initializable { constructor() { _disableInitializers(); } } -contract DisableBad is Initializable { +contract DisableOld is Initializable { + constructor() initializer {} +} + +contract DisableBad1 is DisableNew, DisableOld {} + +contract DisableBad2 is Initializable { constructor() initializer { _disableInitializers(); } } + +contract DisableOk is DisableOld, DisableNew {} diff --git a/contracts/proxy/utils/Initializable.sol b/contracts/proxy/utils/Initializable.sol index 9c4297c0157..897e9eff87f 100644 --- a/contracts/proxy/utils/Initializable.sol +++ b/contracts/proxy/utils/Initializable.sol @@ -102,9 +102,22 @@ abstract contract Initializable { * Emits an {Initialized} event. */ modifier initializer() { - bool isTopLevelCall = _beforeInitialize(1); + // solhint-disable-next-line var-name-mixedcase + InitializableStorage storage $ = _getInitializableStorage(); + + bool isTopLevelCall = !$._initializing; + if (!(isTopLevelCall && $._initialized < 1) && !(address(this).code.length == 0 && $._initialized == 1)) { + revert AlreadyInitialized(); + } + $._initialized = 1; + if (isTopLevelCall) { + $._initializing = true; + } _; - _afterInitialize(isTopLevelCall, 1); + if (isTopLevelCall) { + $._initializing = false; + emit Initialized(1); + } } /** @@ -126,9 +139,17 @@ abstract contract Initializable { * Emits an {Initialized} event. */ modifier reinitializer(uint64 version) { - bool isTopLevelCall = _beforeInitialize(version); + // solhint-disable-next-line var-name-mixedcase + InitializableStorage storage $ = _getInitializableStorage(); + + if ($._initializing || $._initialized >= version) { + revert AlreadyInitialized(); + } + $._initialized = version; + $._initializing = true; _; - _afterInitialize(isTopLevelCall, version); + $._initializing = false; + emit Initialized(version); } /** @@ -186,52 +207,4 @@ abstract contract Initializable { $.slot := _INITIALIZABLE_STORAGE } } - - /** - * @dev Sets the initialized version. - * - * Requirements: - * - * - If the contract is initializing the version set must not be that of an reinitializer. - * - If the contract is not initializing the version must not be already set. - */ - function _setInitializedVersion(uint64 version) private returns (bool) { - // solhint-disable-next-line var-name-mixedcase - InitializableStorage storage $ = _getInitializableStorage(); - - bool initializing = $._initializing; - - // When it's initializing check if it's a reinitializer, otherwise check if - // the intended version is not already set. - if (initializing ? version > 1 : version <= $._initialized) { - revert AlreadyInitialized(); - } - - $._initialized = version; - - return !initializing; - } - - /** - * @dev Runs before initialization. - * It sets the initialized version and sets the initializing flag to true. - */ - function _beforeInitialize(uint64 version) private returns (bool) { - bool isTopLevelCall = _setInitializedVersion(version); - if (isTopLevelCall) { - _getInitializableStorage()._initializing = true; - } - return isTopLevelCall; - } - - /** - * @dev Runs after initialization. - * It clears the initializing flag and emits an {Initialized} event if it is a top level call. - */ - function _afterInitialize(bool isTopLevelCall, uint64 version) private { - if (isTopLevelCall) { - _getInitializableStorage()._initializing = false; - emit Initialized(version); - } - } } diff --git a/test/proxy/utils/Initializable.test.js b/test/proxy/utils/Initializable.test.js index 3e50111a42a..cb45995ea44 100644 --- a/test/proxy/utils/Initializable.test.js +++ b/test/proxy/utils/Initializable.test.js @@ -4,9 +4,11 @@ const { expectRevertCustomError } = require('../../helpers/customError'); const InitializableMock = artifacts.require('InitializableMock'); const ConstructorInitializableMock = artifacts.require('ConstructorInitializableMock'); +const ChildConstructorInitializableMock = artifacts.require('ChildConstructorInitializableMock'); const ReinitializerMock = artifacts.require('ReinitializerMock'); const SampleChild = artifacts.require('SampleChild'); -const DisableBad = artifacts.require('DisableBad'); +const DisableBad1 = artifacts.require('DisableBad1'); +const DisableBad2 = artifacts.require('DisableBad2'); const DisableOk = artifacts.require('DisableOk'); contract('Initializable', function () { @@ -44,6 +46,10 @@ contract('Initializable', function () { }); describe('nested under an initializer', function () { + it('initializer modifier reverts', async function () { + await expectRevertCustomError(this.contract.initializerNested(), 'AlreadyInitialized', []); + }); + it('onlyInitializing modifier succeeds', async function () { await this.contract.onlyInitializingNested(); expect(await this.contract.onlyInitializingRan()).to.equal(true); @@ -61,6 +67,13 @@ contract('Initializable', function () { expect(await contract2.onlyInitializingRan()).to.equal(true); }); + it('multiple constructor levels can be initializers', async function () { + const contract2 = await ChildConstructorInitializableMock.new(); + expect(await contract2.initializerRan()).to.equal(true); + expect(await contract2.childInitializerRan()).to.equal(true); + expect(await contract2.onlyInitializingRan()).to.equal(true); + }); + describe('reinitialization', function () { beforeEach('deploying', async function () { this.contract = await ReinitializerMock.new(); @@ -192,13 +205,15 @@ contract('Initializable', function () { }); describe('disabling initialization', function () { - it('disables initializers', async function () { - const ok = await DisableOk.new(); - await expectEvent.inConstruction(ok, 'Initialized', { version: '18446744073709551615' }); // MAX_UINT64 + it('old and new patterns in bad sequence', async function () { + await expectRevertCustomError(DisableBad1.new(), 'AlreadyInitialized', []); + await expectRevertCustomError(DisableBad2.new(), 'AlreadyInitialized', []); }); - it('reverts disabling initializers and using the initializer modifier', async function () { - await expectRevertCustomError(DisableBad.new(), 'AlreadyInitialized', []); + 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: '18446744073709551615' }); // MAX_UINT64 }); }); });