diff --git a/contracts/zero-ex/CHANGELOG.json b/contracts/zero-ex/CHANGELOG.json index ba26a2b504..5b59a8c479 100644 --- a/contracts/zero-ex/CHANGELOG.json +++ b/contracts/zero-ex/CHANGELOG.json @@ -17,6 +17,10 @@ { "note": "Refund unused protocol fees to `refundReceiver` in FQT", "pr": 2657 + }, + { + "note": "Fix `TransformerDeployer.kill()` calling the wrong `die()` interface.", + "pr": 2624 } ] }, diff --git a/contracts/zero-ex/contracts/src/external/TransformerDeployer.sol b/contracts/zero-ex/contracts/src/external/TransformerDeployer.sol index 3e3243605c..6e369e523d 100644 --- a/contracts/zero-ex/contracts/src/external/TransformerDeployer.sol +++ b/contracts/zero-ex/contracts/src/external/TransformerDeployer.sol @@ -24,7 +24,7 @@ import "@0x/contracts-utils/contracts/src/v06/AuthorizableV06.sol"; /// @dev A contract with a `die()` function. interface IKillable { - function die() external; + function die(address payable ethRecipient) external; } /// @dev Deployer contract for ERC20 transformers. @@ -67,16 +67,19 @@ contract TransformerDeployer is assembly { deployedAddress := create(callvalue(), add(bytecode, 32), mload(bytecode)) } + require(deployedAddress != address(0), 'TransformerDeployer/DEPLOY_FAILED'); toDeploymentNonce[deployedAddress] = deploymentNonce; emit Deployed(deployedAddress, deploymentNonce, msg.sender); } /// @dev Call `die()` on a contract. Only callable by an authority. - function kill(IKillable target) + /// @param target The target contract to call `die()` on. + /// @param ethRecipient The Recipient of any ETH locked in `target`. + function kill(IKillable target, address payable ethRecipient) public onlyAuthorized { - target.die(); + target.die(ethRecipient); emit Killed(address(target), msg.sender); } } diff --git a/contracts/zero-ex/contracts/test/TestTransformerDeployerTransformer.sol b/contracts/zero-ex/contracts/test/TestTransformerDeployerTransformer.sol index f234d33c03..2491920718 100644 --- a/contracts/zero-ex/contracts/test/TestTransformerDeployerTransformer.sol +++ b/contracts/zero-ex/contracts/test/TestTransformerDeployerTransformer.sol @@ -24,10 +24,15 @@ import "../src/transformers/LibERC20Transformer.sol"; contract TestTransformerDeployerTransformer { + uint256 public constant CONSTRUCTOR_FAIL_VALUE = 3333; address payable public immutable deployer; constructor() public payable { deployer = msg.sender; + require( + msg.value != CONSTRUCTOR_FAIL_VALUE, + "TestTransformerDeployerTransformer/CONSTRUCTOR_FAIL" + ); } modifier onlyDeployer() { @@ -35,11 +40,11 @@ contract TestTransformerDeployerTransformer { _; } - function die() + function die(address payable ethRecipient) external onlyDeployer { - selfdestruct(deployer); + selfdestruct(ethRecipient); } function isDeployedByDeployer(uint32 nonce) diff --git a/contracts/zero-ex/test/transformer_deployer_test.ts b/contracts/zero-ex/test/transformer_deployer_test.ts index c3d7e15658..2d6f614102 100644 --- a/contracts/zero-ex/test/transformer_deployer_test.ts +++ b/contracts/zero-ex/test/transformer_deployer_test.ts @@ -59,6 +59,12 @@ blockchainTests.resets('TransformerDeployer', env => { expect(await env.web3Wrapper.getBalanceInWeiAsync(targetAddress)).to.bignumber.eq(1); }); + it('reverts if constructor throws', async () => { + const CONSTRUCTOR_FAIL_VALUE = new BigNumber(3333); + const tx = deployer.deploy(deployBytes).callAsync({ value: CONSTRUCTOR_FAIL_VALUE, from: authority }); + return expect(tx).to.revertWith('TransformerDeployer/DEPLOY_FAILED'); + }); + it('updates nonce', async () => { expect(await deployer.nonce().callAsync()).to.bignumber.eq(1); await deployer.deploy(deployBytes).awaitTransactionSuccessAsync({ from: authority }); @@ -82,6 +88,7 @@ blockchainTests.resets('TransformerDeployer', env => { }); describe('kill()', () => { + const ethRecipient = randomAddress(); let target: TestTransformerDeployerTransformerContract; before(async () => { @@ -90,14 +97,16 @@ blockchainTests.resets('TransformerDeployer', env => { await deployer.deploy(deployBytes).awaitTransactionSuccessAsync({ from: authority }); }); - it('authority cannot call', async () => { + it('non-authority cannot call', async () => { const nonAuthority = randomAddress(); - const tx = deployer.kill(target.address).callAsync({ from: nonAuthority }); + const tx = deployer.kill(target.address, ethRecipient).callAsync({ from: nonAuthority }); return expect(tx).to.revertWith(new AuthorizableRevertErrors.SenderNotAuthorizedError(nonAuthority)); }); it('authority can kill a contract', async () => { - const receipt = await deployer.kill(target.address).awaitTransactionSuccessAsync({ from: authority }); + const receipt = await deployer + .kill(target.address, ethRecipient) + .awaitTransactionSuccessAsync({ from: authority }); verifyEventsFromLogs( receipt.logs, [{ target: target.address, sender: authority }],