Restore previous behavior of initializer during construction (OpenZep…

…pelin#3344)
marxxt · May 25, 2022 · 61294a6 · 61294a6
1 parent f8157ac
commit 61294a6
Show file tree

Hide file tree

Showing 3 changed files with 34 additions and 9 deletions.
diff --git 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
@@ -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
@@ -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');
     });