Update initializer modifier to prevent reentrancy during initialization

CHANGELOG.md

@@ -2,11 +2,28 @@
 
 ## Unreleased
 
-* `GovernorTimelockControl`: improve the `state()` function to have it reflect cases where a proposal has been canceled directly on the timelock. ([#2977](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2977))
-* `Math`: add a `abs(int256)` method that returns the unsigned absolute value of a signed value. ([#2984](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2984))
-* Preset contracts are now deprecated in favor of [Contracts Wizard](https://wizard.openzeppelin.com). ([#2986](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2986))
+ * `GovernorTimelockControl`: improve the `state()` function to have it reflect cases where a proposal has been canceled directly on the timelock. ([#2977](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2977))
+ * `Math`: add a `abs(int256)` method that returns the unsigned absolute value of a signed value. ([#2984](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2984))
+ * Preset contracts are now deprecated in favor of [Contracts Wizard](https://wizard.openzeppelin.com). ([#2986](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2986))
  * `Governor`: add a relay function to help recover assets sent to a governor that is not its own executor (e.g. when using a timelock). ([#2926](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2926))
  * `GovernorExtendedVoting`: add new module to ensure a minimum voting duration is available after the quorum is reached. ([#2973](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2973))
+ * `Initializable`: change the existing `initializer` modifier and add a new `onlyInitializing` modifier to prevent reentrancy risk. ([#3006](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3006))
+
+### Breaking changes
+
+It is no longer possible to call an `initializer`-protected function from within another `initializer` function outside the context of a constructor. Projects using OpenZeppelin upgradeable proxies should continue to work as is, since in the common case the initializer is invoked in the constructor directly. If this is not the case for you, the suggested change is to use the new `onlyInitializing` modifier in the following way:
+
+```diff
+ contract A {
+-    function initialize() public   initializer { ... }
++    function initialize() internal onlyInitializing { ... }
+ }
+ contract B is A {
+     function initialize() public initializer {
+         A.initialize(); 
+     }
+ }
+```
 
 ## 4.4.0 (2021-11-25)
 

contracts/mocks/InitializableMock.sol

@@ -10,16 +10,25 @@ import "../proxy/utils/Initializable.sol";
  */
 contract InitializableMock is Initializable {
     bool public initializerRan;
+    bool public initializerRan2;
     uint256 public x;
 
     function initialize() public initializer {
         initializerRan = true;
     }
 
+    function initialize2() public onlyInitializing {
+        initializerRan2 = true;
+    }
+
     function initializeNested() public initializer {
         initialize();
     }
 
+    function initializeNested2() public initializer {
+        initialize2();
+    }
+
     function initializeWithX(uint256 _x) public payable initializer {
         x = _x;
     }
@@ -32,3 +41,21 @@ contract InitializableMock is Initializable {
         require(false, "InitializableMock forced failure");
     }
 }
+
+contract ConstructorInitializableMock is Initializable {
+    bool public initializerRan;
+    bool public initializerRan2;
+
+    constructor() initializer {
+        initialize();
+        initialize2();
+    }
+
+    function initialize() public initializer {
+        initializerRan = true;
+    }
+
+    function initialize2() public onlyInitializing {
+        initializerRan2 = true;
+    }
+}

contracts/mocks/MultipleInheritanceInitializableMocks.sol

@@ -22,6 +22,16 @@ contract SampleHuman is Initializable {
     bool public isHuman;
 
     function initialize() public initializer {
+        __SampleHuman_init();
+    }
+
+    // solhint-disable-next-line func-name-mixedcase
+    function __SampleHuman_init() internal onlyInitializing {
+        __SampleHuman_init_unchained();
+    }
+
+    // solhint-disable-next-line func-name-mixedcase
+    function __SampleHuman_init_unchained() internal onlyInitializing {
         isHuman = true;
     }
 }
@@ -33,7 +43,17 @@ contract SampleMother is Initializable, SampleHuman {
     uint256 public mother;
 
     function initialize(uint256 value) public virtual initializer {
-        SampleHuman.initialize();
+        __SampleMother_init(value);
+    }
+
+    // solhint-disable-next-line func-name-mixedcase
+    function __SampleMother_init(uint256 value) internal onlyInitializing {
+        __SampleHuman_init();
+        __SampleMother_init_unchained(value);
+    }
+
+    // solhint-disable-next-line func-name-mixedcase
+    function __SampleMother_init_unchained(uint256 value) internal onlyInitializing {
         mother = value;
     }
 }
@@ -45,7 +65,17 @@ contract SampleGramps is Initializable, SampleHuman {
     string public gramps;
 
     function initialize(string memory value) public virtual initializer {
-        SampleHuman.initialize();
+        __SampleGramps_init(value);
+    }
+
+    // solhint-disable-next-line func-name-mixedcase
+    function __SampleGramps_init(string memory value) internal onlyInitializing {
+        __SampleHuman_init();
+        __SampleGramps_init_unchained(value);
+    }
+
+    // solhint-disable-next-line func-name-mixedcase
+    function __SampleGramps_init_unchained(string memory value) internal onlyInitializing {
         gramps = value;
     }
 }
@@ -57,7 +87,17 @@ contract SampleFather is Initializable, SampleGramps {
     uint256 public father;
 
     function initialize(string memory _gramps, uint256 _father) public initializer {
-        SampleGramps.initialize(_gramps);
+        __SampleFather_init(_gramps, _father);
+    }
+
+    // solhint-disable-next-line func-name-mixedcase
+    function __SampleFather_init(string memory _gramps, uint256 _father) internal onlyInitializing {
+        __SampleGramps_init(_gramps);
+        __SampleFather_init_unchained(_father);
+    }
+
+    // solhint-disable-next-line func-name-mixedcase
+    function __SampleFather_init_unchained(uint256 _father) internal onlyInitializing {
         father = _father;
     }
 }
@@ -74,8 +114,23 @@ contract SampleChild is Initializable, SampleMother, SampleFather {
         uint256 _father,
         uint256 _child
     ) public initializer {
-        SampleMother.initialize(_mother);
-        SampleFather.initialize(_gramps, _father);
+        __SampleChild_init(_mother, _gramps, _father, _child);
+    }
+
+    // solhint-disable-next-line func-name-mixedcase
+    function __SampleChild_init(
+        uint256 _mother,
+        string memory _gramps,
+        uint256 _father,
+        uint256 _child
+    ) internal onlyInitializing {
+        __SampleMother_init(_mother);
+        __SampleFather_init(_gramps, _father);
+        __SampleChild_init_unchained(_child);
+    }
+
+    // solhint-disable-next-line func-name-mixedcase
+    function __SampleChild_init_unchained(uint256 _child) internal onlyInitializing {
         child = _child;
     }
 }

contracts/proxy/utils/Initializable.sol

@@ -3,6 +3,8 @@
 
 pragma solidity ^0.8.0;
 
+import "../../utils/Address.sol";
+
 /**
  * @dev This is a base contract to aid in writing upgradeable contracts, or any kind of contract that will be deployed
  * behind a proxy. Since proxied contracts do not make use of a constructor, it's common to move constructor logic to an
@@ -45,7 +47,10 @@ abstract contract Initializable {
      * @dev Modifier to protect an initializer function from being invoked twice.
      */
     modifier initializer() {
-        require(_initializing || !_initialized, "Initializable: contract is already initialized");
+        // 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, because in other contexts the
+        // contract may have been reentered.
+        require(_initializing ? _isConstructor() : !_initialized, "Initializable: contract is already initialized");
 
         bool isTopLevelCall = !_initializing;
         if (isTopLevelCall) {
@@ -59,4 +64,17 @@ abstract contract Initializable {
             _initializing = false;
         }
     }
+
+    /**
+     * @dev Modifier to protect an initialization function so that it can only be invoked by functions with the
+     * {initializer} modifier, directly or indirectly.
+     */
+    modifier onlyInitializing() {
+        require(_initializing, "Initializable: contract is not initializing");
+        _;
+    }
+
+    function _isConstructor() private view returns (bool) {
+        return !Address.isContract(address(this));
+    }
 }

test/proxy/utils/Initializable.test.js

@@ -3,6 +3,7 @@ const { expectRevert } = require('@openzeppelin/test-helpers');
 const { assert } = require('chai');
 
 const InitializableMock = artifacts.require('InitializableMock');
+const ConstructorInitializableMock = artifacts.require('ConstructorInitializableMock');
 const SampleChild = artifacts.require('SampleChild');
 
 contract('Initializable', function (accounts) {
@@ -32,14 +33,37 @@ contract('Initializable', function (accounts) {
     });
 
     context('after nested initialize', function () {
-      beforeEach('initializing', async function () {
-        await this.contract.initializeNested();
+      context('with initializer methods', function () {
+        it('nested initializer reverts', async function () {
+          await expectRevert(this.contract.initializeNested(), 'Initializable: contract is already initialized');
+        });
       });
 
-      it('initializer has run', async function () {
-        assert.isTrue(await this.contract.initializerRan());
+      context('with onlyInitializing methods', function () {
+        beforeEach('initializing', async function () {
+          await this.contract.initializeNested2();
+        });
+
+        it('nested initializer has run', async function () {
+          assert.isTrue(await this.contract.initializerRan2());
+        });
       });
     });
+
+    it('cannot call onlyInitializable function outside the scope of an initializable function', async function () {
+      await expectRevert(this.contract.initialize2(), 'Initializable: contract is not initializing');
+    });
+  });
+
+  describe('initialization during construction', function () {
+    beforeEach('initializing', async function () {
+      this.contract2 = await ConstructorInitializableMock.new();
+    });
+
+    it('nested initializer can run during construction', async function () {
+      assert.isTrue(await this.contract2.initializerRan());
+      assert.isTrue(await this.contract2.initializerRan2());
+    });
   });
 
   describe('complex testing with inheritance', function () {