Make TransparentUpgradeableProxy admin immutable

.changeset/happy-falcons-walk.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': major
+---
+
+`TransparentUpgradeableProxy`: Added an immutable admin set during construction to avoid unnecessary storage reads on every proxy call, and removed the ability to change the admin from both proxy and `ProxyAdmin`

contracts/mocks/DummyImplementation.sol

@@ -2,6 +2,9 @@
 
 pragma solidity ^0.8.19;
 
+import "../interfaces/IERC1967.sol";
+import "../utils/StorageSlot.sol";
+
 abstract contract Impl {
     function version() public pure virtual returns (string memory);
 }
@@ -11,6 +14,9 @@ contract DummyImplementation {
     string public text;
     uint256[] public values;
 
+    // bytes32(uint256(keccak256('eip1967.proxy.admin')) - 1)
+    bytes32 internal constant _ADMIN_SLOT = 0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103;
+
     function initializeNonPayable() public {
         value = 10;
     }
@@ -44,6 +50,12 @@ contract DummyImplementation {
     function reverts() public pure {
         require(false, "DummyImplementation reverted");
     }
+
+    // Use for forcing an unsafe TransparentUpgradeableProxy admin override
+    // solhint-disable-next-line private-vars-leading-underscore
+    function _unsafeOverrideAdmin(address newAdmin) public {
+        StorageSlot.getAddressSlot(_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/ERC1967/ERC1967Upgrade.sol

@@ -123,7 +123,7 @@ abstract contract ERC1967Upgrade is IERC1967 {
      * https://eth.wiki/json-rpc/API#eth_getstorageat[`eth_getStorageAt`] RPC call.
      * `0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103`
      */
-    function _getAdmin() internal view returns (address) {
+    function _getAdmin() internal view virtual returns (address) {
         return StorageSlot.getAddressSlot(_ADMIN_SLOT).value;
     }
 

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

@@ -12,8 +12,6 @@ import "../ERC1967/ERC1967Proxy.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;
@@ -46,12 +44,21 @@ 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.
+    address private immutable _admin;
+
     /**
      * @dev The proxy caller is the current admin, and can't fallback to the proxy target.
      */
@@ -67,6 +74,7 @@ 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_;
         _changeAdmin(admin_);
     }
 
@@ -81,8 +89,6 @@ contract TransparentUpgradeableProxy is ERC1967Proxy {
                 ret = _dispatchUpgradeTo();
             } else if (selector == ITransparentUpgradeableProxy.upgradeToAndCall.selector) {
                 ret = _dispatchUpgradeToAndCall();
-            } else if (selector == ITransparentUpgradeableProxy.changeAdmin.selector) {
-                ret = _dispatchChangeAdmin();
             } else {
                 revert ProxyDeniedAdminAccess();
             }
@@ -95,17 +101,12 @@ contract TransparentUpgradeableProxy is ERC1967Proxy {
     }
 
     /**
-     * @dev Changes the admin of the proxy.
+     * @dev Returns the current immutable admin.
      *
-     * Emits an {AdminChanged} event.
+     * Overrides ERC1967's admin in favor of an immutable value to avoid unnecessary SLOADs on each proxy call.
      */
-    function _dispatchChangeAdmin() private returns (bytes memory) {
-        _requireZeroValue();
-
-        address newAdmin = abi.decode(msg.data[4:], (address));
-        _changeAdmin(newAdmin);
-
-        return "";
+    function _getAdmin() internal view virtual override returns (address) {
+        return _admin;
     }
 
     /**

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,25 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx
     });
   });
 
+  describe('proxyAdmin', function () {
+    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 +277,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 +297,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');
       });
     });