diff --git a/.changeset/thick-pumpkins-exercise.md b/.changeset/thick-pumpkins-exercise.md new file mode 100644 index 00000000000..8df8b51cc47 --- /dev/null +++ b/.changeset/thick-pumpkins-exercise.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': major +--- + +`Initializable`: Use the namespaced storage pattern to avoid putting critical variables in slot 0. Allow reinitializer versions greater than 256. diff --git a/contracts/mocks/InitializableMock.sol b/contracts/mocks/InitializableMock.sol index 155aaefb799..ab5ea031132 100644 --- a/contracts/mocks/InitializableMock.sol +++ b/contracts/mocks/InitializableMock.sol @@ -79,7 +79,7 @@ contract ChildConstructorInitializableMock is ConstructorInitializableMock { contract ReinitializerMock is Initializable { uint256 public counter; - function getInitializedVersion() public view returns (uint8) { + function getInitializedVersion() public view returns (uint64) { return _getInitializedVersion(); } @@ -87,15 +87,15 @@ contract ReinitializerMock is Initializable { doStuff(); } - function reinitialize(uint8 i) public reinitializer(i) { + function reinitialize(uint64 i) public reinitializer(i) { doStuff(); } - function nestedReinitialize(uint8 i, uint8 j) public reinitializer(i) { + function nestedReinitialize(uint64 i, uint64 j) public reinitializer(i) { reinitialize(j); } - function chainReinitialize(uint8 i, uint8 j) public { + function chainReinitialize(uint64 i, uint64 j) public { reinitialize(i); reinitialize(j); } diff --git a/contracts/proxy/utils/Initializable.sol b/contracts/proxy/utils/Initializable.sol index 43f82feca3c..7f33a6267c0 100644 --- a/contracts/proxy/utils/Initializable.sol +++ b/contracts/proxy/utils/Initializable.sol @@ -1,7 +1,7 @@ // SPDX-License-Identifier: MIT // OpenZeppelin Contracts (last updated v4.9.0) (proxy/utils/Initializable.sol) -pragma solidity ^0.8.19; +pragma solidity ^0.8.20; /** * @dev This is a base contract to aid in writing upgradeable contracts, or any kind of contract that will be deployed @@ -55,14 +55,27 @@ pragma solidity ^0.8.19; */ abstract contract Initializable { /** - * @dev Indicates that the contract has been initialized. + * @dev Storage of the initializable contract. + * + * It's implemented on a custom ERC-7201 namespace to reduce the risk of storage collisions + * when using with upgradeable contracts. + * + * @custom:storage-location erc7201:openzeppelin.storage.Initializable */ - uint8 private _initialized; + struct InitializableStorage { + /** + * @dev Indicates that the contract has been initialized. + */ + uint64 _initialized; + /** + * @dev Indicates that the contract is in the process of being initialized. + */ + bool _initializing; + } - /** - * @dev Indicates that the contract is in the process of being initialized. - */ - bool private _initializing; + // keccak256(abi.encode(uint256(keccak256("openzeppelin.storage.Initializable")) - 1)) + bytes32 private constant _INITIALIZABLE_STORAGE = + 0xf0c57e16840df040f15088dc2f81fe391c3923bec73e23a9662efc9c229c6a0e; /** * @dev The contract is already initialized. @@ -77,7 +90,7 @@ abstract contract Initializable { /** * @dev Triggered when the contract has been initialized or reinitialized. */ - event Initialized(uint8 version); + event Initialized(uint64 version); /** * @dev A modifier that defines a protected initializer function that can be invoked at most once. In its scope, @@ -89,17 +102,20 @@ abstract contract Initializable { * Emits an {Initialized} event. */ modifier initializer() { - bool isTopLevelCall = !_initializing; - if (!(isTopLevelCall && _initialized < 1) && !(address(this).code.length == 0 && _initialized == 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; + $._initialized = 1; if (isTopLevelCall) { - _initializing = true; + $._initializing = true; } _; if (isTopLevelCall) { - _initializing = false; + $._initializing = false; emit Initialized(1); } } @@ -122,14 +138,17 @@ abstract contract Initializable { * * Emits an {Initialized} event. */ - modifier reinitializer(uint8 version) { - if (_initializing || _initialized >= version) { + modifier reinitializer(uint64 version) { + // solhint-disable-next-line var-name-mixedcase + InitializableStorage storage $ = _getInitializableStorage(); + + if ($._initializing || $._initialized >= version) { revert AlreadyInitialized(); } - _initialized = version; - _initializing = true; + $._initialized = version; + $._initializing = true; _; - _initializing = false; + $._initializing = false; emit Initialized(version); } @@ -146,7 +165,7 @@ abstract contract Initializable { * @dev Reverts if the contract is not in an initializing state. See {onlyInitializing}. */ function _checkInitializing() internal view virtual { - if (!_initializing) { + if (!_isInitializing()) { revert NotInitializing(); } } @@ -160,26 +179,39 @@ abstract contract Initializable { * Emits an {Initialized} event the first time it is successfully executed. */ function _disableInitializers() internal virtual { - if (_initializing) { + // solhint-disable-next-line var-name-mixedcase + InitializableStorage storage $ = _getInitializableStorage(); + + if ($._initializing) { revert AlreadyInitialized(); } - if (_initialized != type(uint8).max) { - _initialized = type(uint8).max; - emit Initialized(type(uint8).max); + if ($._initialized != type(uint64).max) { + $._initialized = type(uint64).max; + emit Initialized(type(uint64).max); } } /** * @dev Returns the highest version that has been initialized. See {reinitializer}. */ - function _getInitializedVersion() internal view returns (uint8) { - return _initialized; + function _getInitializedVersion() internal view returns (uint64) { + return _getInitializableStorage()._initialized; } /** * @dev Returns `true` if the contract is currently initializing. See {onlyInitializing}. */ function _isInitializing() internal view returns (bool) { - return _initializing; + return _getInitializableStorage()._initializing; + } + + /** + * @dev Returns a pointer to the storage namespace. + */ + // solhint-disable-next-line var-name-mixedcase + function _getInitializableStorage() private pure returns (InitializableStorage storage $) { + assembly { + $.slot := _INITIALIZABLE_STORAGE + } } } diff --git a/test/helpers/constants.js b/test/helpers/constants.js new file mode 100644 index 00000000000..0f4d028cfce --- /dev/null +++ b/test/helpers/constants.js @@ -0,0 +1,7 @@ +const MAX_UINT48 = web3.utils.toBN(1).shln(48).subn(1).toString(); +const MAX_UINT64 = web3.utils.toBN(1).shln(64).subn(1).toString(); + +module.exports = { + MAX_UINT48, + MAX_UINT64, +}; diff --git a/test/metatx/ERC2771Context.test.js b/test/metatx/ERC2771Context.test.js index 0ec8d98dde6..678deb5addb 100644 --- a/test/metatx/ERC2771Context.test.js +++ b/test/metatx/ERC2771Context.test.js @@ -1,6 +1,7 @@ const ethSigUtil = require('eth-sig-util'); const Wallet = require('ethereumjs-wallet').default; const { getDomain, domainType } = require('../helpers/eip712'); +const { MAX_UINT48 } = require('../helpers/constants'); const { expectEvent } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); @@ -14,8 +15,6 @@ const { shouldBehaveLikeRegularContext } = require('../utils/Context.behavior'); contract('ERC2771Context', function (accounts) { const [, trustedForwarder] = accounts; - const MAX_UINT48 = web3.utils.toBN(1).shln(48).subn(1).toString(); - beforeEach(async function () { this.forwarder = await ERC2771Forwarder.new('ERC2771Forwarder'); this.recipient = await ERC2771ContextMock.new(this.forwarder.address); diff --git a/test/proxy/utils/Initializable.test.js b/test/proxy/utils/Initializable.test.js index e3e0fc02f77..98ed19d36a2 100644 --- a/test/proxy/utils/Initializable.test.js +++ b/test/proxy/utils/Initializable.test.js @@ -1,6 +1,7 @@ const { expectEvent } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); const { expectRevertCustomError } = require('../../helpers/customError'); +const { MAX_UINT64 } = require('../../helpers/constants'); const InitializableMock = artifacts.require('InitializableMock'); const ConstructorInitializableMock = artifacts.require('ConstructorInitializableMock'); @@ -213,7 +214,7 @@ contract('Initializable', function () { 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: '255' }); + await expectEvent.inConstruction(ok, 'Initialized', { version: MAX_UINT64 }); }); }); });