Skip to content

Add revert if the bytecode length is zero #2117

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Mar 28, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
* `PullPayment`, `Escrow`: `withdrawWithGas` was removed. The old `withdraw` function now forwards all gas. ([#2125](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2125))
* `Roles` was removed, use `AccessControl` as a replacement. ([#2112](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2112))
* `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))
* `Pausable`: moved to the `utils` directory. ([#2122](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2122))
* `Strings`: moved to the `utils` directory. ([#2122](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2122))
* `Counters`: moved to the `utils` directory. ([#2122](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2122))
Expand Down
10 changes: 6 additions & 4 deletions contracts/mocks/Create2Impl.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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 {}
}
2 changes: 1 addition & 1 deletion contracts/mocks/ERC20Mock.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
23 changes: 17 additions & 6 deletions contracts/utils/Create2.sol
Original file line number Diff line number Diff line change
Expand Up @@ -12,22 +12,33 @@ 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(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 >= amount, "Create2: insufficient balance");
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(amount, add(bytecode, 0x20), mload(bytecode), salt)
}
require(addr != address(0), "Create2: Failed on deploy");
return addr;
}

/**
* @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));
Expand Down
35 changes: 30 additions & 5 deletions test/utils/Create2.test.js
Original file line number Diff line number Diff line change
@@ -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');

Expand Down Expand Up @@ -40,23 +40,48 @@ 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));
});

it('should deploy a ERC20Mock with correct balances', async 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('2');
await send.ether(deployerAccount, this.factory.address, deposit);
expect(await balance.current(this.factory.address)).to.be.bignumber.equal(deposit);

const onChainComputed = await this.factory
.computeAddressWithDeployer(saltHex, web3.utils.keccak256(constructorByteCode), this.factory.address);

await this.factory.deploy(deposit, saltHex, constructorByteCode);
expect(await balance.current(onChainComputed)).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(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(saltHex, constructorByteCode, { from: deployerAccount }), 'Create2: Failed on deploy'
this.factory.deploy(1, saltHex, constructorByteCode, { from: deployerAccount }),
'Create2: insufficient balance'
);
});
});
Expand Down