From 37ddfef4e8a463cbcde7ec6c5eb19cf087c5ee43 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Sat, 16 Apr 2022 08:54:56 +0200 Subject: [PATCH 1/8] Fix issue where multiple contract in the inheritance tree have initializer constructors --- contracts/mocks/InitializableMock.sol | 12 ++++++++++++ contracts/proxy/utils/Initializable.sol | 2 +- test/proxy/utils/Initializable.test.js | 8 ++++++++ 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/contracts/mocks/InitializableMock.sol b/contracts/mocks/InitializableMock.sol index bdf53991fbc..91ceabe91c0 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 3ec66bb6ea9..24289035121 100644 --- a/contracts/proxy/utils/Initializable.sol +++ b/contracts/proxy/utils/Initializable.sol @@ -141,7 +141,7 @@ abstract contract Initializable { ); return false; } else { - require(_initialized < version, "Initializable: contract is already initialized"); + require(_initialized < version || (version == 1 && !Address.isContract(address(this))), "Initializable: contract is already initialized"); _initialized = version; return true; } diff --git a/test/proxy/utils/Initializable.test.js b/test/proxy/utils/Initializable.test.js index 9879b482052..4b07905b07f 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(); From fb6a39dfb62b0a247910da88967b3d9bf263f410 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Sat, 16 Apr 2022 09:03:47 +0200 Subject: [PATCH 2/8] fix lint --- contracts/proxy/utils/Initializable.sol | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/contracts/proxy/utils/Initializable.sol b/contracts/proxy/utils/Initializable.sol index 24289035121..4c4208508b8 100644 --- a/contracts/proxy/utils/Initializable.sol +++ b/contracts/proxy/utils/Initializable.sol @@ -141,7 +141,10 @@ abstract contract Initializable { ); return false; } else { - require(_initialized < version || (version == 1 && !Address.isContract(address(this))), "Initializable: contract is already initialized"); + require( + _initialized < version || (version == 1 && !Address.isContract(address(this))), + "Initializable: contract is already initialized" + ); _initialized = version; return true; } From 1bc9de4bb814c94683bdfc856bc74d0cd7ef65b0 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 19 Apr 2022 11:51:33 +0200 Subject: [PATCH 3/8] refactor _setInitializedVersion --- contracts/proxy/utils/Initializable.sol | 26 +++++++++++++------------ test/proxy/utils/Initializable.test.js | 12 +++++++++--- 2 files changed, 23 insertions(+), 15 deletions(-) diff --git a/contracts/proxy/utils/Initializable.sol b/contracts/proxy/utils/Initializable.sol index 6aef37e1119..620b8d2aeaa 100644 --- a/contracts/proxy/utils/Initializable.sol +++ b/contracts/proxy/utils/Initializable.sol @@ -134,19 +134,21 @@ 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 || (version == 1 && !Address.isContract(address(this))), - "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 + (!isTopLevelCall && version == currentVersion) || // nested with same version or + (!Address.isContract(address(this)) && version == 1), // 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 4b07905b07f..5aaeaf36141 100644 --- a/test/proxy/utils/Initializable.test.js +++ b/test/proxy/utils/Initializable.test.js @@ -34,8 +34,8 @@ contract('Initializable', function (accounts) { }); describe('nested under an initializer', function () { - it('initializer modifier reverts', async function () { - await expectRevert(this.contract.initializerNested(), 'Initializable: contract is already initialized'); + it('initializer modifier can be nested', async function () { + await this.contract.initializerNested(); }); it('onlyInitializing modifier succeeds', async function () { @@ -85,7 +85,13 @@ contract('Initializable', function (accounts) { expect(await this.contract.counter()).to.be.bignumber.equal('2'); }); - it('cannot nest reinitializers', async function () { + it('can nest reinitializers with same version', async function () { + expect(await this.contract.counter()).to.be.bignumber.equal('0'); + await this.contract.nestedReinitialize(2, 2); + expect(await this.contract.counter()).to.be.bignumber.equal('1'); + }); + + it('cannot nest reinitializers with different versions', async function () { expect(await this.contract.counter()).to.be.bignumber.equal('0'); await expectRevert(this.contract.nestedReinitialize(2, 3), 'Initializable: contract is already initialized'); await expectRevert(this.contract.nestedReinitialize(3, 2), 'Initializable: contract is already initialized'); From daa734e9073b21e449c9edeb72782bec6419e128 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Sat, 7 May 2022 14:57:34 -0300 Subject: [PATCH 4/8] fix reentrancy introduced by the refactor --- contracts/proxy/utils/Initializable.sol | 1 - test/proxy/utils/Initializable.test.js | 5 ++--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/contracts/proxy/utils/Initializable.sol b/contracts/proxy/utils/Initializable.sol index 620b8d2aeaa..4633e7e7543 100644 --- a/contracts/proxy/utils/Initializable.sol +++ b/contracts/proxy/utils/Initializable.sol @@ -140,7 +140,6 @@ abstract contract Initializable { require( (isTopLevelCall && version > currentVersion) || // not nested with increasing version or - (!isTopLevelCall && version == currentVersion) || // nested with same version or (!Address.isContract(address(this)) && version == 1), // contract being constructed "Initializable: contract is already initialized" ); diff --git a/test/proxy/utils/Initializable.test.js b/test/proxy/utils/Initializable.test.js index 5aaeaf36141..816c15de2f7 100644 --- a/test/proxy/utils/Initializable.test.js +++ b/test/proxy/utils/Initializable.test.js @@ -35,7 +35,7 @@ contract('Initializable', function (accounts) { describe('nested under an initializer', function () { it('initializer modifier can be nested', async function () { - await this.contract.initializerNested(); + await expectRevert(this.contract.initializerNested(), 'Initializable: contract is already initialized'); }); it('onlyInitializing modifier succeeds', async function () { @@ -87,8 +87,7 @@ contract('Initializable', function (accounts) { it('can nest reinitializers with same version', async function () { expect(await this.contract.counter()).to.be.bignumber.equal('0'); - await this.contract.nestedReinitialize(2, 2); - expect(await this.contract.counter()).to.be.bignumber.equal('1'); + await expectRevert(this.contract.nestedReinitialize(2, 2), 'Initializable: contract is already initialized'); }); it('cannot nest reinitializers with different versions', async function () { From f66cedac92e828c7896ebb7ad605ee0a1943e51a Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Sat, 7 May 2022 14:59:24 -0300 Subject: [PATCH 5/8] test wording --- test/proxy/utils/Initializable.test.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/test/proxy/utils/Initializable.test.js b/test/proxy/utils/Initializable.test.js index 816c15de2f7..6aafb8ca9d7 100644 --- a/test/proxy/utils/Initializable.test.js +++ b/test/proxy/utils/Initializable.test.js @@ -85,13 +85,9 @@ contract('Initializable', function (accounts) { expect(await this.contract.counter()).to.be.bignumber.equal('2'); }); - it('can nest reinitializers with same version', async function () { + 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'); - }); - - it('cannot nest reinitializers with different versions', async function () { - expect(await this.contract.counter()).to.be.bignumber.equal('0'); await expectRevert(this.contract.nestedReinitialize(2, 3), 'Initializable: contract is already initialized'); await expectRevert(this.contract.nestedReinitialize(3, 2), 'Initializable: contract is already initialized'); }); From 516ff359c84dd6627048cc75a5238ba4aabb33e9 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Sat, 7 May 2022 15:00:13 -0300 Subject: [PATCH 6/8] test wording --- test/proxy/utils/Initializable.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/proxy/utils/Initializable.test.js b/test/proxy/utils/Initializable.test.js index 6aafb8ca9d7..28f272adca8 100644 --- a/test/proxy/utils/Initializable.test.js +++ b/test/proxy/utils/Initializable.test.js @@ -34,7 +34,7 @@ contract('Initializable', function (accounts) { }); describe('nested under an initializer', function () { - it('initializer modifier can be nested', async function () { + it('initializer modifier reverts', async function () { await expectRevert(this.contract.initializerNested(), 'Initializable: contract is already initialized'); }); From 4dbfa2e223d334369976751058b5cf5876b5b80b Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 18 May 2022 21:33:26 +0200 Subject: [PATCH 7/8] fix lint --- contracts/proxy/utils/Initializable.sol | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/contracts/proxy/utils/Initializable.sol b/contracts/proxy/utils/Initializable.sol index 4633e7e7543..3a5cfc2eb2b 100644 --- a/contracts/proxy/utils/Initializable.sol +++ b/contracts/proxy/utils/Initializable.sol @@ -139,8 +139,9 @@ abstract contract Initializable { uint8 currentVersion = _initialized; // cache sload require( - (isTopLevelCall && version > currentVersion) || // not nested with increasing version or - (!Address.isContract(address(this)) && version == 1), // contract being constructed + // not nested with increasing version or + // contract being constructed and initializing version 1 + (isTopLevelCall && version > currentVersion) || (!Address.isContract(address(this)) && version == 1), "Initializable: contract is already initialized" ); From b7ec0ab965f3eabe1668b09bd553f0304b4d84e8 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 25 May 2022 09:31:05 +0200 Subject: [PATCH 8/8] initialization during construction workflow --- contracts/proxy/utils/Initializable.sol | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/contracts/proxy/utils/Initializable.sol b/contracts/proxy/utils/Initializable.sol index 3a5cfc2eb2b..6a4bb4d3aa2 100644 --- a/contracts/proxy/utils/Initializable.sol +++ b/contracts/proxy/utils/Initializable.sol @@ -139,9 +139,8 @@ abstract contract Initializable { uint8 currentVersion = _initialized; // cache sload require( - // not nested with increasing version or - // contract being constructed and initializing version 1 - (isTopLevelCall && version > currentVersion) || (!Address.isContract(address(this)) && version == 1), + (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" );