From 0b1b0f0443edc31608d3080e79327e0253932fe3 Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Fri, 20 Sep 2024 10:39:00 -0400 Subject: [PATCH 01/15] Add check and option --- .../ROOT/pages/api-hardhat-upgrades.adoc | 1 + packages/core/src/index.ts | 1 + packages/core/src/infer-proxy-admin.ts | 17 +++++++++++++++++ packages/plugin-hardhat/src/deploy-proxy.ts | 9 +++++++++ packages/plugin-hardhat/src/utils/options.ts | 5 +++++ 5 files changed, 33 insertions(+) create mode 100644 packages/core/src/infer-proxy-admin.ts diff --git a/docs/modules/ROOT/pages/api-hardhat-upgrades.adoc b/docs/modules/ROOT/pages/api-hardhat-upgrades.adoc index 278489fbb..8bd9cb8e0 100644 --- a/docs/modules/ROOT/pages/api-hardhat-upgrades.adoc +++ b/docs/modules/ROOT/pages/api-hardhat-upgrades.adoc @@ -22,6 +22,7 @@ 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. * `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"`. diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 9e9ea2cfd..f3373fe28 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'; \ No newline at end of file diff --git a/packages/core/src/infer-proxy-admin.ts b/packages/core/src/infer-proxy-admin.ts new file mode 100644 index 000000000..42a9c6d0e --- /dev/null +++ b/packages/core/src/infer-proxy-admin.ts @@ -0,0 +1,17 @@ +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; +} \ No newline at end of file diff --git a/packages/plugin-hardhat/src/deploy-proxy.ts b/packages/plugin-hardhat/src/deploy-proxy.ts index 9261827cd..9881d3f64 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,13 @@ 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 & From 70ca1dab602a0e850d14490916b8f15aeac11829 Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Fri, 20 Sep 2024 11:10:37 -0400 Subject: [PATCH 02/15] Tests --- packages/core/src/infer-proxy-admin.test.ts | 50 +++++++++++++++++++++ packages/core/src/utils/address.ts | 2 +- 2 files changed, 51 insertions(+), 1 deletion(-) create mode 100644 packages/core/src/infer-proxy-admin.test.ts 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..5bbbb8b80 --- /dev/null +++ b/packages/core/src/infer-proxy-admin.test.ts @@ -0,0 +1,50 @@ +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 owner is more than 20 bytes', async t => { + const provider = makeProviderReturning( + '0x0000000000000000000000011000000000000000000000000000000000000123', + ); + t.false(await inferProxyAdmin(provider, addr)); +}); + +test('inferProxyAdmin returns false when owner 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/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 From 28b95c79c012e5717cb4a48fe363ba198b476a91 Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Fri, 20 Sep 2024 13:57:22 -0400 Subject: [PATCH 03/15] Add tests --- .../plugin-hardhat/contracts/HasOwner.sol | 23 ++++++++++ .../test/transparent-admin-initial-owner.js | 42 +++++++++++++++++++ 2 files changed, 65 insertions(+) create mode 100644 packages/plugin-hardhat/contracts/HasOwner.sol 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/test/transparent-admin-initial-owner.js b/packages/plugin-hardhat/test/transparent-admin-initial-owner.js index 7d9751bcb..efb27c879 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,41 @@ 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 }); +}); \ No newline at end of file From 0bdc8815b4fff4209304713c162127910d2ba0b4 Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Fri, 20 Sep 2024 14:54:07 -0400 Subject: [PATCH 04/15] lint --- packages/core/src/index.ts | 2 +- packages/core/src/infer-proxy-admin.test.ts | 12 +++++------- packages/core/src/infer-proxy-admin.ts | 7 ++----- packages/plugin-hardhat/src/deploy-proxy.ts | 9 +++++---- .../test/transparent-admin-initial-owner.js | 11 +++++++---- 5 files changed, 20 insertions(+), 21 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index f3373fe28..6a4d640c5 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -62,4 +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'; \ No newline at end of file +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 index 5bbbb8b80..58a3c7a4d 100644 --- a/packages/core/src/infer-proxy-admin.test.ts +++ b/packages/core/src/infer-proxy-admin.test.ts @@ -18,22 +18,20 @@ function makeProviderError(msg: string): EthereumProvider { test('inferProxyAdmin returns true when owner looks like an address', async t => { // abi encoding of address 0x1000000000000000000000000000000000000123 - const provider = makeProviderReturning( - '0x0000000000000000000000001000000000000000000000000000000000000123', - ); + const provider = makeProviderReturning('0x0000000000000000000000001000000000000000000000000000000000000123'); t.true(await inferProxyAdmin(provider, addr)); }); test('inferProxyAdmin returns false when owner is more than 20 bytes', async t => { - const provider = makeProviderReturning( - '0x0000000000000000000000011000000000000000000000000000000000000123', - ); + const provider = makeProviderReturning('0x0000000000000000000000011000000000000000000000000000000000000123'); t.false(await inferProxyAdmin(provider, addr)); }); test('inferProxyAdmin returns false when owner is a string', async t => { // abi encoding of string 'foo' - const provider = makeProviderReturning('0x00000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000003666f6f0000000000000000000000000000000000000000000000000000000000'); + const provider = makeProviderReturning( + '0x00000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000003666f6f0000000000000000000000000000000000000000000000000000000000', + ); t.false(await inferProxyAdmin(provider, addr)); }); diff --git a/packages/core/src/infer-proxy-admin.ts b/packages/core/src/infer-proxy-admin.ts index 42a9c6d0e..a455bb777 100644 --- a/packages/core/src/infer-proxy-admin.ts +++ b/packages/core/src/infer-proxy-admin.ts @@ -8,10 +8,7 @@ import { parseAddress } from './utils/address'; * @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 { +export async function inferProxyAdmin(provider: EthereumProvider, possibleContractAddress: string): Promise { const owner = await callOptionalSignature(provider, possibleContractAddress, 'owner()'); return owner !== undefined && parseAddress(owner) !== undefined; -} \ No newline at end of file +} diff --git a/packages/plugin-hardhat/src/deploy-proxy.ts b/packages/plugin-hardhat/src/deploy-proxy.ts index 9881d3f64..1fdf68b0b 100644 --- a/packages/plugin-hardhat/src/deploy-proxy.ts +++ b/packages/plugin-hardhat/src/deploy-proxy.ts @@ -87,11 +87,12 @@ export function makeDeployProxy(hre: HardhatRuntimeEnvironment, defenderModule: case 'transparent': { const initialOwner = await getInitialOwner(opts, signer); - if (!opts.unsafeSkipProxyAdminCheck && await inferProxyAdmin(provider, initialOwner)) { + 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.`, - ) + '`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); diff --git a/packages/plugin-hardhat/test/transparent-admin-initial-owner.js b/packages/plugin-hardhat/test/transparent-admin-initial-owner.js index efb27c879..4c849c656 100644 --- a/packages/plugin-hardhat/test/transparent-admin-initial-owner.js +++ b/packages/plugin-hardhat/test/transparent-admin-initial-owner.js @@ -75,7 +75,7 @@ test('initial owner - must not be ProxyAdmin ', async t => { const predeployedProxyAdminAddress = await proxyAdmin.getAddress(); const e = await t.throwsAsync(() => - upgrades.deployProxy(Greeter, ['hello'], { initialOwner: predeployedProxyAdminAddress }) + 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); @@ -89,7 +89,7 @@ test('initial owner - must not be ownable', async t => { const predeployedOwnableAddress = await hasOwner.getAddress(); const e = await t.throwsAsync(() => - upgrades.deployProxy(Greeter, ['hello'], { initialOwner: predeployedOwnableAddress }) + 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); @@ -102,5 +102,8 @@ test('initial owner - skip ProxyAdmin check', async t => { const hasOwner = await HasOwner.deploy(defaultSigner.address); const predeployedOwnableAddress = await hasOwner.getAddress(); - await upgrades.deployProxy(Greeter, ['hello'], { initialOwner: predeployedOwnableAddress, unsafeSkipProxyAdminCheck: true }); -}); \ No newline at end of file + await upgrades.deployProxy(Greeter, ['hello'], { + initialOwner: predeployedOwnableAddress, + unsafeSkipProxyAdminCheck: true, + }); +}); From 88285164baccd5def56bd5c07dcc0138a9b3efdf Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Fri, 20 Sep 2024 15:29:35 -0400 Subject: [PATCH 05/15] Update changelogs --- packages/core/CHANGELOG.md | 4 ++++ packages/plugin-hardhat/CHANGELOG.md | 5 +++++ 2 files changed, 9 insertions(+) diff --git a/packages/core/CHANGELOG.md b/packages/core/CHANGELOG.md index 2212f40d4..55fa83355 100644 --- a/packages/core/CHANGELOG.md +++ b/packages/core/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## Unreleased + +- Adds a check to ensure `initialOwner` is not a ProxyAdmin contract when deploying a transparent proxy from Hardhat. + ## 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/plugin-hardhat/CHANGELOG.md b/packages/plugin-hardhat/CHANGELOG.md index 13fb6b1a6..56f3fdaa0 100644 --- a/packages/plugin-hardhat/CHANGELOG.md +++ b/packages/plugin-hardhat/CHANGELOG.md @@ -1,5 +1,10 @@ # Changelog +## Unreleased + +### Potentially breaking changes +- Adds a check to ensure `initialOwner` is not a ProxyAdmin contract when deploying a transparent proxy. + ## 3.3.0 (2024-09-16) - Defender: Add `metadata` option. ([#1079](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/1079)) From d85028edefcba80b3da8864469eaa640bc07af46 Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Fri, 20 Sep 2024 15:34:34 -0400 Subject: [PATCH 06/15] Bump versions --- packages/core/package.json | 2 +- packages/plugin-hardhat/package.json | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) 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/plugin-hardhat/package.json b/packages/plugin-hardhat/package.json index bd4ee4020..a01123a9f 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", From 267b40ae688d736621310c1ecc1eb08f3a6a3117 Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Fri, 20 Sep 2024 15:35:03 -0400 Subject: [PATCH 07/15] Clarify changelog --- packages/core/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/CHANGELOG.md b/packages/core/CHANGELOG.md index 55fa83355..91b72443d 100644 --- a/packages/core/CHANGELOG.md +++ b/packages/core/CHANGELOG.md @@ -2,7 +2,7 @@ ## Unreleased -- Adds a check to ensure `initialOwner` is not a ProxyAdmin contract when deploying a transparent proxy from Hardhat. +- Supports checking to ensure `initialOwner` is not a ProxyAdmin contract when deploying a transparent proxy from Hardhat. ## 1.37.1 (2024-09-09) From d3870b248e409dbc96abbffea41b19a950b41553 Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Fri, 20 Sep 2024 15:42:11 -0400 Subject: [PATCH 08/15] Update changelogs --- packages/core/CHANGELOG.md | 2 +- packages/plugin-hardhat/CHANGELOG.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/CHANGELOG.md b/packages/core/CHANGELOG.md index 91b72443d..8e1e13740 100644 --- a/packages/core/CHANGELOG.md +++ b/packages/core/CHANGELOG.md @@ -2,7 +2,7 @@ ## Unreleased -- Supports checking to ensure `initialOwner` is not a ProxyAdmin contract when deploying a transparent proxy from Hardhat. +- 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) diff --git a/packages/plugin-hardhat/CHANGELOG.md b/packages/plugin-hardhat/CHANGELOG.md index 56f3fdaa0..2f534a9b7 100644 --- a/packages/plugin-hardhat/CHANGELOG.md +++ b/packages/plugin-hardhat/CHANGELOG.md @@ -3,7 +3,7 @@ ## Unreleased ### Potentially breaking changes -- Adds a check to ensure `initialOwner` is not a ProxyAdmin contract when deploying a transparent proxy. +- 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) From 7c9480aeb32388494343eec8f8b5061f08840b23 Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Fri, 20 Sep 2024 15:43:50 -0400 Subject: [PATCH 09/15] Update doc --- docs/modules/ROOT/pages/api-hardhat-upgrades.adoc | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/modules/ROOT/pages/api-hardhat-upgrades.adoc b/docs/modules/ROOT/pages/api-hardhat-upgrades.adoc index 8bd9cb8e0..1eda02492 100644 --- a/docs/modules/ROOT/pages/api-hardhat-upgrades.adoc +++ b/docs/modules/ROOT/pages/api-hardhat-upgrades.adoc @@ -57,6 +57,7 @@ async function deployProxy( unsafeAllow?: ValidationError[], constructorArgs?: unknown[], initialOwner?: string, + unsafeSkipProxyAdminCheck?: boolean, timeout?: number, pollingInterval?: number, redeployImplementation?: 'always' | 'never' | 'onchange', From caf7553715191c75e481b34fc1f030d7ab966a69 Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Mon, 23 Sep 2024 13:15:09 -0400 Subject: [PATCH 10/15] Clarify testcase and add comment --- packages/core/src/infer-proxy-admin.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/core/src/infer-proxy-admin.test.ts b/packages/core/src/infer-proxy-admin.test.ts index 58a3c7a4d..486a64241 100644 --- a/packages/core/src/infer-proxy-admin.test.ts +++ b/packages/core/src/infer-proxy-admin.test.ts @@ -22,7 +22,8 @@ test('inferProxyAdmin returns true when owner looks like an address', async t => t.true(await inferProxyAdmin(provider, addr)); }); -test('inferProxyAdmin returns false when owner is more than 20 bytes', async t => { +test('inferProxyAdmin returns false when returned value has is more than 20 bytes', async t => { + // some higher order byte beyond 20 bytes is non-zero const provider = makeProviderReturning('0x0000000000000000000000011000000000000000000000000000000000000123'); t.false(await inferProxyAdmin(provider, addr)); }); From 29d2996f99ac47af20bd247403f1fa3d66a03664 Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Mon, 23 Sep 2024 13:17:21 -0400 Subject: [PATCH 11/15] Update changelogs --- packages/core/CHANGELOG.md | 2 +- packages/plugin-hardhat/CHANGELOG.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/CHANGELOG.md b/packages/core/CHANGELOG.md index 8e1e13740..dda1a933f 100644 --- a/packages/core/CHANGELOG.md +++ b/packages/core/CHANGELOG.md @@ -1,6 +1,6 @@ # Changelog -## Unreleased +## 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)) diff --git a/packages/plugin-hardhat/CHANGELOG.md b/packages/plugin-hardhat/CHANGELOG.md index 2f534a9b7..6ed398b6e 100644 --- a/packages/plugin-hardhat/CHANGELOG.md +++ b/packages/plugin-hardhat/CHANGELOG.md @@ -1,6 +1,6 @@ # Changelog -## Unreleased +## 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)) From 8cfa3a9bd783f4d26812da8b71d59c4231faf2aa Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Mon, 23 Sep 2024 13:20:42 -0400 Subject: [PATCH 12/15] Add since --- docs/modules/ROOT/pages/api-hardhat-upgrades.adoc | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/modules/ROOT/pages/api-hardhat-upgrades.adoc b/docs/modules/ROOT/pages/api-hardhat-upgrades.adoc index 1eda02492..11831b4e0 100644 --- a/docs/modules/ROOT/pages/api-hardhat-upgrades.adoc +++ b/docs/modules/ROOT/pages/api-hardhat-upgrades.adoc @@ -23,6 +23,7 @@ The following options are common to some functions. * `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"`. From 31d1073efd265796351e262ea7bdfeca037b9119 Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Mon, 23 Sep 2024 13:25:33 -0400 Subject: [PATCH 13/15] Clarify testcase names --- packages/core/src/infer-proxy-admin.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/src/infer-proxy-admin.test.ts b/packages/core/src/infer-proxy-admin.test.ts index 486a64241..626f7a983 100644 --- a/packages/core/src/infer-proxy-admin.test.ts +++ b/packages/core/src/infer-proxy-admin.test.ts @@ -22,13 +22,13 @@ test('inferProxyAdmin returns true when owner looks like an address', async t => t.true(await inferProxyAdmin(provider, addr)); }); -test('inferProxyAdmin returns false when returned value has is more than 20 bytes', async t => { +test('inferProxyAdmin returns false when returned value is 32 bytes but clearly not an address', async t => { // some higher order byte beyond 20 bytes is non-zero const provider = makeProviderReturning('0x0000000000000000000000011000000000000000000000000000000000000123'); t.false(await inferProxyAdmin(provider, addr)); }); -test('inferProxyAdmin returns false when owner is a string', async t => { +test('inferProxyAdmin returns false when returned value is a string', async t => { // abi encoding of string 'foo' const provider = makeProviderReturning( '0x00000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000003666f6f0000000000000000000000000000000000000000000000000000000000', From 1922f67c15afcbd8e5e2cedf3c7962beded81690 Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Mon, 23 Sep 2024 13:29:30 -0400 Subject: [PATCH 14/15] Clarify comment --- packages/core/src/infer-proxy-admin.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/src/infer-proxy-admin.test.ts b/packages/core/src/infer-proxy-admin.test.ts index 626f7a983..27c720a14 100644 --- a/packages/core/src/infer-proxy-admin.test.ts +++ b/packages/core/src/infer-proxy-admin.test.ts @@ -23,8 +23,8 @@ test('inferProxyAdmin returns true when owner looks like an address', async t => }); test('inferProxyAdmin returns false when returned value is 32 bytes but clearly not an address', async t => { - // some higher order byte beyond 20 bytes is non-zero - const provider = makeProviderReturning('0x0000000000000000000000011000000000000000000000000000000000000123'); + // some higher order bytes beyond 20 bytes are non-zero (the 'abc' in the below) + const provider = makeProviderReturning('0x000000000000000000000abc1000000000000000000000000000000000000123'); t.false(await inferProxyAdmin(provider, addr)); }); From 8ec05d63cbfa0e8548da3d4b34461c74112534b9 Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Mon, 23 Sep 2024 13:59:28 -0400 Subject: [PATCH 15/15] Clarify comment --- packages/core/src/infer-proxy-admin.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/infer-proxy-admin.test.ts b/packages/core/src/infer-proxy-admin.test.ts index 27c720a14..47edf9210 100644 --- a/packages/core/src/infer-proxy-admin.test.ts +++ b/packages/core/src/infer-proxy-admin.test.ts @@ -23,7 +23,7 @@ test('inferProxyAdmin returns true when owner looks like an address', async t => }); test('inferProxyAdmin returns false when returned value is 32 bytes but clearly not an address', async t => { - // some higher order bytes beyond 20 bytes are non-zero (the 'abc' in the below) + // dirty upper bits beyond 20 bytes (the 'abc' in the below) const provider = makeProviderReturning('0x000000000000000000000abc1000000000000000000000000000000000000123'); t.false(await inferProxyAdmin(provider, addr)); });