Make TransparentUpgradeableProxy deploy its ProxyAdmin and optimize proxy interfaces

.changeset/empty-taxis-kiss.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': major
+---
+
+`UUPSUpgradeable`, `TransparentUpgradeableProxy` and `ProxyAdmin`: Removed `upgradeTo` and `upgrade` functions, and made `upgradeToAndCall` and `upgradeAndCall` ignore the data argument if it is empty. It is no longer possible to invoke the receive function (or send value with empty data) along with an upgrade.

.changeset/tender-shirts-turn.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': major
+---
+
+`BeaconProxy`: Reject value in initialization unless a payable function is explicitly invoked.

contracts/mocks/UpgreadeableBeaconMock.sol

@@ -0,0 +1,12 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.19;
+
+import {IBeacon} from "../proxy/beacon/IBeacon.sol";
+
+contract UpgradeableBeaconMock is IBeacon {
+    address public implementation;
+
+    constructor(address impl) {
+        implementation = impl;
+    }
+}

contracts/mocks/proxy/ClashingImplementation.sol

@@ -9,7 +9,7 @@ pragma solidity ^0.8.19;
 contract ClashingImplementation {
     event ClashingImplementationCall();
 
-    function upgradeTo(address) external payable {
+    function upgradeToAndCall(address, bytes calldata) external payable {
         emit ClashingImplementationCall();
     }
 

contracts/mocks/proxy/UUPSUpgradeableMock.sol

@@ -23,12 +23,8 @@ contract UUPSUpgradeableMock is NonUpgradeableMock, UUPSUpgradeable {
 }
 
 contract UUPSUpgradeableUnsafeMock is UUPSUpgradeableMock {
-    function upgradeTo(address newImplementation) public override {
-        ERC1967Utils.upgradeToAndCall(newImplementation, bytes(""), false);
-    }
-
     function upgradeToAndCall(address newImplementation, bytes memory data) public payable override {
-        ERC1967Utils.upgradeToAndCall(newImplementation, data, false);
+        ERC1967Utils.upgradeToAndCall(newImplementation, data);
     }
 }
 

contracts/proxy/ERC1967/ERC1967Proxy.sol

@@ -18,9 +18,13 @@ contract ERC1967Proxy is Proxy {
      *
      * If `_data` is nonempty, it's used as data in a delegate call to `_logic`. This will typically be an encoded
      * function call, and allows initializing the storage of the proxy like a Solidity constructor.
+     *
+     * Requirements:
+     *
+     * - If `data` is empty, `msg.value` must be zero.
      */
     constructor(address _logic, bytes memory _data) payable {
-        ERC1967Utils.upgradeToAndCall(_logic, _data, false);
+        ERC1967Utils.upgradeToAndCall(_logic, _data);
     }
 
     /**

contracts/proxy/ERC1967/ERC1967Utils.sol

@@ -52,6 +52,11 @@ library ERC1967Utils {
      */
     error ERC1967InvalidBeacon(address beacon);
 
+    /**
+     * @dev An upgrade function sees `msg.value > 0` that may be lost.
+     */
+    error ERC1967NonPayable();
+
     /**
      * @dev Returns the current implementation address.
      */
@@ -70,24 +75,20 @@ library ERC1967Utils {
     }
 
     /**
-     * @dev Perform implementation upgrade
+     * @dev Performs implementation upgrade with additional setup call if data is nonempty.
+     * This function is payable only if the setup call is performed, otherwise `msg.value` is rejected
+     * to avoid stuck value in the contract.
      *
      * Emits an {IERC1967-Upgraded} event.
      */
-    function upgradeTo(address newImplementation) internal {
+    function upgradeToAndCall(address newImplementation, bytes memory data) internal {
         _setImplementation(newImplementation);
         emit Upgraded(newImplementation);
-    }
 
-    /**
-     * @dev Perform implementation upgrade with additional setup call.
-     *
-     * Emits an {IERC1967-Upgraded} event.
-     */
-    function upgradeToAndCall(address newImplementation, bytes memory data, bool forceCall) internal {
-        upgradeTo(newImplementation);
-        if (data.length > 0 || forceCall) {
+        if (data.length > 0) {
             Address.functionDelegateCall(newImplementation, data);
+        } else {
+            _checkNonPayable();
         }
     }
 
@@ -161,19 +162,33 @@ library ERC1967Utils {
     }
 
     /**
-     * @dev Change the beacon and trigger a setup call.
+     * @dev Change the beacon and trigger a setup call if data is nonempty.
+     * This function is payable only if the setup call is performed, otherwise `msg.value` is rejected
+     * to avoid stuck value in the contract.
      *
      * Emits an {IERC1967-BeaconUpgraded} event.
      *
      * CAUTION: Invoking this function has no effect on an instance of {BeaconProxy} since v5, since
      * it uses an immutable beacon without looking at the value of the ERC-1967 beacon slot for
      * efficiency.
      */
-    function upgradeBeaconToAndCall(address newBeacon, bytes memory data, bool forceCall) internal {
+    function upgradeBeaconToAndCall(address newBeacon, bytes memory data) internal {
         _setBeacon(newBeacon);
         emit BeaconUpgraded(newBeacon);
-        if (data.length > 0 || forceCall) {
+
+        if (data.length > 0) {
             Address.functionDelegateCall(IBeacon(newBeacon).implementation(), data);
+        } else {
+            _checkNonPayable();
+        }
+    }
+
+    /**
+     * @dev Reverts if `msg.value` is not zero.
+     */
+    function _checkNonPayable() private {
+        if (msg.value > 0) {
+            revert ERC1967NonPayable();
         }
     }
 }

contracts/proxy/beacon/BeaconProxy.sol

@@ -34,9 +34,10 @@ contract BeaconProxy is Proxy {
      * Requirements:
      *
      * - `beacon` must be a contract with the interface {IBeacon}.
+     * - If `data` is empty, `msg.value` must be zero.
      */
     constructor(address beacon, bytes memory data) payable {
-        ERC1967Utils.upgradeBeaconToAndCall(beacon, data, false);
+        ERC1967Utils.upgradeBeaconToAndCall(beacon, data);
         _beacon = beacon;
     }
 

contracts/proxy/transparent/ProxyAdmin.sol

@@ -12,28 +12,28 @@ import {Ownable} from "../../access/Ownable.sol";
  */
 contract ProxyAdmin is Ownable {
     /**
-     * @dev Sets the initial owner who can perform upgrades.
+     * @dev The version of the upgrade interface of the contract. If this getter is missing, both `upgrade(address)`
+     * and `upgradeAndCall(address,bytes)` are present, and `upgradeTo` must be used if no function should be called,
+     * while `upgradeAndCall` will invoke the `receive` function if the second argument is the empty byte string.
+     * If the getter returns `"5.0.0"`, only `upgradeAndCall(address,bytes)` is present, and the second argument must
+     * be the empty byte string if no function should be called, being impossible to invoke the `receive` function
+     * during an upgrade.
      */
-    constructor(address initialOwner) Ownable(initialOwner) {}
+    string public constant UPGRADE_INTERFACE_VERSION = "5.0.0";
 
     /**
-     * @dev Upgrades `proxy` to `implementation`. See {TransparentUpgradeableProxy-upgradeTo}.
-     *
-     * Requirements:
-     *
-     * - This contract must be the admin of `proxy`.
+     * @dev Sets the initial owner who can perform upgrades.
      */
-    function upgrade(ITransparentUpgradeableProxy proxy, address implementation) public virtual onlyOwner {
-        proxy.upgradeTo(implementation);
-    }
+    constructor(address initialOwner) Ownable(initialOwner) {}
 
     /**
-     * @dev Upgrades `proxy` to `implementation` and calls a function on the new implementation. See
-     * {TransparentUpgradeableProxy-upgradeToAndCall}.
+     * @dev Upgrades `proxy` to `implementation` and calls a function on the new implementation.
+     * See {TransparentUpgradeableProxy-_dispatchUpgradeToAndCall}.
      *
      * Requirements:
      *
      * - This contract must be the admin of `proxy`.
+     * - If `data` is empty, `msg.value` must be zero.
      */
     function upgradeAndCall(
         ITransparentUpgradeableProxy proxy,

contracts/proxy/transparent/TransparentUpgradeableProxy.sol

@@ -6,45 +6,43 @@ pragma solidity ^0.8.19;
 import {ERC1967Utils} from "../ERC1967/ERC1967Utils.sol";
 import {ERC1967Proxy} from "../ERC1967/ERC1967Proxy.sol";
 import {IERC1967} from "../../interfaces/IERC1967.sol";
+import {ProxyAdmin} from "./ProxyAdmin.sol";
 
 /**
  * @dev Interface for {TransparentUpgradeableProxy}. In order to implement transparency, {TransparentUpgradeableProxy}
- * does not implement this interface directly, and some of its functions are implemented by an internal dispatch
+ * does not implement this interface directly, and its upgradeability mechanism is implemented by an internal dispatch
  * mechanism. The compiler is unaware that these functions are implemented by {TransparentUpgradeableProxy} and will not
  * include them in the ABI so this interface must be used to interact with it.
  */
 interface ITransparentUpgradeableProxy is IERC1967 {
-    function upgradeTo(address) external;
-
-    function upgradeToAndCall(address, bytes memory) external payable;
+    function upgradeToAndCall(address, bytes calldata) external payable;
 }
 
 /**
- * @dev This contract implements a proxy that is upgradeable by an immutable admin.
+ * @dev This contract implements a proxy that is upgradeable through an associated {ProxyAdmin} instance.
  *
  * To avoid https://medium.com/nomic-labs-blog/malicious-backdoors-in-ethereum-proxies-62629adf3357[proxy selector
  * clashing], which can potentially be used in an attack, this contract uses the
  * https://blog.openzeppelin.com/the-transparent-proxy-pattern/[transparent proxy pattern]. This pattern implies two
  * things that go hand in hand:
  *
  * 1. If any account other than the admin calls the proxy, the call will be forwarded to the implementation, even if
- * that call matches one of the admin functions exposed by the proxy itself.
- * 2. If the admin calls the proxy, it can access the admin functions, but its calls will never be forwarded to the
+ * that call matches the {ITransparentUpgradeableProxy-upgradeToAndCall} function exposed by the proxy itself.
+ * 2. If the admin calls the proxy, it can call the `upgradeToAndCall` function but any other call won't be forwarded to the
  * implementation. If the admin tries to call a function on the implementation it will fail with an error indicating the
  * proxy admin cannot fallback to the target implementation.
  *
  * These properties mean that the admin account can only be used for upgrading the proxy, so it's best if it's a dedicated
  * account that is not used for anything else. This will avoid headaches due to sudden errors when trying to call a function
- * from the proxy implementation.
- *
- * Our recommendation is for the dedicated account to be an instance of the {ProxyAdmin} contract. If set up this way,
- * you should think of the `ProxyAdmin` instance as the real administrative interface of your proxy, which extends from the
- * {Ownable} contract to allow for changing the proxy's admin owner.
+ * from the proxy implementation. For this reason, the proxy deploys an instance of {ProxyAdmin} and allows upgrades
+ * only if they come through it.
+ * You should think of the `ProxyAdmin` instance as the administrative interface of the proxy, including the ability to
+ * change who can trigger upgrades by transferring ownership.
  *
  * NOTE: The real interface of this proxy is that defined in `ITransparentUpgradeableProxy`. This contract does not
- * inherit from that interface, and instead the admin functions are implicitly implemented using a custom dispatch
- * mechanism in `_fallback`. Consequently, the compiler will not produce an ABI for this contract. This is necessary to
- * fully implement transparency without decoding reverts caused by selector clashes between the proxy and the
+ * inherit from that interface, and instead `upgradeToAndCall` is implicitly implemented using a custom dispatch mechanism
+ * in `_fallback`. Consequently, the compiler will not produce an ABI for this contract. This is necessary to fully
+ * implement transparency without decoding reverts caused by selector clashes between the proxy and the
  * implementation.
  *
  * IMPORTANT: This contract avoids unnecessary storage reads by setting the admin only during construction as an immutable variable,
@@ -55,10 +53,10 @@ interface ITransparentUpgradeableProxy is IERC1967 {
  * WARNING: It is not recommended to extend this contract to add additional external functions. If you do so, the compiler
  * will not check that there are no selector conflicts, due to the note above. A selector clash between any new function
  * and the functions declared in {ITransparentUpgradeableProxy} will be resolved in favor of the new one. This could
- * render the admin operations inaccessible, which could prevent upgradeability. Transparency may also be compromised.
+ * render the `upgradeToAndCall` function inaccessible, preventing upgradeability and compromising transparency.
  */
 contract TransparentUpgradeableProxy is ERC1967Proxy {
-    // An immutable address for the admin avoid unnecessary SLOADs before each call
+    // An immutable address for the admin to avoid unnecessary SLOADs before each call
     // at the expense of removing the ability to change the admin once it's set.
     // This is acceptable if the admin is always a ProxyAdmin instance or similar contract
     // with its own ability to transfer the permissions to another account.
@@ -70,73 +68,40 @@ contract TransparentUpgradeableProxy is ERC1967Proxy {
     error ProxyDeniedAdminAccess();
 
     /**
-     * @dev msg.value is not 0.
-     */
-    error ProxyNonPayableFunction();
-
-    /**
-     * @dev Initializes an upgradeable proxy managed by `_admin`, backed by the implementation at `_logic`, and
-     * optionally initialized with `_data` as explained in {ERC1967Proxy-constructor}.
+     * @dev Initializes an upgradeable proxy managed by an instance of a {ProxyAdmin} with an `initialOwner`,
+     * backed by the implementation at `_logic`, and optionally initialized with `_data` as explained in
+     * {ERC1967Proxy-constructor}.
      */
-    constructor(address _logic, address admin_, bytes memory _data) payable ERC1967Proxy(_logic, _data) {
-        _admin = admin_;
+    constructor(address _logic, address initialOwner, bytes memory _data) payable ERC1967Proxy(_logic, _data) {
+        _admin = address(new ProxyAdmin(initialOwner));
         // Set the storage value and emit an event for ERC-1967 compatibility
-        ERC1967Utils.changeAdmin(admin_);
+        ERC1967Utils.changeAdmin(_admin);
     }
 
     /**
-     * @dev If caller is the admin process the call internally, otherwise transparently fallback to the proxy behavior
+     * @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 == _admin) {
-            bytes memory ret;
-            bytes4 selector = msg.sig;
-            if (selector == ITransparentUpgradeableProxy.upgradeTo.selector) {
-                ret = _dispatchUpgradeTo();
-            } else if (selector == ITransparentUpgradeableProxy.upgradeToAndCall.selector) {
-                ret = _dispatchUpgradeToAndCall();
+            if (msg.sig == ITransparentUpgradeableProxy.upgradeToAndCall.selector) {
+                _dispatchUpgradeToAndCall();
             } else {
                 revert ProxyDeniedAdminAccess();
             }
-            assembly {
-                return(add(ret, 0x20), mload(ret))
-            }
         } else {
             super._fallback();
         }
     }
 
     /**
-     * @dev Upgrade the implementation of the proxy.
-     */
-    function _dispatchUpgradeTo() private returns (bytes memory) {
-        _requireZeroValue();
-
-        address newImplementation = abi.decode(msg.data[4:], (address));
-        ERC1967Utils.upgradeToAndCall(newImplementation, bytes(""), false);
-
-        return "";
-    }
-
-    /**
-     * @dev Upgrade the implementation of the proxy, and then call a function from the new implementation as specified
-     * by `data`, which should be an encoded function call. This is useful to initialize new storage variables in the
-     * proxied contract.
+     * @dev Upgrade the implementation of the proxy. See {ERC1967Utils-upgradeToAndCall}.
+     *
+     * Requirements:
+     *
+     * - If `data` is empty, `msg.value` must be zero.
      */
-    function _dispatchUpgradeToAndCall() private returns (bytes memory) {
+    function _dispatchUpgradeToAndCall() private {
         (address newImplementation, bytes memory data) = abi.decode(msg.data[4:], (address, bytes));
-        ERC1967Utils.upgradeToAndCall(newImplementation, data, true);
-
-        return "";
-    }
-
-    /**
-     * @dev To keep this contract fully transparent, the fallback is payable. This helper is here to enforce
-     * non-payability of function implemented through dispatchers while still allowing value to pass through.
-     */
-    function _requireZeroValue() private {
-        if (msg.value != 0) {
-            revert ProxyNonPayableFunction();
-        }
+        ERC1967Utils.upgradeToAndCall(newImplementation, data);
     }
 }