Remove admin and implementation getters from TransparentUpgradeableProxy (#3820)

Co-authored-by: Francisco <frangio.1@gmail.com>

CHANGELOG.md

@@ -3,7 +3,9 @@
 ## Unreleased (Breaking)
 
  * `TimelockController`: Changed the role architecture to use `DEFAULT_ADMIN_ROLE` as the admin for all roles, instead of the bespoke `TIMELOCK_ADMIN_ROLE` that was used previously. This aligns with the general recommendation for `AccessControl` and makes the addition of new roles easier. Accordingly, the `admin` parameter and timelock will now be granted `DEFAULT_ADMIN_ROLE` instead of `TIMELOCK_ADMIN_ROLE`. ([#3799](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3799))
-* Removed presets in favor of [OpenZeppelin Contracts Wizard](https://wizard.openzeppelin.com/).
+ * Removed presets in favor of [OpenZeppelin Contracts Wizard](https://wizard.openzeppelin.com/).
+ * `TransparentUpgradeableProxy`: Removed `admin` and `implementation` getters, which were only callable by the proxy owner and thus not very useful. ([#3820](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3820))
+ * `ProxyAdmin`: Removed `getProxyAdmin` and `getProxyImplementation` getters. ([#3820](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3820))
 
 ## Unreleased
 

contracts/proxy/ERC1967/ERC1967Proxy.sol

@@ -25,6 +25,10 @@ contract ERC1967Proxy is Proxy, ERC1967Upgrade {
 
     /**
      * @dev Returns the current implementation address.
+     *
+     * 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() internal view virtual override returns (address impl) {
         return ERC1967Upgrade._getImplementation();

contracts/proxy/ERC1967/ERC1967Upgrade.sol

@@ -112,6 +112,10 @@ abstract contract ERC1967Upgrade {
 
     /**
      * @dev Returns the current admin.
+     *
+     * 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 _getAdmin() internal view returns (address) {
         return StorageSlot.getAddressSlot(_ADMIN_SLOT).value;

contracts/proxy/transparent/ProxyAdmin.sol

@@ -11,36 +11,6 @@ import "../../access/Ownable.sol";
  * explanation of why you would want to use this see the documentation for {TransparentUpgradeableProxy}.
  */
 contract ProxyAdmin is Ownable {
-    /**
-     * @dev Returns the current implementation of `proxy`.
-     *
-     * Requirements:
-     *
-     * - This contract must be the admin of `proxy`.
-     */
-    function getProxyImplementation(TransparentUpgradeableProxy 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");
-        require(success);
-        return abi.decode(returndata, (address));
-    }
-
-    /**
-     * @dev Returns the current admin of `proxy`.
-     *
-     * Requirements:
-     *
-     * - This contract must be the admin of `proxy`.
-     */
-    function getProxyAdmin(TransparentUpgradeableProxy 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");
-        require(success);
-        return abi.decode(returndata, (address));
-    }
-
     /**
      * @dev Changes the admin of `proxy` to `newAdmin`.
      *

contracts/proxy/transparent/TransparentUpgradeableProxy.sol

@@ -50,32 +50,6 @@ contract TransparentUpgradeableProxy is ERC1967Proxy {
         }
     }
 
-    /**
-     * @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 ifAdmin returns (address admin_) {
-        admin_ = _getAdmin();
-    }
-
-    /**
-     * @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 ifAdmin returns (address implementation_) {
-        implementation_ = _implementation();
-    }
-
     /**
      * @dev Changes the admin of the proxy.
      *

test/helpers/erc1967.js

@@ -13,6 +13,11 @@ function getSlot (address, slot) {
   );
 }
 
+async function getAddressInSlot (address, slot) {
+  const slotValue = await getSlot(address, slot);
+  return web3.utils.toChecksumAddress(slotValue.substr(-40));
+}
+
 module.exports = {
   ImplementationLabel,
   AdminLabel,
@@ -21,4 +26,5 @@ module.exports = {
   AdminSlot: labelToSlot(AdminLabel),
   BeaconSlot: labelToSlot(BeaconLabel),
   getSlot,
+  getAddressInSlot,
 };

test/proxy/transparent/ProxyAdmin.test.js

@@ -1,7 +1,6 @@
 const { expectRevert } = require('@openzeppelin/test-helpers');
-
+const { getAddressInSlot, ImplementationSlot, AdminSlot } = require('../../helpers/erc1967');
 const { expect } = require('chai');
-
 const ImplV1 = artifacts.require('DummyImplementation');
 const ImplV2 = artifacts.require('DummyImplementationV2');
 const ProxyAdmin = artifacts.require('ProxyAdmin');
@@ -30,17 +29,6 @@ contract('ProxyAdmin', function (accounts) {
     expect(await this.proxyAdmin.owner()).to.equal(proxyAdminOwner);
   });
 
-  describe('#getProxyAdmin', function () {
-    it('returns proxyAdmin as admin of the proxy', async function () {
-      const admin = await this.proxyAdmin.getProxyAdmin(this.proxy.address);
-      expect(admin).to.be.equal(this.proxyAdmin.address);
-    });
-
-    it('call to invalid proxy', async function () {
-      await expectRevert.unspecified(this.proxyAdmin.getProxyAdmin(this.implementationV1.address));
-    });
-  });
-
   describe('#changeProxyAdmin', function () {
     it('fails to change proxy admin if its not the proxy owner', async function () {
       await expectRevert(
@@ -51,18 +39,9 @@ contract('ProxyAdmin', function (accounts) {
 
     it('changes proxy admin', async function () {
       await this.proxyAdmin.changeProxyAdmin(this.proxy.address, newAdmin, { from: proxyAdminOwner });
-      expect(await this.proxy.admin.call({ from: newAdmin })).to.eq(newAdmin);
-    });
-  });
 
-  describe('#getProxyImplementation', function () {
-    it('returns proxy implementation address', async function () {
-      const implementationAddress = await this.proxyAdmin.getProxyImplementation(this.proxy.address);
-      expect(implementationAddress).to.be.equal(this.implementationV1.address);
-    });
-
-    it('call to invalid proxy', async function () {
-      await expectRevert.unspecified(this.proxyAdmin.getProxyImplementation(this.implementationV1.address));
+      const newProxyAdmin = await getAddressInSlot(this.proxy, AdminSlot);
+      expect(newProxyAdmin).to.be.eq(newAdmin);
     });
   });
 
@@ -79,8 +58,9 @@ contract('ProxyAdmin', function (accounts) {
     context('with authorized account', function () {
       it('upgrades implementation', async function () {
         await this.proxyAdmin.upgrade(this.proxy.address, this.implementationV2.address, { from: proxyAdminOwner });
-        const implementationAddress = await this.proxyAdmin.getProxyImplementation(this.proxy.address);
-        expect(implementationAddress).to.be.equal(this.implementationV2.address);
+
+        const implementationAddress = await getAddressInSlot(this.proxy, ImplementationSlot);
+        expect(implementationAddress).to.be.eq(this.implementationV2.address);
       });
     });
   });
@@ -116,8 +96,8 @@ contract('ProxyAdmin', function (accounts) {
           await this.proxyAdmin.upgradeAndCall(this.proxy.address, this.implementationV2.address, callData,
             { from: proxyAdminOwner },
           );
-          const implementationAddress = await this.proxyAdmin.getProxyImplementation(this.proxy.address);
-          expect(implementationAddress).to.be.equal(this.implementationV2.address);
+          const implementationAddress = await getAddressInSlot(this.proxy, ImplementationSlot);
+          expect(implementationAddress).to.be.eq(this.implementationV2.address);
         });
       });
     });

test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js

@@ -1,6 +1,6 @@
 const { BN, expectRevert, expectEvent, constants } = require('@openzeppelin/test-helpers');
 const { ZERO_ADDRESS } = constants;
-const { getSlot, ImplementationSlot, AdminSlot } = require('../../helpers/erc1967');
+const { getAddressInSlot, ImplementationSlot, AdminSlot } = require('../../helpers/erc1967');
 
 const { expect } = require('chai');
 
@@ -34,9 +34,8 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro
 
   describe('implementation', function () {
     it('returns the current implementation address', async function () {
-      const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress });
-
-      expect(implementation).to.be.equal(this.implementationV0);
+      const implementationAddress = await getAddressInSlot(this.proxy, ImplementationSlot);
+      expect(implementationAddress).to.be.equal(this.implementationV0);
     });
 
     it('delegates to the implementation', async function () {
@@ -55,8 +54,8 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro
         it('upgrades to the requested implementation', async function () {
           await this.proxy.upgradeTo(this.implementationV1, { from });
 
-          const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress });
-          expect(implementation).to.be.equal(this.implementationV1);
+          const implementationAddress = await getAddressInSlot(this.proxy, ImplementationSlot);
+          expect(implementationAddress).to.be.equal(this.implementationV1);
         });
 
         it('emits an event', async function () {
@@ -108,8 +107,8 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro
           });
 
           it('upgrades to the requested implementation', async function () {
-            const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress });
-            expect(implementation).to.be.equal(this.behavior.address);
+            const implementationAddress = await getAddressInSlot(this.proxy, ImplementationSlot);
+            expect(implementationAddress).to.be.equal(this.behavior.address);
           });
 
           it('emits an event', function () {
@@ -173,7 +172,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro
           });
 
           it('upgrades to the requested version and emits an event', async function () {
-            const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress });
+            const implementation = await getAddressInSlot(this.proxy, ImplementationSlot);
             expect(implementation).to.be.equal(this.behaviorV1.address);
             expectEvent(this.receipt, 'Upgraded', { implementation: this.behaviorV1.address });
           });
@@ -199,7 +198,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro
             });
 
             it('upgrades to the requested version and emits an event', async function () {
-              const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress });
+              const implementation = await getAddressInSlot(this.proxy, ImplementationSlot);
               expect(implementation).to.be.equal(this.behaviorV2.address);
               expectEvent(this.receipt, 'Upgraded', { implementation: this.behaviorV2.address });
             });
@@ -228,7 +227,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro
               });
 
               it('upgrades to the requested version and emits an event', async function () {
-                const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress });
+                const implementation = await getAddressInSlot(this.proxy, ImplementationSlot);
                 expect(implementation).to.be.equal(this.behaviorV3.address);
                 expectEvent(this.receipt, 'Upgraded', { implementation: this.behaviorV3.address });
               });
@@ -274,7 +273,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro
         });
 
         it('assigns new proxy admin', async function () {
-          const newProxyAdmin = await this.proxy.admin.call({ from: newAdmin });
+          const newProxyAdmin = await getAddressInSlot(this.proxy, AdminSlot);
           expect(newProxyAdmin).to.be.equal(anotherAccount);
         });
 
@@ -303,20 +302,6 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro
     });
   });
 
-  describe('storage', function () {
-    it('should store the implementation address in specified location', async function () {
-      const implementationSlot = await getSlot(this.proxy, ImplementationSlot);
-      const implementationAddress = web3.utils.toChecksumAddress(implementationSlot.substr(-40));
-      expect(implementationAddress).to.be.equal(this.implementationV0);
-    });
-
-    it('should store the admin proxy in specified location', async function () {
-      const proxyAdminSlot = await getSlot(this.proxy, AdminSlot);
-      const proxyAdminAddress = web3.utils.toChecksumAddress(proxyAdminSlot.substr(-40));
-      expect(proxyAdminAddress).to.be.equal(proxyAdminAddress);
-    });
-  });
-
   describe('transparent proxy', function () {
     beforeEach('creating proxy', async function () {
       const initializeData = Buffer.from('');
@@ -332,18 +317,6 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro
         'TransparentUpgradeableProxy: admin cannot fallback to proxy target',
       );
     });
-
-    context('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 });
-        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 });
-        expect(value).to.be.equal('0x0000000000000000000000000000000011111142');
-      });
-    });
   });
 
   describe('regression', () => {