Browse Source

Restrict upgrade to proxy context in UUPSUpgradeable

Co-authored-by: Francisco Giordano <frangio.1@gmail.com>
Hadrien Croubois 4 years ago
parent
commit
6241995ad3
2 changed files with 23 additions and 3 deletions
  1. 4 0
      CHANGELOG.md
  2. 19 3
      contracts/proxy/utils/UUPSUpgradeable.sol

+ 4 - 0
CHANGELOG.md

@@ -6,6 +6,10 @@
  * `AccessControl`: add internal `_grantRole(bytes32,address)` and `_revokeRole(bytes32,address)`. ([#2568](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#2568))
  * `AccessControl`: add internal `_grantRole(bytes32,address)` and `_revokeRole(bytes32,address)`. ([#2568](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#2568))
  * `AccessControl`: mark `_setupRole(bytes32,address)` as deprecated in favor of `_grantRole(bytes32,address)`. ([#2568](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#2568))
  * `AccessControl`: mark `_setupRole(bytes32,address)` as deprecated in favor of `_grantRole(bytes32,address)`. ([#2568](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#2568))
 
 
+## 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
 ## 4.3.1
 
 
  * `TimelockController`: Add additional isOperationReady check.
  * `TimelockController`: Add additional isOperationReady check.

+ 19 - 3
contracts/proxy/utils/UUPSUpgradeable.sol

@@ -17,6 +17,22 @@ import "../ERC1967/ERC1967Upgrade.sol";
  * _Available since v4.1._
  * _Available since v4.1._
  */
  */
 abstract contract UUPSUpgradeable is ERC1967Upgrade {
 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`.
      * @dev Upgrade the implementation of the proxy to `newImplementation`.
      *
      *
@@ -24,9 +40,9 @@ abstract contract UUPSUpgradeable is ERC1967Upgrade {
      *
      *
      * Emits an {Upgraded} event.
      * Emits an {Upgraded} event.
      */
      */
-    function upgradeTo(address newImplementation) external virtual {
+    function upgradeTo(address newImplementation) external virtual onlyProxy {
         _authorizeUpgrade(newImplementation);
         _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.
      * 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);
         _authorizeUpgrade(newImplementation);
         _upgradeToAndCallSecure(newImplementation, data, true);
         _upgradeToAndCallSecure(newImplementation, data, true);
     }
     }