Avoid uninitialized proxies

Author: ernestognw

contracts/mocks/proxy/ClashingImplementation.sol

@@ -9,6 +9,10 @@ pragma solidity ^0.8.20;
 contract ClashingImplementation {
     event ClashingImplementationCall();
 
+    function initialize() external payable {
+        // placeholder
+    }
+
     function upgradeToAndCall(address, bytes calldata) external payable {
         emit ClashingImplementationCall();
     }

contracts/proxy/ERC1967/ERC1967Proxy.sol

@@ -24,7 +24,10 @@ contract ERC1967Proxy is Proxy {
      * - If `data` is empty, `msg.value` must be zero.
      */
     constructor(address implementation, bytes memory _data) payable {
-        ERC1967Utils.upgradeToAndCall(implementation, _data);
+        require(
+            _unsafeAllowUninitialized() || ERC1967Utils.upgradeToAndCall(implementation, _data),
+            ERC1967Utils.ERC1967ProxyUninitialized()
+        );
     }
 
     /**
@@ -37,4 +40,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;
+    }
 }

contracts/proxy/ERC1967/ERC1967Utils.sol

@@ -40,6 +40,11 @@ library ERC1967Utils {
      */
     error ERC1967NonPayable();
 
+    /**
+     * @dev The proxy is left uninitialized.
+     */
+    error ERC1967ProxyUninitialized();
+
     /**
      * @dev Returns the current implementation address.
      */
@@ -64,15 +69,18 @@ library ERC1967Utils {
      *
      * Emits an {IERC1967-Upgraded} event.
      */
-    function upgradeToAndCall(address newImplementation, bytes memory data) internal {
+    function upgradeToAndCall(address newImplementation, bytes memory data) internal returns (bool) {
         _setImplementation(newImplementation);
         emit IERC1967.Upgraded(newImplementation);
+        bool called = data.length > 0;
 
-        if (data.length > 0) {
+        if (called) {
             Address.functionDelegateCall(newImplementation, data);
         } else {
             _checkNonPayable();
         }
+
+        return called;
     }
 
     /**

test/proxy/Proxy.behaviour.js

@@ -12,6 +12,14 @@ module.exports = function shouldBehaveLikeProxy() {
       .withArgs(this.nonContractAddress);
   });
 
+  it('reverts without initialization', async function () {
+    const contractFactory = await ethers.getContractFactory('ERC1967Proxy');
+    await expect(this.createProxy(this.implementation, '0x')).to.be.revertedWithCustomError(
+      contractFactory,
+      'ERC1967ProxyUninitialized',
+    );
+  });
+
   const assertProxyInitialization = function ({ value, balance }) {
     it('sets the implementation address', async function () {
       expect(await getAddressInSlot(this.proxy, ImplementationSlot)).to.equal(this.implementation);
@@ -27,26 +35,6 @@ module.exports = function shouldBehaveLikeProxy() {
     });
   };
 
-  describe('without initialization', function () {
-    const initializeData = '0x';
-
-    describe('when not sending balance', function () {
-      beforeEach('creating proxy', async function () {
-        this.proxy = await this.createProxy(this.implementation, initializeData);
-      });
-
-      assertProxyInitialization({ value: 0n, balance: 0n });
-    });
-
-    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;
-      });
-    });
-  });
-
   describe('initialization without parameters', function () {
     describe('non payable', function () {
       const expectedInitializedValue = 10n;

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.implementation.interface.encodeFunctionData('initialize', [100n, 'test', [1, 2, 3]]),
+      ),
+    );
   });
 
   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('initialize'),
+        ),
+      );
     });
 
     it('proxy admin cannot call delegated functions', async function () {
@@ -267,91 +279,87 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy() {
   });
 
   describe('regression', function () {
-    const initializeData = '0x';
+    beforeEach(async function () {
+      this.impl1 = await ethers.deployContract('Implementation1');
+      this.impl2 = await ethers.deployContract('Implementation2');
+      this.impl3 = await ethers.deployContract('Implementation3');
+      this.impl4 = await ethers.deployContract('Implementation4');
+      this.initializeData = this.impl1.interface.encodeFunctionData('initialize');
+    });
 
     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,
+        this.impl1,
+        this.initializeData,
       );
 
       await instance.setValue(42n);
 
       // `getValue` is not available in impl1
-      await expect(impl2.attach(instance).getValue()).to.be.reverted;
+      await expect(this.impl2.attach(instance).getValue()).to.be.reverted;
 
       // do upgrade
-      await proxy.connect(proxyAdminAsSigner).upgradeToAndCall(impl2, '0x');
+      await proxy.connect(proxyAdminAsSigner).upgradeToAndCall(this.impl2, '0x');
 
       // `getValue` is available in impl2
-      expect(await impl2.attach(instance).getValue()).to.equal(42n);
+      expect(await this.impl2.attach(instance).getValue()).to.equal(42n);
     });
 
     it('should remove function', async function () {
-      const impl1 = await ethers.deployContract('Implementation1');
-      const impl2 = await ethers.deployContract('Implementation2');
       const { instance, proxy, proxyAdminAsSigner } = await this.createProxyWithImpersonatedProxyAdmin(
-        impl2,
-        initializeData,
+        this.impl2,
+        this.initializeData,
       );
 
       await instance.setValue(42n);
 
       // `getValue` is available in impl2
-      expect(await impl2.attach(instance).getValue()).to.equal(42n);
+      expect(await this.impl2.attach(instance).getValue()).to.equal(42n);
 
       // do downgrade
-      await proxy.connect(proxyAdminAsSigner).upgradeToAndCall(impl1, '0x');
+      await proxy.connect(proxyAdminAsSigner).upgradeToAndCall(this.impl1, '0x');
 
       // `getValue` is not available in impl1
-      await expect(impl2.attach(instance).getValue()).to.be.reverted;
+      await expect(this.impl2.attach(instance).getValue()).to.be.reverted;
     });
 
     it('should change function signature', async function () {
-      const impl1 = await ethers.deployContract('Implementation1');
-      const impl3 = await ethers.deployContract('Implementation3');
       const { instance, proxy, proxyAdminAsSigner } = await this.createProxyWithImpersonatedProxyAdmin(
-        impl1,
-        initializeData,
+        this.impl1,
+        this.initializeData,
       );
 
       await instance.setValue(42n);
 
-      await proxy.connect(proxyAdminAsSigner).upgradeToAndCall(impl3, '0x');
+      await proxy.connect(proxyAdminAsSigner).upgradeToAndCall(this.impl3, '0x');
 
-      expect(await impl3.attach(instance).getValue(8n)).to.equal(50n);
+      expect(await this.impl3.attach(instance).getValue(8n)).to.equal(50n);
     });
 
     it('should add fallback function', async function () {
-      const impl1 = await ethers.deployContract('Implementation1');
-      const impl4 = await ethers.deployContract('Implementation4');
       const { instance, proxy, proxyAdminAsSigner } = await this.createProxyWithImpersonatedProxyAdmin(
-        impl1,
-        initializeData,
+        this.impl1,
+        this.initializeData,
       );
 
-      await proxy.connect(proxyAdminAsSigner).upgradeToAndCall(impl4, '0x');
+      await proxy.connect(proxyAdminAsSigner).upgradeToAndCall(this.impl4, '0x');
 
       await this.other.sendTransaction({ to: proxy });
 
-      expect(await impl4.attach(instance).getValue()).to.equal(1n);
+      expect(await this.impl4.attach(instance).getValue()).to.equal(1n);
     });
 
     it('should remove fallback function', async function () {
-      const impl2 = await ethers.deployContract('Implementation2');
-      const impl4 = await ethers.deployContract('Implementation4');
       const { instance, proxy, proxyAdminAsSigner } = await this.createProxyWithImpersonatedProxyAdmin(
-        impl4,
-        initializeData,
+        this.impl4,
+        this.initializeData,
       );
 
-      await proxy.connect(proxyAdminAsSigner).upgradeToAndCall(impl2, '0x');
+      await proxy.connect(proxyAdminAsSigner).upgradeToAndCall(this.impl2, '0x');
 
       await expect(this.other.sendTransaction({ to: proxy })).to.be.reverted;
 
-      expect(await impl2.attach(instance).getValue()).to.equal(0n);
+      expect(await this.impl2.attach(instance).getValue()).to.equal(0n);
     });
   });
 };