Add check to ensure initialOwner is not a ProxyAdmin contract when …

…deploying a transparent proxy (#1083)

docs/modules/ROOT/pages/api-hardhat-upgrades.adoc

@@ -22,6 +22,8 @@ The following options are common to some functions.
 * `constructorArgs`: (`unknown[]`) Provide arguments for the constructor of the implementation contract. Note that these are different from initializer arguments, and will be used in the deployment of the implementation contract itself. Can be used to initialize immutable variables.
 * `initialOwner`: (`string`) the address to set as the initial owner of a transparent proxy's admin or initial owner of a beacon. Defaults to the externally owned account that is deploying the transparent proxy or beacon. Not supported for UUPS proxies.
 ** *Since:* `@openzeppelin/[email protected]`
+* `unsafeSkipProxyAdminCheck`: (`boolean`) Skips checking the `initialOwner` option when deploying a transparent proxy. 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.
+** *Since:* `@openzeppelin/[email protected]`
 * `timeout`: (`number`) Timeout in milliseconds to wait for the transaction confirmation when deploying an implementation contract. Defaults to `60000`. Use `0` to wait indefinitely.
 * `pollingInterval`: (`number`) Polling interval in milliseconds between checks for the transaction confirmation when deploying an implementation contract. Defaults to `5000`.
 * `redeployImplementation`: (`"always" | "never" | "onchange"`) Determines whether the implementation contract will be redeployed. Defaults to `"onchange"`.
@@ -56,6 +58,7 @@ async function deployProxy(
     unsafeAllow?: ValidationError[],
     constructorArgs?: unknown[],
     initialOwner?: string,
+    unsafeSkipProxyAdminCheck?: boolean,
     timeout?: number,
     pollingInterval?: number,
     redeployImplementation?: 'always' | 'never' | 'onchange',

packages/core/CHANGELOG.md

@@ -1,5 +1,9 @@
 # Changelog
 
+## 1.38.0 (2024-09-23)
+
+- Supports checking to ensure `initialOwner` is not a ProxyAdmin contract when deploying a transparent proxy from Hardhat. ([#1083](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/1083))
+
 ## 1.37.1 (2024-09-09)
 
 - Fix Hardhat compile error when using solc version `0.8.27`. ([#1075](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/1075))

packages/core/package.json

@@ -1,6 +1,6 @@
 {
   "name": "@openzeppelin/upgrades-core",
-  "version": "1.37.1",
+  "version": "1.38.0",
   "description": "",
   "repository": "https://github.com/OpenZeppelin/openzeppelin-upgrades/tree/master/packages/core",
   "license": "MIT",

packages/core/src/index.ts

@@ -62,3 +62,4 @@ export { ValidateUpgradeSafetyOptions, validateUpgradeSafety, ProjectReport, Ref
 export { getUpgradeInterfaceVersion } from './upgrade-interface-version';
 export { makeNamespacedInput } from './utils/make-namespaced';
 export { isNamespaceSupported } from './storage/namespace';
+export { inferProxyAdmin } from './infer-proxy-admin';

packages/core/src/infer-proxy-admin.test.ts

@@ -0,0 +1,49 @@
+import test from 'ava';
+import { EthereumProvider } from './provider';
+import { inferProxyAdmin } from './infer-proxy-admin';
+
+const addr = '0x123';
+
+function makeProviderReturning(result: unknown): EthereumProvider {
+  return { send: (_method: string, _params: unknown[]) => Promise.resolve(result) } as EthereumProvider;
+}
+
+function makeProviderError(msg: string): EthereumProvider {
+  return {
+    send: (_method: string, _params: unknown[]) => {
+      throw new Error(msg);
+    },
+  } as EthereumProvider;
+}
+
+test('inferProxyAdmin returns true when owner looks like an address', async t => {
+  // abi encoding of address 0x1000000000000000000000000000000000000123
+  const provider = makeProviderReturning('0x0000000000000000000000001000000000000000000000000000000000000123');
+  t.true(await inferProxyAdmin(provider, addr));
+});
+
+test('inferProxyAdmin returns false when returned value is 32 bytes but clearly not an address', async t => {
+  // dirty upper bits beyond 20 bytes (the 'abc' in the below)
+  const provider = makeProviderReturning('0x000000000000000000000abc1000000000000000000000000000000000000123');
+  t.false(await inferProxyAdmin(provider, addr));
+});
+
+test('inferProxyAdmin returns false when returned value is a string', async t => {
+  // abi encoding of string 'foo'
+  const provider = makeProviderReturning(
+    '0x00000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000003666f6f0000000000000000000000000000000000000000000000000000000000',
+  );
+  t.false(await inferProxyAdmin(provider, addr));
+});
+
+test('inferProxyAdmin throws unrelated error', async t => {
+  const provider = makeProviderError('unrelated error');
+  await t.throwsAsync(() => inferProxyAdmin(provider, addr), { message: 'unrelated error' });
+});
+
+test('inferProxyAdmin returns false for invalid selector', async t => {
+  const provider = makeProviderError(
+    `Transaction reverted: function selector was not recognized and there's no fallback function`,
+  );
+  t.false(await inferProxyAdmin(provider, addr));
+});

packages/core/src/infer-proxy-admin.ts

@@ -0,0 +1,14 @@
+import { callOptionalSignature } from './call-optional-signature';
+import { EthereumProvider } from './provider';
+import { parseAddress } from './utils/address';
+
+/**
+ * Infers whether the address might be a ProxyAdmin contract, by checking if it has an owner() function that returns an address.
+ * @param provider Ethereum provider
+ * @param possibleContractAddress The address to check
+ * @returns true if the address might be a ProxyAdmin contract, false otherwise
+ */
+export async function inferProxyAdmin(provider: EthereumProvider, possibleContractAddress: string): Promise<boolean> {
+  const owner = await callOptionalSignature(provider, possibleContractAddress, 'owner()');
+  return owner !== undefined && parseAddress(owner) !== undefined;
+}

packages/core/src/utils/address.ts

@@ -8,7 +8,7 @@ import { toChecksumAddress } from 'ethereumjs-util';
  */
 export function parseAddress(addressString: string): string | undefined {
   const buf = Buffer.from(addressString.replace(/^0x/, ''), 'hex');
-  if (!buf.slice(0, 12).equals(Buffer.alloc(12, 0))) {
+  if (!buf.slice(0, 12).equals(Buffer.alloc(12, 0)) || buf.length !== 32) {
     return undefined;
   }
   const address = '0x' + buf.toString('hex', 12, 32); // grab the last 20 bytes

packages/plugin-hardhat/CHANGELOG.md

@@ -1,5 +1,10 @@
 # Changelog
 
+## 3.4.0 (2024-09-23)
+
+### Potentially breaking changes
+- Adds a check to ensure `initialOwner` is not a ProxyAdmin contract when deploying a transparent proxy. ([#1083](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/1083))
+
 ## 3.3.0 (2024-09-16)
 
 - Defender: Add `metadata` option. ([#1079](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/1079))

packages/plugin-hardhat/contracts/HasOwner.sol

@@ -0,0 +1,23 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.20;
+
+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";
+
+/**
+ * This contract is for testing only.
+ * 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);
+    }
+}

packages/plugin-hardhat/package.json

@@ -1,6 +1,6 @@
 {
   "name": "@openzeppelin/hardhat-upgrades",
-  "version": "3.3.0",
+  "version": "3.4.0",
   "description": "",
   "repository": "https://github.com/OpenZeppelin/openzeppelin-upgrades/tree/master/packages/plugin-hardhat",
   "license": "MIT",
@@ -38,7 +38,7 @@
     "@openzeppelin/defender-sdk-base-client": "^1.14.4",
     "@openzeppelin/defender-sdk-deploy-client": "^1.14.4",
     "@openzeppelin/defender-sdk-network-client": "^1.14.4",
-    "@openzeppelin/upgrades-core": "^1.35.0",
+    "@openzeppelin/upgrades-core": "^1.38.0",
     "chalk": "^4.1.0",
     "debug": "^4.1.1",
     "ethereumjs-util": "^7.1.5",

packages/plugin-hardhat/src/deploy-proxy.ts

@@ -8,6 +8,8 @@ import {
   BeaconProxyUnsupportedError,
   RemoteDeploymentId,
   InitialOwnerUnsupportedKindError,
+  UpgradesError,
+  inferProxyAdmin,
 } from '@openzeppelin/upgrades-core';
 
 import {
@@ -85,6 +87,14 @@ export function makeDeployProxy(hre: HardhatRuntimeEnvironment, defenderModule:
       case 'transparent': {
         const initialOwner = await getInitialOwner(opts, signer);
 
+        if (!opts.unsafeSkipProxyAdminCheck && (await inferProxyAdmin(provider, initialOwner))) {
+          throw new UpgradesError(
+            '`initialOwner` must not be a ProxyAdmin contract.',
+            () =>
+              `If the contract at address ${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.`,
+          );
+        }
+
         const TransparentUpgradeableProxyFactory = await getTransparentUpgradeableProxyFactory(hre, signer);
         proxyDeployment = Object.assign(
           { kind },

packages/plugin-hardhat/src/utils/options.ts

@@ -82,6 +82,11 @@ export type EthersDeployOptions = {
 
 export type InitialOwner = {
   initialOwner?: string;
+
+  /**
+   * Skips checking the `initialOwner` option when deploying a transparent proxy.
+   */
+  unsafeSkipProxyAdminCheck?: boolean;
 };
 
 export type DeployBeaconProxyOptions = EthersDeployOptions &

packages/plugin-hardhat/test/transparent-admin-initial-owner.js

@@ -3,10 +3,14 @@ const test = require('ava');
 const { ethers, upgrades } = require('hardhat');
 const hre = require('hardhat');
 
+const ProxyAdmin = require('@openzeppelin/upgrades-core/artifacts/@openzeppelin/contracts-v5/proxy/transparent/ProxyAdmin.sol/ProxyAdmin.json');
+
 const OWNABLE_ABI = ['function owner() view returns (address)'];
 
 test.before(async t => {
   t.context.Greeter = await ethers.getContractFactory('Greeter');
+  t.context.HasOwner = await ethers.getContractFactory('HasOwner');
+  t.context.ProxyAdmin = await ethers.getContractFactory(ProxyAdmin.abi, ProxyAdmin.bytecode);
 });
 
 test('initial owner using default signer', async t => {
@@ -62,3 +66,44 @@ test('initial owner - no signer in ContractFactory', async t => {
 
   t.is(await admin.owner(), initialOwner.address);
 });
+
+test('initial owner - must not be ProxyAdmin ', async t => {
+  const { Greeter, ProxyAdmin } = t.context;
+
+  const defaultSigner = await ethers.provider.getSigner(0);
+  const proxyAdmin = await ProxyAdmin.deploy(defaultSigner.address);
+  const predeployedProxyAdminAddress = await proxyAdmin.getAddress();
+
+  const e = await t.throwsAsync(() =>
+    upgrades.deployProxy(Greeter, ['hello'], { initialOwner: predeployedProxyAdminAddress }),
+  );
+  t.true(e.message.includes('`initialOwner` must not be a ProxyAdmin contract.'), e.message);
+  t.true(e.message.includes(predeployedProxyAdminAddress), e.message);
+});
+
+test('initial owner - must not be ownable', async t => {
+  const { Greeter, HasOwner } = t.context;
+
+  const defaultSigner = await ethers.provider.getSigner(0);
+  const hasOwner = await HasOwner.deploy(defaultSigner.address); // not actually a proxy admin, but it looks like one because it has an owner
+  const predeployedOwnableAddress = await hasOwner.getAddress();
+
+  const e = await t.throwsAsync(() =>
+    upgrades.deployProxy(Greeter, ['hello'], { initialOwner: predeployedOwnableAddress }),
+  );
+  t.true(e.message.includes('`initialOwner` must not be a ProxyAdmin contract.'), e.message);
+  t.true(e.message.includes(predeployedOwnableAddress), e.message);
+});
+
+test('initial owner - skip ProxyAdmin check', async t => {
+  const { Greeter, HasOwner } = t.context;
+
+  const defaultSigner = await ethers.provider.getSigner(0);
+  const hasOwner = await HasOwner.deploy(defaultSigner.address);
+  const predeployedOwnableAddress = await hasOwner.getAddress();
+
+  await upgrades.deployProxy(Greeter, ['hello'], {
+    initialOwner: predeployedOwnableAddress,
+    unsafeSkipProxyAdminCheck: true,
+  });
+});