Make `TransparentUpgradeableProxy` admin immutable (#4354)

Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Co-authored-by: Francisco <fg@frang.io>

.changeset/happy-falcons-walk.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': major
+---
+
+`TransparentUpgradeableProxy`: Admin is now stored in an immutable variable (set during construction) to avoid unnecessary storage reads on every proxy call. This removed the ability to ever change the admin. Transfer of the upgrade capability is exclusively handled through the ownership of the `ProxyAdmin`.

contracts/mocks/DummyImplementation.sol

@@ -2,6 +2,8 @@
 
 pragma solidity ^0.8.19;
 
+import "../proxy/ERC1967/ERC1967Utils.sol";
+
 abstract contract Impl {
     function version() public pure virtual returns (string memory);
 }
@@ -44,6 +46,11 @@ contract DummyImplementation {
     function reverts() public pure {
         require(false, "DummyImplementation reverted");
     }
+
+    // Use for forcing an unsafe TransparentUpgradeableProxy admin override
+    function unsafeOverrideAdmin(address newAdmin) public {
+        StorageSlot.getAddressSlot(ERC1967Utils.ADMIN_SLOT).value = newAdmin;
+    }
 }
 
 contract DummyImplementationV2 is DummyImplementation {

contracts/mocks/proxy/ClashingImplementation.sol

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

contracts/proxy/README.adoc

@@ -16,7 +16,7 @@ In order to avoid clashes with the storage variables of the implementation contr
 
 There are two alternative ways to add upgradeability to an ERC1967 proxy. Their differences are explained below in <<transparent-vs-uups>>.
 
-- {TransparentUpgradeableProxy}: A proxy with a built in admin and upgrade interface.
+- {TransparentUpgradeableProxy}: A proxy with a built-in immutable admin and upgrade interface.
 - {UUPSUpgradeable}: An upgradeability mechanism to be included in the implementation contract.
 
 CAUTION: Using upgradeable proxies correctly and securely is a difficult task that requires deep knowledge of the proxy pattern, Solidity, and the EVM. Unless you want a lot of low level control, we recommend using the xref:upgrades-plugins::index.adoc[OpenZeppelin Upgrades Plugins] for Truffle and Hardhat.

contracts/proxy/transparent/ProxyAdmin.sol

@@ -16,17 +16,6 @@ contract ProxyAdmin is Ownable {
      */
     constructor(address initialOwner) Ownable(initialOwner) {}
 
-    /**
-     * @dev Changes the admin of `proxy` to `newAdmin`.
-     *
-     * Requirements:
-     *
-     * - This contract must be the current admin of `proxy`.
-     */
-    function changeProxyAdmin(ITransparentUpgradeableProxy proxy, address newAdmin) public virtual onlyOwner {
-        proxy.changeAdmin(newAdmin);
-    }
-
     /**
      * @dev Upgrades `proxy` to `implementation`. See {TransparentUpgradeableProxy-upgradeTo}.
      *

contracts/proxy/transparent/TransparentUpgradeableProxy.sol

@@ -13,15 +13,13 @@ import "../../interfaces/IERC1967.sol";
  * include them in the ABI so this interface must be used to interact with it.
  */
 interface ITransparentUpgradeableProxy is IERC1967 {
-    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.
+ * @dev This contract implements a proxy that is upgradeable by an immutable admin.
  *
  * 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
@@ -31,15 +29,16 @@ interface ITransparentUpgradeableProxy is IERC1967 {
  * 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
- * implementation. If the admin tries to call a function on the implementation it will fail with an error that says
- * "admin cannot fallback to proxy target".
+ * 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 admin actions like upgrading the proxy or changing
- * the admin, 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.
+ * 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.
+ * 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.
  *
  * 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
@@ -47,12 +46,23 @@ interface ITransparentUpgradeableProxy is IERC1967 {
  * 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,
+ * preventing any changes thereafter. However, the admin slot defined in ERC-1967 can still be overwritten by the implementation
+ * logic pointed to by this proxy. In such cases, the contract may end up in an undesirable state where the admin slot is different
+ * from the actual admin.
+ *
  * 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.
  */
 contract TransparentUpgradeableProxy is ERC1967Proxy {
+    // An immutable address for the admin 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.
+    address private immutable _admin;
+
     /**
      * @dev The proxy caller is the current admin, and can't fallback to the proxy target.
      */
@@ -68,6 +78,8 @@ contract TransparentUpgradeableProxy is ERC1967Proxy {
      * optionally initialized with `_data` as explained in {ERC1967Proxy-constructor}.
      */
     constructor(address _logic, address admin_, bytes memory _data) payable ERC1967Proxy(_logic, _data) {
+        _admin = admin_;
+        // Set the storage value and emit an event for ERC-1967 compatibility
         ERC1967Utils.changeAdmin(admin_);
     }
 
@@ -75,15 +87,13 @@ 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 == ERC1967Utils.getAdmin()) {
+        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();
-            } else if (selector == ITransparentUpgradeableProxy.changeAdmin.selector) {
-                ret = _dispatchChangeAdmin();
             } else {
                 revert ProxyDeniedAdminAccess();
             }
@@ -95,20 +105,6 @@ contract TransparentUpgradeableProxy is ERC1967Proxy {
         }
     }
 
-    /**
-     * @dev Changes the admin of the proxy.
-     *
-     * Emits an {AdminChanged} event.
-     */
-    function _dispatchChangeAdmin() private returns (bytes memory) {
-        _requireZeroValue();
-
-        address newAdmin = abi.decode(msg.data[4:], (address));
-        ERC1967Utils.changeAdmin(newAdmin);
-
-        return "";
-    }
-
     /**
      * @dev Upgrade the implementation of the proxy.
      */

test/proxy/transparent/ProxyAdmin.test.js

@@ -6,11 +6,11 @@ const ProxyAdmin = artifacts.require('ProxyAdmin');
 const TransparentUpgradeableProxy = artifacts.require('TransparentUpgradeableProxy');
 const ITransparentUpgradeableProxy = artifacts.require('ITransparentUpgradeableProxy');
 
-const { getAddressInSlot, ImplementationSlot, AdminSlot } = require('../../helpers/erc1967');
+const { getAddressInSlot, ImplementationSlot } = require('../../helpers/erc1967');
 const { expectRevertCustomError } = require('../../helpers/customError');
 
 contract('ProxyAdmin', function (accounts) {
-  const [proxyAdminOwner, newAdmin, anotherAccount] = accounts;
+  const [proxyAdminOwner, anotherAccount] = accounts;
 
   before('set implementations', async function () {
     this.implementationV1 = await ImplV1.new();
@@ -32,23 +32,6 @@ contract('ProxyAdmin', function (accounts) {
     expect(await this.proxyAdmin.owner()).to.equal(proxyAdminOwner);
   });
 
-  describe('#changeProxyAdmin', function () {
-    it('fails to change proxy admin if its not the proxy owner', async function () {
-      await expectRevertCustomError(
-        this.proxyAdmin.changeProxyAdmin(this.proxy.address, newAdmin, { from: anotherAccount }),
-        'OwnableUnauthorizedAccount',
-        [anotherAccount],
-      );
-    });
-
-    it('changes proxy admin', async function () {
-      await this.proxyAdmin.changeProxyAdmin(this.proxy.address, newAdmin, { from: proxyAdminOwner });
-
-      const newProxyAdmin = await getAddressInSlot(this.proxy, AdminSlot);
-      expect(newProxyAdmin).to.be.equal(newAdmin);
-    });
-  });
-
   describe('#upgrade', function () {
     context('with unauthorized account', function () {
       it('fails to upgrade', async function () {

test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js

@@ -47,6 +47,32 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx
     });
   });
 
+  describe('proxy admin', function () {
+    it('emits AdminChanged event during construction', async function () {
+      expectEvent.inConstruction(this.proxy, 'AdminChanged', {
+        previousAdmin: ZERO_ADDRESS,
+        newAdmin: proxyAdminAddress,
+      });
+    });
+
+    it('sets the admin in the storage', async function () {
+      expect(await getAddressInSlot(this.proxy, AdminSlot)).to.be.equal(proxyAdminAddress);
+    });
+
+    it('can overwrite the admin by the implementation', async function () {
+      const dummy = new DummyImplementation(this.proxyAddress);
+      await dummy.unsafeOverrideAdmin(anotherAccount);
+      const ERC1967AdminSlotValue = await getAddressInSlot(this.proxy, AdminSlot);
+      expect(ERC1967AdminSlotValue).to.be.equal(anotherAccount);
+
+      // Still allows previous admin to execute admin operations
+      expect(ERC1967AdminSlotValue).to.not.equal(proxyAdminAddress);
+      expectEvent(await this.proxy.upgradeTo(this.implementationV1, { from: proxyAdminAddress }), 'Upgraded', {
+        implementation: this.implementationV1,
+      });
+    });
+  });
+
   describe('upgradeTo', function () {
     describe('when the sender is the admin', function () {
       const from = proxyAdminAddress;
@@ -258,51 +284,14 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx
     });
   });
 
-  describe('changeAdmin', function () {
-    describe('when the new proposed admin is not the zero address', function () {
-      const newAdmin = anotherAccount;
-
-      describe('when the sender is the admin', function () {
-        beforeEach('transferring', async function () {
-          this.receipt = await this.proxy.changeAdmin(newAdmin, { from: proxyAdminAddress });
-        });
-
-        it('assigns new proxy admin', async function () {
-          const newProxyAdmin = await getAddressInSlot(this.proxy, AdminSlot);
-          expect(newProxyAdmin).to.be.equal(anotherAccount);
-        });
-
-        it('emits an event', function () {
-          expectEvent(this.receipt, 'AdminChanged', {
-            previousAdmin: proxyAdminAddress,
-            newAdmin: newAdmin,
-          });
-        });
-      });
-
-      describe('when the sender is not the admin', function () {
-        it('reverts', async function () {
-          await expectRevert.unspecified(this.proxy.changeAdmin(newAdmin, { from: anotherAccount }));
-        });
-      });
-    });
-
-    describe('when the new proposed admin is the zero address', function () {
-      it('reverts', async function () {
-        await expectRevertCustomError(
-          this.proxy.changeAdmin(ZERO_ADDRESS, { from: proxyAdminAddress }),
-          'ERC1967InvalidAdmin',
-          [ZERO_ADDRESS],
-        );
-      });
-    });
-  });
-
   describe('transparent proxy', function () {
     beforeEach('creating proxy', async function () {
       const initializeData = Buffer.from('');
-      this.impl = await ClashingImplementation.new();
-      this.proxy = await createProxy(this.impl.address, proxyAdminAddress, initializeData, { from: proxyAdminOwner });
+      this.clashingImplV0 = (await ClashingImplementation.new()).address;
+      this.clashingImplV1 = (await ClashingImplementation.new()).address;
+      this.proxy = await createProxy(this.clashingImplV0, proxyAdminAddress, initializeData, {
+        from: proxyAdminOwner,
+      });
       this.clashing = new ClashingImplementation(this.proxy.address);
     });
 
@@ -315,24 +304,28 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx
     });
 
     describe('when function names clash', function () {
-      it('when sender is proxy admin should run the proxy function', async function () {
-        const receipt = await this.proxy.changeAdmin(anotherAccount, { from: proxyAdminAddress, value: 0 });
-        expectEvent(receipt, 'AdminChanged');
+      it('executes the proxy function if the sender is the admin', async function () {
+        const receipt = await this.proxy.upgradeTo(this.clashingImplV1, { from: proxyAdminAddress, value: 0 });
+        expectEvent(receipt, 'Upgraded', { implementation: this.clashingImplV1 });
       });
 
-      it('when sender is other should delegate to implementation', async function () {
-        const receipt = await this.proxy.changeAdmin(anotherAccount, { from: anotherAccount, value: 0 });
-        expectEvent.notEmitted(receipt, 'AdminChanged');
+      it('delegates the call to implementation when sender is not the admin', async function () {
+        const receipt = await this.proxy.upgradeTo(this.clashingImplV1, { from: anotherAccount, value: 0 });
+        expectEvent.notEmitted(receipt, 'Upgraded');
         expectEvent.inTransaction(receipt.tx, this.clashing, 'ClashingImplementationCall');
       });
 
-      it('when sender is proxy admin value should not be accepted', async function () {
-        await expectRevert.unspecified(this.proxy.changeAdmin(anotherAccount, { from: proxyAdminAddress, value: 1 }));
+      it('requires 0 value calling upgradeTo by proxy admin', async function () {
+        await expectRevertCustomError(
+          this.proxy.upgradeTo(this.clashingImplV1, { from: proxyAdminAddress, value: 1 }),
+          'ProxyNonPayableFunction',
+          [],
+        );
       });
 
-      it('when sender is other value should be accepted', async function () {
-        const receipt = await this.proxy.changeAdmin(anotherAccount, { from: anotherAccount, value: 1 });
-        expectEvent.notEmitted(receipt, 'AdminChanged');
+      it('allows calling with value if sender is not the admin', async function () {
+        const receipt = await this.proxy.upgradeTo(this.clashingImplV1, { from: anotherAccount, value: 1 });
+        expectEvent.notEmitted(receipt, 'Upgraded');
         expectEvent.inTransaction(receipt.tx, this.clashing, 'ClashingImplementationCall');
       });
     });