From 548c30d6a036a61f8c455a92e6eb3929088a63ed Mon Sep 17 00:00:00 2001 From: Lawrence Forman Date: Tue, 26 May 2020 20:57:55 -0400 Subject: [PATCH] ZeroEx: Audit Fixes (#2586) * `@0x/utils`: Remove unused revert error. * `@0x/contracts-zero-ex`: Address audit feedback. --- contracts/zero-ex/contracts/src/ZeroEx.sol | 6 +---- .../src/errors/LibOwnableRichErrors.sol | 10 --------- .../contracts/src/features/Ownable.sol | 21 +++++++++--------- .../src/features/SimpleFunctionRegistry.sol | 22 +++++++++++++++++-- .../contracts/src/fixins/FixinCommon.sol | 13 ++--------- .../src/migrations/InitialMigration.sol | 6 +++++ .../src/storage/LibOwnableStorage.sol | 3 +++ .../contracts/src/storage/LibProxyStorage.sol | 5 +++++ .../LibSimpleFunctionRegistryStorage.sol | 3 +++ .../src/storage/LibTokenSpenderStorage.sol | 3 +++ .../src/storage/LibTransformERC20Storage.sol | 3 +++ .../zero-ex/test/initial_migration_test.ts | 17 +++++++++++++- .../zero-ex/ownable_revert_errors.ts | 7 +----- 13 files changed, 73 insertions(+), 46 deletions(-) diff --git a/contracts/zero-ex/contracts/src/ZeroEx.sol b/contracts/zero-ex/contracts/src/ZeroEx.sol index 4ea386f445..2e9b0f543b 100644 --- a/contracts/zero-ex/contracts/src/ZeroEx.sol +++ b/contracts/zero-ex/contracts/src/ZeroEx.sol @@ -30,13 +30,9 @@ import "./errors/LibProxyRichErrors.sol"; /// interacting with the 0x protocol. contract ZeroEx { // solhint-disable separate-by-one-line-in-contract,indent,var-name-mixedcase - using LibBytesV06 for bytes; - /// @dev Magic bytes returned by the bootstrapper to indicate success. - bytes4 internal constant BOOTSTRAP_SUCCESS = 0xd150751b; - - /// @dev Construct this contract and set the bootstrap migration contract. + /// @dev Construct this contract and register the `Bootstrap` feature. /// After constructing this contract, `bootstrap()` should be called /// to seed the initial feature set. constructor() public { diff --git a/contracts/zero-ex/contracts/src/errors/LibOwnableRichErrors.sol b/contracts/zero-ex/contracts/src/errors/LibOwnableRichErrors.sol index 313d8822f9..8b9d7679c2 100644 --- a/contracts/zero-ex/contracts/src/errors/LibOwnableRichErrors.sol +++ b/contracts/zero-ex/contracts/src/errors/LibOwnableRichErrors.sol @@ -48,16 +48,6 @@ library LibOwnableRichErrors { ); } - function AlreadyMigratingError() - internal - pure - returns (bytes memory) - { - return abi.encodeWithSelector( - bytes4(keccak256("AlreadyMigratingError()")) - ); - } - function MigrateCallFailedError(address target, bytes memory resultData) internal pure diff --git a/contracts/zero-ex/contracts/src/features/Ownable.sol b/contracts/zero-ex/contracts/src/features/Ownable.sol index 8261d96b89..230200d51f 100644 --- a/contracts/zero-ex/contracts/src/features/Ownable.sol +++ b/contracts/zero-ex/contracts/src/features/Ownable.sol @@ -27,7 +27,7 @@ import "../migrations/LibBootstrap.sol"; import "../migrations/LibMigrate.sol"; import "./IFeature.sol"; import "./IOwnable.sol"; -import "./ISimpleFunctionRegistry.sol"; +import "./SimpleFunctionRegistry.sol"; /// @dev Owner management features. @@ -62,9 +62,9 @@ contract Ownable is LibOwnableStorage.getStorage().owner = address(this); // Register feature functions. - ISimpleFunctionRegistry(address(this)).extend(this.transferOwnership.selector, _implementation); - ISimpleFunctionRegistry(address(this)).extend(this.owner.selector, _implementation); - ISimpleFunctionRegistry(address(this)).extend(this.migrate.selector, _implementation); + SimpleFunctionRegistry(address(this))._extendSelf(this.transferOwnership.selector, _implementation); + SimpleFunctionRegistry(address(this))._extendSelf(this.owner.selector, _implementation); + SimpleFunctionRegistry(address(this))._extendSelf(this.migrate.selector, _implementation); return LibBootstrap.BOOTSTRAP_SUCCESS; } @@ -89,7 +89,7 @@ contract Ownable is /// @dev Execute a migration function in the context of the ZeroEx contract. /// The result of the function being called should be the magic bytes /// 0x2c64c5ef (`keccack('MIGRATE_SUCCESS')`). Only callable by the owner. - /// The owner will be temporarily set to `address(this)` inside the call. + /// Temporarily sets the owner to ourselves so we can perform admin functions. /// Before returning, the owner will be set to `newOwner`. /// @param target The migrator contract address. /// @param data The call data. @@ -99,13 +99,12 @@ contract Ownable is override onlyOwner { - LibOwnableStorage.Storage storage stor = LibOwnableStorage.getStorage(); - address prevOwner = stor.owner; - if (prevOwner == address(this)) { - // If the owner is already set to ourselves then we've reentered. - LibOwnableRichErrors.AlreadyMigratingError().rrevert(); + if (newOwner == address(0)) { + LibOwnableRichErrors.TransferOwnerToZeroError().rrevert(); } - // Temporarily set the owner to ourselves so we can perform admin functions. + + LibOwnableStorage.Storage storage stor = LibOwnableStorage.getStorage(); + // The owner will be temporarily set to `address(this)` inside the call. stor.owner = address(this); // Perform the migration. diff --git a/contracts/zero-ex/contracts/src/features/SimpleFunctionRegistry.sol b/contracts/zero-ex/contracts/src/features/SimpleFunctionRegistry.sol index 60107e520a..283dc5ea5c 100644 --- a/contracts/zero-ex/contracts/src/features/SimpleFunctionRegistry.sol +++ b/contracts/zero-ex/contracts/src/features/SimpleFunctionRegistry.sol @@ -50,11 +50,15 @@ contract SimpleFunctionRegistry is _implementation = address(this); } - /// @dev Initializes this feature. + /// @dev Initializes this feature, registering its own functions. /// @return success Magic bytes if successful. - function bootstrap() external returns (bytes4 success) { + function bootstrap() + external + returns (bytes4 success) + { // Register the registration functions (inception vibes). _extend(this.extend.selector, _implementation); + _extend(this._extendSelf.selector, _implementation); // Register the rollback function. _extend(this.rollback.selector, _implementation); // Register getters. @@ -114,6 +118,20 @@ contract SimpleFunctionRegistry is _extend(selector, impl); } + /// @dev Register or replace a function. + /// Only callable from within. + /// This function is only used during the bootstrap process and + /// should be deregistered by the deployer after bootstrapping is + /// complete. + /// @param selector The function selector. + /// @param impl The implementation contract for the function. + function _extendSelf(bytes4 selector, address impl) + external + onlySelf + { + _extend(selector, impl); + } + /// @dev Retrieve the length of the rollback history for a function. /// @param selector The function selector. /// @return rollbackLength The number of items in the rollback history for diff --git a/contracts/zero-ex/contracts/src/fixins/FixinCommon.sol b/contracts/zero-ex/contracts/src/fixins/FixinCommon.sol index 5908d1322e..e1b78fbbf3 100644 --- a/contracts/zero-ex/contracts/src/fixins/FixinCommon.sol +++ b/contracts/zero-ex/contracts/src/fixins/FixinCommon.sol @@ -23,10 +23,9 @@ import "@0x/contracts-utils/contracts/src/v06/errors/LibRichErrorsV06.sol"; import "../errors/LibCommonRichErrors.sol"; import "../errors/LibOwnableRichErrors.sol"; import "../features/IOwnable.sol"; -import "../storage/LibOwnableStorage.sol"; -/// @dev Common utilities. +/// @dev Common feature utilities. contract FixinCommon { using LibRichErrorsV06 for bytes; @@ -42,7 +41,7 @@ contract FixinCommon { /// @dev The caller of this function must be the owner. modifier onlyOwner() virtual { { - address owner = _getOwner(); + address owner = IOwnable(address(this)).owner(); if (msg.sender != owner) { LibOwnableRichErrors.OnlyOwnerError( msg.sender, @@ -53,14 +52,6 @@ contract FixinCommon { _; } - /// @dev Get the owner of this contract. - /// @return owner The owner of this contract. - function _getOwner() internal virtual view returns (address owner) { - // We access Ownable's storage directly here instead of using the external - // API because `onlyOwner` needs to function during bootstrapping. - return LibOwnableStorage.getStorage().owner; - } - /// @dev Encode a feature version as a `uint256`. /// @param major The major version number of the feature. /// @param minor The minor version number of the feature. diff --git a/contracts/zero-ex/contracts/src/migrations/InitialMigration.sol b/contracts/zero-ex/contracts/src/migrations/InitialMigration.sol index d3928c7ff3..50959cf946 100644 --- a/contracts/zero-ex/contracts/src/migrations/InitialMigration.sol +++ b/contracts/zero-ex/contracts/src/migrations/InitialMigration.sol @@ -106,6 +106,12 @@ contract InitialMigration { ) ); + // De-register `SimpleFunctionRegistry._extendSelf`. + SimpleFunctionRegistry(address(this)).rollback( + SimpleFunctionRegistry._extendSelf.selector, + address(0) + ); + // Transfer ownership to the real owner. Ownable(address(this)).transferOwnership(owner); diff --git a/contracts/zero-ex/contracts/src/storage/LibOwnableStorage.sol b/contracts/zero-ex/contracts/src/storage/LibOwnableStorage.sol index 811109acfc..86d7197137 100644 --- a/contracts/zero-ex/contracts/src/storage/LibOwnableStorage.sol +++ b/contracts/zero-ex/contracts/src/storage/LibOwnableStorage.sol @@ -36,6 +36,9 @@ library LibOwnableStorage { uint256 storageSlot = LibStorage.getStorageSlot( LibStorage.StorageId.Ownable ); + // Dip into assembly to change the slot pointed to by the local + // variable `stor`. + // See https://solidity.readthedocs.io/en/v0.6.8/assembly.html?highlight=slot#access-to-external-variables-functions-and-libraries assembly { stor_slot := storageSlot } } } diff --git a/contracts/zero-ex/contracts/src/storage/LibProxyStorage.sol b/contracts/zero-ex/contracts/src/storage/LibProxyStorage.sol index 2a37a9439d..0e0040aac7 100644 --- a/contracts/zero-ex/contracts/src/storage/LibProxyStorage.sol +++ b/contracts/zero-ex/contracts/src/storage/LibProxyStorage.sol @@ -29,6 +29,8 @@ library LibProxyStorage { struct Storage { // Mapping of function selector -> function implementation mapping(bytes4 => address) impls; + // The owner of the proxy contract. + address owner; } /// @dev Get the storage bucket for this contract. @@ -36,6 +38,9 @@ library LibProxyStorage { uint256 storageSlot = LibStorage.getStorageSlot( LibStorage.StorageId.Proxy ); + // Dip into assembly to change the slot pointed to by the local + // variable `stor`. + // See https://solidity.readthedocs.io/en/v0.6.8/assembly.html?highlight=slot#access-to-external-variables-functions-and-libraries assembly { stor_slot := storageSlot } } } diff --git a/contracts/zero-ex/contracts/src/storage/LibSimpleFunctionRegistryStorage.sol b/contracts/zero-ex/contracts/src/storage/LibSimpleFunctionRegistryStorage.sol index 9b536a018d..cd8161ab71 100644 --- a/contracts/zero-ex/contracts/src/storage/LibSimpleFunctionRegistryStorage.sol +++ b/contracts/zero-ex/contracts/src/storage/LibSimpleFunctionRegistryStorage.sol @@ -36,6 +36,9 @@ library LibSimpleFunctionRegistryStorage { uint256 storageSlot = LibStorage.getStorageSlot( LibStorage.StorageId.SimpleFunctionRegistry ); + // Dip into assembly to change the slot pointed to by the local + // variable `stor`. + // See https://solidity.readthedocs.io/en/v0.6.8/assembly.html?highlight=slot#access-to-external-variables-functions-and-libraries assembly { stor_slot := storageSlot } } } diff --git a/contracts/zero-ex/contracts/src/storage/LibTokenSpenderStorage.sol b/contracts/zero-ex/contracts/src/storage/LibTokenSpenderStorage.sol index b9848e05cb..b86921e7d5 100644 --- a/contracts/zero-ex/contracts/src/storage/LibTokenSpenderStorage.sol +++ b/contracts/zero-ex/contracts/src/storage/LibTokenSpenderStorage.sol @@ -37,6 +37,9 @@ library LibTokenSpenderStorage { uint256 storageSlot = LibStorage.getStorageSlot( LibStorage.StorageId.TokenSpender ); + // Dip into assembly to change the slot pointed to by the local + // variable `stor`. + // See https://solidity.readthedocs.io/en/v0.6.8/assembly.html?highlight=slot#access-to-external-variables-functions-and-libraries assembly { stor_slot := storageSlot } } } diff --git a/contracts/zero-ex/contracts/src/storage/LibTransformERC20Storage.sol b/contracts/zero-ex/contracts/src/storage/LibTransformERC20Storage.sol index 9472c24f56..2049d60077 100644 --- a/contracts/zero-ex/contracts/src/storage/LibTransformERC20Storage.sol +++ b/contracts/zero-ex/contracts/src/storage/LibTransformERC20Storage.sol @@ -37,6 +37,9 @@ library LibTransformERC20Storage { uint256 storageSlot = LibStorage.getStorageSlot( LibStorage.StorageId.TransformERC20 ); + // Dip into assembly to change the slot pointed to by the local + // variable `stor`. + // See https://solidity.readthedocs.io/en/v0.6.8/assembly.html?highlight=slot#access-to-external-variables-functions-and-libraries assembly { stor_slot := storageSlot } } } diff --git a/contracts/zero-ex/test/initial_migration_test.ts b/contracts/zero-ex/test/initial_migration_test.ts index 7772cd33e9..0121cb0514 100644 --- a/contracts/zero-ex/test/initial_migration_test.ts +++ b/contracts/zero-ex/test/initial_migration_test.ts @@ -1,5 +1,5 @@ import { blockchainTests, expect, randomAddress } from '@0x/contracts-test-utils'; -import { ZeroExRevertErrors } from '@0x/utils'; +import { hexUtils, ZeroExRevertErrors } from '@0x/utils'; import { artifacts } from './artifacts'; import { BootstrapFeatures, deployBootstrapFeaturesAsync, toFeatureAdddresses } from './utils/migration'; @@ -7,6 +7,7 @@ import { IBootstrapContract, InitialMigrationContract, IOwnableContract, + SimpleFunctionRegistryContract, TestInitialMigrationContract, ZeroExContract, } from './wrappers'; @@ -87,4 +88,18 @@ blockchainTests.resets('Initial migration', env => { expect(actualOwner).to.eq(owner); }); }); + + describe('SimpleFunctionRegistry feature', () => { + let registry: SimpleFunctionRegistryContract; + + before(async () => { + registry = new SimpleFunctionRegistryContract(zeroEx.address, env.provider, env.txDefaults); + }); + + it('_extendSelf() is deregistered', async () => { + const selector = registry.getSelector('_extendSelf'); + const tx = registry._extendSelf(hexUtils.random(4), randomAddress()).callAsync({ from: zeroEx.address }); + return expect(tx).to.revertWith(new ZeroExRevertErrors.Proxy.NotImplementedError(selector)); + }); + }); }); diff --git a/packages/utils/src/revert_errors/zero-ex/ownable_revert_errors.ts b/packages/utils/src/revert_errors/zero-ex/ownable_revert_errors.ts index 4daf91f40a..0810093c66 100644 --- a/packages/utils/src/revert_errors/zero-ex/ownable_revert_errors.ts +++ b/packages/utils/src/revert_errors/zero-ex/ownable_revert_errors.ts @@ -1,11 +1,6 @@ import { RevertError } from '../../revert_error'; // tslint:disable:max-classes-per-file -export class AlreadyMigratingError extends RevertError { - constructor() { - super('AlreadyMigratingError', 'AlreadyMigratingError()', {}); - } -} export class MigrateCallFailedError extends RevertError { constructor(target?: string, resultData?: string) { @@ -32,7 +27,7 @@ export class OnlyOwnerError extends RevertError { // } // } -const types = [AlreadyMigratingError, MigrateCallFailedError, OnlyOwnerError]; +const types = [MigrateCallFailedError, OnlyOwnerError]; // Register the types we've defined. for (const type of types) {