Fix TransparentUpgradeableProxy's transparency (#4154)

Co-authored-by: Francisco <fg@frang.io>
Co-authored-by: Ernesto García <ernestognw@gmail.com>

.changeset/thirty-shrimps-mix.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': patch
+---
+
+`TransparentUpgradeableProxy`: Fix transparency in case of selector clash with non-decodable calldata.

contracts/interfaces/IERC1967.sol

@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: MIT
+
+pragma solidity ^0.8.0;
+
+/**
+ * @dev ERC-1967: Proxy Storage Slots. This interface contains the events defined in the ERC.
+ *
+ * _Available since v4.9._
+ */
+interface IERC1967 {
+    /**
+     * @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);
+}

contracts/proxy/ERC1967/ERC1967Upgrade.sol

@@ -4,6 +4,7 @@
 pragma solidity ^0.8.2;
 
 import "../beacon/IBeacon.sol";
+import "../../interfaces/IERC1967.sol";
 import "../../interfaces/draft-IERC1822.sol";
 import "../../utils/Address.sol";
 import "../../utils/StorageSlot.sol";
@@ -14,7 +15,7 @@ import "../../utils/StorageSlot.sol";
  *
  * _Available since v4.1._
  */
-abstract contract ERC1967Upgrade {
+abstract contract ERC1967Upgrade is IERC1967 {
     // This is the keccak-256 hash of "eip1967.proxy.rollback" subtracted by 1
     bytes32 private constant _ROLLBACK_SLOT = 0x4910fdfa16fed3260ed0e7147f7cc6da11a60208b5b9406d12a635614ffd9143;
 
@@ -25,11 +26,6 @@ abstract contract ERC1967Upgrade {
      */
     bytes32 internal constant _IMPLEMENTATION_SLOT = 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc;
 
-    /**
-     * @dev Emitted when the implementation is upgraded.
-     */
-    event Upgraded(address indexed implementation);
-
     /**
      * @dev Returns the current implementation address.
      */
@@ -95,11 +91,6 @@ abstract contract ERC1967Upgrade {
      */
     bytes32 internal constant _ADMIN_SLOT = 0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103;
 
-    /**
-     * @dev Emitted when the admin account has changed.
-     */
-    event AdminChanged(address previousAdmin, address newAdmin);
-
     /**
      * @dev Returns the current admin.
      */
@@ -131,11 +122,6 @@ abstract contract ERC1967Upgrade {
      */
     bytes32 internal constant _BEACON_SLOT = 0xa3f0ad74e5423aebfd80d3ef4346578335a9a72aeaee59ff6cb3582b35133d50;
 
-    /**
-     * @dev Emitted when the beacon is upgraded.
-     */
-    event BeaconUpgraded(address indexed beacon);
-
     /**
      * @dev Returns the current beacon.
      */

contracts/proxy/transparent/ProxyAdmin.sol

@@ -18,7 +18,7 @@ contract ProxyAdmin is Ownable {
      *
      * - This contract must be the admin of `proxy`.
      */
-    function getProxyImplementation(TransparentUpgradeableProxy proxy) public view virtual returns (address) {
+    function getProxyImplementation(ITransparentUpgradeableProxy proxy) public view virtual returns (address) {
         // We need to manually run the static call since the getter cannot be flagged as view
         // bytes4(keccak256("implementation()")) == 0x5c60da1b
         (bool success, bytes memory returndata) = address(proxy).staticcall(hex"5c60da1b");
@@ -33,7 +33,7 @@ contract ProxyAdmin is Ownable {
      *
      * - This contract must be the admin of `proxy`.
      */
-    function getProxyAdmin(TransparentUpgradeableProxy proxy) public view virtual returns (address) {
+    function getProxyAdmin(ITransparentUpgradeableProxy proxy) public view virtual returns (address) {
         // We need to manually run the static call since the getter cannot be flagged as view
         // bytes4(keccak256("admin()")) == 0xf851a440
         (bool success, bytes memory returndata) = address(proxy).staticcall(hex"f851a440");
@@ -48,7 +48,7 @@ contract ProxyAdmin is Ownable {
      *
      * - This contract must be the current admin of `proxy`.
      */
-    function changeProxyAdmin(TransparentUpgradeableProxy proxy, address newAdmin) public virtual onlyOwner {
+    function changeProxyAdmin(ITransparentUpgradeableProxy proxy, address newAdmin) public virtual onlyOwner {
         proxy.changeAdmin(newAdmin);
     }
 
@@ -59,7 +59,7 @@ contract ProxyAdmin is Ownable {
      *
      * - This contract must be the admin of `proxy`.
      */
-    function upgrade(TransparentUpgradeableProxy proxy, address implementation) public virtual onlyOwner {
+    function upgrade(ITransparentUpgradeableProxy proxy, address implementation) public virtual onlyOwner {
         proxy.upgradeTo(implementation);
     }
 
@@ -72,7 +72,7 @@ contract ProxyAdmin is Ownable {
      * - This contract must be the admin of `proxy`.
      */
     function upgradeAndCall(
-        TransparentUpgradeableProxy proxy,
+        ITransparentUpgradeableProxy proxy,
         address implementation,
         bytes memory data
     ) public payable virtual onlyOwner {

contracts/proxy/transparent/TransparentUpgradeableProxy.sol

@@ -5,6 +5,24 @@ pragma solidity ^0.8.0;
 
 import "../ERC1967/ERC1967Proxy.sol";
 
+/**
+ * @dev Interface for the {TransparentUpgradeableProxy}. This is useful because {TransparentUpgradeableProxy} uses a
+ * custom call-routing mechanism, the compiler is unaware of the functions being exposed, and cannot list them. Also
+ * {TransparentUpgradeableProxy} does not inherit from this interface because it's implemented in a way that the
+ * compiler doesn't understand and cannot verify.
+ */
+interface ITransparentUpgradeableProxy is IERC1967 {
+    function admin() external view returns (address);
+
+    function implementation() external view returns (address);
+
+    function changeAdmin(address) external;
+
+    function upgradeTo(address) external;
+
+    function upgradeToAndCall(address, bytes memory) external payable;
+}
+
 /**
  * @dev This contract implements a proxy that is upgradeable by an admin.
  *
@@ -25,6 +43,13 @@ import "../ERC1967/ERC1967Proxy.sol";
  *
  * 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.
+ *
+ * WARNING: This contract does not inherit from {ITransparentUpgradeableProxy}, and the admin function is implicitly
+ * implemented using a custom call-routing mechanism in `_fallback`. Consequently, the compiler will not produce an
+ * ABI for this contract. Also, if you inherit from this contract and add additional functions, the compiler will not
+ * check that there are no selector conflicts. 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.
  */
 contract TransparentUpgradeableProxy is ERC1967Proxy {
     /**
@@ -37,6 +62,9 @@ contract TransparentUpgradeableProxy is ERC1967Proxy {
 
     /**
      * @dev Modifier used internally that will delegate the call to the implementation unless the sender is the admin.
+     *
+     * CAUTION: This modifier is deprecated, as it could cause issues if the modified function has arguments, and the
+     * implementation provides a function with the same selector.
      */
     modifier ifAdmin() {
         if (msg.sender == _getAdmin()) {
@@ -46,65 +74,98 @@ contract TransparentUpgradeableProxy is ERC1967Proxy {
         }
     }
 
+    /**
+     * @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()) {
+            bytes memory ret;
+            bytes4 selector = msg.sig;
+            if (selector == ITransparentUpgradeableProxy.upgradeTo.selector) {
+                ret = _dispatchUpgradeTo();
+            } else if (selector == ITransparentUpgradeableProxy.upgradeToAndCall.selector) {
+                ret = _dispatchUpgradeToAndCall();
+            } else if (selector == ITransparentUpgradeableProxy.changeAdmin.selector) {
+                ret = _dispatchChangeAdmin();
+            } else if (selector == ITransparentUpgradeableProxy.admin.selector) {
+                ret = _dispatchAdmin();
+            } else if (selector == ITransparentUpgradeableProxy.implementation.selector) {
+                ret = _dispatchImplementation();
+            } else {
+                revert("TransparentUpgradeableProxy: admin cannot fallback to proxy target");
+            }
+            assembly {
+                return(add(ret, 0x20), mload(ret))
+            }
+        } else {
+            super._fallback();
+        }
+    }
+
     /**
      * @dev Returns the current admin.
      *
-     * NOTE: Only the admin can call this function. See {ProxyAdmin-getProxyAdmin}.
-     *
      * TIP: To get this value clients can read directly from the storage slot shown below (specified by EIP1967) using the
      * https://eth.wiki/json-rpc/API#eth_getstorageat[`eth_getStorageAt`] RPC call.
      * `0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103`
      */
-    function admin() external payable ifAdmin returns (address admin_) {
+    function _dispatchAdmin() private returns (bytes memory) {
         _requireZeroValue();
-        admin_ = _getAdmin();
+
+        address admin = _getAdmin();
+        return abi.encode(admin);
     }
 
     /**
      * @dev Returns the current implementation.
      *
-     * NOTE: Only the admin can call this function. See {ProxyAdmin-getProxyImplementation}.
-     *
      * TIP: To get this value clients can read directly from the storage slot shown below (specified by EIP1967) using the
      * https://eth.wiki/json-rpc/API#eth_getstorageat[`eth_getStorageAt`] RPC call.
      * `0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc`
      */
-    function implementation() external payable ifAdmin returns (address implementation_) {
+    function _dispatchImplementation() private returns (bytes memory) {
         _requireZeroValue();
-        implementation_ = _implementation();
+
+        address implementation = _implementation();
+        return abi.encode(implementation);
     }
 
     /**
      * @dev Changes the admin of the proxy.
      *
      * Emits an {AdminChanged} event.
-     *
-     * NOTE: Only the admin can call this function. See {ProxyAdmin-changeProxyAdmin}.
      */
-    function changeAdmin(address newAdmin) external payable virtual ifAdmin {
+    function _dispatchChangeAdmin() private returns (bytes memory) {
         _requireZeroValue();
+
+        address newAdmin = abi.decode(msg.data[4:], (address));
         _changeAdmin(newAdmin);
+
+        return "";
     }
 
     /**
      * @dev Upgrade the implementation of the proxy.
-     *
-     * NOTE: Only the admin can call this function. See {ProxyAdmin-upgrade}.
      */
-    function upgradeTo(address newImplementation) external payable ifAdmin {
+    function _dispatchUpgradeTo() private returns (bytes memory) {
         _requireZeroValue();
+
+        address newImplementation = abi.decode(msg.data[4:], (address));
         _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.
-     *
-     * NOTE: Only the admin can call this function. See {ProxyAdmin-upgradeAndCall}.
      */
-    function upgradeToAndCall(address newImplementation, bytes calldata data) external payable ifAdmin {
+    function _dispatchUpgradeToAndCall() private returns (bytes memory) {
+        (address newImplementation, bytes memory data) = abi.decode(msg.data[4:], (address, bytes));
         _upgradeToAndCall(newImplementation, data, true);
+
+        return "";
     }
 
     /**
@@ -114,14 +175,6 @@ contract TransparentUpgradeableProxy is ERC1967Proxy {
         return _getAdmin();
     }
 
-    /**
-     * @dev Makes sure the admin cannot access the fallback function. See {Proxy-_beforeFallback}.
-     */
-    function _beforeFallback() internal virtual override {
-        require(msg.sender != _getAdmin(), "TransparentUpgradeableProxy: admin cannot fallback to proxy target");
-        super._beforeFallback();
-    }
-
     /**
      * @dev To keep this contract fully transparent, all `ifAdmin` functions must be payable. This helper is here to
      * emulate some proxy functions being non-payable while still allowing value to pass through.

test/proxy/transparent/ProxyAdmin.test.js

@@ -6,6 +6,7 @@ const ImplV1 = artifacts.require('DummyImplementation');
 const ImplV2 = artifacts.require('DummyImplementationV2');
 const ProxyAdmin = artifacts.require('ProxyAdmin');
 const TransparentUpgradeableProxy = artifacts.require('TransparentUpgradeableProxy');
+const ITransparentUpgradeableProxy = artifacts.require('ITransparentUpgradeableProxy');
 
 contract('ProxyAdmin', function (accounts) {
   const [proxyAdminOwner, newAdmin, anotherAccount] = accounts;
@@ -18,12 +19,13 @@ contract('ProxyAdmin', function (accounts) {
   beforeEach(async function () {
     const initializeData = Buffer.from('');
     this.proxyAdmin = await ProxyAdmin.new({ from: proxyAdminOwner });
-    this.proxy = await TransparentUpgradeableProxy.new(
+    const proxy = await TransparentUpgradeableProxy.new(
       this.implementationV1.address,
       this.proxyAdmin.address,
       initializeData,
       { from: proxyAdminOwner },
     );
+    this.proxy = await ITransparentUpgradeableProxy.at(proxy.address);
   });
 
   it('has an owner', async function () {