From ed565f403f789f854d06e254477d375747367c52 Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Mon, 23 Sep 2024 14:06:41 -0400 Subject: [PATCH] Add check to ensure `initialOwner` is not a ProxyAdmin contract when deploying a transparent proxy (#1083) --- .../ROOT/pages/api-hardhat-upgrades.adoc | 3 ++ packages/core/CHANGELOG.md | 4 ++ packages/core/package.json | 2 +- packages/core/src/index.ts | 1 + packages/core/src/infer-proxy-admin.test.ts | 49 +++++++++++++++++++ packages/core/src/infer-proxy-admin.ts | 14 ++++++ packages/core/src/utils/address.ts | 2 +- packages/plugin-hardhat/CHANGELOG.md | 5 ++ .../plugin-hardhat/contracts/HasOwner.sol | 23 +++++++++ packages/plugin-hardhat/package.json | 4 +- packages/plugin-hardhat/src/deploy-proxy.ts | 10 ++++ packages/plugin-hardhat/src/utils/options.ts | 5 ++ .../test/transparent-admin-initial-owner.js | 45 +++++++++++++++++ 13 files changed, 163 insertions(+), 4 deletions(-) create mode 100644 packages/core/src/infer-proxy-admin.test.ts create mode 100644 packages/core/src/infer-proxy-admin.ts create mode 100644 packages/plugin-hardhat/contracts/HasOwner.sol diff --git a/docs/modules/ROOT/pages/api-hardhat-upgrades.adoc b/docs/modules/ROOT/pages/api-hardhat-upgrades.adoc index 278489fbb..11831b4e0 100644 --- a/docs/modules/ROOT/pages/api-hardhat-upgrades.adoc +++ b/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/hardhat-upgrades@3.0.0` +* `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/hardhat-upgrades@3.4.0` * `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', diff --git a/packages/core/CHANGELOG.md b/packages/core/CHANGELOG.md index 2212f40d4..dda1a933f 100644 --- a/packages/core/CHANGELOG.md +++ b/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)) diff --git a/packages/core/package.json b/packages/core/package.json index 84a50c713..929136ede 100644 --- a/packages/core/package.json +++ b/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", diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 9e9ea2cfd..6a4d640c5 100644 --- a/packages/core/src/index.ts +++ b/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'; diff --git a/packages/core/src/infer-proxy-admin.test.ts b/packages/core/src/infer-proxy-admin.test.ts new file mode 100644 index 000000000..47edf9210 --- /dev/null +++ b/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)); +}); diff --git a/packages/core/src/infer-proxy-admin.ts b/packages/core/src/infer-proxy-admin.ts new file mode 100644 index 000000000..a455bb777 --- /dev/null +++ b/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 { + const owner = await callOptionalSignature(provider, possibleContractAddress, 'owner()'); + return owner !== undefined && parseAddress(owner) !== undefined; +} diff --git a/packages/core/src/utils/address.ts b/packages/core/src/utils/address.ts index 466d786b2..a4c81371e 100644 --- a/packages/core/src/utils/address.ts +++ b/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 diff --git a/packages/plugin-hardhat/CHANGELOG.md b/packages/plugin-hardhat/CHANGELOG.md index 13fb6b1a6..6ed398b6e 100644 --- a/packages/plugin-hardhat/CHANGELOG.md +++ b/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)) diff --git a/packages/plugin-hardhat/contracts/HasOwner.sol b/packages/plugin-hardhat/contracts/HasOwner.sol new file mode 100644 index 000000000..31b902a38 --- /dev/null +++ b/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); + } +} \ No newline at end of file diff --git a/packages/plugin-hardhat/package.json b/packages/plugin-hardhat/package.json index 0dc30c3d7..b170f8f0b 100644 --- a/packages/plugin-hardhat/package.json +++ b/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", diff --git a/packages/plugin-hardhat/src/deploy-proxy.ts b/packages/plugin-hardhat/src/deploy-proxy.ts index 9261827cd..1fdf68b0b 100644 --- a/packages/plugin-hardhat/src/deploy-proxy.ts +++ b/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 }, diff --git a/packages/plugin-hardhat/src/utils/options.ts b/packages/plugin-hardhat/src/utils/options.ts index 9d8fc12ef..cd2ba413c 100644 --- a/packages/plugin-hardhat/src/utils/options.ts +++ b/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 & diff --git a/packages/plugin-hardhat/test/transparent-admin-initial-owner.js b/packages/plugin-hardhat/test/transparent-admin-initial-owner.js index 7d9751bcb..4c849c656 100644 --- a/packages/plugin-hardhat/test/transparent-admin-initial-owner.js +++ b/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, + }); +});