@0x/contracts-zero-ex: Lock the caller of InitialMigration.deploy() and actually self-destruct.

contracts/zero-ex/contracts/src/migrations/InitialMigration.sol

@@ -28,25 +28,42 @@ import "./LibBootstrap.sol";
 
 /// @dev A contract for deploying and configuring a minimal ZeroEx contract.
 contract InitialMigration {
-    /// @dev The deployed ZereEx instance.
+
-    ZeroEx private _zeroEx;
+    /// @dev The allowed caller of `deploy()`. In production, this would be
+    ///      the governor.
+    address public immutable deployer;
+    /// @dev The real address of this contract.
+    address private immutable _implementation;
+
+    /// @dev Instantiate this contract and set the allowed caller of `deploy()`
+    ///      to `deployer_`.
+    /// @param deployer_ The allowed caller of `deploy()`.
+    constructor(address deployer_) public {
+        deployer = deployer_;
+        _implementation = address(this);
+    }
 
     /// @dev Deploy the `ZeroEx` contract with the minimum feature set,
     ///      transfers ownership to `owner`, then self-destructs.
+    ///      Only callable by `deployer` set in the contstructor.
     /// @param owner The owner of the contract.
     /// @return zeroEx The deployed and configured `ZeroEx` contract.
-    function deploy(address owner) public virtual returns (ZeroEx zeroEx) {
+    function deploy(address payable owner) public virtual returns (ZeroEx zeroEx) {
-        // Must not have already been run.
+        // Must be called by the allowed deployer.
-        require(address(_zeroEx) == address(0), "InitialMigration/ALREADY_DEPLOYED");
+        require(msg.sender == deployer, "InitialMigration/INVALID_SENDER");
 
         // Deploy the ZeroEx contract, setting ourselves as the bootstrapper.
-        zeroEx = _zeroEx = new ZeroEx();
+        zeroEx = new ZeroEx();
 
         // Bootstrap the initial feature set.
         IBootstrap(address(zeroEx)).bootstrap(
             address(this),
             abi.encodeWithSelector(this.bootstrap.selector, owner)
         );
+
+        // Self-destruct. This contract should not hold any funds but we send
+        // them to the owner just in case.
+        this.die(owner);
     }
 
     /// @dev Sets up the initial state of the `ZeroEx` contract.
@@ -76,4 +93,11 @@ contract InitialMigration {
 
         success = LibBootstrap.BOOTSTRAP_SUCCESS;
     }
+
+    /// @dev Self-destructs this contract. Only callable by this contract.
+    /// @param ethRecipient Who to transfer outstanding ETH to.
+    function die(address payable ethRecipient) public virtual {
+        require(msg.sender == _implementation, "InitialMigration/INVALID_SENDER");
+        selfdestruct(ethRecipient);
+    }
 }

contracts/zero-ex/contracts/test/TestInitialMigration.sol

@@ -28,6 +28,10 @@ contract TestInitialMigration is
     InitialMigration
 {
     address public bootstrapFeature;
+    address public dieRecipient;
+
+    // solhint-disable-next-line no-empty-blocks
+    constructor(address deployer) public InitialMigration(deployer) {}
 
     function callBootstrap(ZeroEx zeroEx) external {
         IBootstrap(address(zeroEx)).bootstrap(address(this), new bytes(0));
@@ -43,4 +47,8 @@ contract TestInitialMigration is
         bootstrapFeature = ZeroEx(address(uint160(address(this))))
             .getFunctionImplementation(IBootstrap.bootstrap.selector);
     }
+
+    function die(address payable ethRecipient) public override {
+        dieRecipient = ethRecipient;
+    }
 }

contracts/zero-ex/test/initial_migration_test.ts

@@ -1,8 +1,14 @@
-import { blockchainTests, expect } from '@0x/contracts-test-utils';
+import { blockchainTests, expect, randomAddress } from '@0x/contracts-test-utils';
 import { ZeroExRevertErrors } from '@0x/utils';
 
 import { artifacts } from './artifacts';
-import { IBootstrapContract, IOwnableContract, TestInitialMigrationContract, ZeroExContract } from './wrappers';
+import {
+    IBootstrapContract,
+    InitialMigrationContract,
+    IOwnableContract,
+    TestInitialMigrationContract,
+    ZeroExContract,
+} from './wrappers';
 
 blockchainTests.resets('Initial migration', env => {
     let owner: string;
@@ -17,6 +23,7 @@ blockchainTests.resets('Initial migration', env => {
             env.provider,
             env.txDefaults,
             artifacts,
+            env.txDefaults.from as string,
         );
         bootstrapFeature = new IBootstrapContract(
             await migrator.bootstrapFeature().callAsync(),
@@ -29,12 +36,40 @@ blockchainTests.resets('Initial migration', env => {
         await deployCall.awaitTransactionSuccessAsync();
     });
 
+    it('Self-destructs after deployment', async () => {
+        const dieRecipient = await migrator.dieRecipient().callAsync();
+        expect(dieRecipient).to.eq(owner);
+    });
+
+    it('Non-deployer cannot call deploy()', async () => {
+        const notDeployer = randomAddress();
+        const tx = migrator.deploy(owner).callAsync({ from: notDeployer });
+        return expect(tx).to.revertWith('InitialMigration/INVALID_SENDER');
+    });
+
+    it('External contract cannot call die()', async () => {
+        const _migrator = await InitialMigrationContract.deployFrom0xArtifactAsync(
+            artifacts.InitialMigration,
+            env.provider,
+            env.txDefaults,
+            artifacts,
+            env.txDefaults.from as string,
+        );
+        const tx = _migrator.die(owner).callAsync();
+        return expect(tx).to.revertWith('InitialMigration/INVALID_SENDER');
+    });
+
     describe('bootstrapping', () => {
         it('Migrator cannot call bootstrap() again', async () => {
             const tx = migrator.callBootstrap(zeroEx.address).awaitTransactionSuccessAsync();
             const selector = bootstrapFeature.getSelector('bootstrap');
             return expect(tx).to.revertWith(new ZeroExRevertErrors.Proxy.NotImplementedError(selector));
         });
+
+        it('Bootstrap feature self destructs after deployment', async () => {
+            const codeSize = await migrator.getCodeSizeOf(bootstrapFeature.address).callAsync();
+            expect(codeSize).to.bignumber.eq(0);
+        });
     });
 
     describe('Ownable feature', () => {
@@ -49,9 +84,4 @@ blockchainTests.resets('Initial migration', env => {
             expect(actualOwner).to.eq(owner);
         });
     });
-
-    it('bootstrap feature self destructs after deployment', async () => {
-        const codeSize = await migrator.getCodeSizeOf(bootstrapFeature.address).callAsync();
-        expect(codeSize).to.bignumber.eq(0);
-    });
 });

contracts/zero-ex/test/utils/migration.ts

@@ -15,6 +15,7 @@ export async function initialMigrateAsync(
         provider,
         txDefaults,
         artifacts,
+        txDefaults.from as string,
     );
     const deployCall = migrator.deploy(owner);
     const zeroEx = new ZeroExContract(await deployCall.callAsync(), provider, {});