From 69f7343be501eece1716ddcc0dcfa33352d91024 Mon Sep 17 00:00:00 2001 From: Lawrence Forman Date: Wed, 29 Apr 2020 13:48:11 -0400 Subject: [PATCH] `@0x/contracts-zero-ex`: Lock the caller of `InitialMigration.deploy()` and actually self-destruct. --- .../src/migrations/InitialMigration.sol | 36 ++++++++++++--- .../contracts/test/TestInitialMigration.sol | 8 ++++ .../zero-ex/test/initial_migration_test.ts | 44 ++++++++++++++++--- contracts/zero-ex/test/utils/migration.ts | 1 + 4 files changed, 76 insertions(+), 13 deletions(-) diff --git a/contracts/zero-ex/contracts/src/migrations/InitialMigration.sol b/contracts/zero-ex/contracts/src/migrations/InitialMigration.sol index 1ac217b252..dd49672aeb 100644 --- a/contracts/zero-ex/contracts/src/migrations/InitialMigration.sol +++ b/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) { - // Must not have already been run. - require(address(_zeroEx) == address(0), "InitialMigration/ALREADY_DEPLOYED"); + function deploy(address payable owner) public virtual returns (ZeroEx zeroEx) { + // Must be called by the allowed deployer. + 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); + } } diff --git a/contracts/zero-ex/contracts/test/TestInitialMigration.sol b/contracts/zero-ex/contracts/test/TestInitialMigration.sol index 135e2cb246..dc4c31c747 100644 --- a/contracts/zero-ex/contracts/test/TestInitialMigration.sol +++ b/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; + } } diff --git a/contracts/zero-ex/test/initial_migration_test.ts b/contracts/zero-ex/test/initial_migration_test.ts index e83945dbff..bad7a56f62 100644 --- a/contracts/zero-ex/test/initial_migration_test.ts +++ b/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); - }); }); diff --git a/contracts/zero-ex/test/utils/migration.ts b/contracts/zero-ex/test/utils/migration.ts index 07578e1004..f01919a6a1 100644 --- a/contracts/zero-ex/test/utils/migration.ts +++ b/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, {});