Avoid uninitialized proxies (#5906)

Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>

Author: ernestognw

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 ### Breaking changes
 
+- `ERC1967Proxy` and `TransparentUpgradeableProxy`: Mandate initialization during construction. Deployment now reverts with `ERC1967ProxyUninitialized` if an initialize call is not provided. Developers that rely on the previous behavior and want to disable this check can do so by overriding the internal `_unsafeAllowUninitialized` function to return true.
 - `ERC721` and `ERC1155`: Prevent setting an operator for `address(0)`. In the case of `ERC721` this type of operator allowance could lead to obfuscated mint permission.
 - `RLP`: The `encode(bytes32)` function now encodes `bytes32` as a fixed size item and not as a scalar in `encode(uint256)`. Users must replace calls to `encode(bytes32)` with `encode(uint256(bytes32))` to preserve the same behavior.
 

contracts/mocks/proxy/ERC1967ProxyUnsafe.sol

@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: MIT
+
+pragma solidity ^0.8.22;
+
+import {ERC1967Proxy} from "../../proxy/ERC1967/ERC1967Proxy.sol";
+
+contract ERC1967ProxyUnsafe is ERC1967Proxy {
+    constructor(address implementation, bytes memory _data) payable ERC1967Proxy(implementation, _data) {}
+
+    function _unsafeAllowUninitialized() internal pure override returns (bool) {
+        return true;
+    }
+}

contracts/proxy/ERC1967/ERC1967Proxy.sol

@@ -13,17 +13,28 @@ import {ERC1967Utils} from "./ERC1967Utils.sol";
  * implementation behind the proxy.
  */
 contract ERC1967Proxy is Proxy {
+    /**
+     * @dev The proxy is left uninitialized.
+     */
+    error ERC1967ProxyUninitialized();
+
     /**
      * @dev Initializes the upgradeable proxy with an initial implementation specified by `implementation`.
      *
-     * If `_data` is nonempty, it's used as data in a delegate call to `implementation`. This will typically be an
-     * encoded function call, and allows initializing the storage of the proxy like a Solidity constructor.
+     * Provided `_data` is passed in a delegate call to `implementation`. This will typically be an encoded function
+     * call, and allows initializing the storage of the proxy like a Solidity constructor. By default construction
+     * will fail if `_data` is empty. This behavior can be overridden using a custom {_unsafeAllowUninitialized} that
+     * returns true. In that case, empty `_data` is ignored and no delegate call to the implementation is performed
+     * during construction.
      *
      * Requirements:
      *
      * - If `data` is empty, `msg.value` must be zero.
      */
     constructor(address implementation, bytes memory _data) payable {
+        if (!_unsafeAllowUninitialized() && _data.length == 0) {
+            revert ERC1967ProxyUninitialized();
+        }
         ERC1967Utils.upgradeToAndCall(implementation, _data);
     }
 
@@ -37,4 +48,15 @@ contract ERC1967Proxy is Proxy {
     function _implementation() internal view virtual override returns (address) {
         return ERC1967Utils.getImplementation();
     }
+
+    /**
+     * @dev Returns whether the proxy can be left uninitialized.
+     *
+     * NOTE: Override this function to allow the proxy to be left uninitialized.
+     * Consider uninitialized proxies might be susceptible to man-in-the-middle threats
+     * where the proxy is replaced with a malicious one.
+     */
+    function _unsafeAllowUninitialized() internal pure virtual returns (bool) {
+        return false;
+    }
 }

scripts/upgradeable/transpile.sh

@@ -32,6 +32,7 @@ npx @openzeppelin/upgrade-safe-transpiler -D \
   -b "$build_info" \
   -i contracts/proxy/utils/Initializable.sol \
   -x 'contracts-exposed/**/*' \
+  -x 'contracts/mocks/**/*Proxy*.sol' \
   -x 'contracts/proxy/**/*Proxy*.sol' \
   -x 'contracts/proxy/beacon/UpgradeableBeacon.sol' \
   -p 'contracts/access/manager/AccessManager.sol' \

test/proxy/ERC1967/ERC1967Proxy.test.js

@@ -8,16 +8,29 @@ const fixture = async () => {
 
   const implementation = await ethers.deployContract('DummyImplementation');
 
-  const createProxy = (implementation, initData, opts) =>
-    ethers.deployContract('ERC1967Proxy', [implementation, initData], opts);
-
-  return { nonContractAddress, implementation, createProxy };
+  return { nonContractAddress, implementation };
 };
 
 describe('ERC1967Proxy', function () {
   beforeEach(async function () {
     Object.assign(this, await loadFixture(fixture));
   });
 
-  shouldBehaveLikeProxy();
+  describe('(default) allowUninitialized is false', function () {
+    before(function () {
+      this.createProxy = (implementation, initData, opts) =>
+        ethers.deployContract('ERC1967Proxy', [implementation, initData], opts);
+    });
+
+    shouldBehaveLikeProxy(false);
+  });
+
+  describe('(unsafe) allowUninitialized is true', function () {
+    before(function () {
+      this.createProxy = (implementation, initData, opts) =>
+        ethers.deployContract('ERC1967ProxyUnsafe', [implementation, initData], opts);
+    });
+
+    shouldBehaveLikeProxy(true);
+  });
 });

test/proxy/Proxy.behaviour.js

@@ -3,10 +3,11 @@ const { expect } = require('chai');
 
 const { getAddressInSlot, ImplementationSlot } = require('../helpers/storage');
 
-module.exports = function shouldBehaveLikeProxy() {
+module.exports = function shouldBehaveLikeProxy(allowUninitialized = false) {
   it('cannot be initialized with a non-contract address', async function () {
-    const initializeData = '0x';
+    const initializeData = '0x00'; // non empty data to avoid uninitialized error
     const contractFactory = await ethers.getContractFactory('ERC1967Proxy');
+
     await expect(this.createProxy(this.nonContractAddress, initializeData))
       .to.be.revertedWithCustomError(contractFactory, 'ERC1967InvalidImplementation')
       .withArgs(this.nonContractAddress);
@@ -28,23 +29,35 @@ module.exports = function shouldBehaveLikeProxy() {
   };
 
   describe('without initialization', function () {
-    const initializeData = '0x';
+    if (allowUninitialized) {
+      const initializeData = '0x';
 
-    describe('when not sending balance', function () {
-      beforeEach('creating proxy', async function () {
-        this.proxy = await this.createProxy(this.implementation, initializeData);
+      describe('when not sending balance', function () {
+        beforeEach('creating proxy', async function () {
+          this.proxy = await this.createProxy(this.implementation, initializeData);
+        });
+
+        assertProxyInitialization({ value: 0n, balance: 0n });
       });
 
-      assertProxyInitialization({ value: 0n, balance: 0n });
-    });
+      describe('when sending some balance', function () {
+        const value = 10n ** 5n;
 
-    describe('when sending some balance', function () {
-      const value = 10n ** 5n;
+        it('reverts', async function () {
+          await expect(this.createProxy(this.implementation, initializeData, { value })).to.be.reverted;
+        });
+      });
+    } else {
+      it('reverts without initialization', async function () {
+        const initializeData = '0x'; // empty data causes uninitialized error
+        const contractFactory = await ethers.getContractFactory('ERC1967Proxy');
 
-      it('reverts', async function () {
-        await expect(this.createProxy(this.implementation, initializeData, { value })).to.be.reverted;
+        await expect(this.createProxy(this.implementation, initializeData)).to.be.revertedWithCustomError(
+          contractFactory,
+          'ERC1967ProxyUninitialized',
+        );
       });
-    });
+    }
   });
 
   describe('initialization without parameters', function () {

test/proxy/transparent/ProxyAdmin.test.js

@@ -11,7 +11,7 @@ async function fixture() {
   const v2 = await ethers.deployContract('DummyImplementationV2');
 
   const proxy = await ethers
-    .deployContract('TransparentUpgradeableProxy', [v1, admin, '0x'])
+    .deployContract('TransparentUpgradeableProxy', [v1, admin, v1.interface.encodeFunctionData('initializeNonPayable')])
     .then(instance => ethers.getContractAt('ITransparentUpgradeableProxy', instance));
 
   const proxyAdmin = await ethers.getContractAt(

test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js

@@ -38,7 +38,13 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy() {
   });
 
   beforeEach(async function () {
-    Object.assign(this, await this.createProxyWithImpersonatedProxyAdmin(this.implementationV0, '0x'));
+    Object.assign(
+      this,
+      await this.createProxyWithImpersonatedProxyAdmin(
+        this.implementationV0,
+        this.implementationV0.interface.encodeFunctionData('initializeNonPayable'),
+      ),
+    );
   });
 
   describe('implementation', function () {
@@ -239,7 +245,13 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy() {
       this.clashingImplV0 = await ethers.deployContract('ClashingImplementation');
       this.clashingImplV1 = await ethers.deployContract('ClashingImplementation');
 
-      Object.assign(this, await this.createProxyWithImpersonatedProxyAdmin(this.clashingImplV0, '0x'));
+      Object.assign(
+        this,
+        await this.createProxyWithImpersonatedProxyAdmin(
+          this.clashingImplV0,
+          this.clashingImplV0.interface.encodeFunctionData('delegatedFunction'), // use a pure function with no state change as dummy initializer.
+        ),
+      );
     });
 
     it('proxy admin cannot call delegated functions', async function () {
@@ -267,14 +279,12 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy() {
   });
 
   describe('regression', function () {
-    const initializeData = '0x';
-
     it('should add new function', async function () {
       const impl1 = await ethers.deployContract('Implementation1');
       const impl2 = await ethers.deployContract('Implementation2');
       const { instance, proxy, proxyAdminAsSigner } = await this.createProxyWithImpersonatedProxyAdmin(
         impl1,
-        initializeData,
+        impl1.interface.encodeFunctionData('initialize'),
       );
 
       await instance.setValue(42n);
@@ -294,7 +304,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy() {
       const impl2 = await ethers.deployContract('Implementation2');
       const { instance, proxy, proxyAdminAsSigner } = await this.createProxyWithImpersonatedProxyAdmin(
         impl2,
-        initializeData,
+        impl2.interface.encodeFunctionData('initialize'),
       );
 
       await instance.setValue(42n);
@@ -314,7 +324,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy() {
       const impl3 = await ethers.deployContract('Implementation3');
       const { instance, proxy, proxyAdminAsSigner } = await this.createProxyWithImpersonatedProxyAdmin(
         impl1,
-        initializeData,
+        impl1.interface.encodeFunctionData('initialize'),
       );
 
       await instance.setValue(42n);
@@ -329,7 +339,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy() {
       const impl4 = await ethers.deployContract('Implementation4');
       const { instance, proxy, proxyAdminAsSigner } = await this.createProxyWithImpersonatedProxyAdmin(
         impl1,
-        initializeData,
+        impl1.interface.encodeFunctionData('initialize'),
       );
 
       await proxy.connect(proxyAdminAsSigner).upgradeToAndCall(impl4, '0x');
@@ -344,7 +354,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy() {
       const impl4 = await ethers.deployContract('Implementation4');
       const { instance, proxy, proxyAdminAsSigner } = await this.createProxyWithImpersonatedProxyAdmin(
         impl4,
-        initializeData,
+        impl4.interface.encodeFunctionData('initialize'),
       );
 
       await proxy.connect(proxyAdminAsSigner).upgradeToAndCall(impl2, '0x');

test/proxy/utils/UUPSUpgradeable.test.js

@@ -14,7 +14,7 @@ async function fixture() {
   const cloneFactory = await ethers.deployContract('$Clones');
 
   const instance = await ethers
-    .deployContract('ERC1967Proxy', [implInitial, '0x'])
+    .deployContract('ERC1967ProxyUnsafe', [implInitial, '0x'])
     .then(proxy => implInitial.attach(proxy.target));
 
   return {
@@ -110,7 +110,7 @@ describe('UUPSUpgradeable', function () {
 
   it('reject proxy address as implementation', async function () {
     const otherInstance = await ethers
-      .deployContract('ERC1967Proxy', [this.implInitial, '0x'])
+      .deployContract('ERC1967ProxyUnsafe', [this.implInitial, '0x'])
       .then(proxy => this.implInitial.attach(proxy.target));
 
     await expect(this.instance.upgradeToAndCall(otherInstance, '0x'))