From 0ec7f4c25d60437e0ae5bb72cb4549a58e53a387 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 24 Aug 2021 16:09:39 -0300 Subject: [PATCH 1/4] Add additional isOperationReady check in TimelockController (cherry picked from commit cec4f2ef57495d8b1742d62846da212515d99dd5) --- CHANGELOG.md | 4 ++++ contracts/governance/TimelockController.sol | 7 ++++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 53b3d4580..7407e25ac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## 4.3.1 + + * `TimelockController`: Add additional isOperationReady check. + ## 4.3.0 (2021-08-17) * `ERC2771Context`: use private variable from storage to store the forwarder address. Fixes issues where `_msgSender()` was not callable from constructors. ([#2754](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2754)) diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index 10aeb4ee4..e52cb0d3f 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -268,7 +268,7 @@ contract TimelockController is AccessControl { bytes32 salt ) public payable virtual onlyRoleOrOpenRole(EXECUTOR_ROLE) { bytes32 id = hashOperation(target, value, data, predecessor, salt); - _beforeCall(predecessor); + _beforeCall(id, predecessor); _call(id, 0, target, value, data); _afterCall(id); } @@ -293,7 +293,7 @@ contract TimelockController is AccessControl { require(targets.length == datas.length, "TimelockController: length mismatch"); bytes32 id = hashOperationBatch(targets, values, datas, predecessor, salt); - _beforeCall(predecessor); + _beforeCall(id, predecessor); for (uint256 i = 0; i < targets.length; ++i) { _call(id, i, targets[i], values[i], datas[i]); } @@ -303,7 +303,8 @@ contract TimelockController is AccessControl { /** * @dev Checks before execution of an operation's calls. */ - function _beforeCall(bytes32 predecessor) private view { + function _beforeCall(bytes32 id, bytes32 predecessor) private view { + require(isOperationReady(id), "TimelockController: operation is not ready"); require(predecessor == bytes32(0) || isOperationDone(predecessor), "TimelockController: missing dependency"); } From 6edb6dd1ca43d05a762d84c688116b3327f5e490 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 26 Aug 2021 17:59:03 -0300 Subject: [PATCH 2/4] 4.3.1 --- CHANGELOG.md | 2 +- contracts/package.json | 2 +- package-lock.json | 4 ++-- package.json | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7407e25ac..a6976a94d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ # Changelog -## 4.3.1 +## 4.3.1 (2021-08-26) * `TimelockController`: Add additional isOperationReady check. diff --git a/contracts/package.json b/contracts/package.json index 1a4cbba9f..654d0f820 100644 --- a/contracts/package.json +++ b/contracts/package.json @@ -1,7 +1,7 @@ { "name": "@openzeppelin/contracts", "description": "Secure Smart Contract library for Solidity", - "version": "4.3.0", + "version": "4.3.1", "files": [ "**/*.sol", "/build/contracts/*.json", diff --git a/package-lock.json b/package-lock.json index b378116d9..c267c0a60 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "openzeppelin-solidity", - "version": "4.3.0", + "version": "4.3.1", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "openzeppelin-solidity", - "version": "4.3.0-rc.0", + "version": "4.3.1", "license": "MIT", "bin": { "openzeppelin-contracts-migrate-imports": "scripts/migrate-imports.js" diff --git a/package.json b/package.json index 8cd7c1b09..b23f0a1c7 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "openzeppelin-solidity", "description": "Secure Smart Contract library for Solidity", - "version": "4.3.0", + "version": "4.3.1", "files": [ "/contracts/**/*.sol", "/build/contracts/*.json", From 024cc50df478d2e8f78539819749e94d6df60592 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 14 Sep 2021 16:50:27 -0300 Subject: [PATCH 3/4] Restrict upgrade to proxy context in UUPSUpgradeable Co-authored-by: Francisco Giordano (cherry picked from commit 6241995ad323952e38f8d405103ed994a2dcde8e) --- CHANGELOG.md | 4 ++++ contracts/proxy/utils/UUPSUpgradeable.sol | 22 +++++++++++++++++++--- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a6976a94d..7fa6e4ad5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## 4.3.2 + + * `UUPSUpgradeable`: Add modifiers to prevent `upgradeTo` and `upgradeToAndCall` being executed on any contract that is not the active ERC1967 proxy. This prevents these functions being called on implementation contracts or minimal ERC1167 clones, in particular. + ## 4.3.1 (2021-08-26) * `TimelockController`: Add additional isOperationReady check. diff --git a/contracts/proxy/utils/UUPSUpgradeable.sol b/contracts/proxy/utils/UUPSUpgradeable.sol index eeb528967..a88b2c23b 100644 --- a/contracts/proxy/utils/UUPSUpgradeable.sol +++ b/contracts/proxy/utils/UUPSUpgradeable.sol @@ -17,6 +17,22 @@ import "../ERC1967/ERC1967Upgrade.sol"; * _Available since v4.1._ */ abstract contract UUPSUpgradeable is ERC1967Upgrade { + /// @custom:oz-upgrades-unsafe-allow state-variable-immutable state-variable-assignment + address private immutable __self = address(this); + + /** + * @dev Check that the execution is being performed through a delegatecall call and that the execution context is + * a proxy contract with an implementation (as defined in ERC1967) pointing to self. This should only be the case + * for UUPS and transparent proxies that are using the current contract as their implementation. Execution of a + * function through ERC1167 minimal proxies (clones) would not normally pass this test, but is not guaranteed to + * fail. + */ + modifier onlyProxy() { + require(address(this) != __self, "Function must be called through delegatecall"); + require(_getImplementation() == __self, "Function must be called through active proxy"); + _; + } + /** * @dev Upgrade the implementation of the proxy to `newImplementation`. * @@ -24,9 +40,9 @@ abstract contract UUPSUpgradeable is ERC1967Upgrade { * * Emits an {Upgraded} event. */ - function upgradeTo(address newImplementation) external virtual { + function upgradeTo(address newImplementation) external virtual onlyProxy { _authorizeUpgrade(newImplementation); - _upgradeToAndCallSecure(newImplementation, bytes(""), false); + _upgradeToAndCallSecure(newImplementation, new bytes(0), false); } /** @@ -37,7 +53,7 @@ abstract contract UUPSUpgradeable is ERC1967Upgrade { * * Emits an {Upgraded} event. */ - function upgradeToAndCall(address newImplementation, bytes memory data) external payable virtual { + function upgradeToAndCall(address newImplementation, bytes memory data) external payable virtual onlyProxy { _authorizeUpgrade(newImplementation); _upgradeToAndCallSecure(newImplementation, data, true); } From 0c4de6721d9668d7b5b2c5a9400fd0b2a5e8de90 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 14 Sep 2021 18:06:39 -0300 Subject: [PATCH 4/4] 4.3.2 --- CHANGELOG.md | 2 +- contracts/package.json | 2 +- package-lock.json | 4 ++-- package.json | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7fa6e4ad5..0fdad388f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ # Changelog -## 4.3.2 +## 4.3.2 (2021-09-14) * `UUPSUpgradeable`: Add modifiers to prevent `upgradeTo` and `upgradeToAndCall` being executed on any contract that is not the active ERC1967 proxy. This prevents these functions being called on implementation contracts or minimal ERC1167 clones, in particular. diff --git a/contracts/package.json b/contracts/package.json index 654d0f820..c8705f0e5 100644 --- a/contracts/package.json +++ b/contracts/package.json @@ -1,7 +1,7 @@ { "name": "@openzeppelin/contracts", "description": "Secure Smart Contract library for Solidity", - "version": "4.3.1", + "version": "4.3.2", "files": [ "**/*.sol", "/build/contracts/*.json", diff --git a/package-lock.json b/package-lock.json index c267c0a60..11b003f02 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "openzeppelin-solidity", - "version": "4.3.1", + "version": "4.3.2", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "openzeppelin-solidity", - "version": "4.3.1", + "version": "4.3.2", "license": "MIT", "bin": { "openzeppelin-contracts-migrate-imports": "scripts/migrate-imports.js" diff --git a/package.json b/package.json index b23f0a1c7..ab358d22f 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "openzeppelin-solidity", "description": "Secure Smart Contract library for Solidity", - "version": "4.3.1", + "version": "4.3.2", "files": [ "/contracts/**/*.sol", "/build/contracts/*.json",