Improve TransparentUpgradeableProxy's transparency (#3977)

Co-authored-by: Francisco <fg@frang.io> (cherry picked from commit 2a62fb4)
OpenZeppelin · Apr 5, 2023 · 8dfeb5d · 8dfeb5d
1 parent 9eee01c
commit 8dfeb5d
Show file tree

Hide file tree

Showing 4 changed files with 36 additions and 11 deletions.
diff --git a/.changeset/many-panthers-hide.md b/.changeset/many-panthers-hide.md
@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': minor
+---
+
+`TransparentUpgradeableProxy`: support value passthrough for all ifAdmin function.
diff --git a/contracts/mocks/ClashingImplementation.sol b/contracts/mocks/ClashingImplementation.sol
@@ -3,12 +3,11 @@
 pragma solidity ^0.8.0;
 
 /**
- * @dev Implementation contract with an admin() function made to clash with
- * @dev TransparentUpgradeableProxy's to test correct functioning of the
- * @dev Transparent Proxy feature.
+ * @dev Implementation contract with a payable admin() function made to clash with TransparentUpgradeableProxy's to
+ * test correct functioning of the Transparent Proxy feature.
  */
 contract ClashingImplementation {
-    function admin() external pure returns (address) {
+    function admin() external payable returns (address) {
         return 0x0000000000000000000000000000000011111142;
     }
 

diff --git a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol
@@ -59,7 +59,8 @@ contract TransparentUpgradeableProxy is ERC1967Proxy {
      * https://eth.wiki/json-rpc/API#eth_getstorageat[`eth_getStorageAt`] RPC call.
      * `0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103`
      */
-    function admin() external ifAdmin returns (address admin_) {
+    function admin() external payable ifAdmin returns (address admin_) {
+        _requireZeroValue();
         admin_ = _getAdmin();
     }
 
@@ -72,7 +73,8 @@ contract TransparentUpgradeableProxy is ERC1967Proxy {
      * https://eth.wiki/json-rpc/API#eth_getstorageat[`eth_getStorageAt`] RPC call.
      * `0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc`
      */
-    function implementation() external ifAdmin returns (address implementation_) {
+    function implementation() external payable ifAdmin returns (address implementation_) {
+        _requireZeroValue();
         implementation_ = _implementation();
     }
 
@@ -83,7 +85,8 @@ contract TransparentUpgradeableProxy is ERC1967Proxy {
      *
      * NOTE: Only the admin can call this function. See {ProxyAdmin-changeProxyAdmin}.
      */
-    function changeAdmin(address newAdmin) external virtual ifAdmin {
+    function changeAdmin(address newAdmin) external payable virtual ifAdmin {
+        _requireZeroValue();
         _changeAdmin(newAdmin);
     }
 
@@ -92,7 +95,8 @@ contract TransparentUpgradeableProxy is ERC1967Proxy {
      *
      * NOTE: Only the admin can call this function. See {ProxyAdmin-upgrade}.
      */
-    function upgradeTo(address newImplementation) external ifAdmin {
+    function upgradeTo(address newImplementation) external payable ifAdmin {
+        _requireZeroValue();
         _upgradeToAndCall(newImplementation, bytes(""), false);
     }
 
@@ -121,4 +125,12 @@ contract TransparentUpgradeableProxy is ERC1967Proxy {
         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.
+     */
+    function _requireZeroValue() private {
+        require(msg.value == 0);
+    }
 }
diff --git a/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js b/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js
@@ -333,14 +333,23 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro
       );
     });
 
-    context('when function names clash', function () {
+    describe('when function names clash', function () {
       it('when sender is proxy admin should run the proxy function', async function () {
-        const value = await this.proxy.admin.call({ from: proxyAdminAddress });
+        const value = await this.proxy.admin.call({ from: proxyAdminAddress, value: 0 });
         expect(value).to.be.equal(proxyAdminAddress);
       });
 
       it('when sender is other should delegate to implementation', async function () {
-        const value = await this.proxy.admin.call({ from: anotherAccount });
+        const value = await this.proxy.admin.call({ from: anotherAccount, value: 0 });
+        expect(value).to.be.equal('0x0000000000000000000000000000000011111142');
+      });
+
+      it('when sender is proxy admin value should not be accepted', async function () {
+        await expectRevert.unspecified(this.proxy.admin.call({ from: proxyAdminAddress, value: 1 }));
+      });
+
+      it('when sender is other value should be accepted', async function () {
+        const value = await this.proxy.admin.call({ from: anotherAccount, value: 1 });
         expect(value).to.be.equal('0x0000000000000000000000000000000011111142');
       });
     });