Merge remote-tracking branch 'origin/fix/zero-ex/transformer-deployer-kill-fix' into fix/ep/meta-transactions

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
             }
         ]
     },

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);
     }
 }

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)

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 }],