Add check to ensure initialOwner for deployTransparentProxy is no…

…t a ProxyAdmin (#76)

CHANGELOG.md

@@ -1,5 +1,10 @@
 # Changelog
 
+## 0.3.5 (2024-09-20)
+
+### Potentially breaking changes
+- Adds a check to ensure `initialOwner` for `deployTransparentProxy` is not a ProxyAdmin contract. ([#76](https://github.com/OpenZeppelin/openzeppelin-foundry-upgrades/pull/76))
+
 ## 0.3.4 (2024-09-16)
 
 - Defender: Add `metadata` option. ([#75](https://github.com/OpenZeppelin/openzeppelin-foundry-upgrades/pull/75))

docs/modules/api/pages/Options.adoc

@@ -14,6 +14,7 @@ struct Options {
   bytes constructorData;
   string unsafeAllow;
   bool unsafeAllowRenames;
+  bool unsafeSkipProxyAdminCheck;
   bool unsafeSkipStorageCheck;
   bool unsafeSkipAllChecks;
   struct DefenderOptions defender;

src/Options.sol

@@ -25,6 +25,12 @@ struct Options {
      * Configure storage layout check to allow variable renaming
      */
     bool unsafeAllowRenames;
+    /*
+     * Skips checking the `initialOwner` parameter of `Upgrades.deployTransparentProxy`.
+     * When deploying a transparent proxy, the `initialOwner` must be the address of an EOA or a contract that can call functions on a ProxyAdmin. It must not be a ProxyAdmin contract itself.
+     * Use this if you encounter an error due to this check and are sure that the `initialOwner` is not a ProxyAdmin contract.
+     */
+    bool unsafeSkipProxyAdminCheck;
     /*
      * Skips checking for storage layout compatibility errors. This is a dangerous option meant to be used as a last resort.
      */

src/Upgrades.sol

@@ -6,8 +6,10 @@ import {TransparentUpgradeableProxy} from "@openzeppelin/contracts/proxy/transpa
 import {UpgradeableBeacon} from "@openzeppelin/contracts/proxy/beacon/UpgradeableBeacon.sol";
 import {BeaconProxy} from "@openzeppelin/contracts/proxy/beacon/BeaconProxy.sol";
 
+import {Vm} from "forge-std/Vm.sol";
 import {Options} from "./Options.sol";
 import {Core} from "./internal/Core.sol";
+import {Utils} from "./internal/Utils.sol";
 
 /**
  * @dev Library for deploying and managing upgradeable contracts from Forge scripts or tests.
@@ -60,6 +62,16 @@ library Upgrades {
         bytes memory initializerData,
         Options memory opts
     ) internal returns (address) {
+        if (!opts.unsafeSkipAllChecks && !opts.unsafeSkipProxyAdminCheck && Core.inferProxyAdmin(initialOwner)) {
+            revert(
+                string.concat(
+                    "`initialOwner` must not be a ProxyAdmin contract. If the contract at address ",
+                    Vm(Utils.CHEATCODE_ADDRESS).toString(initialOwner),
+                    " is not a ProxyAdmin contract and you are sure that this contract is able to call functions on an actual ProxyAdmin, skip this check with the `unsafeSkipProxyAdminCheck` option."
+                )
+            );
+        }
+
         address impl = deployImplementation(contractName, opts);
 
         return

src/internal/Core.sol

@@ -307,7 +307,7 @@ library Core {
      * If the contract does not have the getter or the return data does not look like a string, this function returns an empty string.
      */
     function getUpgradeInterfaceVersion(address addr) internal view returns (string memory) {
-        // Use staticcall to prevent forge from broadcasting it
+        // Use staticcall to prevent forge from broadcasting it, and to ensure no state changes
         (bool success, bytes memory returndata) = addr.staticcall(
             abi.encodeWithSignature("UPGRADE_INTERFACE_VERSION()")
         );
@@ -318,6 +318,23 @@ library Core {
         }
     }
 
+    /**
+     * @dev Infers whether the address might be a ProxyAdmin contract.
+     */
+    function inferProxyAdmin(address addr) internal view returns (bool) {
+        return _hasOwner(addr);
+    }
+
+    /**
+     * @dev Returns true if the address is a contract with an `owner()` function that is not state-changing and returns something that might be an address,
+     * otherwise returns false.
+     */
+    function _hasOwner(address addr) private view returns (bool) {
+        // Use staticcall to prevent forge from broadcasting it, and to ensure no state changes
+        (bool success, bytes memory returndata) = addr.staticcall(abi.encodeWithSignature("owner()"));
+        return (success && returndata.length == 32);
+    }
+
     function _validate(string memory contractName, Options memory opts, bool requireReference) private {
         if (opts.unsafeSkipAllChecks) {
             return;

test/Upgrades.t.sol

@@ -6,12 +6,17 @@ import {Test} from "forge-std/Test.sol";
 import {Upgrades, Options} from "openzeppelin-foundry-upgrades/Upgrades.sol";
 
 import {IBeacon} from "@openzeppelin/contracts/proxy/beacon/IBeacon.sol";
+import {ProxyAdmin} from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";
+import {ITransparentUpgradeableProxy} from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
 
 import {Greeter} from "./contracts/Greeter.sol";
 import {GreeterProxiable} from "./contracts/GreeterProxiable.sol";
 import {GreeterV2} from "./contracts/GreeterV2.sol";
 import {GreeterV2Proxiable} from "./contracts/GreeterV2Proxiable.sol";
 import {WithConstructor, NoInitializer} from "./contracts/WithConstructor.sol";
+import {HasOwner} from "./contracts/HasOwner.sol";
+
+import {strings} from "solidity-stringutils/src/strings.sol";
 
 // Import additional contracts to include them for compilation
 import "./contracts/Validations.sol";
@@ -20,6 +25,8 @@ import "./contracts/Validations.sol";
  * @dev Tests for the Upgrades library.
  */
 contract UpgradesTest is Test {
+    using strings for *;
+
     function testUUPS() public {
         address proxy = Upgrades.deployUUPSProxy(
             "GreeterProxiable.sol",
@@ -139,8 +146,8 @@ contract UpgradesTest is Test {
 
     function testValidateImplementation() public {
         Options memory opts;
-        Validator v = new Validator();
-        try v.validateImplementation("Validations.sol:Unsafe", opts) {
+        Invoker i = new Invoker();
+        try i.validateImplementation("Validations.sol:Unsafe", opts) {
             fail();
         } catch {
             // TODO: check error message
@@ -150,8 +157,8 @@ contract UpgradesTest is Test {
     function testValidateLayout() public {
         Options memory opts;
         opts.referenceContract = "Validations.sol:LayoutV1";
-        Validator v = new Validator();
-        try v.validateUpgrade("Validations.sol:LayoutV2_Bad", opts) {
+        Invoker i = new Invoker();
+        try i.validateUpgrade("Validations.sol:LayoutV2_Bad", opts) {
             fail();
         } catch {
             // TODO: check error message
@@ -160,8 +167,8 @@ contract UpgradesTest is Test {
 
     function testValidateLayoutUpgradesFrom() public {
         Options memory opts;
-        Validator v = new Validator();
-        try v.validateUpgrade("Validations.sol:LayoutV2_UpgradesFrom_Bad", opts) {
+        Invoker i = new Invoker();
+        try i.validateUpgrade("Validations.sol:LayoutV2_UpgradesFrom_Bad", opts) {
             fail();
         } catch {
             // TODO: check error message
@@ -171,8 +178,8 @@ contract UpgradesTest is Test {
     function testValidateNamespaced() public {
         Options memory opts;
         opts.referenceContract = "Validations.sol:NamespacedV1";
-        Validator v = new Validator();
-        try v.validateUpgrade("Validations.sol:NamespacedV2_Bad", opts) {
+        Invoker i = new Invoker();
+        try i.validateUpgrade("Validations.sol:NamespacedV2_Bad", opts) {
             fail();
         } catch {
             // TODO: check error message
@@ -181,8 +188,8 @@ contract UpgradesTest is Test {
 
     function testValidateNamespacedUpgradesFrom() public {
         Options memory opts;
-        Validator v = new Validator();
-        try v.validateUpgrade("Validations.sol:NamespacedV2_UpgradesFrom_Bad", opts) {
+        Invoker i = new Invoker();
+        try i.validateUpgrade("Validations.sol:NamespacedV2_UpgradesFrom_Bad", opts) {
             fail();
         } catch {
             // TODO: check error message
@@ -202,9 +209,9 @@ contract UpgradesTest is Test {
 
     function testValidateNamespacedNoReference() public {
         Options memory opts;
-        Validator v = new Validator();
+        Invoker i = new Invoker();
         // validate upgrade without reference contract - an error is expected from upgrades-core CLI
-        try v.validateUpgrade("Validations.sol:NamespacedV2_Ok", opts) {
+        try i.validateUpgrade("Validations.sol:NamespacedV2_Ok", opts) {
             fail();
         } catch {
             // TODO: check error message
@@ -260,9 +267,92 @@ contract UpgradesTest is Test {
         address proxy = Upgrades.deployTransparentProxy("WithConstructor.sol:NoInitializer", msg.sender, "", opts);
         assertEq(WithConstructor(proxy).a(), 123);
     }
+
+    function testProxyAdminCheck() public {
+        ProxyAdmin admin = new ProxyAdmin(msg.sender);
+
+        Invoker i = new Invoker();
+        try
+            i.deployTransparentProxy(
+                "Greeter.sol",
+                address(admin), // NOT SAFE
+                abi.encodeCall(Greeter.initialize, (msg.sender, "hello"))
+            )
+        {
+            fail();
+        } catch Error(string memory reason) {
+            strings.slice memory slice = reason.toSlice();
+            assertTrue(slice.contains("`initialOwner` must not be a ProxyAdmin contract.".toSlice()));
+            assertTrue(slice.contains(vm.toString(address(admin)).toSlice()));
+        }
+    }
+
+    function testProxyAdminCheck_emptyOpts() public {
+        HasOwner hasOwner = new HasOwner(msg.sender);
+        Options memory opts;
+
+        Invoker i = new Invoker();
+        try
+            i.deployTransparentProxy(
+                "Greeter.sol",
+                address(hasOwner), // false positive
+                abi.encodeCall(Greeter.initialize, (msg.sender, "hello")),
+                opts
+            )
+        {
+            fail();
+        } catch Error(string memory reason) {
+            strings.slice memory slice = reason.toSlice();
+            assertTrue(slice.contains("`initialOwner` must not be a ProxyAdmin contract.".toSlice()));
+            assertTrue(slice.contains(vm.toString(address(hasOwner)).toSlice()));
+        }
+    }
+
+    function testProxyAdminCheck_skip() public {
+        HasOwner hasOwner = new HasOwner(msg.sender);
+        Options memory opts;
+        opts.unsafeSkipProxyAdminCheck = true;
+
+        Upgrades.deployTransparentProxy(
+            "Greeter.sol",
+            address(hasOwner),
+            abi.encodeCall(Greeter.initialize, (msg.sender, "hello")),
+            opts
+        );
+    }
+
+    function testProxyAdminCheck_skipAll() public {
+        HasOwner hasOwner = new HasOwner(msg.sender);
+        Options memory opts;
+        opts.unsafeSkipAllChecks = true;
+
+        Upgrades.deployTransparentProxy(
+            "Greeter.sol",
+            address(hasOwner),
+            abi.encodeCall(Greeter.initialize, (msg.sender, "hello")),
+            opts
+        );
+    }
 }
 
-contract Validator {
+contract Invoker {
+    function deployTransparentProxy(
+        string memory contractName,
+        address admin,
+        bytes memory data
+    ) public returns (address) {
+        return Upgrades.deployTransparentProxy(contractName, admin, data);
+    }
+
+    function deployTransparentProxy(
+        string memory contractName,
+        address admin,
+        bytes memory data,
+        Options memory opts
+    ) public returns (address) {
+        return Upgrades.deployTransparentProxy(contractName, admin, data, opts);
+    }
+
     function validateImplementation(string memory contractName, Options memory opts) public {
         Upgrades.validateImplementation(contractName, opts);
     }

test/contracts/HasOwner.sol

@@ -0,0 +1,40 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.20;
+
+// These contracts are for testing only, they are not safe for use in production.
+
+import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
+import {ITransparentUpgradeableProxy} from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
+import {ProxyAdmin} from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";
+
+// Basic but pointless contract that has its own owner and can call ProxyAdmin functions
+contract HasOwner is Ownable {
+    constructor(address initialOwner) Ownable(initialOwner) {}
+
+    function upgradeAndCall(
+        ProxyAdmin admin,
+        ITransparentUpgradeableProxy proxy,
+        address implementation,
+        bytes memory data
+    ) public payable virtual onlyOwner {
+        admin.upgradeAndCall{value: msg.value}(proxy, implementation, data);
+    }
+}
+
+contract NoGetter {}
+
+contract StringOwner {
+    string public owner;
+
+    constructor(string memory initialOwner) {
+        owner = initialOwner;
+    }
+}
+
+contract StateChanging {
+    bool public triggered;
+
+    function owner() public {
+        triggered = true;
+    }
+}

test/internal/Core.t.sol

@@ -6,6 +6,9 @@ import {Test} from "forge-std/Test.sol";
 import {Core} from "openzeppelin-foundry-upgrades/internal/Core.sol";
 
 import {UpgradeInterfaceVersionString, UpgradeInterfaceVersionNoGetter, UpgradeInterfaceVersionEmpty, UpgradeInterfaceVersionInteger, UpgradeInterfaceVersionVoid} from "../contracts/UpgradeInterfaceVersions.sol";
+import {HasOwner, NoGetter, StringOwner, StateChanging} from "../contracts/HasOwner.sol";
+
+import {ProxyAdmin} from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";
 
 /**
  * @dev Tests the Core internal library.
@@ -35,4 +38,33 @@ contract CoreTest is Test {
         UpgradeInterfaceVersionVoid u = new UpgradeInterfaceVersionVoid();
         assertEq(Core.getUpgradeInterfaceVersion(address(u)), "");
     }
+
+    function testInferProxyAdmin() public {
+        ProxyAdmin admin = new ProxyAdmin(msg.sender);
+        assertEq(Core.inferProxyAdmin(address(admin)), true);
+    }
+
+    function testInferProxyAdmin_hasOwner() public {
+        HasOwner c = new HasOwner(msg.sender);
+        assertEq(Core.inferProxyAdmin(address(c)), true); // not actually a proxy admin, but has an owner
+    }
+
+    function testInferProxyAdmin_noOwner() public {
+        NoGetter c = new NoGetter();
+        assertEq(Core.inferProxyAdmin(address(c)), false);
+    }
+
+    function testInferProxyAdmin_stringOwner() public {
+        StringOwner c = new StringOwner("foo");
+        assertEq(Core.inferProxyAdmin(address(c)), false);
+    }
+
+    function testInferProxyAdmin_notContract() public view {
+        assertEq(Core.inferProxyAdmin(address(0)), false);
+    }
+
+    function testInferProxyAdmin_stateChanging() public {
+        StateChanging c = new StateChanging();
+        assertEq(Core.inferProxyAdmin(address(c)), false);
+    }
 }