From 47ab2a1b1d263adf708901e4689bcd3fc14b12f3 Mon Sep 17 00:00:00 2001 From: Alex Towle Date: Thu, 1 Aug 2019 14:27:26 -0700 Subject: [PATCH] Reverted the asset-proxy back to an older version of Ownable that it was actually deployed with --- .../asset-proxy/contracts/archive/Ownable.sol | 33 +++++++++++++++++++ .../contracts/archive/interfaces/IOwnable.sol | 8 +++++ .../src/MixinAssetProxyDispatcher.sol | 18 +++++----- .../contracts/src/MixinAuthorizable.sol | 2 +- .../src/interfaces/IAuthorizable.sol | 6 ++-- contracts/asset-proxy/test/authorizable.ts | 30 ++++++++++------- 6 files changed, 72 insertions(+), 25 deletions(-) create mode 100644 contracts/asset-proxy/contracts/archive/Ownable.sol create mode 100644 contracts/asset-proxy/contracts/archive/interfaces/IOwnable.sol diff --git a/contracts/asset-proxy/contracts/archive/Ownable.sol b/contracts/asset-proxy/contracts/archive/Ownable.sol new file mode 100644 index 0000000000..d04c141f58 --- /dev/null +++ b/contracts/asset-proxy/contracts/archive/Ownable.sol @@ -0,0 +1,33 @@ +pragma solidity ^0.5.9; + +import "./interfaces/IOwnable.sol"; + + +contract Ownable is + IOwnable +{ + address public owner; + + constructor () + public + { + owner = msg.sender; + } + + modifier onlyOwner() { + require( + msg.sender == owner, + "ONLY_CONTRACT_OWNER" + ); + _; + } + + function transferOwnership(address newOwner) + public + onlyOwner + { + if (newOwner != address(0)) { + owner = newOwner; + } + } +} diff --git a/contracts/asset-proxy/contracts/archive/interfaces/IOwnable.sol b/contracts/asset-proxy/contracts/archive/interfaces/IOwnable.sol new file mode 100644 index 0000000000..d4b1c5c6b6 --- /dev/null +++ b/contracts/asset-proxy/contracts/archive/interfaces/IOwnable.sol @@ -0,0 +1,8 @@ +pragma solidity ^0.5.9; + + +contract IOwnable { + + function transferOwnership(address newOwner) + public; +} diff --git a/contracts/asset-proxy/contracts/src/MixinAssetProxyDispatcher.sol b/contracts/asset-proxy/contracts/src/MixinAssetProxyDispatcher.sol index 804b0a8686..a4e63029e3 100644 --- a/contracts/asset-proxy/contracts/src/MixinAssetProxyDispatcher.sol +++ b/contracts/asset-proxy/contracts/src/MixinAssetProxyDispatcher.sol @@ -1,6 +1,6 @@ /* - Copyright 2018 ZeroEx Intl. + Copyright 2019 ZeroEx Intl. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -18,7 +18,7 @@ pragma solidity ^0.5.9; -import "@0x/contracts-utils/contracts/src/Ownable.sol"; +import "../archive/Ownable.sol"; import "./interfaces/IAssetProxy.sol"; import "./interfaces/IAssetProxyDispatcher.sol"; @@ -84,7 +84,7 @@ contract MixinAssetProxyDispatcher is assetData.length > 3, "LENGTH_GREATER_THAN_3_REQUIRED" ); - + // Lookup assetProxy. We do not use `LibBytes.readBytes4` for gas efficiency reasons. bytes4 assetProxyId; assembly { @@ -100,10 +100,10 @@ contract MixinAssetProxyDispatcher is assetProxy != address(0), "ASSET_PROXY_DOES_NOT_EXIST" ); - + // We construct calldata for the `assetProxy.transferFrom` ABI. // The layout of this calldata is in the table below. - // + // // | Area | Offset | Length | Contents | // | -------- |--------|---------|-------------------------------------------- | // | Header | 0 | 4 | function selector | @@ -127,12 +127,12 @@ contract MixinAssetProxyDispatcher is // `cdEnd` is the end of the calldata for `assetProxy.transferFrom`. let cdEnd := add(cdStart, add(132, dataAreaLength)) - + /////// Setup Header Area /////// // This area holds the 4-byte `transferFromSelector`. // bytes4(keccak256("transferFrom(bytes,address,address,uint256)")) = 0xa85e59e4 mstore(cdStart, 0xa85e59e400000000000000000000000000000000000000000000000000000000) - + /////// Setup Params Area /////// // Each parameter is padded to 32-bytes. The entire Params Area is 128 bytes. // Notes: @@ -142,7 +142,7 @@ contract MixinAssetProxyDispatcher is mstore(add(cdStart, 36), and(from, 0xffffffffffffffffffffffffffffffffffffffff)) mstore(add(cdStart, 68), and(to, 0xffffffffffffffffffffffffffffffffffffffff)) mstore(add(cdStart, 100), amount) - + /////// Setup Data Area /////// // This area holds `assetData`. let dataArea := add(cdStart, 132) @@ -159,7 +159,7 @@ contract MixinAssetProxyDispatcher is assetProxy, // call address of asset proxy 0, // don't send any ETH cdStart, // pointer to start of input - sub(cdEnd, cdStart), // length of input + sub(cdEnd, cdStart), // length of input cdStart, // write output over input 512 // reserve 512 bytes for output ) diff --git a/contracts/asset-proxy/contracts/src/MixinAuthorizable.sol b/contracts/asset-proxy/contracts/src/MixinAuthorizable.sol index 241cbcaf29..58608113e9 100644 --- a/contracts/asset-proxy/contracts/src/MixinAuthorizable.sol +++ b/contracts/asset-proxy/contracts/src/MixinAuthorizable.sol @@ -18,7 +18,7 @@ pragma solidity ^0.5.9; -import "@0x/contracts-utils/contracts/src/Ownable.sol"; +import "../archive/Ownable.sol"; import "./interfaces/IAuthorizable.sol"; diff --git a/contracts/asset-proxy/contracts/src/interfaces/IAuthorizable.sol b/contracts/asset-proxy/contracts/src/interfaces/IAuthorizable.sol index 517e5d69bf..96fa905338 100644 --- a/contracts/asset-proxy/contracts/src/interfaces/IAuthorizable.sol +++ b/contracts/asset-proxy/contracts/src/interfaces/IAuthorizable.sol @@ -1,6 +1,6 @@ /* - Copyright 2018 ZeroEx Intl. + Copyright 2019 ZeroEx Intl. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -18,7 +18,7 @@ pragma solidity ^0.5.9; -import "@0x/contracts-utils/contracts/src/interfaces/IOwnable.sol"; +import "../../archive/interfaces/IOwnable.sol"; contract IAuthorizable is @@ -54,7 +54,7 @@ contract IAuthorizable is uint256 index ) external; - + /// @dev Gets all authorized addresses. /// @return Array of authorized addresses. function getAuthorizedAddresses() diff --git a/contracts/asset-proxy/test/authorizable.ts b/contracts/asset-proxy/test/authorizable.ts index d900fdd9c7..2f93334c6a 100644 --- a/contracts/asset-proxy/test/authorizable.ts +++ b/contracts/asset-proxy/test/authorizable.ts @@ -8,7 +8,7 @@ import { } from '@0x/contracts-test-utils'; import { BlockchainLifecycle } from '@0x/dev-utils'; import { RevertReason } from '@0x/types'; -import { BigNumber, OwnableRevertErrors } from '@0x/utils'; +import { BigNumber } from '@0x/utils'; import * as chai from 'chai'; import * as _ from 'lodash'; @@ -52,9 +52,10 @@ describe('Authorizable', () => { describe('addAuthorizedAddress', () => { it('should revert if not called by owner', async () => { - const expectedError = new OwnableRevertErrors.OnlyOwnerError(notOwner, owner); - const tx = authorizable.addAuthorizedAddress.sendTransactionAsync(notOwner, { from: notOwner }); - return expect(tx).to.revertWith(expectedError); + await expectTransactionFailedAsync( + authorizable.addAuthorizedAddress.sendTransactionAsync(notOwner, { from: notOwner }), + RevertReason.OnlyContractOwner, + ); }); it('should allow owner to add an authorized address', async () => { @@ -87,9 +88,10 @@ describe('Authorizable', () => { { from: owner }, constants.AWAIT_TRANSACTION_MINED_MS, ); - const expectedError = new OwnableRevertErrors.OnlyOwnerError(notOwner, owner); - const tx = authorizable.removeAuthorizedAddress.sendTransactionAsync(address, { from: notOwner }); - return expect(tx).to.revertWith(expectedError); + await expectTransactionFailedAsync( + authorizable.removeAuthorizedAddress.sendTransactionAsync(address, { from: notOwner }), + RevertReason.OnlyContractOwner, + ); }); it('should allow owner to remove an authorized address', async () => { @@ -125,12 +127,14 @@ describe('Authorizable', () => { constants.AWAIT_TRANSACTION_MINED_MS, ); const index = new BigNumber(0); - const expectedError = new OwnableRevertErrors.OnlyOwnerError(notOwner, owner); - const tx = authorizable.removeAuthorizedAddressAtIndex.sendTransactionAsync(address, index, { - from: notOwner, - }); - return expect(tx).to.revertWith(expectedError); + await expectTransactionFailedAsync( + authorizable.removeAuthorizedAddressAtIndex.sendTransactionAsync(address, index, { + from: notOwner, + }), + RevertReason.OnlyContractOwner, + ); }); + it('should revert if index is >= authorities.length', async () => { await authorizable.addAuthorizedAddress.awaitTransactionSuccessAsync( address, @@ -145,6 +149,7 @@ describe('Authorizable', () => { RevertReason.IndexOutOfBounds, ); }); + it('should revert if owner attempts to remove an address that is not authorized', async () => { const index = new BigNumber(0); return expectTransactionFailedAsync( @@ -154,6 +159,7 @@ describe('Authorizable', () => { RevertReason.TargetNotAuthorized, ); }); + it('should revert if address at index does not match target', async () => { const address1 = address; const address2 = notOwner;