crowetic/protocol
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

ZeroEx: Audit Fixes (#2586)

Browse Source 
* `@0x/utils`: Remove unused revert error.

* `@0x/contracts-zero-ex`: Address audit feedback.
This commit is contained in:
Lawrence Forman 2020-05-26 20:57:55 -04:00 committed by GitHub
parent 511cd90c44
commit 548c30d6a0
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
13 changed files with 73 additions and 46 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
contracts/zero-ex/contracts/src/ZeroEx.sol
View File

@ -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 {

10
contracts/zero-ex/contracts/src/errors/LibOwnableRichErrors.sol
View File

@ -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

21
contracts/zero-ex/contracts/src/features/Ownable.sol
View File

@ -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.

22
contracts/zero-ex/contracts/src/features/SimpleFunctionRegistry.sol
View File

@ -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

13
contracts/zero-ex/contracts/src/fixins/FixinCommon.sol
View File

@ -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.

6
contracts/zero-ex/contracts/src/migrations/InitialMigration.sol
View File

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

3
contracts/zero-ex/contracts/src/storage/LibOwnableStorage.sol
View File

@ -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 }
}
}

5
contracts/zero-ex/contracts/src/storage/LibProxyStorage.sol
View File

@ -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 }
}
}

3
contracts/zero-ex/contracts/src/storage/LibSimpleFunctionRegistryStorage.sol
View File

@ -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 }
}
}

3
contracts/zero-ex/contracts/src/storage/LibTokenSpenderStorage.sol
View File

@ -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 }
}
}

3
contracts/zero-ex/contracts/src/storage/LibTransformERC20Storage.sol
View File

@ -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 }
}
}

17
contracts/zero-ex/test/initial_migration_test.ts
View File

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

7
packages/utils/src/revert_errors/zero-ex/ownable_revert_errors.ts
View File

@ -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) {