From 16fd2e0403a3e95ee4f417405259ca7a089fb4c2 Mon Sep 17 00:00:00 2001 From: cd10012 Date: Wed, 11 Mar 2020 13:16:00 -0400 Subject: [PATCH 1/9] Add revert if the bytecode length is not greater than zero --- contracts/utils/Create2.sol | 1 + test/utils/Create2.test.js | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/contracts/utils/Create2.sol b/contracts/utils/Create2.sol index 8db5d0fc53f..3e048a01720 100644 --- a/contracts/utils/Create2.sol +++ b/contracts/utils/Create2.sol @@ -19,6 +19,7 @@ library Create2 { */ function deploy(bytes32 salt, bytes memory bytecode) internal returns (address) { address addr; + require(bytecode.length > 0, "Create2: bytecode must have a length"); // solhint-disable-next-line no-inline-assembly assembly { addr := create2(0, add(bytecode, 0x20), mload(bytecode), salt) diff --git a/test/utils/Create2.test.js b/test/utils/Create2.test.js index 69c3c0f46eb..7a7e0f5c991 100644 --- a/test/utils/Create2.test.js +++ b/test/utils/Create2.test.js @@ -59,6 +59,11 @@ describe('Create2', function () { this.factory.deploy(saltHex, constructorByteCode, { from: deployerAccount }), 'Create2: Failed on deploy' ); }); + it('should failed deploying a contract if the bytecode length is zero', async function () { + await expectRevert( + this.factory.deploy(saltHex, '0x', { from: deployerAccount }), 'Create2: bytecode must have a length' + ); + }); }); function computeCreate2Address (saltHex, bytecode, deployer) { From e38905e2ef0387fc1359fdc422f1d0106e6f3a56 Mon Sep 17 00:00:00 2001 From: cd10012 Date: Fri, 13 Mar 2020 20:21:00 -0400 Subject: [PATCH 2/9] Add value parameter to create2 deploy function Add tests for contract balance revert and depositing funds --- contracts/mocks/Create2Impl.sol | 8 ++++---- contracts/utils/Create2.sol | 7 ++++--- test/utils/Create2.test.js | 28 +++++++++++++++++++++------- 3 files changed, 29 insertions(+), 14 deletions(-) diff --git a/contracts/mocks/Create2Impl.sol b/contracts/mocks/Create2Impl.sol index 3b86e26174b..64ffa5e1414 100644 --- a/contracts/mocks/Create2Impl.sol +++ b/contracts/mocks/Create2Impl.sol @@ -4,13 +4,13 @@ import "../utils/Create2.sol"; import "../token/ERC20/ERC20.sol"; contract Create2Impl { - function deploy(bytes32 salt, bytes memory code) public { - Create2.deploy(salt, code); + function deploy(uint256 value, bytes32 salt, bytes memory code) public { + Create2.deploy(value, salt, code); } - function deployERC20(bytes32 salt) public { + function deployERC20(uint256 value, bytes32 salt) public { // solhint-disable-next-line indent - Create2.deploy(salt, type(ERC20).creationCode); + Create2.deploy(value, salt, type(ERC20).creationCode); } function computeAddress(bytes32 salt, bytes32 codeHash) public view returns (address) { diff --git a/contracts/utils/Create2.sol b/contracts/utils/Create2.sol index 3e048a01720..a2aa7a58927 100644 --- a/contracts/utils/Create2.sol +++ b/contracts/utils/Create2.sol @@ -17,12 +17,13 @@ library Create2 { * will be deployed can be known in advance via {computeAddress}. Note that * a contract cannot be deployed twice using the same salt. */ - function deploy(bytes32 salt, bytes memory bytecode) internal returns (address) { + function deploy(uint256 value, bytes32 salt, bytes memory bytecode) internal returns (address) { address addr; - require(bytecode.length > 0, "Create2: bytecode must have a length"); + require(address(this).balance >= value, "Factory does not have enough funds to fufill deposit"); + require(bytecode.length > 0, "Create2: bytecode length is zero"); // solhint-disable-next-line no-inline-assembly assembly { - addr := create2(0, add(bytecode, 0x20), mload(bytecode), salt) + addr := create2(value, add(bytecode, 0x20), mload(bytecode), salt) } require(addr != address(0), "Create2: Failed on deploy"); return addr; diff --git a/test/utils/Create2.test.js b/test/utils/Create2.test.js index 7a7e0f5c991..8c824647b7a 100644 --- a/test/utils/Create2.test.js +++ b/test/utils/Create2.test.js @@ -1,5 +1,5 @@ const { contract, accounts, web3 } = require('@openzeppelin/test-environment'); -const { BN, expectRevert } = require('@openzeppelin/test-helpers'); +const { balance, BN, ether, expectRevert, send } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); @@ -40,7 +40,7 @@ describe('Create2', function () { const offChainComputed = computeCreate2Address(saltHex, ERC20.bytecode, this.factory.address); await this.factory - .deploy(saltHex, ERC20.bytecode, { from: deployerAccount }); + .deploy(0, saltHex, ERC20.bytecode, { from: deployerAccount }); expect(ERC20.bytecode).to.include((await web3.eth.getCode(offChainComputed)).slice(2)); }); @@ -48,20 +48,34 @@ describe('Create2', function () { const offChainComputed = computeCreate2Address(saltHex, constructorByteCode, this.factory.address); await this.factory - .deploy(saltHex, constructorByteCode, { from: deployerAccount }); + .deploy(0, saltHex, constructorByteCode, { from: deployerAccount }); const erc20 = await ERC20Mock.at(offChainComputed); expect(await erc20.balanceOf(deployerAccount)).to.be.bignumber.equal(new BN(100)); }); + it('should deploy a contract with funds deposited in the factory', async function () { + const deposit = ether('1'); + await send.ether(deployerAccount, this.factory.address, deposit); + const generatedAddress = + await this.factory.deploy(deposit, saltHex, constructorByteCode, { from: deployerAccount }); + expect(await balance.current(generatedAddress)).to.be.bignumber.equal(deposit); + }); + it('should failed deploying a contract in an existent address', async function () { - await this.factory.deploy(saltHex, constructorByteCode, { from: deployerAccount }); + await this.factory.deploy(0, saltHex, constructorByteCode, { from: deployerAccount }); + await expectRevert( + this.factory.deploy(0, saltHex, constructorByteCode, { from: deployerAccount }), 'Create2: Failed on deploy' + ); + }); + it('should fail deploying a contract if the bytecode length is zero', async function () { await expectRevert( - this.factory.deploy(saltHex, constructorByteCode, { from: deployerAccount }), 'Create2: Failed on deploy' + this.factory.deploy(0, saltHex, '0x', { from: deployerAccount }), 'Create2: bytecode length is zero' ); }); - it('should failed deploying a contract if the bytecode length is zero', async function () { + it('should fail deploying a contract if factory contract does not have sufficient balance', async function () { await expectRevert( - this.factory.deploy(saltHex, '0x', { from: deployerAccount }), 'Create2: bytecode must have a length' + this.factory.deploy(1, saltHex, constructorByteCode, { from: deployerAccount }), + 'Factory does not have enough funds to fufill deposit' ); }); }); From 34f31af657a8957e667cd26cc4a6c7bf8045742b Mon Sep 17 00:00:00 2001 From: cd10012 Date: Sat, 21 Mar 2020 14:23:28 -0400 Subject: [PATCH 3/9] Change parameter name to amount for clarity --- contracts/mocks/Create2Impl.sol | 2 ++ contracts/utils/Create2.sol | 8 ++++---- test/utils/Create2.test.js | 14 ++++++++++---- 3 files changed, 16 insertions(+), 8 deletions(-) diff --git a/contracts/mocks/Create2Impl.sol b/contracts/mocks/Create2Impl.sol index 64ffa5e1414..6ef6cc86bec 100644 --- a/contracts/mocks/Create2Impl.sol +++ b/contracts/mocks/Create2Impl.sol @@ -20,4 +20,6 @@ contract Create2Impl { function computeAddressWithDeployer(bytes32 salt, bytes32 codeHash, address deployer) public pure returns (address) { return Create2.computeAddress(salt, codeHash, deployer); } + + receive() payable external {} } diff --git a/contracts/utils/Create2.sol b/contracts/utils/Create2.sol index a2aa7a58927..900ee854262 100644 --- a/contracts/utils/Create2.sol +++ b/contracts/utils/Create2.sol @@ -17,13 +17,13 @@ library Create2 { * will be deployed can be known in advance via {computeAddress}. Note that * a contract cannot be deployed twice using the same salt. */ - function deploy(uint256 value, bytes32 salt, bytes memory bytecode) internal returns (address) { + function deploy(uint256 amount, bytes32 salt, bytes memory bytecode) internal returns (address) { address addr; - require(address(this).balance >= value, "Factory does not have enough funds to fufill deposit"); - require(bytecode.length > 0, "Create2: bytecode length is zero"); + require(address(this).balance >= amount, "Factory does not have enough funds to fufill deposit"); + require(bytecode.length != 0, "Create2: bytecode length is zero"); // solhint-disable-next-line no-inline-assembly assembly { - addr := create2(value, add(bytecode, 0x20), mload(bytecode), salt) + addr := create2(amount, add(bytecode, 0x20), mload(bytecode), salt) } require(addr != address(0), "Create2: Failed on deploy"); return addr; diff --git a/test/utils/Create2.test.js b/test/utils/Create2.test.js index 8c824647b7a..67439283c6d 100644 --- a/test/utils/Create2.test.js +++ b/test/utils/Create2.test.js @@ -54,11 +54,17 @@ describe('Create2', function () { }); it('should deploy a contract with funds deposited in the factory', async function () { - const deposit = ether('1'); + const deposit = ether('2'); await send.ether(deployerAccount, this.factory.address, deposit); - const generatedAddress = - await this.factory.deploy(deposit, saltHex, constructorByteCode, { from: deployerAccount }); - expect(await balance.current(generatedAddress)).to.be.bignumber.equal(deposit); + expect(await balance.current(this.factory.address)).to.be.bignumber.equal(deposit); + + const onChainComputed = await this.factory + .computeAddressWithDeployer(saltHex, web3.utils.keccak256(constructorByteCode), deployerAccount); + + const amount = new BN(0); + + await this.factory.deploy(amount, saltHex, constructorByteCode, { from: deployerAccount }); + expect(await balance.current(onChainComputed)).to.be.bignumber.equal(amount); }); it('should failed deploying a contract in an existent address', async function () { From cfc881c2becce8eb03fd8806c2eb3411b3ea78db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Wed, 25 Mar 2020 16:17:14 -0300 Subject: [PATCH 4/9] Fix test for value sending --- contracts/mocks/ERC20Mock.sol | 2 +- test/utils/Create2.test.js | 8 +++----- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/contracts/mocks/ERC20Mock.sol b/contracts/mocks/ERC20Mock.sol index ab0390927cb..1ab6a8491f1 100644 --- a/contracts/mocks/ERC20Mock.sol +++ b/contracts/mocks/ERC20Mock.sol @@ -4,7 +4,7 @@ import "../token/ERC20/ERC20.sol"; // mock class using ERC20 contract ERC20Mock is ERC20 { - constructor (address initialAccount, uint256 initialBalance) public { + constructor (address initialAccount, uint256 initialBalance) public payable { _mint(initialAccount, initialBalance); } diff --git a/test/utils/Create2.test.js b/test/utils/Create2.test.js index 67439283c6d..101c79ef66a 100644 --- a/test/utils/Create2.test.js +++ b/test/utils/Create2.test.js @@ -59,12 +59,10 @@ describe('Create2', function () { expect(await balance.current(this.factory.address)).to.be.bignumber.equal(deposit); const onChainComputed = await this.factory - .computeAddressWithDeployer(saltHex, web3.utils.keccak256(constructorByteCode), deployerAccount); - - const amount = new BN(0); + .computeAddressWithDeployer(saltHex, web3.utils.keccak256(constructorByteCode), this.factory.address); - await this.factory.deploy(amount, saltHex, constructorByteCode, { from: deployerAccount }); - expect(await balance.current(onChainComputed)).to.be.bignumber.equal(amount); + await this.factory.deploy(deposit, saltHex, constructorByteCode, { from: deployerAccount }); + expect(await balance.current(onChainComputed)).to.be.bignumber.equal(deposit); }); it('should failed deploying a contract in an existent address', async function () { From 37b03fa1784db8f53afbb72d65ec126f4b71f958 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Wed, 25 Mar 2020 16:17:38 -0300 Subject: [PATCH 5/9] Fix linter error --- test/utils/Create2.test.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/utils/Create2.test.js b/test/utils/Create2.test.js index 101c79ef66a..f8746a09f83 100644 --- a/test/utils/Create2.test.js +++ b/test/utils/Create2.test.js @@ -71,11 +71,13 @@ describe('Create2', function () { this.factory.deploy(0, saltHex, constructorByteCode, { from: deployerAccount }), 'Create2: Failed on deploy' ); }); + it('should fail deploying a contract if the bytecode length is zero', async function () { await expectRevert( this.factory.deploy(0, saltHex, '0x', { from: deployerAccount }), 'Create2: bytecode length is zero' ); }); + it('should fail deploying a contract if factory contract does not have sufficient balance', async function () { await expectRevert( this.factory.deploy(1, saltHex, constructorByteCode, { from: deployerAccount }), From 65d88cb94673719ab9c5c3f63f195441cb336f4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Wed, 25 Mar 2020 16:19:42 -0300 Subject: [PATCH 6/9] Change revert reason --- contracts/utils/Create2.sol | 2 +- test/utils/Create2.test.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/utils/Create2.sol b/contracts/utils/Create2.sol index 900ee854262..9659c47932b 100644 --- a/contracts/utils/Create2.sol +++ b/contracts/utils/Create2.sol @@ -19,7 +19,7 @@ library Create2 { */ function deploy(uint256 amount, bytes32 salt, bytes memory bytecode) internal returns (address) { address addr; - require(address(this).balance >= amount, "Factory does not have enough funds to fufill deposit"); + require(address(this).balance >= amount, "Create2: insufficient balance"); require(bytecode.length != 0, "Create2: bytecode length is zero"); // solhint-disable-next-line no-inline-assembly assembly { diff --git a/test/utils/Create2.test.js b/test/utils/Create2.test.js index f8746a09f83..77dbb70174a 100644 --- a/test/utils/Create2.test.js +++ b/test/utils/Create2.test.js @@ -81,7 +81,7 @@ describe('Create2', function () { it('should fail deploying a contract if factory contract does not have sufficient balance', async function () { await expectRevert( this.factory.deploy(1, saltHex, constructorByteCode, { from: deployerAccount }), - 'Factory does not have enough funds to fufill deposit' + 'Create2: insufficient balance' ); }); }); From 3c3e412c2dafdcd82d4161ce6b175e16e392b627 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Wed, 25 Mar 2020 16:39:51 -0300 Subject: [PATCH 7/9] Improve Create2.deploy documentation --- contracts/utils/Create2.sol | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/contracts/utils/Create2.sol b/contracts/utils/Create2.sol index 9659c47932b..1c7b030a231 100644 --- a/contracts/utils/Create2.sol +++ b/contracts/utils/Create2.sol @@ -14,8 +14,17 @@ pragma solidity ^0.6.0; library Create2 { /** * @dev Deploys a contract using `CREATE2`. The address where the contract - * will be deployed can be known in advance via {computeAddress}. Note that - * a contract cannot be deployed twice using the same salt. + * will be deployed can be known in advance via {computeAddress}. + * + * The bytecode for a contract can be obtained from Solidity with + * `type(contractName).creationCode`. + * + * Requirements: + * + * - `bytecode` must not be empty. + * - `salt` must have not been used for `bytecode` already. + * - the factory must have a balance of at least `amount`. + * - if `amount` is non-zero, `bytecode` must have a `payable` constructor. */ function deploy(uint256 amount, bytes32 salt, bytes memory bytecode) internal returns (address) { address addr; @@ -30,8 +39,8 @@ library Create2 { } /** - * @dev Returns the address where a contract will be stored if deployed via {deploy}. Any change in the `bytecodeHash` - * or `salt` will result in a new destination address. + * @dev Returns the address where a contract will be stored if deployed via {deploy}. Any change in the + * `bytecodeHash` or `salt` will result in a new destination address. */ function computeAddress(bytes32 salt, bytes32 bytecodeHash) internal view returns (address) { return computeAddress(salt, bytecodeHash, address(this)); From 707cf8d183ebe7404acc098fccb78bee7bfb65af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Wed, 25 Mar 2020 16:45:14 -0300 Subject: [PATCH 8/9] Slight test improvement --- test/utils/Create2.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/utils/Create2.test.js b/test/utils/Create2.test.js index 77dbb70174a..c74273cc22a 100644 --- a/test/utils/Create2.test.js +++ b/test/utils/Create2.test.js @@ -61,7 +61,7 @@ describe('Create2', function () { const onChainComputed = await this.factory .computeAddressWithDeployer(saltHex, web3.utils.keccak256(constructorByteCode), this.factory.address); - await this.factory.deploy(deposit, saltHex, constructorByteCode, { from: deployerAccount }); + await this.factory.deploy(deposit, saltHex, constructorByteCode); expect(await balance.current(onChainComputed)).to.be.bignumber.equal(deposit); }); From e3b6ad44d2507f9c8c9543fad002e7b33dc8b9b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Wed, 25 Mar 2020 18:44:54 -0300 Subject: [PATCH 9/9] Add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 85e0cf6d5ce..7992fdb4d07 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Breaking Changes * `ECDSA`: when receiving an invalid signature, `recover` now reverts instead of returning the zero address. ([#2114](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2114)) + * `Create2`: added an `amount` argument to `deploy` for contracts with `payable` constructors. ([#2117](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2117)) ## 2.5.0 (2020-02-04)