Ignore UPGRADE_INTERFACE_VERSION if selector returns anything other t…

…han "5.0.0" (#926)

Co-authored-by: Ernesto García <[email protected]>

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))

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);
+});

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<string | undefined> {
   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

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))

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";
+    }
+}

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";
+    }
+}

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";
+    }
+}

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";
+    }
+}

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<Contract>;
 
-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,37 +59,43 @@ 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 {
       // Admin contract: redirect upgrade call through it
       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`,
-          );
         }
       };
     }

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`,
+    ),
+  );
+});