diff --git a/contracts/mocks/proxy/ClashingImplementation.sol b/contracts/mocks/proxy/ClashingImplementation.sol index 43d5a34f24e..46d836daccf 100644 --- a/contracts/mocks/proxy/ClashingImplementation.sol +++ b/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(); } diff --git a/contracts/proxy/ERC1967/ERC1967Proxy.sol b/contracts/proxy/ERC1967/ERC1967Proxy.sol index eb482f6ecac..3b024b706c0 100644 --- a/contracts/proxy/ERC1967/ERC1967Proxy.sol +++ b/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; + } } diff --git a/contracts/proxy/ERC1967/ERC1967Utils.sol b/contracts/proxy/ERC1967/ERC1967Utils.sol index ffa77cc7da8..1b7b6d13631 100644 --- a/contracts/proxy/ERC1967/ERC1967Utils.sol +++ b/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; } /** diff --git a/test/proxy/Proxy.behaviour.js b/test/proxy/Proxy.behaviour.js index f459c092627..6d0011673b5 100644 --- a/test/proxy/Proxy.behaviour.js +++ b/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; diff --git a/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js b/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js index 8e1d62eaad3..a09a4f5ec7d 100644 --- a/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js +++ b/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); }); }); };