From ff85c7b0eb90c85f080a05b65e2b335104ff3923 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 15 Jun 2023 22:01:04 +0200 Subject: [PATCH] Make ERC1967Upgrades a library instead of an abstract contract (#4325) --- .changeset/grumpy-worms-tease.md | 5 ++ contracts/mocks/proxy/UUPSLegacy.sol | 15 ++-- contracts/mocks/proxy/UUPSUpgradeableMock.sol | 4 +- contracts/proxy/ERC1967/ERC1967Proxy.sol | 8 +- .../{ERC1967Upgrade.sol => ERC1967Utils.sol} | 80 ++++++++++++------- contracts/proxy/README.adoc | 4 +- contracts/proxy/beacon/BeaconProxy.sol | 8 +- .../TransparentUpgradeableProxy.sol | 10 +-- contracts/proxy/utils/UUPSUpgradeable.sol | 12 +-- hardhat.config.js | 2 +- scripts/upgradeable/transpile.sh | 2 +- 11 files changed, 89 insertions(+), 61 deletions(-) create mode 100644 .changeset/grumpy-worms-tease.md rename contracts/proxy/ERC1967/{ERC1967Upgrade.sol => ERC1967Utils.sol} (65%) diff --git a/.changeset/grumpy-worms-tease.md b/.changeset/grumpy-worms-tease.md new file mode 100644 index 00000000000..910b996c604 --- /dev/null +++ b/.changeset/grumpy-worms-tease.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': major +--- + +`ERC1967Utils`: Refactor the `ERC1967Upgrade` abstract contract as a library. diff --git a/contracts/mocks/proxy/UUPSLegacy.sol b/contracts/mocks/proxy/UUPSLegacy.sol index f8ea7214ba8..6812f39b7ed 100644 --- a/contracts/mocks/proxy/UUPSLegacy.sol +++ b/contracts/mocks/proxy/UUPSLegacy.sol @@ -7,18 +7,18 @@ import "./UUPSUpgradeableMock.sol"; // This contract implements the pre-4.5 UUPS upgrade function with a rollback test. // It's used to test that newer UUPS contracts are considered valid upgrades by older UUPS contracts. contract UUPSUpgradeableLegacyMock is UUPSUpgradeableMock { - // Inlined from ERC1967Upgrade + // Inlined from ERC1967Utils bytes32 private constant _ROLLBACK_SLOT = 0x4910fdfa16fed3260ed0e7147f7cc6da11a60208b5b9406d12a635614ffd9143; - // ERC1967Upgrade._setImplementation is private so we reproduce it here. + // ERC1967Utils._setImplementation is private so we reproduce it here. // An extra underscore prevents a name clash error. function __setImplementation(address newImplementation) private { require(newImplementation.code.length > 0, "ERC1967: new implementation is not a contract"); - StorageSlot.getAddressSlot(_IMPLEMENTATION_SLOT).value = newImplementation; + StorageSlot.getAddressSlot(ERC1967Utils.IMPLEMENTATION_SLOT).value = newImplementation; } function _upgradeToAndCallSecureLegacyV1(address newImplementation, bytes memory data, bool forceCall) internal { - address oldImplementation = _getImplementation(); + address oldImplementation = ERC1967Utils.getImplementation(); // Initial upgrade and setup call __setImplementation(newImplementation); @@ -34,9 +34,12 @@ contract UUPSUpgradeableLegacyMock is UUPSUpgradeableMock { Address.functionDelegateCall(newImplementation, abi.encodeCall(this.upgradeTo, (oldImplementation))); rollbackTesting.value = false; // Check rollback was effective - require(oldImplementation == _getImplementation(), "ERC1967Upgrade: upgrade breaks further upgrades"); + require( + oldImplementation == ERC1967Utils.getImplementation(), + "ERC1967Utils: upgrade breaks further upgrades" + ); // Finally reset to the new implementation and log the upgrade - _upgradeTo(newImplementation); + ERC1967Utils.upgradeTo(newImplementation); } } diff --git a/contracts/mocks/proxy/UUPSUpgradeableMock.sol b/contracts/mocks/proxy/UUPSUpgradeableMock.sol index 60eed4c9397..81af9649080 100644 --- a/contracts/mocks/proxy/UUPSUpgradeableMock.sol +++ b/contracts/mocks/proxy/UUPSUpgradeableMock.sol @@ -23,10 +23,10 @@ contract UUPSUpgradeableMock is NonUpgradeableMock, UUPSUpgradeable { contract UUPSUpgradeableUnsafeMock is UUPSUpgradeableMock { function upgradeTo(address newImplementation) public override { - _upgradeToAndCall(newImplementation, bytes(""), false); + ERC1967Utils.upgradeToAndCall(newImplementation, bytes(""), false); } function upgradeToAndCall(address newImplementation, bytes memory data) public payable override { - _upgradeToAndCall(newImplementation, data, false); + ERC1967Utils.upgradeToAndCall(newImplementation, data, false); } } diff --git a/contracts/proxy/ERC1967/ERC1967Proxy.sol b/contracts/proxy/ERC1967/ERC1967Proxy.sol index ea5c204f867..5a752f13d8b 100644 --- a/contracts/proxy/ERC1967/ERC1967Proxy.sol +++ b/contracts/proxy/ERC1967/ERC1967Proxy.sol @@ -4,7 +4,7 @@ pragma solidity ^0.8.19; import "../Proxy.sol"; -import "./ERC1967Upgrade.sol"; +import "./ERC1967Utils.sol"; /** * @dev This contract implements an upgradeable proxy. It is upgradeable because calls are delegated to an @@ -12,7 +12,7 @@ import "./ERC1967Upgrade.sol"; * https://eips.ethereum.org/EIPS/eip-1967[EIP1967], so that it doesn't conflict with the storage layout of the * implementation behind the proxy. */ -contract ERC1967Proxy is Proxy, ERC1967Upgrade { +contract ERC1967Proxy is Proxy { /** * @dev Initializes the upgradeable proxy with an initial implementation specified by `_logic`. * @@ -20,7 +20,7 @@ contract ERC1967Proxy is Proxy, ERC1967Upgrade { * function call, and allows initializing the storage of the proxy like a Solidity constructor. */ constructor(address _logic, bytes memory _data) payable { - _upgradeToAndCall(_logic, _data, false); + ERC1967Utils.upgradeToAndCall(_logic, _data, false); } /** @@ -31,6 +31,6 @@ contract ERC1967Proxy is Proxy, ERC1967Upgrade { * `0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc` */ function _implementation() internal view virtual override returns (address impl) { - return _getImplementation(); + return ERC1967Utils.getImplementation(); } } diff --git a/contracts/proxy/ERC1967/ERC1967Upgrade.sol b/contracts/proxy/ERC1967/ERC1967Utils.sol similarity index 65% rename from contracts/proxy/ERC1967/ERC1967Upgrade.sol rename to contracts/proxy/ERC1967/ERC1967Utils.sol index ca6b92580f0..55f5c6aabac 100644 --- a/contracts/proxy/ERC1967/ERC1967Upgrade.sol +++ b/contracts/proxy/ERC1967/ERC1967Utils.sol @@ -1,7 +1,7 @@ // SPDX-License-Identifier: MIT -// OpenZeppelin Contracts (last updated v4.9.0) (proxy/ERC1967/ERC1967Upgrade.sol) +// OpenZeppelin Contracts (last updated v4.9.0) (proxy/ERC1967/ERC1967Utils.sol) -pragma solidity ^0.8.19; +pragma solidity ^0.8.20; import "../beacon/IBeacon.sol"; import "../../interfaces/IERC1967.sol"; @@ -15,7 +15,24 @@ import "../../utils/StorageSlot.sol"; * * _Available since v4.1._ */ -abstract contract ERC1967Upgrade is IERC1967 { +library ERC1967Utils { + // We re-declare ERC-1967 events here because they can't be used directly from IERC1967. + // This will be fixed in Solidity 0.8.21. At that point we should remove these events. + /** + * @dev Emitted when the implementation is upgraded. + */ + event Upgraded(address indexed implementation); + + /** + * @dev Emitted when the admin account has changed. + */ + event AdminChanged(address previousAdmin, address newAdmin); + + /** + * @dev Emitted when the beacon is changed. + */ + event BeaconUpgraded(address indexed beacon); + // This is the keccak-256 hash of "eip1967.proxy.rollback" subtracted by 1 bytes32 private constant _ROLLBACK_SLOT = 0x4910fdfa16fed3260ed0e7147f7cc6da11a60208b5b9406d12a635614ffd9143; @@ -24,7 +41,8 @@ abstract contract ERC1967Upgrade is IERC1967 { * This is the keccak-256 hash of "eip1967.proxy.implementation" subtracted by 1, and is * validated in the constructor. */ - bytes32 internal constant _IMPLEMENTATION_SLOT = 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc; + // solhint-disable-next-line private-vars-leading-underscore + bytes32 internal constant IMPLEMENTATION_SLOT = 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc; /** * @dev The `implementation` of the proxy is invalid. @@ -49,8 +67,8 @@ abstract contract ERC1967Upgrade is IERC1967 { /** * @dev Returns the current implementation address. */ - function _getImplementation() internal view returns (address) { - return StorageSlot.getAddressSlot(_IMPLEMENTATION_SLOT).value; + function getImplementation() internal view returns (address) { + return StorageSlot.getAddressSlot(IMPLEMENTATION_SLOT).value; } /** @@ -60,15 +78,15 @@ abstract contract ERC1967Upgrade is IERC1967 { if (newImplementation.code.length == 0) { revert ERC1967InvalidImplementation(newImplementation); } - StorageSlot.getAddressSlot(_IMPLEMENTATION_SLOT).value = newImplementation; + StorageSlot.getAddressSlot(IMPLEMENTATION_SLOT).value = newImplementation; } /** * @dev Perform implementation upgrade * - * Emits an {Upgraded} event. + * Emits an {IERC1967-Upgraded} event. */ - function _upgradeTo(address newImplementation) internal { + function upgradeTo(address newImplementation) internal { _setImplementation(newImplementation); emit Upgraded(newImplementation); } @@ -76,10 +94,10 @@ abstract contract ERC1967Upgrade is IERC1967 { /** * @dev Perform implementation upgrade with additional setup call. * - * Emits an {Upgraded} event. + * Emits an {IERC1967-Upgraded} event. */ - function _upgradeToAndCall(address newImplementation, bytes memory data, bool forceCall) internal { - _upgradeTo(newImplementation); + function upgradeToAndCall(address newImplementation, bytes memory data, bool forceCall) internal { + upgradeTo(newImplementation); if (data.length > 0 || forceCall) { Address.functionDelegateCall(newImplementation, data); } @@ -88,9 +106,9 @@ abstract contract ERC1967Upgrade is IERC1967 { /** * @dev Perform implementation upgrade with security checks for UUPS proxies, and additional setup call. * - * Emits an {Upgraded} event. + * Emits an {IERC1967-Upgraded} event. */ - function _upgradeToAndCallUUPS(address newImplementation, bytes memory data, bool forceCall) internal { + function upgradeToAndCallUUPS(address newImplementation, bytes memory data, bool forceCall) internal { // Upgrades from old implementations will perform a rollback test. This test requires the new // implementation to upgrade back to the old, non-ERC1822 compliant, implementation. Removing // this special case will break upgrade paths from old UUPS implementation to new ones. @@ -98,14 +116,14 @@ abstract contract ERC1967Upgrade is IERC1967 { _setImplementation(newImplementation); } else { try IERC1822Proxiable(newImplementation).proxiableUUID() returns (bytes32 slot) { - if (slot != _IMPLEMENTATION_SLOT) { + if (slot != IMPLEMENTATION_SLOT) { revert ERC1967UnsupportedProxiableUUID(slot); } } catch { // The implementation is not UUPS revert ERC1967InvalidImplementation(newImplementation); } - _upgradeToAndCall(newImplementation, data, forceCall); + upgradeToAndCall(newImplementation, data, forceCall); } } @@ -114,7 +132,8 @@ abstract contract ERC1967Upgrade is IERC1967 { * This is the keccak-256 hash of "eip1967.proxy.admin" subtracted by 1, and is * validated in the constructor. */ - bytes32 internal constant _ADMIN_SLOT = 0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103; + // solhint-disable-next-line private-vars-leading-underscore + bytes32 internal constant ADMIN_SLOT = 0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103; /** * @dev Returns the current admin. @@ -123,8 +142,8 @@ abstract contract ERC1967Upgrade is IERC1967 { * https://eth.wiki/json-rpc/API#eth_getstorageat[`eth_getStorageAt`] RPC call. * `0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103` */ - function _getAdmin() internal view returns (address) { - return StorageSlot.getAddressSlot(_ADMIN_SLOT).value; + function getAdmin() internal view returns (address) { + return StorageSlot.getAddressSlot(ADMIN_SLOT).value; } /** @@ -134,30 +153,31 @@ abstract contract ERC1967Upgrade is IERC1967 { if (newAdmin == address(0)) { revert ERC1967InvalidAdmin(address(0)); } - StorageSlot.getAddressSlot(_ADMIN_SLOT).value = newAdmin; + StorageSlot.getAddressSlot(ADMIN_SLOT).value = newAdmin; } /** * @dev Changes the admin of the proxy. * - * Emits an {AdminChanged} event. + * Emits an {IERC1967-AdminChanged} event. */ - function _changeAdmin(address newAdmin) internal { - emit AdminChanged(_getAdmin(), newAdmin); + function changeAdmin(address newAdmin) internal { + emit AdminChanged(getAdmin(), newAdmin); _setAdmin(newAdmin); } /** * @dev The storage slot of the UpgradeableBeacon contract which defines the implementation for this proxy. - * This is bytes32(uint256(keccak256('eip1967.proxy.beacon')) - 1)) and is validated in the constructor. + * This is bytes32(uint256(keccak256('eip1967.proxy.beacon')) - 1) and is validated in the constructor. */ - bytes32 internal constant _BEACON_SLOT = 0xa3f0ad74e5423aebfd80d3ef4346578335a9a72aeaee59ff6cb3582b35133d50; + // solhint-disable-next-line private-vars-leading-underscore + bytes32 internal constant BEACON_SLOT = 0xa3f0ad74e5423aebfd80d3ef4346578335a9a72aeaee59ff6cb3582b35133d50; /** * @dev Returns the current beacon. */ - function _getBeacon() internal view returns (address) { - return StorageSlot.getAddressSlot(_BEACON_SLOT).value; + function getBeacon() internal view returns (address) { + return StorageSlot.getAddressSlot(BEACON_SLOT).value; } /** @@ -173,16 +193,16 @@ abstract contract ERC1967Upgrade is IERC1967 { revert ERC1967InvalidImplementation(beaconImplementation); } - StorageSlot.getAddressSlot(_BEACON_SLOT).value = newBeacon; + StorageSlot.getAddressSlot(BEACON_SLOT).value = newBeacon; } /** * @dev Perform beacon upgrade with additional setup call. Note: This upgrades the address of the beacon, it does * not upgrade the implementation contained in the beacon (see {UpgradeableBeacon-_setImplementation} for that). * - * Emits a {BeaconUpgraded} event. + * Emits an {IERC1967-BeaconUpgraded} event. */ - function _upgradeBeaconToAndCall(address newBeacon, bytes memory data, bool forceCall) internal { + function upgradeBeaconToAndCall(address newBeacon, bytes memory data, bool forceCall) internal { _setBeacon(newBeacon); emit BeaconUpgraded(newBeacon); if (data.length > 0 || forceCall) { diff --git a/contracts/proxy/README.adoc b/contracts/proxy/README.adoc index 89717a7bf73..43b7f65b134 100644 --- a/contracts/proxy/README.adoc +++ b/contracts/proxy/README.adoc @@ -11,7 +11,7 @@ Most of the proxies below are built on an abstract base contract. In order to avoid clashes with the storage variables of the implementation contract behind a proxy, we use https://eips.ethereum.org/EIPS/eip-1967[EIP1967] storage slots. -- {ERC1967Upgrade}: Internal functions to get and set the storage slots defined in EIP1967. +- {ERC1967Utils}: Internal functions to get and set the storage slots defined in EIP1967. - {ERC1967Proxy}: A proxy using EIP1967 storage slots. Not upgradeable by default. There are two alternative ways to add upgradeability to an ERC1967 proxy. Their differences are explained below in <>. @@ -60,7 +60,7 @@ The current implementation of this security mechanism uses https://eips.ethereum {{ERC1967Proxy}} -{{ERC1967Upgrade}} +{{ERC1967Utils}} == Transparent Proxy diff --git a/contracts/proxy/beacon/BeaconProxy.sol b/contracts/proxy/beacon/BeaconProxy.sol index d603e38a7f7..dc6b5c90b67 100644 --- a/contracts/proxy/beacon/BeaconProxy.sol +++ b/contracts/proxy/beacon/BeaconProxy.sol @@ -5,7 +5,7 @@ pragma solidity ^0.8.19; import "./IBeacon.sol"; import "../Proxy.sol"; -import "../ERC1967/ERC1967Upgrade.sol"; +import "../ERC1967/ERC1967Utils.sol"; /** * @dev This contract implements a proxy that gets the implementation address for each call from an {UpgradeableBeacon}. @@ -15,7 +15,7 @@ import "../ERC1967/ERC1967Upgrade.sol"; * * _Available since v3.4._ */ -contract BeaconProxy is Proxy, ERC1967Upgrade { +contract BeaconProxy is Proxy { /** * @dev Initializes the proxy with `beacon`. * @@ -28,13 +28,13 @@ contract BeaconProxy is Proxy, ERC1967Upgrade { * - `beacon` must be a contract with the interface {IBeacon}. */ constructor(address beacon, bytes memory data) payable { - _upgradeBeaconToAndCall(beacon, data, false); + ERC1967Utils.upgradeBeaconToAndCall(beacon, data, false); } /** * @dev Returns the current implementation address of the associated beacon. */ function _implementation() internal view virtual override returns (address) { - return IBeacon(_getBeacon()).implementation(); + return IBeacon(ERC1967Utils.getBeacon()).implementation(); } } diff --git a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol index 4536e28c881..2b022ccad9f 100644 --- a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol +++ b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol @@ -67,14 +67,14 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { * optionally initialized with `_data` as explained in {ERC1967Proxy-constructor}. */ constructor(address _logic, address admin_, bytes memory _data) payable ERC1967Proxy(_logic, _data) { - _changeAdmin(admin_); + ERC1967Utils.changeAdmin(admin_); } /** * @dev If caller is the admin process the call internally, otherwise transparently fallback to the proxy behavior */ function _fallback() internal virtual override { - if (msg.sender == _getAdmin()) { + if (msg.sender == ERC1967Utils.getAdmin()) { bytes memory ret; bytes4 selector = msg.sig; if (selector == ITransparentUpgradeableProxy.upgradeTo.selector) { @@ -103,7 +103,7 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { _requireZeroValue(); address newAdmin = abi.decode(msg.data[4:], (address)); - _changeAdmin(newAdmin); + ERC1967Utils.changeAdmin(newAdmin); return ""; } @@ -115,7 +115,7 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { _requireZeroValue(); address newImplementation = abi.decode(msg.data[4:], (address)); - _upgradeToAndCall(newImplementation, bytes(""), false); + ERC1967Utils.upgradeToAndCall(newImplementation, bytes(""), false); return ""; } @@ -127,7 +127,7 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { */ function _dispatchUpgradeToAndCall() private returns (bytes memory) { (address newImplementation, bytes memory data) = abi.decode(msg.data[4:], (address, bytes)); - _upgradeToAndCall(newImplementation, data, true); + ERC1967Utils.upgradeToAndCall(newImplementation, data, true); return ""; } diff --git a/contracts/proxy/utils/UUPSUpgradeable.sol b/contracts/proxy/utils/UUPSUpgradeable.sol index 41a72ef9cd9..d213f2d6164 100644 --- a/contracts/proxy/utils/UUPSUpgradeable.sol +++ b/contracts/proxy/utils/UUPSUpgradeable.sol @@ -4,7 +4,7 @@ pragma solidity ^0.8.19; import "../../interfaces/draft-IERC1822.sol"; -import "../ERC1967/ERC1967Upgrade.sol"; +import "../ERC1967/ERC1967Utils.sol"; /** * @dev An upgradeability mechanism designed for UUPS proxies. The functions included here can perform an upgrade of an @@ -18,7 +18,7 @@ import "../ERC1967/ERC1967Upgrade.sol"; * * _Available since v4.1._ */ -abstract contract UUPSUpgradeable is IERC1822Proxiable, ERC1967Upgrade { +abstract contract UUPSUpgradeable is IERC1822Proxiable { /// @custom:oz-upgrades-unsafe-allow state-variable-immutable state-variable-assignment address private immutable __self = address(this); @@ -39,7 +39,7 @@ abstract contract UUPSUpgradeable is IERC1822Proxiable, ERC1967Upgrade { // Must be called through delegatecall revert UUPSUnauthorizedCallContext(); } - if (_getImplementation() != __self) { + if (ERC1967Utils.getImplementation() != __self) { // Must be called through an active proxy revert UUPSUnauthorizedCallContext(); } @@ -67,7 +67,7 @@ abstract contract UUPSUpgradeable is IERC1822Proxiable, ERC1967Upgrade { * function revert if invoked through a proxy. This is guaranteed by the `notDelegated` modifier. */ function proxiableUUID() external view virtual notDelegated returns (bytes32) { - return _IMPLEMENTATION_SLOT; + return ERC1967Utils.IMPLEMENTATION_SLOT; } /** @@ -81,7 +81,7 @@ abstract contract UUPSUpgradeable is IERC1822Proxiable, ERC1967Upgrade { */ function upgradeTo(address newImplementation) public virtual onlyProxy { _authorizeUpgrade(newImplementation); - _upgradeToAndCallUUPS(newImplementation, new bytes(0), false); + ERC1967Utils.upgradeToAndCallUUPS(newImplementation, new bytes(0), false); } /** @@ -96,7 +96,7 @@ abstract contract UUPSUpgradeable is IERC1822Proxiable, ERC1967Upgrade { */ function upgradeToAndCall(address newImplementation, bytes memory data) public payable virtual onlyProxy { _authorizeUpgrade(newImplementation); - _upgradeToAndCallUUPS(newImplementation, data, true); + ERC1967Utils.upgradeToAndCallUUPS(newImplementation, data, true); } /** diff --git a/hardhat.config.js b/hardhat.config.js index 66367e96e02..6cb8b91441f 100644 --- a/hardhat.config.js +++ b/hardhat.config.js @@ -40,7 +40,7 @@ const argv = require('yargs/yargs')() compiler: { alias: 'compileVersion', type: 'string', - default: '0.8.19', + default: '0.8.20', }, coinmarketcap: { alias: 'coinmarketcapApiKey', diff --git a/scripts/upgradeable/transpile.sh b/scripts/upgradeable/transpile.sh index e3aa31cc03c..fbffe844e34 100644 --- a/scripts/upgradeable/transpile.sh +++ b/scripts/upgradeable/transpile.sh @@ -29,7 +29,7 @@ npx @openzeppelin/upgrade-safe-transpiler@latest -D \ -x 'contracts/proxy/**/*' \ -x '!contracts/proxy/Clones.sol' \ -x '!contracts/proxy/ERC1967/ERC1967Storage.sol' \ - -x '!contracts/proxy/ERC1967/ERC1967Upgrade.sol' \ + -x '!contracts/proxy/ERC1967/ERC1967Utils.sol' \ -x '!contracts/proxy/utils/UUPSUpgradeable.sol' \ -x '!contracts/proxy/beacon/IBeacon.sol' \ -p 'contracts/**/presets/**/*'