diff --git a/packages/core/CHANGELOG.md b/packages/core/CHANGELOG.md index 61835b8e8..c7edb7a8b 100644 --- a/packages/core/CHANGELOG.md +++ b/packages/core/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## Unreleased + +- Fix `upgradeProxy` in Hardhat from an implementation that has a fallback function and is not using OpenZeppelin Contracts 5.0. ([#926](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/926)) + ## 1.31.1 (2023-11-01) - CLI: Throw error if `--requireReference` and `--unsafeSkipStorageCheck` are both enabled. ([#913](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/913)) diff --git a/packages/core/src/upgrade-interface-version.test.ts b/packages/core/src/upgrade-interface-version.test.ts index a0986dec2..bc85cc6de 100644 --- a/packages/core/src/upgrade-interface-version.test.ts +++ b/packages/core/src/upgrade-interface-version.test.ts @@ -35,3 +35,9 @@ test('getUpgradeInterfaceVersion returns undefined for invalid selector', async ); t.is(await getUpgradeInterfaceVersion(provider, hash), undefined); }); + +test('getUpgradeInterfaceVersion returns undefined for non-string type', async t => { + // abi encoding of boolean 'true' + const provider = makeProviderReturning('0x0000000000000000000000000000000000000000000000000000000000000001'); + t.is(await getUpgradeInterfaceVersion(provider, hash), undefined); +}); diff --git a/packages/core/src/upgrade-interface-version.ts b/packages/core/src/upgrade-interface-version.ts index 8667c397a..f074ad0da 100644 --- a/packages/core/src/upgrade-interface-version.ts +++ b/packages/core/src/upgrade-interface-version.ts @@ -1,9 +1,11 @@ +import debug from './utils/debug'; import { callOptionalSignature } from './call-optional-signature'; import { EthereumProvider } from './provider'; export async function getUpgradeInterfaceVersion( provider: EthereumProvider, address: string, + log = debug, ): Promise { const encodedVersion = await callOptionalSignature(provider, address, 'UPGRADE_INTERFACE_VERSION()'); if (encodedVersion !== undefined) { @@ -13,7 +15,10 @@ export async function getUpgradeInterfaceVersion( // The first 32 bytes represent the offset, which should be 32 for a string const offset = parseInt(buf.slice(0, 32).toString('hex'), 16); if (offset !== 32) { - throw new Error(`Unexpected type for UPGRADE_INTERFACE_VERSION at address ${address}. Expected a string`); + // Log as debug and return undefined if the interface version is not a string. + // Do not throw an error because this could be caused by a fallback function. + log(`Unexpected type for UPGRADE_INTERFACE_VERSION at address ${address}. Expected a string`); + return undefined; } // The next 32 bytes represent the length of the string diff --git a/packages/plugin-hardhat/CHANGELOG.md b/packages/plugin-hardhat/CHANGELOG.md index e42063672..91db9c484 100644 --- a/packages/plugin-hardhat/CHANGELOG.md +++ b/packages/plugin-hardhat/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## Unreleased + +- Fix `upgradeProxy` from an implementation that has a fallback function and is not using OpenZeppelin Contracts 5.0. ([#926](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/926)) + ## 2.4.1 (2023-11-14) - Update Defender SDK. ([#924](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/924)) diff --git a/packages/plugin-hardhat/contracts/GreeterProxiable40Fallback.sol b/packages/plugin-hardhat/contracts/GreeterProxiable40Fallback.sol new file mode 100644 index 000000000..63cfdca5c --- /dev/null +++ b/packages/plugin-hardhat/contracts/GreeterProxiable40Fallback.sol @@ -0,0 +1,23 @@ +pragma solidity >= 0.6.0 <0.8.0; + +import { Proxiable } from "./utils/Proxiable.sol"; + +contract GreeterProxiable40Fallback is Proxiable { + string greeting; + + function initialize(string memory _greeting) public { + greeting = _greeting; + } + + function greet() public view returns (string memory) { + return greeting; + } + + fallback() external {} +} + +contract GreeterProxiable40FallbackV2 is GreeterProxiable40Fallback { + function resetGreeting() public { + greeting = "Hello World"; + } +} \ No newline at end of file diff --git a/packages/plugin-hardhat/contracts/GreeterProxiable40FallbackString.sol b/packages/plugin-hardhat/contracts/GreeterProxiable40FallbackString.sol new file mode 100644 index 000000000..f5cc8978f --- /dev/null +++ b/packages/plugin-hardhat/contracts/GreeterProxiable40FallbackString.sol @@ -0,0 +1,25 @@ +pragma solidity >= 0.6.0 <0.8.0; + +import { Proxiable } from "./utils/Proxiable.sol"; + +contract GreeterProxiable40FallbackString is Proxiable { + string greeting; + + function initialize(string memory _greeting) public { + greeting = _greeting; + } + + function greet() public view returns (string memory) { + return greeting; + } + + fallback(bytes calldata) external returns (bytes memory) { + return abi.encode(greeting); + } +} + +contract GreeterProxiable40FallbackStringV2 is GreeterProxiable40FallbackString { + function resetGreeting() public { + greeting = "Hello World"; + } +} \ No newline at end of file diff --git a/packages/plugin-hardhat/contracts/GreeterTransparent40Fallback.sol b/packages/plugin-hardhat/contracts/GreeterTransparent40Fallback.sol new file mode 100644 index 000000000..ab2ddc04a --- /dev/null +++ b/packages/plugin-hardhat/contracts/GreeterTransparent40Fallback.sol @@ -0,0 +1,44 @@ +pragma solidity >= 0.6.0 <0.8.0; + +// These contracts are for testing only, they are not safe for use in production. + +interface ITransparentUpgradeableProxy { + function upgradeTo(address) external; + function upgradeToAndCall(address, bytes memory) external payable; +} + +contract UnsafeAdminFallback { + // NOT SAFE FOR PRODUCTION USE. ANYONE CAN UPGRADE THE PROXY THROUGH THE BELOW. + + function upgrade(ITransparentUpgradeableProxy proxy, address implementation) public virtual { + proxy.upgradeTo(implementation); + } + + function upgradeAndCall( + ITransparentUpgradeableProxy proxy, + address implementation, + bytes memory data + ) public payable virtual { + proxy.upgradeToAndCall{value: msg.value}(implementation, data); + } + + fallback() external {} +} + +contract GreeterTransparent40Fallback { + string greeting; + + function initialize(string memory _greeting) public { + greeting = _greeting; + } + + function greet() public view returns (string memory) { + return greeting; + } +} + +contract GreeterTransparent40FallbackV2 is GreeterTransparent40Fallback { + function resetGreeting() public { + greeting = "Hello World"; + } +} \ No newline at end of file diff --git a/packages/plugin-hardhat/contracts/GreeterTransparent40FallbackString.sol b/packages/plugin-hardhat/contracts/GreeterTransparent40FallbackString.sol new file mode 100644 index 000000000..e76642f73 --- /dev/null +++ b/packages/plugin-hardhat/contracts/GreeterTransparent40FallbackString.sol @@ -0,0 +1,46 @@ +pragma solidity >= 0.6.0 <0.8.0; + +// These contracts are for testing only, they are not safe for use in production. + +interface ITransparentUpgradeableProxy { + function upgradeTo(address) external; + function upgradeToAndCall(address, bytes memory) external payable; +} + +contract UnsafeAdminFallbackString { + // NOT SAFE FOR PRODUCTION USE + + function upgrade(ITransparentUpgradeableProxy proxy, address implementation) public virtual { + proxy.upgradeTo(implementation); + } + + function upgradeAndCall( + ITransparentUpgradeableProxy proxy, + address implementation, + bytes memory data + ) public payable virtual { + proxy.upgradeToAndCall{value: msg.value}(implementation, data); + } + + fallback(bytes calldata) external returns (bytes memory) { + return abi.encode("foo"); + } +} + +contract GreeterTransparent40FallbackString { + string greeting; + + function initialize(string memory _greeting) public { + greeting = _greeting; + } + + function greet() public view returns (string memory) { + return greeting; + } +} + +contract GreeterTransparent40FallbackStringV2 is GreeterTransparent40FallbackString { + function resetGreeting() public { + greeting = "Hello World"; + } +} \ No newline at end of file diff --git a/packages/plugin-hardhat/src/upgrade-proxy.ts b/packages/plugin-hardhat/src/upgrade-proxy.ts index cc40fc539..97e9d4e66 100644 --- a/packages/plugin-hardhat/src/upgrade-proxy.ts +++ b/packages/plugin-hardhat/src/upgrade-proxy.ts @@ -1,5 +1,6 @@ import { HardhatRuntimeEnvironment } from 'hardhat/types'; import type { ethers, ContractFactory, Contract, Signer } from 'ethers'; +import debug from './utils/debug'; import { getAdminAddress, getCode, getUpgradeInterfaceVersion, isEmptySlot } from '@openzeppelin/upgrades-core'; @@ -21,7 +22,11 @@ export type UpgradeFunction = ( opts?: UpgradeProxyOptions, ) => Promise; -export function makeUpgradeProxy(hre: HardhatRuntimeEnvironment, defenderModule: boolean): UpgradeFunction { +export function makeUpgradeProxy( + hre: HardhatRuntimeEnvironment, + defenderModule: boolean, + log = debug, +): UpgradeFunction { return async function upgradeProxy(proxy, ImplFactory, opts: UpgradeProxyOptions = {}) { disableDefender(hre, defenderModule, opts, upgradeProxy.name); @@ -54,17 +59,20 @@ export function makeUpgradeProxy(hre: HardhatRuntimeEnvironment, defenderModule: const ITransparentUpgradeableProxyFactory = await getITransparentUpgradeableProxyFactory(hre, signer); const proxy = attach(ITransparentUpgradeableProxyFactory, proxyAddress); - const upgradeInterfaceVersion = await getUpgradeInterfaceVersion(provider, proxyAddress); + const upgradeInterfaceVersion = await getUpgradeInterfaceVersion(provider, proxyAddress, log); return (nextImpl, call) => { - if (upgradeInterfaceVersion === undefined) { - return call ? proxy.upgradeToAndCall(nextImpl, call, ...overrides) : proxy.upgradeTo(nextImpl, ...overrides); - } else if (upgradeInterfaceVersion === '5.0.0') { + if (upgradeInterfaceVersion === '5.0.0') { return proxy.upgradeToAndCall(nextImpl, call ?? '0x', ...overrides); } else { - throw new Error( - `Unknown UPGRADE_INTERFACE_VERSION ${upgradeInterfaceVersion} for proxy at ${proxyAddress}. Expected 5.0.0`, - ); + if (upgradeInterfaceVersion !== undefined) { + // Log as debug if the interface version is an unknown string. + // Do not throw an error because this could be caused by a fallback function. + log( + `Unknown UPGRADE_INTERFACE_VERSION ${upgradeInterfaceVersion} for proxy at ${proxyAddress}. Expected 5.0.0`, + ); + } + return call ? proxy.upgradeToAndCall(nextImpl, call, ...overrides) : proxy.upgradeTo(nextImpl, ...overrides); } }; } else { @@ -72,19 +80,22 @@ export function makeUpgradeProxy(hre: HardhatRuntimeEnvironment, defenderModule: const AdminFactory = await getProxyAdminFactory(hre, signer); const admin = attach(AdminFactory, adminAddress); - const upgradeInterfaceVersion = await getUpgradeInterfaceVersion(provider, adminAddress); + const upgradeInterfaceVersion = await getUpgradeInterfaceVersion(provider, adminAddress, log); return (nextImpl, call) => { - if (upgradeInterfaceVersion === undefined) { + if (upgradeInterfaceVersion === '5.0.0') { + return admin.upgradeAndCall(proxyAddress, nextImpl, call ?? '0x', ...overrides); + } else { + if (upgradeInterfaceVersion !== undefined) { + // Log as debug if the interface version is an unknown string. + // Do not throw an error because this could be caused by a fallback function. + log( + `Unknown UPGRADE_INTERFACE_VERSION ${upgradeInterfaceVersion} for proxy admin at ${adminAddress}. Expected 5.0.0`, + ); + } return call ? admin.upgradeAndCall(proxyAddress, nextImpl, call, ...overrides) : admin.upgrade(proxyAddress, nextImpl, ...overrides); - } else if (upgradeInterfaceVersion === '5.0.0') { - return admin.upgradeAndCall(proxyAddress, nextImpl, call ?? '0x', ...overrides); - } else { - throw new Error( - `Unknown UPGRADE_INTERFACE_VERSION ${upgradeInterfaceVersion} for proxy admin at ${adminAddress}. Expected 5.0.0`, - ); } }; } diff --git a/packages/plugin-hardhat/test/transparent-admin-unknown-upgrade-interface.js b/packages/plugin-hardhat/test/transparent-admin-unknown-upgrade-interface.js new file mode 100644 index 000000000..a295b3785 --- /dev/null +++ b/packages/plugin-hardhat/test/transparent-admin-unknown-upgrade-interface.js @@ -0,0 +1,104 @@ +const test = require('ava'); +const sinon = require('sinon'); + +const { ethers, upgrades } = require('hardhat'); +const hre = require('hardhat'); + +const TransparentUpgradableProxy = require('@openzeppelin/upgrades-core/artifacts/@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol/TransparentUpgradeableProxy.json'); + +test.before(async t => { + t.context.GreeterTransparent40Fallback = await ethers.getContractFactory('GreeterTransparent40Fallback'); + t.context.GreeterTransparent40FallbackV2 = await ethers.getContractFactory('GreeterTransparent40FallbackV2'); + t.context.UnsafeAdminFallback = await ethers.getContractFactory('UnsafeAdminFallback'); + + t.context.GreeterTransparent40FallbackString = await ethers.getContractFactory('GreeterTransparent40FallbackString'); + t.context.GreeterTransparent40FallbackStringV2 = await ethers.getContractFactory( + 'GreeterTransparent40FallbackStringV2', + ); + t.context.UnsafeAdminFallbackString = await ethers.getContractFactory('UnsafeAdminFallbackString'); + + t.context.TransparentUpgradableProxy = await ethers.getContractFactory( + TransparentUpgradableProxy.abi, + TransparentUpgradableProxy.bytecode, + ); +}); + +function getInitializerData(contractInterface, args) { + return contractInterface.encodeFunctionData('initialize', args); +} + +test('admin with unknown upgrades interface version due to fallback returning non-string', async t => { + const { + GreeterTransparent40Fallback, + GreeterTransparent40FallbackV2, + UnsafeAdminFallback, + TransparentUpgradableProxy, + } = t.context; + + const impl = await GreeterTransparent40Fallback.deploy(); + await impl.waitForDeployment(); + const admin = await UnsafeAdminFallback.deploy(); + await admin.waitForDeployment(); + const proxy = await TransparentUpgradableProxy.deploy( + await impl.getAddress(), + await admin.getAddress(), + getInitializerData(GreeterTransparent40Fallback.interface, ['Hello, Hardhat!']), + ); + await proxy.waitForDeployment(); + + const greeter = GreeterTransparent40Fallback.attach(await proxy.getAddress()); + t.is(await greeter.greet(), 'Hello, Hardhat!'); + + await upgrades.forceImport(await proxy.getAddress(), GreeterTransparent40Fallback); + + const debugStub = sinon.stub(); + const upgradeProxy = require('../dist/upgrade-proxy').makeUpgradeProxy(hre, false, debugStub); + + const greeter2 = await upgradeProxy(proxy, GreeterTransparent40FallbackV2); + await greeter2.resetGreeting(); + t.is(await greeter2.greet(), 'Hello World'); + + t.true( + debugStub.calledWith( + `Unexpected type for UPGRADE_INTERFACE_VERSION at address ${await admin.getAddress()}. Expected a string`, + ), + ); +}); + +test('admin with unknown upgrades interface version due to fallback returning string', async t => { + const { + GreeterTransparent40FallbackString, + GreeterTransparent40FallbackStringV2, + UnsafeAdminFallbackString, + TransparentUpgradableProxy, + } = t.context; + + const impl = await GreeterTransparent40FallbackString.deploy(); + await impl.waitForDeployment(); + const admin = await UnsafeAdminFallbackString.deploy(); + await admin.waitForDeployment(); + const proxy = await TransparentUpgradableProxy.deploy( + await impl.getAddress(), + await admin.getAddress(), + getInitializerData(GreeterTransparent40FallbackString.interface, ['Hello, Hardhat!']), + ); + await proxy.waitForDeployment(); + + const greeter = GreeterTransparent40FallbackString.attach(await proxy.getAddress()); + t.is(await greeter.greet(), 'Hello, Hardhat!'); + + await upgrades.forceImport(await proxy.getAddress(), GreeterTransparent40FallbackString); + + const debugStub = sinon.stub(); + const upgradeProxy = require('../dist/upgrade-proxy').makeUpgradeProxy(hre, false, debugStub); + + const greeter2 = await upgradeProxy(proxy, GreeterTransparent40FallbackStringV2); + await greeter2.resetGreeting(); + t.is(await greeter2.greet(), 'Hello World'); + + t.true( + debugStub.calledWith( + `Unknown UPGRADE_INTERFACE_VERSION foo for proxy admin at ${await admin.getAddress()}. Expected 5.0.0`, + ), + ); +}); diff --git a/packages/plugin-hardhat/test/uups-unknown-upgrade-interface.js b/packages/plugin-hardhat/test/uups-unknown-upgrade-interface.js new file mode 100644 index 000000000..06b449a9b --- /dev/null +++ b/packages/plugin-hardhat/test/uups-unknown-upgrade-interface.js @@ -0,0 +1,54 @@ +const test = require('ava'); +const sinon = require('sinon'); + +const { ethers, upgrades } = require('hardhat'); +const hre = require('hardhat'); + +test.before(async t => { + t.context.GreeterProxiable40Fallback = await ethers.getContractFactory('GreeterProxiable40Fallback'); + t.context.GreeterProxiable40FallbackV2 = await ethers.getContractFactory('GreeterProxiable40FallbackV2'); + + t.context.GreeterProxiable40FallbackString = await ethers.getContractFactory('GreeterProxiable40FallbackString'); + t.context.GreeterProxiable40FallbackStringV2 = await ethers.getContractFactory('GreeterProxiable40FallbackStringV2'); +}); + +test('unknown upgrades interface version due to fallback returning non-string', async t => { + const { GreeterProxiable40Fallback, GreeterProxiable40FallbackV2 } = t.context; + + const greeter = await upgrades.deployProxy(GreeterProxiable40Fallback, ['Hello, Hardhat!'], { kind: 'uups' }); + t.is(await greeter.greet(), 'Hello, Hardhat!'); + + const debugStub = sinon.stub(); + const upgradeProxy = require('../dist/upgrade-proxy').makeUpgradeProxy(hre, false, debugStub); + + const greeter2 = await upgradeProxy(greeter, GreeterProxiable40FallbackV2); + + await greeter2.resetGreeting(); + t.is(await greeter2.greet(), 'Hello World'); + + t.true( + debugStub.calledWith( + `Unexpected type for UPGRADE_INTERFACE_VERSION at address ${await greeter.getAddress()}. Expected a string`, + ), + ); +}); + +test('unknown upgrades interface version due to fallback returning string', async t => { + const { GreeterProxiable40FallbackString, GreeterProxiable40FallbackStringV2 } = t.context; + + const greeter = await upgrades.deployProxy(GreeterProxiable40FallbackString, ['Hello, Hardhat!'], { kind: 'uups' }); + t.is(await greeter.greet(), 'Hello, Hardhat!'); + + const debugStub = sinon.stub(); + const upgradeProxy = require('../dist/upgrade-proxy').makeUpgradeProxy(hre, false, debugStub); + + const greeter2 = await upgradeProxy(greeter, GreeterProxiable40FallbackStringV2); + await greeter2.resetGreeting(); + t.is(await greeter2.greet(), 'Hello World'); + + t.true( + debugStub.calledWith( + `Unknown UPGRADE_INTERFACE_VERSION Hello, Hardhat! for proxy at ${await greeter.getAddress()}. Expected 5.0.0`, + ), + ); +});