diff --git a/contracts/mocks/InitializableMock.sol b/contracts/mocks/InitializableMock.sol index bdf53991f..91ceabe91 100644 --- a/contracts/mocks/InitializableMock.sol +++ b/contracts/mocks/InitializableMock.sol @@ -60,6 +60,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; diff --git a/contracts/proxy/utils/Initializable.sol b/contracts/proxy/utils/Initializable.sol index fc9461d86..9e8bb6e67 100644 --- a/contracts/proxy/utils/Initializable.sol +++ b/contracts/proxy/utils/Initializable.sol @@ -134,16 +134,20 @@ abstract contract Initializable { // 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. - if (_initializing) { - require( - version == 1 && !Address.isContract(address(this)), - "Initializable: contract is already initialized" - ); - return false; - } else { - require(_initialized < version, "Initializable: contract is already initialized"); + + 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 true; } + + return isTopLevelCall; } } diff --git a/test/proxy/utils/Initializable.test.js b/test/proxy/utils/Initializable.test.js index 9879b4820..28f272adc 100644 --- a/test/proxy/utils/Initializable.test.js +++ b/test/proxy/utils/Initializable.test.js @@ -3,6 +3,7 @@ const { expect } = require('chai'); 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'); @@ -54,6 +55,13 @@ contract('Initializable', function (accounts) { 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(); @@ -79,6 +87,7 @@ contract('Initializable', function (accounts) { it('cannot nest reinitializers', async function () { expect(await this.contract.counter()).to.be.bignumber.equal('0'); + await expectRevert(this.contract.nestedReinitialize(2, 2), 'Initializable: contract is already initialized'); await expectRevert(this.contract.nestedReinitialize(2, 3), 'Initializable: contract is already initialized'); await expectRevert(this.contract.nestedReinitialize(3, 2), 'Initializable: contract is already initialized'); });