From bd2e37eb68d6c7a8e6412393c050970ecf83b40f Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Wed, 27 Sep 2023 16:12:30 -0400 Subject: [PATCH 1/4] Assert non-zero admin address when import transparent proxy --- packages/plugin-hardhat/src/force-import.ts | 11 +++++++++++ packages/plugin-hardhat/test/import-50.js | 17 ++++++++++------- packages/plugin-hardhat/test/import.js | 17 ++++++++++------- 3 files changed, 31 insertions(+), 14 deletions(-) diff --git a/packages/plugin-hardhat/src/force-import.ts b/packages/plugin-hardhat/src/force-import.ts index 0d254e6d7..0f63c2b23 100644 --- a/packages/plugin-hardhat/src/force-import.ts +++ b/packages/plugin-hardhat/src/force-import.ts @@ -13,6 +13,8 @@ import { ProxyDeployment, hasCode, NoContractImportError, + isEmptySlot, + UpgradesError, } from '@openzeppelin/upgrades-core'; import { @@ -108,5 +110,14 @@ async function addAdminToManifest( opts: ForceImportOptions, ) { const adminAddress = await getAdminAddress(provider, proxyAddress); + if (isEmptySlot(adminAddress)) { + // Asser that the admin slot of a transparent proxy is not zero, otherwise the simulation below would fail due to no code at the address. + // Note: Transparent proxies should not have the zero address as the admin, according to TransparentUpgradeableProxy's _setAdmin function. + throw new UpgradesError( + `Proxy at ${proxyAddress} doesn't look like a transparent proxy`, + () => + `Set the \`kind\` option to the kind of proxy that was deployed at ${proxyAddress} (either 'uups' or 'beacon')`, + ); + } await simulateDeployAdmin(hre, ImplFactory, opts, adminAddress); } diff --git a/packages/plugin-hardhat/test/import-50.js b/packages/plugin-hardhat/test/import-50.js index 899fad9bd..35db16bc5 100644 --- a/packages/plugin-hardhat/test/import-50.js +++ b/packages/plugin-hardhat/test/import-50.js @@ -38,7 +38,7 @@ function getInitializerData(contractInterface, args) { return contractInterface.encodeFunctionData(fragment, args); } -const REQUESTED_UPGRADE_WRONG_KIND = 'Requested an upgrade of kind uups but proxy is transparent'; +const NOT_TRANSPARENT_PROXY = `doesn't look like a transparent proxy`; test('implementation happy path', async t => { const { GreeterProxiable } = t.context; @@ -186,13 +186,16 @@ test('wrong kind', async t => { ); await proxy.waitForDeployment(); - // specify wrong kind - const greeter = await upgrades.forceImport(await proxy.getAddress(), GreeterProxiable, { kind: 'transparent' }); - t.is(await greeter.greet(), 'Hello, Hardhat!'); + // specify wrong kind. + // an error is expected since the admin adress is zero + const e = await t.throwsAsync(async () => + upgrades.forceImport(await proxy.getAddress(), GreeterProxiable, { kind: 'transparent' }), + ); + t.true(e.message.includes(NOT_TRANSPARENT_PROXY), e.message); - // an error is expected since the user force imported the wrong kind - const e = await t.throwsAsync(() => upgrades.upgradeProxy(greeter, GreeterV2Proxiable)); - t.true(e.message.startsWith(REQUESTED_UPGRADE_WRONG_KIND), e.message); + // import with correct kind + const greeter = await upgrades.forceImport(await proxy.getAddress(), GreeterProxiable, { kind: 'uups' }); + await upgrades.upgradeProxy(greeter, GreeterV2Proxiable); }); test('import custom UUPS proxy', async t => { diff --git a/packages/plugin-hardhat/test/import.js b/packages/plugin-hardhat/test/import.js index 7122da95e..533ab36d7 100644 --- a/packages/plugin-hardhat/test/import.js +++ b/packages/plugin-hardhat/test/import.js @@ -38,7 +38,7 @@ function getInitializerData(contractInterface, args) { return contractInterface.encodeFunctionData(fragment, args); } -const REQUESTED_UPGRADE_WRONG_KIND = 'Requested an upgrade of kind uups but proxy is transparent'; +const NOT_TRANSPARENT_PROXY = `doesn't look like a transparent proxy`; test('implementation happy path', async t => { const { GreeterProxiable } = t.context; @@ -180,13 +180,16 @@ test('wrong kind', async t => { ); await proxy.waitForDeployment(); - // specify wrong kind - const greeter = await upgrades.forceImport(await proxy.getAddress(), GreeterProxiable, { kind: 'transparent' }); - t.is(await greeter.greet(), 'Hello, Hardhat!'); + // specify wrong kind. + // an error is expected since the admin adress is zero + const e = await t.throwsAsync(async () => + upgrades.forceImport(await proxy.getAddress(), GreeterProxiable, { kind: 'transparent' }), + ); + t.true(e.message.includes(NOT_TRANSPARENT_PROXY), e.message); - // an error is expected since the user force imported the wrong kind - const e = await t.throwsAsync(() => upgrades.upgradeProxy(greeter, GreeterV2Proxiable)); - t.true(e.message.startsWith(REQUESTED_UPGRADE_WRONG_KIND), e.message); + // import with correct kind + const greeter = await upgrades.forceImport(await proxy.getAddress(), GreeterProxiable, { kind: 'uups' }); + await upgrades.upgradeProxy(greeter, GreeterV2Proxiable); }); test('import custom UUPS proxy', async t => { From 6fcf4e695e95e8c66815bb97e9c4bdcf1c4bbb35 Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Wed, 27 Sep 2023 16:24:37 -0400 Subject: [PATCH 2/4] Update changelog --- packages/plugin-hardhat/CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/plugin-hardhat/CHANGELOG.md b/packages/plugin-hardhat/CHANGELOG.md index f8b2a524a..9b2429389 100644 --- a/packages/plugin-hardhat/CHANGELOG.md +++ b/packages/plugin-hardhat/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## Unreleased + +- Check for non-zero admin address when importing transparent proxy. ([#887](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/887)) + ## 2.3.0 (2023-09-27) - Support new upgrade interface in OpenZeppelin Contracts 5.0. ([#883](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/883)) From 1eaed0053d6c3286244798ca27601f20da370ee7 Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Wed, 27 Sep 2023 16:37:44 -0400 Subject: [PATCH 3/4] Update packages/plugin-hardhat/src/force-import.ts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ernesto García --- packages/plugin-hardhat/src/force-import.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/plugin-hardhat/src/force-import.ts b/packages/plugin-hardhat/src/force-import.ts index 0f63c2b23..641dbd067 100644 --- a/packages/plugin-hardhat/src/force-import.ts +++ b/packages/plugin-hardhat/src/force-import.ts @@ -111,7 +111,7 @@ async function addAdminToManifest( ) { const adminAddress = await getAdminAddress(provider, proxyAddress); if (isEmptySlot(adminAddress)) { - // Asser that the admin slot of a transparent proxy is not zero, otherwise the simulation below would fail due to no code at the address. + // Assert that the admin slot of a transparent proxy is not zero, otherwise the simulation below would fail due to no code at the address. // Note: Transparent proxies should not have the zero address as the admin, according to TransparentUpgradeableProxy's _setAdmin function. throw new UpgradesError( `Proxy at ${proxyAddress} doesn't look like a transparent proxy`, From 6d58e19948dee7f62f2cae5eeb5ea55c2ee78af1 Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Wed, 27 Sep 2023 22:26:35 -0400 Subject: [PATCH 4/4] Add more context to error message --- packages/plugin-hardhat/src/force-import.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/plugin-hardhat/src/force-import.ts b/packages/plugin-hardhat/src/force-import.ts index 641dbd067..bebba9aa9 100644 --- a/packages/plugin-hardhat/src/force-import.ts +++ b/packages/plugin-hardhat/src/force-import.ts @@ -116,6 +116,7 @@ async function addAdminToManifest( throw new UpgradesError( `Proxy at ${proxyAddress} doesn't look like a transparent proxy`, () => + `The proxy doesn't look like a transparent proxy because its admin address slot is empty. ` + `Set the \`kind\` option to the kind of proxy that was deployed at ${proxyAddress} (either 'uups' or 'beacon')`, ); }