From fbcf8decb3add5952a33e10f675c3ef9ad463e46 Mon Sep 17 00:00:00 2001 From: dimitriospapathanasiou Date: Wed, 22 May 2024 14:42:17 +0300 Subject: [PATCH 1/6] Update contract deployment to fix returndata ignoring --- .changeset/nervous-eyes-teach.md | 5 +++++ contracts/utils/Create2.sol | 19 ++++++++++++++++++- 2 files changed, 23 insertions(+), 1 deletion(-) create mode 100644 .changeset/nervous-eyes-teach.md diff --git a/.changeset/nervous-eyes-teach.md b/.changeset/nervous-eyes-teach.md new file mode 100644 index 00000000000..56bdc67fb91 --- /dev/null +++ b/.changeset/nervous-eyes-teach.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`Create2`: Capture and Utilization of returndata from failed deployment diff --git a/contracts/utils/Create2.sol b/contracts/utils/Create2.sol index 9523c404142..675eaaac430 100644 --- a/contracts/utils/Create2.sol +++ b/contracts/utils/Create2.sol @@ -41,12 +41,29 @@ library Create2 { if (bytecode.length == 0) { revert Create2EmptyBytecode(); } + + uint256 returndataSize; + bytes memory returndata; + /// @solidity memory-safe-assembly assembly { addr := create2(amount, add(bytecode, 0x20), mload(bytecode), salt) + if iszero(addr) { + returndataSize := returndatasize() + returndata := mload(0x40) + mstore(0x40, add(returndata, add(returndataSize, 0x20))) + returndatacopy(returndata, 0, returndataSize) + } } + if (addr == address(0)) { - revert Errors.FailedDeployment(); + if (returndataSize > 0) { + assembly { + revert(add(returndata, 0x20), returndataSize) + } + } else { + revert Errors.FailedDeployment(); + } } } From 446eb8447ea1f773f34ccce2d1ebc3a2bc916263 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 23 May 2024 11:45:51 +0200 Subject: [PATCH 2/6] refactor revert bubble + testing --- contracts/mocks/ConstructorMock.sol | 28 ++++++++++++++ contracts/utils/Create2.sol | 20 +++------- test/utils/Create2.test.js | 58 ++++++++++++++++++++++++++++- 3 files changed, 90 insertions(+), 16 deletions(-) create mode 100644 contracts/mocks/ConstructorMock.sol diff --git a/contracts/mocks/ConstructorMock.sol b/contracts/mocks/ConstructorMock.sol new file mode 100644 index 00000000000..15f2e93387c --- /dev/null +++ b/contracts/mocks/ConstructorMock.sol @@ -0,0 +1,28 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.20; + +contract ConstructorMock { + enum RevertType { + None, + RevertWithoutMessage, + RevertWithMessage, + RevertWithCustomError, + Panic + } + + error CustomError(); + + constructor(RevertType error) { + if (error == RevertType.RevertWithoutMessage) { + revert(); + } else if (error == RevertType.RevertWithMessage) { + revert("ConstructorMock: reverting"); + } else if (error == RevertType.RevertWithCustomError) { + revert CustomError(); + } else if (error == RevertType.Panic) { + uint256 a = uint256(0) / uint256(0); + a; + } + } +} diff --git a/contracts/utils/Create2.sol b/contracts/utils/Create2.sol index 675eaaac430..b926b521db7 100644 --- a/contracts/utils/Create2.sol +++ b/contracts/utils/Create2.sol @@ -42,28 +42,18 @@ library Create2 { revert Create2EmptyBytecode(); } - uint256 returndataSize; - bytes memory returndata; - /// @solidity memory-safe-assembly assembly { addr := create2(amount, add(bytecode, 0x20), mload(bytecode), salt) - if iszero(addr) { - returndataSize := returndatasize() - returndata := mload(0x40) - mstore(0x40, add(returndata, add(returndataSize, 0x20))) - returndatacopy(returndata, 0, returndataSize) + // if no address was created, and returndata is not empty, bubble revert + if and(iszero(addr), not(iszero(returndatasize()))) { + returndatacopy(0, 0, returndatasize()) + revert(0, returndatasize()) } } if (addr == address(0)) { - if (returndataSize > 0) { - assembly { - revert(add(returndata, 0x20), returndataSize) - } - } else { - revert Errors.FailedDeployment(); - } + revert Errors.FailedDeployment(); } } diff --git a/test/utils/Create2.test.js b/test/utils/Create2.test.js index aba4deeb002..80b9cc16415 100644 --- a/test/utils/Create2.test.js +++ b/test/utils/Create2.test.js @@ -1,6 +1,9 @@ const { ethers } = require('hardhat'); const { expect } = require('chai'); const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers'); +const { PANIC_CODES } = require('@nomicfoundation/hardhat-chai-matchers/panic'); + +const { RevertType } = require('../helpers/enums'); async function fixture() { const [deployer, other] = await ethers.getSigners(); @@ -19,7 +22,9 @@ async function fixture() { .getContractFactory('$Create2') .then(({ bytecode, interface }) => ethers.concat([bytecode, interface.encodeDeploy([])])); - return { deployer, other, factory, constructorByteCode, constructorLessBytecode }; + const mockFactory = await ethers.getContractFactory('ConstructorMock'); + + return { deployer, other, factory, constructorByteCode, constructorLessBytecode, mockFactory }; } describe('Create2', function () { @@ -130,5 +135,56 @@ describe('Create2', function () { .to.be.revertedWithCustomError(this.factory, 'InsufficientBalance') .withArgs(0n, 1n); }); + + describe('reverts error thrown during contract creation', function () { + it('Revert without message', async function () { + await expect( + this.factory.$deploy( + 0n, + saltHex, + ethers.concat([ + this.mockFactory.bytecode, + this.mockFactory.interface.encodeDeploy([RevertType.RevertWithoutMessage]), + ]), + ), + ).to.be.revertedWithCustomError(this.factory, 'FailedDeployment'); + }); + + it('Revert with message', async function () { + await expect( + this.factory.$deploy( + 0n, + saltHex, + ethers.concat([ + this.mockFactory.bytecode, + this.mockFactory.interface.encodeDeploy([RevertType.RevertWithMessage]), + ]), + ), + ).to.be.revertedWith('ConstructorMock: reverting'); + }); + + it('Revert with custom error', async function () { + await expect( + this.factory.$deploy( + 0n, + saltHex, + ethers.concat([ + this.mockFactory.bytecode, + this.mockFactory.interface.encodeDeploy([RevertType.RevertWithCustomError]), + ]), + ), + ).to.be.revertedWithCustomError({ interface: this.mockFactory.interface }, 'CustomError'); + }); + + it('Panic', async function () { + await expect( + this.factory.$deploy( + 0n, + saltHex, + ethers.concat([this.mockFactory.bytecode, this.mockFactory.interface.encodeDeploy([RevertType.Panic])]), + ), + ).to.be.revertedWithPanic(PANIC_CODES.DIVISION_BY_ZERO); + }); + }); }); }); From f31d3ccf7b7ba05432035616c1983e8e52866a2f Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 23 May 2024 11:48:10 +0200 Subject: [PATCH 3/6] Update Create2.sol --- contracts/utils/Create2.sol | 2 -- 1 file changed, 2 deletions(-) diff --git a/contracts/utils/Create2.sol b/contracts/utils/Create2.sol index b926b521db7..d3aaa42dd09 100644 --- a/contracts/utils/Create2.sol +++ b/contracts/utils/Create2.sol @@ -41,7 +41,6 @@ library Create2 { if (bytecode.length == 0) { revert Create2EmptyBytecode(); } - /// @solidity memory-safe-assembly assembly { addr := create2(amount, add(bytecode, 0x20), mload(bytecode), salt) @@ -51,7 +50,6 @@ library Create2 { revert(0, returndatasize()) } } - if (addr == address(0)) { revert Errors.FailedDeployment(); } From a45f786a54734167c2300219f8fa6e6af4a775f7 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Fri, 24 May 2024 16:38:56 -0600 Subject: [PATCH 4/6] up --- test/utils/Create2.test.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/utils/Create2.test.js b/test/utils/Create2.test.js index 80b9cc16415..152fdbdf4c1 100644 --- a/test/utils/Create2.test.js +++ b/test/utils/Create2.test.js @@ -137,7 +137,7 @@ describe('Create2', function () { }); describe('reverts error thrown during contract creation', function () { - it('Revert without message', async function () { + it('bubbles up without message', async function () { await expect( this.factory.$deploy( 0n, @@ -150,7 +150,7 @@ describe('Create2', function () { ).to.be.revertedWithCustomError(this.factory, 'FailedDeployment'); }); - it('Revert with message', async function () { + it('bubbles up message', async function () { await expect( this.factory.$deploy( 0n, @@ -163,7 +163,7 @@ describe('Create2', function () { ).to.be.revertedWith('ConstructorMock: reverting'); }); - it('Revert with custom error', async function () { + it('bubbles up custom error', async function () { await expect( this.factory.$deploy( 0n, @@ -176,7 +176,7 @@ describe('Create2', function () { ).to.be.revertedWithCustomError({ interface: this.mockFactory.interface }, 'CustomError'); }); - it('Panic', async function () { + it('bubbles up panic', async function () { await expect( this.factory.$deploy( 0n, From 705d7155284576e937e85b7936912b9b05ac4042 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Fri, 24 May 2024 16:42:51 -0600 Subject: [PATCH 5/6] Fix upgradeable tests --- contracts/mocks/ConstructorMock.sol | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/contracts/mocks/ConstructorMock.sol b/contracts/mocks/ConstructorMock.sol index 15f2e93387c..50e671b6d50 100644 --- a/contracts/mocks/ConstructorMock.sol +++ b/contracts/mocks/ConstructorMock.sol @@ -3,6 +3,8 @@ pragma solidity ^0.8.20; contract ConstructorMock { + bool foo; + enum RevertType { None, RevertWithoutMessage, @@ -14,6 +16,10 @@ contract ConstructorMock { error CustomError(); constructor(RevertType error) { + // After transpilation to upgradeable contract, the constructor will become an initializer + // To silence the `... can be restricted to view` warning, we write to state + foo = true; + if (error == RevertType.RevertWithoutMessage) { revert(); } else if (error == RevertType.RevertWithMessage) { From f51b439ead487b1c5340ebe01b8433e20b102e90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Fri, 24 May 2024 16:46:00 -0600 Subject: [PATCH 6/6] Update .changeset/nervous-eyes-teach.md Co-authored-by: Hadrien Croubois --- .changeset/nervous-eyes-teach.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/nervous-eyes-teach.md b/.changeset/nervous-eyes-teach.md index 56bdc67fb91..f85bc66d8f7 100644 --- a/.changeset/nervous-eyes-teach.md +++ b/.changeset/nervous-eyes-teach.md @@ -2,4 +2,4 @@ 'openzeppelin-solidity': minor --- -`Create2`: Capture and Utilization of returndata from failed deployment +`Create2`: Bubbles up returndata from a deployed contract that reverted during construction.