From ab698cec14f010fa5ac15fe567fd572adecce7ff Mon Sep 17 00:00:00 2001 From: Lawrence Forman Date: Mon, 23 Nov 2020 12:59:02 -0500 Subject: [PATCH] EP: misc fixes (#38) * `@0x/contracts-zero-ex`: Fix NativeOrdersFeature order hash cancellation not emitting proper maker. `@0x/contracts-zero-ex`: Revert to original (deployed) ZeroEx/EP proxy implementation. Optimized one is now at `ZeroExOptimized.sol`. `@0x/contracts-zero-ex`: Add gas limits to first `transferFrom()` call in `LibTokenSpender` and `UniswapFeature`. * `@0x/contracts-zero-ex`: Update changelog * disable `no-empty-blocks` solidity linter rule * `@0x/contracts-zero-ex`: Use bloom filters of greedy tokens in token transfer logic `@0x/contracts-zero-ex`: Turn `LibTokenSpender` into `FixinTokenSpender`. `@0x/contracts-zero-ex`: Misc renames for consistency. * `@0x/contracts-zero-ex`: Export `GREEDY_TOKENS` list * rebase and update changelog * `@0x/contracts-zero-ex`: Change bloom filter hash algo based on discussions * `@0x/contracts-zero-ex`: Fix changelog * update orders docs * `@0x/contracts-zero-ex`: revert if allowance call fails in uniswap feature Co-authored-by: Lawrence Forman --- contracts/.solhint.json | 1 + contracts/zero-ex/CHANGELOG.json | 29 ++++ contracts/zero-ex/contracts/src/ZeroEx.sol | 90 +++++----- .../zero-ex/contracts/src/ZeroExOptimized.sol | 91 ++++++++++ .../ISimpleFunctionRegistryFeature.sol | 8 - .../src/features/LiquidityProviderFeature.sol | 13 +- .../src/features/MetaTransactionsFeature.sol | 14 +- .../src/features/NativeOrdersFeature.sol | 26 +-- .../SimpleFunctionRegistryFeature.sol | 13 -- .../src/features/TransformERC20Feature.sol | 17 +- .../contracts/src/features/UniswapFeature.sol | 163 +++++++++++++----- .../src/fixins/FixinProtocolFees.sol | 1 - .../FixinTokenSpender.sol} | 133 ++++++++++---- ...nSpender.sol => TestFixinTokenSpender.sol} | 29 +++- .../contracts/test/TestInitialMigration.sol | 3 +- ...tMetaTransactionsTransformERC20Feature.sol | 2 + .../contracts/test/TestMintableERC20Token.sol | 8 + .../test/TestNativeOrdersFeature.sol | 11 +- .../test/TestTokenSpenderERC20Token.sol | 8 + .../contracts/test/TestTransformERC20.sol | 2 + contracts/zero-ex/package.json | 2 +- contracts/zero-ex/src/bloom_filter_utils.ts | 41 +++++ contracts/zero-ex/src/constants.ts | 21 +++ contracts/zero-ex/src/index.ts | 2 + contracts/zero-ex/src/migration.ts | 6 + contracts/zero-ex/test/artifacts.ts | 10 +- .../test/features/liquidity_provider_test.ts | 2 + ..._test.ts => native_orders_feature_test.ts} | 2 +- .../features/simple_function_registry_test.ts | 2 +- .../test/features/transform_erc20_test.ts | 3 +- ...er_test.ts => fixin_token_spender_test.ts} | 140 +++++++++++---- contracts/zero-ex/test/full_migration_test.ts | 5 +- contracts/zero-ex/test/wrappers.ts | 5 +- contracts/zero-ex/test/zero_ex_test.ts | 2 +- contracts/zero-ex/tsconfig.json | 7 +- docs/basics/orders.rst | 1 - 36 files changed, 673 insertions(+), 240 deletions(-) create mode 100644 contracts/zero-ex/contracts/src/ZeroExOptimized.sol rename contracts/zero-ex/contracts/src/{features/libs/LibTokenSpender.sol => fixins/FixinTokenSpender.sol} (51%) rename contracts/zero-ex/contracts/test/{TestLibTokenSpender.sol => TestFixinTokenSpender.sol} (75%) create mode 100644 contracts/zero-ex/src/bloom_filter_utils.ts create mode 100644 contracts/zero-ex/src/constants.ts rename contracts/zero-ex/test/features/{limit_orders_feature_test.ts => native_orders_feature_test.ts} (99%) rename contracts/zero-ex/test/{lib_token_spender_test.ts => fixin_token_spender_test.ts} (54%) diff --git a/contracts/.solhint.json b/contracts/.solhint.json index 3be459e776..3d89418c4e 100644 --- a/contracts/.solhint.json +++ b/contracts/.solhint.json @@ -13,6 +13,7 @@ "indent": ["error", 4], "max-line-length": ["warn", 160], "no-inline-assembly": false, + "no-empty-blocks": false, "quotes": ["error", "double"], "separate-by-one-line-in-contract": "error", "space-after-comma": "error", diff --git a/contracts/zero-ex/CHANGELOG.json b/contracts/zero-ex/CHANGELOG.json index 6521d93e1c..3fc6efe662 100644 --- a/contracts/zero-ex/CHANGELOG.json +++ b/contracts/zero-ex/CHANGELOG.json @@ -1,4 +1,29 @@ [ + { + "version": "0.11.0", + "changes": [ + { + "note": "Turn `LibTokenSpender` into `FixinTokenSpender`", + "pr": 38 + }, + { + "note": "Use bloom filters to check if a token is greedy and do not optimistically fall through transferFrom() if so", + "pr": 38 + }, + { + "note": "Revert to original proxy implementation", + "pr": 38 + }, + { + "note": "Fix incorrect cancel order event param", + "pr": 38 + }, + { + "note": "Add a gas limit to first `LibTokenSpender` and `UniswapFeature` transfer", + "pr": 38 + } + ] + }, { "version": "0.10.0", "changes": [ @@ -9,6 +34,10 @@ { "note": "Use new `checkAllowance` flag in LiquidityProviderFeature, TransformERC20Feature, and MetaTransactionsFeature", "pr": 39 + }, + { + "note": "Add native orders features", + "pr": 27 } ], "timestamp": 1605763885 diff --git a/contracts/zero-ex/contracts/src/ZeroEx.sol b/contracts/zero-ex/contracts/src/ZeroEx.sol index 3ebc087f89..a18d762a93 100644 --- a/contracts/zero-ex/contracts/src/ZeroEx.sol +++ b/contracts/zero-ex/contracts/src/ZeroEx.sol @@ -19,12 +19,19 @@ pragma solidity ^0.6.5; pragma experimental ABIEncoderV2; +import "@0x/contracts-utils/contracts/src/v06/LibBytesV06.sol"; +import "./migrations/LibBootstrap.sol"; import "./features/BootstrapFeature.sol"; import "./storage/LibProxyStorage.sol"; +import "./errors/LibProxyRichErrors.sol"; + /// @dev An extensible proxy contract that serves as a universal entry point for /// interacting with the 0x protocol. contract ZeroEx { + // solhint-disable separate-by-one-line-in-contract,indent,var-name-mixedcase + using LibBytesV06 for bytes; + /// @dev Construct this contract and register the `BootstrapFeature` feature. /// After constructing this contract, `bootstrap()` should be called /// by `bootstrap()` to seed the initial feature set. @@ -37,55 +44,48 @@ contract ZeroEx { address(bootstrap); } - // solhint-disable state-visibility /// @dev Forwards calls to the appropriate implementation contract. fallback() external payable { - // This is used in assembly below as impls_slot. - mapping(bytes4 => address) storage impls = - LibProxyStorage.getStorage().impls; - - assembly { - let cdlen := calldatasize() - - // equivalent of receive() external payable {} - if iszero(cdlen) { - return(0, 0) - } - - // Store at 0x40, to leave 0x00-0x3F for slot calculation below. - calldatacopy(0x40, 0, cdlen) - let selector := and(mload(0x40), 0xFFFFFFFF00000000000000000000000000000000000000000000000000000000) - - // Slot for impls[selector] is keccak256(selector . impls_slot). - mstore(0, selector) - mstore(0x20, impls_slot) - let slot := keccak256(0, 0x40) - - let delegate := sload(slot) - if iszero(delegate) { - // Revert with: - // abi.encodeWithSelector( - // bytes4(keccak256("NotImplementedError(bytes4)")), - // selector) - mstore(0, 0x734e6e1c00000000000000000000000000000000000000000000000000000000) - mstore(4, selector) - revert(0, 0x24) - } - - let success := delegatecall( - gas(), - delegate, - 0x40, cdlen, - 0, 0 - ) - let rdlen := returndatasize() - returndatacopy(0, 0, rdlen) - if success { - return(0, rdlen) - } - revert(0, rdlen) + bytes4 selector = msg.data.readBytes4(0); + address impl = getFunctionImplementation(selector); + if (impl == address(0)) { + _revertWithData(LibProxyRichErrors.NotImplementedError(selector)); } + + (bool success, bytes memory resultData) = impl.delegatecall(msg.data); + if (!success) { + _revertWithData(resultData); + } + _returnWithData(resultData); + } + + /// @dev Fallback for just receiving ether. + receive() external payable {} + + // solhint-enable state-visibility + + /// @dev Get the implementation contract of a registered function. + /// @param selector The function selector. + /// @return impl The implementation contract address. + function getFunctionImplementation(bytes4 selector) + public + view + returns (address impl) + { + return LibProxyStorage.getStorage().impls[selector]; + } + + /// @dev Revert with arbitrary bytes. + /// @param data Revert data. + function _revertWithData(bytes memory data) private pure { + assembly { revert(add(data, 32), mload(data)) } + } + + /// @dev Return with arbitrary bytes. + /// @param data Return data. + function _returnWithData(bytes memory data) private pure { + assembly { return(add(data, 32), mload(data)) } } } diff --git a/contracts/zero-ex/contracts/src/ZeroExOptimized.sol b/contracts/zero-ex/contracts/src/ZeroExOptimized.sol new file mode 100644 index 0000000000..c40de68b3e --- /dev/null +++ b/contracts/zero-ex/contracts/src/ZeroExOptimized.sol @@ -0,0 +1,91 @@ +/* + + Copyright 2020 ZeroEx Intl. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + +*/ + +pragma solidity ^0.6.5; +pragma experimental ABIEncoderV2; + +import "./features/BootstrapFeature.sol"; +import "./storage/LibProxyStorage.sol"; + +/// @dev An extensible proxy contract that serves as a universal entry point for +/// interacting with the 0x protocol. Optimized version of ZeroEx. +contract ZeroExOptimized { + /// @dev Construct this contract and register the `BootstrapFeature` feature. + /// After constructing this contract, `bootstrap()` should be called + /// by `bootstrap()` to seed the initial feature set. + /// @param bootstrapper Who can call `bootstrap()`. + constructor(address bootstrapper) public { + // Temporarily create and register the bootstrap feature. + // It will deregister itself after `bootstrap()` has been called. + BootstrapFeature bootstrap = new BootstrapFeature(bootstrapper); + LibProxyStorage.getStorage().impls[bootstrap.bootstrap.selector] = + address(bootstrap); + } + + + // solhint-disable state-visibility + + /// @dev Forwards calls to the appropriate implementation contract. + fallback() external payable { + // This is used in assembly below as impls_slot. + mapping(bytes4 => address) storage impls = + LibProxyStorage.getStorage().impls; + + assembly { + let cdlen := calldatasize() + + // equivalent of receive() external payable {} + if iszero(cdlen) { + return(0, 0) + } + + // Store at 0x40, to leave 0x00-0x3F for slot calculation below. + calldatacopy(0x40, 0, cdlen) + let selector := and(mload(0x40), 0xFFFFFFFF00000000000000000000000000000000000000000000000000000000) + + // Slot for impls[selector] is keccak256(selector . impls_slot). + mstore(0, selector) + mstore(0x20, impls_slot) + let slot := keccak256(0, 0x40) + + let delegate := sload(slot) + if iszero(delegate) { + // Revert with: + // abi.encodeWithSelector( + // bytes4(keccak256("NotImplementedError(bytes4)")), + // selector) + mstore(0, 0x734e6e1c00000000000000000000000000000000000000000000000000000000) + mstore(4, selector) + revert(0, 0x24) + } + + let success := delegatecall( + gas(), + delegate, + 0x40, cdlen, + 0, 0 + ) + let rdlen := returndatasize() + returndatacopy(0, 0, rdlen) + if success { + return(0, rdlen) + } + revert(0, rdlen) + } + } +} diff --git a/contracts/zero-ex/contracts/src/features/ISimpleFunctionRegistryFeature.sol b/contracts/zero-ex/contracts/src/features/ISimpleFunctionRegistryFeature.sol index 583b97f9ad..1e5ef61c25 100644 --- a/contracts/zero-ex/contracts/src/features/ISimpleFunctionRegistryFeature.sol +++ b/contracts/zero-ex/contracts/src/features/ISimpleFunctionRegistryFeature.sol @@ -57,12 +57,4 @@ interface ISimpleFunctionRegistryFeature { external view returns (address impl); - - /// @dev Get the implementation contract of a registered function. - /// @param selector The function selector. - /// @return impl The implementation contract address. - function getFunctionImplementation(bytes4 selector) - external - view - returns (address impl); } diff --git a/contracts/zero-ex/contracts/src/features/LiquidityProviderFeature.sol b/contracts/zero-ex/contracts/src/features/LiquidityProviderFeature.sol index d64dde5eea..1e9e0271a6 100644 --- a/contracts/zero-ex/contracts/src/features/LiquidityProviderFeature.sol +++ b/contracts/zero-ex/contracts/src/features/LiquidityProviderFeature.sol @@ -26,16 +26,17 @@ import "../errors/LibLiquidityProviderRichErrors.sol"; import "../external/ILiquidityProviderSandbox.sol"; import "../external/LiquidityProviderSandbox.sol"; import "../fixins/FixinCommon.sol"; +import "../fixins/FixinTokenSpender.sol"; import "../migrations/LibMigrate.sol"; import "./IFeature.sol"; import "./ILiquidityProviderFeature.sol"; -import "./libs/LibTokenSpender.sol"; contract LiquidityProviderFeature is IFeature, ILiquidityProviderFeature, - FixinCommon + FixinCommon, + FixinTokenSpender { using LibSafeMathV06 for uint256; using LibRichErrorsV06 for bytes; @@ -60,9 +61,10 @@ contract LiquidityProviderFeature is address recipient ); - constructor(LiquidityProviderSandbox sandbox_) + constructor(LiquidityProviderSandbox sandbox_, bytes32 greedyTokensBloomFilter) public FixinCommon() + FixinTokenSpender(greedyTokensBloomFilter) { sandbox = sandbox_; } @@ -112,12 +114,11 @@ contract LiquidityProviderFeature is if (inputToken == ETH_TOKEN_ADDRESS) { provider.transfer(sellAmount); } else { - LibTokenSpender.spendERC20Tokens( + _transferERC20Tokens( IERC20TokenV06(inputToken), msg.sender, provider, - sellAmount, - true + sellAmount ); } diff --git a/contracts/zero-ex/contracts/src/features/MetaTransactionsFeature.sol b/contracts/zero-ex/contracts/src/features/MetaTransactionsFeature.sol index 3fb5984bee..81070d4bbb 100644 --- a/contracts/zero-ex/contracts/src/features/MetaTransactionsFeature.sol +++ b/contracts/zero-ex/contracts/src/features/MetaTransactionsFeature.sol @@ -25,6 +25,7 @@ import "@0x/contracts-utils/contracts/src/v06/LibSafeMathV06.sol"; import "../errors/LibMetaTransactionsRichErrors.sol"; import "../fixins/FixinCommon.sol"; import "../fixins/FixinReentrancyGuard.sol"; +import "../fixins/FixinTokenSpender.sol"; import "../fixins/FixinEIP712.sol"; import "../migrations/LibMigrate.sol"; import "../storage/LibMetaTransactionsStorage.sol"; @@ -33,8 +34,6 @@ import "./IMetaTransactionsFeature.sol"; import "./ITransformERC20Feature.sol"; import "./ISignatureValidatorFeature.sol"; import "./IFeature.sol"; -import "./libs/LibTokenSpender.sol"; - /// @dev MetaTransactions feature. contract MetaTransactionsFeature is @@ -42,7 +41,8 @@ contract MetaTransactionsFeature is IMetaTransactionsFeature, FixinCommon, FixinReentrancyGuard, - FixinEIP712 + FixinEIP712, + FixinTokenSpender { using LibBytesV06 for bytes; using LibRichErrorsV06 for bytes; @@ -105,10 +105,11 @@ contract MetaTransactionsFeature is } } - constructor(address zeroExAddress) + constructor(address zeroExAddress, bytes32 greedyTokensBloomFilter) public FixinCommon() FixinEIP712(zeroExAddress) + FixinTokenSpender(greedyTokensBloomFilter) { // solhint-disable-next-line no-empty-blocks } @@ -279,12 +280,11 @@ contract MetaTransactionsFeature is // Pay the fee to the sender. if (mtx.feeAmount > 0) { - LibTokenSpender.spendERC20Tokens( + _transferERC20Tokens( mtx.feeToken, mtx.signer, sender, - mtx.feeAmount, - true + mtx.feeAmount ); } diff --git a/contracts/zero-ex/contracts/src/features/NativeOrdersFeature.sol b/contracts/zero-ex/contracts/src/features/NativeOrdersFeature.sol index 5788acb8a7..ae40077af9 100644 --- a/contracts/zero-ex/contracts/src/features/NativeOrdersFeature.sol +++ b/contracts/zero-ex/contracts/src/features/NativeOrdersFeature.sol @@ -28,11 +28,11 @@ import "@0x/contracts-utils/contracts/src/v06/LibMathV06.sol"; import "../fixins/FixinCommon.sol"; import "../fixins/FixinProtocolFees.sol"; import "../fixins/FixinEIP712.sol"; +import "../fixins/FixinTokenSpender.sol"; import "../errors/LibNativeOrdersRichErrors.sol"; import "../migrations/LibMigrate.sol"; import "../storage/LibNativeOrdersStorage.sol"; import "../vendor/v3/IStaking.sol"; -import "./libs/LibTokenSpender.sol"; import "./libs/LibSignature.sol"; import "./libs/LibNativeOrder.sol"; import "./INativeOrdersFeature.sol"; @@ -45,7 +45,8 @@ contract NativeOrdersFeature is INativeOrdersFeature, FixinCommon, FixinProtocolFees, - FixinEIP712 + FixinEIP712, + FixinTokenSpender { using LibSafeMathV06 for uint256; using LibSafeMathV06 for uint128; @@ -107,11 +108,13 @@ contract NativeOrdersFeature is address zeroExAddress, IEtherTokenV06 weth, IStaking staking, - uint32 protocolFeeMultiplier + uint32 protocolFeeMultiplier, + bytes32 greedyTokensBloomFilter ) public FixinEIP712(zeroExAddress) FixinProtocolFees(weth, staking, protocolFeeMultiplier) + FixinTokenSpender(greedyTokensBloomFilter) { // solhint-disable no-empty-blocks } @@ -733,7 +736,7 @@ contract NativeOrdersFeature is // a cancel. It's OK to cancel twice. stor.orderHashToTakerTokenFilledAmount[orderHash] |= HIGH_BIT; - emit OrderCancelled(orderHash, msg.sender); + emit OrderCancelled(orderHash, maker); } /// @dev Fill a limit order. Private variant. Does not refund protocol fees. @@ -811,12 +814,11 @@ contract NativeOrdersFeature is params.order.takerAmount, params.order.takerTokenFeeAmount )); - LibTokenSpender.spendERC20Tokens( + _transferERC20Tokens( params.order.takerToken, params.taker, params.order.feeRecipient, - uint256(results.takerTokenFeeFilledAmount), - false + uint256(results.takerTokenFeeFilledAmount) ); } @@ -948,21 +950,19 @@ contract NativeOrdersFeature is settleInfo.takerTokenFilledAmount.safeAdd128(takerTokenFilledAmount); // Transfer taker -> maker. - LibTokenSpender.spendERC20Tokens( + _transferERC20Tokens( settleInfo.takerToken, settleInfo.taker, settleInfo.maker, - takerTokenFilledAmount, - false + takerTokenFilledAmount ); // Transfer maker -> taker. - LibTokenSpender.spendERC20Tokens( + _transferERC20Tokens( settleInfo.makerToken, settleInfo.maker, settleInfo.taker, - makerTokenFilledAmount, - false + makerTokenFilledAmount ); } diff --git a/contracts/zero-ex/contracts/src/features/SimpleFunctionRegistryFeature.sol b/contracts/zero-ex/contracts/src/features/SimpleFunctionRegistryFeature.sol index 32a2bb7ab5..ac98b64dc7 100644 --- a/contracts/zero-ex/contracts/src/features/SimpleFunctionRegistryFeature.sol +++ b/contracts/zero-ex/contracts/src/features/SimpleFunctionRegistryFeature.sol @@ -56,7 +56,6 @@ contract SimpleFunctionRegistryFeature is // Register getters. _extend(this.getRollbackLength.selector, _implementation); _extend(this.getRollbackEntryAtIndex.selector, _implementation); - _extend(this.getFunctionImplementation.selector, _implementation); return LibBootstrap.BOOTSTRAP_SUCCESS; } @@ -152,18 +151,6 @@ contract SimpleFunctionRegistryFeature is return LibSimpleFunctionRegistryStorage.getStorage().implHistory[selector][idx]; } - /// @dev Get the implementation contract of a registered function. - /// @param selector The function selector. - /// @return impl The implementation contract address. - function getFunctionImplementation(bytes4 selector) - external - override - view - returns (address impl) - { - return LibProxyStorage.getStorage().impls[selector]; - } - /// @dev Register or replace a function. /// @param selector The function selector. /// @param impl The implementation contract for the function. diff --git a/contracts/zero-ex/contracts/src/features/TransformERC20Feature.sol b/contracts/zero-ex/contracts/src/features/TransformERC20Feature.sol index 8ed426f011..f92bc8a261 100644 --- a/contracts/zero-ex/contracts/src/features/TransformERC20Feature.sol +++ b/contracts/zero-ex/contracts/src/features/TransformERC20Feature.sol @@ -25,6 +25,7 @@ import "@0x/contracts-utils/contracts/src/v06/errors/LibRichErrorsV06.sol"; import "@0x/contracts-utils/contracts/src/v06/LibSafeMathV06.sol"; import "../errors/LibTransformERC20RichErrors.sol"; import "../fixins/FixinCommon.sol"; +import "../fixins/FixinTokenSpender.sol"; import "../migrations/LibMigrate.sol"; import "../external/IFlashWallet.sol"; import "../external/FlashWallet.sol"; @@ -35,14 +36,14 @@ import "./libs/LibSignedCallData.sol"; import "./ITransformERC20Feature.sol"; import "./IFeature.sol"; import "./ISignatureValidatorFeature.sol"; -import "./libs/LibTokenSpender.sol"; /// @dev Feature to composably transform between ERC20 tokens. contract TransformERC20Feature is IFeature, ITransformERC20Feature, - FixinCommon + FixinCommon, + FixinTokenSpender { using LibSafeMathV06 for uint256; using LibRichErrorsV06 for bytes; @@ -60,6 +61,11 @@ contract TransformERC20Feature is /// @dev Version of this feature. uint256 public immutable override FEATURE_VERSION = _encodeVersion(1, 3, 1); + constructor(bytes32 greedyTokensBloomFilter) + public + FixinTokenSpender(greedyTokensBloomFilter) + {} + /// @dev Initialize and register this feature. /// Should be delegatecalled by `Migrate.migrate()`. /// @param transformerDeployer The trusted deployer for transformers. @@ -211,7 +217,7 @@ contract TransformERC20Feature is // If the input token amount is -1, transform the taker's entire // spendable balance. if (args.inputTokenAmount == uint256(-1)) { - args.inputTokenAmount = LibTokenSpender.getSpendableERC20BalanceOf( + args.inputTokenAmount = _getSpendableERC20BalanceOf( args.inputToken, args.taker ); @@ -317,12 +323,11 @@ contract TransformERC20Feature is // Transfer input tokens. if (!LibERC20Transformer.isTokenETH(inputToken)) { // Token is not ETH, so pull ERC20 tokens. - LibTokenSpender.spendERC20Tokens( + _transferERC20Tokens( inputToken, from, to, - amount, - true + amount ); } else if (msg.value < amount) { // Token is ETH, so the caller must attach enough ETH to the call. diff --git a/contracts/zero-ex/contracts/src/features/UniswapFeature.sol b/contracts/zero-ex/contracts/src/features/UniswapFeature.sol index 9009902021..9ea99f0eb6 100644 --- a/contracts/zero-ex/contracts/src/features/UniswapFeature.sol +++ b/contracts/zero-ex/contracts/src/features/UniswapFeature.sol @@ -38,6 +38,8 @@ contract UniswapFeature is string public constant override FEATURE_NAME = "UniswapFeature"; /// @dev Version of this feature. uint256 public immutable override FEATURE_VERSION = _encodeVersion(1, 1, 0); + /// @dev A bloom filter for tokens that consume all gas when `transferFrom()` fails. + bytes32 public immutable GREEDY_TOKENS_BLOOM_FILTER; /// @dev WETH contract. IEtherTokenV06 private immutable WETH; /// @dev AllowanceTarget instance. @@ -66,6 +68,8 @@ contract UniswapFeature is uint256 constant private UNISWAP_PAIR_SWAP_CALL_SELECTOR_32 = 0x022c0d9f00000000000000000000000000000000000000000000000000000000; // bytes4(keccak256("transferFrom(address,address,uint256)")) uint256 constant private TRANSFER_FROM_CALL_SELECTOR_32 = 0x23b872dd00000000000000000000000000000000000000000000000000000000; + // bytes4(keccak256("allowance(address,address)")) + uint256 constant private ALLOWANCE_CALL_SELECTOR_32 = 0xdd62ed3e00000000000000000000000000000000000000000000000000000000; // bytes4(keccak256("withdraw(uint256)")) uint256 constant private WETH_WITHDRAW_CALL_SELECTOR_32 = 0x2e1a7d4d00000000000000000000000000000000000000000000000000000000; // bytes4(keccak256("deposit()")) @@ -76,9 +80,15 @@ contract UniswapFeature is /// @dev Construct this contract. /// @param weth The WETH contract. /// @param allowanceTarget The AllowanceTarget contract. - constructor(IEtherTokenV06 weth, IAllowanceTarget allowanceTarget) public { + /// @param greedyTokensBloomFilter The bloom filter for greedy tokens. + constructor( + IEtherTokenV06 weth, + IAllowanceTarget allowanceTarget, + bytes32 greedyTokensBloomFilter + ) public { WETH = weth; ALLOWANCE_TARGET = allowanceTarget; + GREEDY_TOKENS_BLOOM_FILTER = greedyTokensBloomFilter; } /// @dev Initialize and register this feature. @@ -114,6 +124,7 @@ contract UniswapFeature is // Load immutables onto the stack. IEtherTokenV06 weth = WETH; IAllowanceTarget allowanceTarget = ALLOWANCE_TARGET; + bytes32 greedyTokensBloomFilter = GREEDY_TOKENS_BLOOM_FILTER; // Store some vars in memory to get around stack limits. assembly { @@ -125,6 +136,8 @@ contract UniswapFeature is mstore(0xA40, weth) // mload(0xA60) == ALLOWANCE_TARGET mstore(0xA60, allowanceTarget) + // mload(0xA80) == GREEDY_TOKENS_BLOOM_FILTER + mstore(0xA80, greedyTokensBloomFilter) } } @@ -155,52 +168,10 @@ contract UniswapFeature is if iszero(i) { switch eq(sellToken, ETH_TOKEN_ADDRESS_32) - case 0 { + case 0 { // Not selling ETH. Selling an ERC20 instead. // For the first pair we need to transfer sellTokens into the // pair contract. - mstore(0xB00, TRANSFER_FROM_CALL_SELECTOR_32) - mstore(0xB04, caller()) - mstore(0xB24, pair) - mstore(0xB44, sellAmount) - - // Copy only the first 32 bytes of return data. We - // only care about reading a boolean in the success - // case, and we discard the return data in the - // failure case. - let success := call(gas(), sellToken, 0, 0xB00, 0x64, 0xC00, 0x20) - - let rdsize := returndatasize() - - // Check for ERC20 success. ERC20 tokens should - // return a boolean, but some return nothing or - // extra data. We accept 0-length return data as - // success, or at least 32 bytes that starts with - // a 32-byte boolean true. - success := and( - success, // call itself succeeded - or( - iszero(rdsize), // no return data, or - and( - iszero(lt(rdsize, 32)), // at least 32 bytes - eq(mload(0xC00), 1) // starts with uint256(1) - ) - ) - ) - - if iszero(success) { - // Try to fall back to the allowance target. - mstore(0xB00, ALLOWANCE_TARGET_EXECUTE_CALL_SELECTOR_32) - mstore(0xB04, sellToken) - mstore(0xB24, 0x40) - mstore(0xB44, 0x64) - mstore(0xB64, TRANSFER_FROM_CALL_SELECTOR_32) - mstore(0xB68, caller()) - mstore(0xB88, pair) - mstore(0xBA8, sellAmount) - if iszero(call(gas(), mload(0xA60), 0, 0xB00, 0xC8, 0x00, 0x0)) { - bubbleRevert() - } - } + moveTakerTokensTo(sellToken, pair, sellAmount) } default { // If selling ETH, we need to wrap it to WETH and transfer to the @@ -389,6 +360,108 @@ contract UniswapFeature is returndatacopy(0, 0, returndatasize()) revert(0, returndatasize()) } + + // Move `amount` tokens from the taker/caller to `to`. + function moveTakerTokensTo(token, to, amount) { + + // If the token is possibly greedy, we check the allowance rather + // than relying on letting the transferFrom() call fail and + // falling through to legacy allowance target because the token + // will eat all our gas. + if isTokenPossiblyGreedy(token) { + // Check if we have enough direct allowance by calling + // `token.allowance()`` + mstore(0xB00, ALLOWANCE_CALL_SELECTOR_32) + mstore(0xB04, caller()) + mstore(0xB24, address()) + let success := call(gas(), token, 0, 0xB00, 0x44, 0xC00, 0x20) + if iszero(success) { + // Call to allowance() failed. + bubbleRevert() + } + // Call succeeded. + // Result is stored in 0xC00-0xC20. + if lt(mload(0xC00), amount) { + // We don't have enough direct allowance, so try + // going through the legacy allowance taregt. + moveTakerTokensToWithLegacyAllowanceTarget(token, to, amount) + leave + } + } + + // Otherwise we will optimistically try to perform a `transferFrom()` + // directly then if it fails we will go through the legacy allowance target. + mstore(0xB00, TRANSFER_FROM_CALL_SELECTOR_32) + mstore(0xB04, caller()) + mstore(0xB24, to) + mstore(0xB44, amount) + + let success := call( + // Cap the gas limit to prvent all gas being consumed + // if the token reverts. + gas(), + token, + 0, + 0xB00, + 0x64, + 0xC00, + // Copy only the first 32 bytes of return data. We + // only care about reading a boolean in the success + // case, and we discard the return data in the + // failure case. + 0x20 + ) + + let rdsize := returndatasize() + + // Check for ERC20 success. ERC20 tokens should + // return a boolean, but some return nothing or + // extra data. We accept 0-length return data as + // success, or at least 32 bytes that starts with + // a 32-byte boolean true. + success := and( + success, // call itself succeeded + or( + iszero(rdsize), // no return data, or + and( + iszero(lt(rdsize, 32)), // at least 32 bytes + eq(mload(0xC00), 1) // starts with uint256(1) + ) + ) + ) + + if iszero(success) { + // Try to fall back to the allowance target. + moveTakerTokensToWithLegacyAllowanceTarget(token, to, amount) + } + } + + // Move tokens by going through the legacy allowance target contract. + function moveTakerTokensToWithLegacyAllowanceTarget(token, to, amount) { + mstore(0xB00, ALLOWANCE_TARGET_EXECUTE_CALL_SELECTOR_32) + mstore(0xB04, token) + mstore(0xB24, 0x40) + mstore(0xB44, 0x64) + mstore(0xB64, TRANSFER_FROM_CALL_SELECTOR_32) + mstore(0xB68, caller()) + mstore(0xB88, to) + mstore(0xBA8, amount) + if iszero(call(gas(), mload(0xA60), 0, 0xB00, 0xC8, 0x00, 0x0)) { + bubbleRevert() + } + // If this fall back failed, the swap will most likely fail + // so there's no need to validate the result. + } + + // Checks if a token possibly belongs to the GREEDY_TOKENS_BLOOM_FILTER + // bloom filter. + function isTokenPossiblyGreedy(token) -> isPossiblyGreedy { + // The hash is given by: + // (1 << (keccak256(token) % 256)) | (1 << (token % 256)) + mstore(0, token) + let h := or(shl(mod(keccak256(0, 32), 256), 1), shl(mod(token, 256), 1)) + isPossiblyGreedy := eq(and(h, mload(0xA80)), h) + } } // Revert if we bought too little. diff --git a/contracts/zero-ex/contracts/src/fixins/FixinProtocolFees.sol b/contracts/zero-ex/contracts/src/fixins/FixinProtocolFees.sol index 84bc832f01..5cfbca010e 100644 --- a/contracts/zero-ex/contracts/src/fixins/FixinProtocolFees.sol +++ b/contracts/zero-ex/contracts/src/fixins/FixinProtocolFees.sol @@ -21,7 +21,6 @@ pragma experimental ABIEncoderV2; import "@0x/contracts-erc20/contracts/src/v06/IEtherTokenV06.sol"; import "../external/FeeCollector.sol"; -import "../features/libs/LibTokenSpender.sol"; import "../vendor/v3/IStaking.sol"; diff --git a/contracts/zero-ex/contracts/src/features/libs/LibTokenSpender.sol b/contracts/zero-ex/contracts/src/fixins/FixinTokenSpender.sol similarity index 51% rename from contracts/zero-ex/contracts/src/features/libs/LibTokenSpender.sol rename to contracts/zero-ex/contracts/src/fixins/FixinTokenSpender.sol index 42620f8132..2bca849717 100644 --- a/contracts/zero-ex/contracts/src/features/libs/LibTokenSpender.sol +++ b/contracts/zero-ex/contracts/src/fixins/FixinTokenSpender.sol @@ -19,48 +19,64 @@ pragma solidity ^0.6.5; pragma experimental ABIEncoderV2; -import "@0x/contracts-erc20/contracts/src/v06/IERC20TokenV06.sol"; +import "@0x/contracts-erc20/contracts/src/v06/IEtherTokenV06.sol"; import "@0x/contracts-utils/contracts/src/v06/LibSafeMathV06.sol"; -import "../../errors/LibSpenderRichErrors.sol"; -import "../ITokenSpenderFeature.sol"; +import "../features/ITokenSpenderFeature.sol"; +import "../errors/LibSpenderRichErrors.sol"; +import "../external/FeeCollector.sol"; +import "../vendor/v3/IStaking.sol"; +import "../vendor/v3/IStaking.sol"; -library LibTokenSpender { + +/// @dev Helpers for moving tokens around. +abstract contract FixinTokenSpender { using LibRichErrorsV06 for bytes; // Mask of the lower 20 bytes of a bytes32. uint256 constant private ADDRESS_MASK = 0x000000000000000000000000ffffffffffffffffffffffffffffffffffffffff; + /// @dev A bloom filter for tokens that consume all gas when `transferFrom()` fails. + bytes32 public immutable GREEDY_TOKENS_BLOOM_FILTER; + + /// @param greedyTokensBloomFilter The bloom filter for all greedy tokens. + constructor(bytes32 greedyTokensBloomFilter) + internal + { + GREEDY_TOKENS_BLOOM_FILTER = greedyTokensBloomFilter; + } /// @dev Transfers ERC20 tokens from `owner` to `to`. /// @param token The token to spend. /// @param owner The owner of the tokens. /// @param to The recipient of the tokens. /// @param amount The amount of `token` to transfer. - /// @param checkAllowance Whether or not to check the owner's allowance - /// prior to attempting the transfer. - function spendERC20Tokens( + function _transferERC20Tokens( IERC20TokenV06 token, address owner, address to, - uint256 amount, - bool checkAllowance + uint256 amount ) internal { bool success; bytes memory revertData; - require(address(token) != address(this), "LibTokenSpender/CANNOT_INVOKE_SELF"); + require(address(token) != address(this), "FixinTokenSpender/CANNOT_INVOKE_SELF"); - if (checkAllowance) { - // If the owner doesn't have a sufficient allowance set on `address(this)`, - // try the old AllowanceTarget. + // If the token eats all gas when failing, we do not want to perform + // optimistic fall through to the old AllowanceTarget contract if the + // direct transferFrom() fails. + if (_isTokenPossiblyGreedy(token)) { + // If the token does not have a direct allowance on us then we use + // the allowance target. if (token.allowance(owner, address(this)) < amount) { - return ITokenSpenderFeature(address(this))._spendERC20Tokens( + _transferFromLegacyAllowanceTarget( token, owner, to, - amount + amount, + "" ); + return; } } @@ -73,7 +89,15 @@ library LibTokenSpender { mstore(add(ptr, 0x24), and(to, ADDRESS_MASK)) mstore(add(ptr, 0x44), amount) - success := call(gas(), and(token, ADDRESS_MASK), 0, ptr, 0x64, 0, 0) + success := call( + gas(), + and(token, ADDRESS_MASK), + 0, + ptr, + 0x64, + 0, + 0 + ) let rdsize := returndatasize() @@ -104,25 +128,13 @@ library LibTokenSpender { } if (!success) { - // Try the old AllowanceTarget. - try ITokenSpenderFeature(address(this))._spendERC20Tokens( - token, - owner, - to, - amount - ) { - } catch { - // Bubble up the first error message. (In general, the fallback to the - // allowance target is opportunistic. We ignore the specific error - // message if it fails.) - LibSpenderRichErrors.SpenderERC20TransferFromFailedError( - address(token), - owner, - to, - amount, - revertData - ).rrevert(); - } + _transferFromLegacyAllowanceTarget( + token, + owner, + to, + amount, + revertData + ); } } @@ -131,7 +143,7 @@ library LibTokenSpender { /// @param token The token to spend. /// @param owner The owner of the tokens. /// @return amount The amount of tokens that can be pulled. - function getSpendableERC20BalanceOf( + function _getSpendableERC20BalanceOf( IERC20TokenV06 token, address owner ) @@ -144,4 +156,53 @@ library LibTokenSpender { token.balanceOf(owner) ); } + + /// @dev Check if a token possibly belongs to the `GREEDY_TOKENS_BLOOM_FILTER` + /// bloom filter. + function _isTokenPossiblyGreedy(IERC20TokenV06 token) + internal + view + returns (bool isPossiblyGreedy) + { + // The hash is given by: + // (1 << (keccak256(token) % 256)) | (1 << (token % 256)) + bytes32 h; + assembly { + mstore(0, token) + h := or(shl(mod(keccak256(0, 32), 256), 1), shl(mod(token, 256), 1)) + } + return (h & GREEDY_TOKENS_BLOOM_FILTER) == h; + } + + /// @dev Transfer tokens using the legacy allowance target instead of + /// allowances directly set on the exchange proxy. + function _transferFromLegacyAllowanceTarget( + IERC20TokenV06 token, + address owner, + address to, + uint256 amount, + bytes memory initialRevertData + ) + private + { + // Try the old AllowanceTarget. + try ITokenSpenderFeature(address(this))._spendERC20Tokens( + token, + owner, + to, + amount + ) { + } catch (bytes memory revertData) { + // Bubble up the first error message. (In general, the fallback to the + // allowance target is opportunistic. We ignore the specific error + // message if it fails.) + LibSpenderRichErrors.SpenderERC20TransferFromFailedError( + address(token), + owner, + to, + amount, + initialRevertData.length != 0 ? initialRevertData : revertData + ).rrevert(); + } + } } diff --git a/contracts/zero-ex/contracts/test/TestLibTokenSpender.sol b/contracts/zero-ex/contracts/test/TestFixinTokenSpender.sol similarity index 75% rename from contracts/zero-ex/contracts/test/TestLibTokenSpender.sol rename to contracts/zero-ex/contracts/test/TestFixinTokenSpender.sol index dbe00736f2..d8b965826e 100644 --- a/contracts/zero-ex/contracts/test/TestLibTokenSpender.sol +++ b/contracts/zero-ex/contracts/test/TestFixinTokenSpender.sol @@ -20,13 +20,19 @@ pragma solidity ^0.6.5; pragma experimental ABIEncoderV2; import "@0x/contracts-erc20/contracts/src/v06/IERC20TokenV06.sol"; +import "../src/fixins/FixinTokenSpender.sol"; -import "../src/features/libs/LibTokenSpender.sol"; - -contract TestLibTokenSpender { +contract TestFixinTokenSpender is + FixinTokenSpender +{ uint256 constant private TRIGGER_FALLBACK_SUCCESS_AMOUNT = 1340; - function spendERC20Tokens( + constructor(bytes32 greedyTokensBloomFilter) + public + FixinTokenSpender(greedyTokensBloomFilter) + {} + + function transferERC20Tokens( IERC20TokenV06 token, address owner, address to, @@ -34,12 +40,11 @@ contract TestLibTokenSpender { ) external { - LibTokenSpender.spendERC20Tokens( + _transferERC20Tokens( token, owner, to, - amount, - false + amount ); } @@ -73,6 +78,14 @@ contract TestLibTokenSpender { view returns (uint256) { - return LibTokenSpender.getSpendableERC20BalanceOf(token, owner); + return _getSpendableERC20BalanceOf(token, owner); + } + + function isTokenPossiblyGreedy(IERC20TokenV06 token) + external + view + returns (bool) + { + return _isTokenPossiblyGreedy(token); } } diff --git a/contracts/zero-ex/contracts/test/TestInitialMigration.sol b/contracts/zero-ex/contracts/test/TestInitialMigration.sol index 15e1e84279..53bce5dcd5 100644 --- a/contracts/zero-ex/contracts/test/TestInitialMigration.sol +++ b/contracts/zero-ex/contracts/test/TestInitialMigration.sol @@ -22,7 +22,6 @@ pragma experimental ABIEncoderV2; import "../src/ZeroEx.sol"; import "../src/features/IBootstrapFeature.sol"; import "../src/migrations/InitialMigration.sol"; -import "../src/features/SimpleFunctionRegistryFeature.sol"; contract TestInitialMigration is @@ -46,7 +45,7 @@ contract TestInitialMigration is success = InitialMigration.bootstrap(owner, features); // Snoop the bootstrap feature contract. bootstrapFeature = - SimpleFunctionRegistryFeature(address(uint160(address(this)))) + ZeroEx(address(uint160(address(this)))) .getFunctionImplementation(IBootstrapFeature.bootstrap.selector); } diff --git a/contracts/zero-ex/contracts/test/TestMetaTransactionsTransformERC20Feature.sol b/contracts/zero-ex/contracts/test/TestMetaTransactionsTransformERC20Feature.sol index bb31d76187..3b794608a7 100644 --- a/contracts/zero-ex/contracts/test/TestMetaTransactionsTransformERC20Feature.sol +++ b/contracts/zero-ex/contracts/test/TestMetaTransactionsTransformERC20Feature.sol @@ -39,6 +39,8 @@ contract TestMetaTransactionsTransformERC20Feature is bytes callDataSignature ); + constructor() public TransformERC20Feature(0) {} + function _transformERC20(TransformERC20Args memory args) public override diff --git a/contracts/zero-ex/contracts/test/TestMintableERC20Token.sol b/contracts/zero-ex/contracts/test/TestMintableERC20Token.sol index f2015c6b70..76dc28ab6e 100644 --- a/contracts/zero-ex/contracts/test/TestMintableERC20Token.sol +++ b/contracts/zero-ex/contracts/test/TestMintableERC20Token.sol @@ -42,6 +42,14 @@ contract TestMintableERC20Token { return true; } + function approveAs(address owner, address spender, uint256 amount) + external + returns (bool) + { + allowance[owner][spender] = amount; + return true; + } + function mint(address owner, uint256 amount) external virtual diff --git a/contracts/zero-ex/contracts/test/TestNativeOrdersFeature.sol b/contracts/zero-ex/contracts/test/TestNativeOrdersFeature.sol index 3ff64eefbc..d9ff51f618 100644 --- a/contracts/zero-ex/contracts/test/TestNativeOrdersFeature.sol +++ b/contracts/zero-ex/contracts/test/TestNativeOrdersFeature.sol @@ -10,10 +10,17 @@ contract TestNativeOrdersFeature is address zeroExAddress, IEtherTokenV06 weth, IStaking staking, - uint32 protocolFeeMultiplier + uint32 protocolFeeMultiplier, + bytes32 greedyTokensBloomFilter ) public - NativeOrdersFeature(zeroExAddress, weth, staking, protocolFeeMultiplier) + NativeOrdersFeature( + zeroExAddress, + weth, + staking, + protocolFeeMultiplier, + greedyTokensBloomFilter + ) { // solhint-disable no-empty-blocks } diff --git a/contracts/zero-ex/contracts/test/TestTokenSpenderERC20Token.sol b/contracts/zero-ex/contracts/test/TestTokenSpenderERC20Token.sol index d77f2394b9..7eebc4b43e 100644 --- a/contracts/zero-ex/contracts/test/TestTokenSpenderERC20Token.sol +++ b/contracts/zero-ex/contracts/test/TestTokenSpenderERC20Token.sol @@ -41,6 +41,12 @@ contract TestTokenSpenderERC20Token is uint256 constant private EXTRA_RETURN_TRUE_AMOUNT = 1341; uint256 constant private EXTRA_RETURN_FALSE_AMOUNT = 1342; + bool private _isGreedyRevert; + + function setGreedyRevert(bool isGreedy) external { + _isGreedyRevert = isGreedy; + } + function transferFrom(address from, address to, uint256 amount) public override @@ -54,9 +60,11 @@ contract TestTokenSpenderERC20Token is return false; } if (amount == REVERT_RETURN_AMOUNT) { + assert(!_isGreedyRevert); revert("TestTokenSpenderERC20Token/Revert"); } if (amount == TRIGGER_FALLBACK_SUCCESS_AMOUNT) { + assert(!_isGreedyRevert); return false; } if (amount == EXTRA_RETURN_TRUE_AMOUNT diff --git a/contracts/zero-ex/contracts/test/TestTransformERC20.sol b/contracts/zero-ex/contracts/test/TestTransformERC20.sol index 6afc5f0dbe..f4f91cf6aa 100644 --- a/contracts/zero-ex/contracts/test/TestTransformERC20.sol +++ b/contracts/zero-ex/contracts/test/TestTransformERC20.sol @@ -28,4 +28,6 @@ contract TestTransformERC20 is modifier onlySelf() override { _; } + + constructor() public TransformERC20Feature(0) {} } diff --git a/contracts/zero-ex/package.json b/contracts/zero-ex/package.json index 2918a2f2e2..5fac3d3ac5 100644 --- a/contracts/zero-ex/package.json +++ b/contracts/zero-ex/package.json @@ -42,7 +42,7 @@ "config": { "publicInterfaceContracts": "IZeroEx,ZeroEx,FullMigration,InitialMigration,IFlashWallet,IAllowanceTarget,IERC20Transformer,IOwnableFeature,ISimpleFunctionRegistryFeature,ITokenSpenderFeature,ITransformERC20Feature,FillQuoteTransformer,PayTakerTransformer,WethTransformer,OwnableFeature,SimpleFunctionRegistryFeature,TransformERC20Feature,TokenSpenderFeature,AffiliateFeeTransformer,SignatureValidatorFeature,MetaTransactionsFeature,LogMetadataTransformer,BridgeAdapter,LiquidityProviderFeature,ILiquidityProviderFeature,NativeOrdersFeature,INativeOrdersFeature", "abis:comment": "This list is auto-generated by contracts-gen. Don't edit manually.", - "abis": "./test/generated-artifacts/@(AffiliateFeeTransformer|AllowanceTarget|BootstrapFeature|BridgeAdapter|FeeCollector|FillQuoteTransformer|FixinCommon|FixinEIP712|FixinProtocolFees|FixinReentrancyGuard|FlashWallet|FullMigration|IAllowanceTarget|IBootstrapFeature|IBridgeAdapter|IERC20Bridge|IERC20Transformer|IExchange|IFeature|IFlashWallet|IGasToken|ILiquidityProvider|ILiquidityProviderFeature|ILiquidityProviderSandbox|IMetaTransactionsFeature|INativeOrdersFeature|IOwnableFeature|ISignatureValidatorFeature|ISimpleFunctionRegistryFeature|IStaking|ITestSimpleFunctionRegistryFeature|ITokenSpenderFeature|ITransformERC20Feature|IUniswapFeature|IZeroEx|InitialMigration|LibBootstrap|LibCommonRichErrors|LibERC20Transformer|LibLiquidityProviderRichErrors|LibMetaTransactionsRichErrors|LibMetaTransactionsStorage|LibMigrate|LibNativeOrder|LibNativeOrdersRichErrors|LibNativeOrdersStorage|LibOrderHash|LibOwnableRichErrors|LibOwnableStorage|LibProxyRichErrors|LibProxyStorage|LibReentrancyGuardStorage|LibSignature|LibSignatureRichErrors|LibSignedCallData|LibSimpleFunctionRegistryRichErrors|LibSimpleFunctionRegistryStorage|LibSpenderRichErrors|LibStorage|LibTokenSpender|LibTokenSpenderStorage|LibTransformERC20RichErrors|LibTransformERC20Storage|LibWalletRichErrors|LiquidityProviderFeature|LiquidityProviderSandbox|LogMetadataTransformer|MetaTransactionsFeature|MixinAdapterAddresses|MixinBalancer|MixinCurve|MixinDodo|MixinKyber|MixinMStable|MixinMooniswap|MixinOasis|MixinShell|MixinSushiswap|MixinUniswap|MixinUniswapV2|MixinZeroExBridge|NativeOrdersFeature|OwnableFeature|PayTakerTransformer|SignatureValidatorFeature|SimpleFunctionRegistryFeature|TestBridge|TestCallTarget|TestDelegateCaller|TestFillQuoteTransformerBridge|TestFillQuoteTransformerExchange|TestFillQuoteTransformerHost|TestFixinProtocolFees|TestFullMigration|TestInitialMigration|TestLibNativeOrder|TestLibSignature|TestLibTokenSpender|TestLiquidityProvider|TestMetaTransactionsTransformERC20Feature|TestMigrator|TestMintTokenERC20Transformer|TestMintableERC20Token|TestNativeOrdersFeature|TestSimpleFunctionRegistryFeatureImpl1|TestSimpleFunctionRegistryFeatureImpl2|TestStaking|TestTokenSpender|TestTokenSpenderERC20Token|TestTransformERC20|TestTransformerBase|TestTransformerDeployerTransformer|TestTransformerHost|TestWeth|TestWethTransformerHost|TestZeroExFeature|TokenSpenderFeature|TransformERC20Feature|Transformer|TransformerDeployer|UniswapFeature|WethTransformer|ZeroEx).json" + "abis": "./test/generated-artifacts/@(AffiliateFeeTransformer|AllowanceTarget|BootstrapFeature|BridgeAdapter|FeeCollector|FillQuoteTransformer|FixinCommon|FixinEIP712|FixinProtocolFees|FixinReentrancyGuard|FixinTokenSpender|FlashWallet|FullMigration|IAllowanceTarget|IBootstrapFeature|IBridgeAdapter|IERC20Bridge|IERC20Transformer|IExchange|IFeature|IFlashWallet|IGasToken|ILiquidityProvider|ILiquidityProviderFeature|ILiquidityProviderSandbox|IMetaTransactionsFeature|INativeOrdersFeature|IOwnableFeature|ISignatureValidatorFeature|ISimpleFunctionRegistryFeature|IStaking|ITestSimpleFunctionRegistryFeature|ITokenSpenderFeature|ITransformERC20Feature|IUniswapFeature|IZeroEx|InitialMigration|LibBootstrap|LibCommonRichErrors|LibERC20Transformer|LibLiquidityProviderRichErrors|LibMetaTransactionsRichErrors|LibMetaTransactionsStorage|LibMigrate|LibNativeOrder|LibNativeOrdersRichErrors|LibNativeOrdersStorage|LibOrderHash|LibOwnableRichErrors|LibOwnableStorage|LibProxyRichErrors|LibProxyStorage|LibReentrancyGuardStorage|LibSignature|LibSignatureRichErrors|LibSignedCallData|LibSimpleFunctionRegistryRichErrors|LibSimpleFunctionRegistryStorage|LibSpenderRichErrors|LibStorage|LibTokenSpenderStorage|LibTransformERC20RichErrors|LibTransformERC20Storage|LibWalletRichErrors|LiquidityProviderFeature|LiquidityProviderSandbox|LogMetadataTransformer|MetaTransactionsFeature|MixinAdapterAddresses|MixinBalancer|MixinCurve|MixinDodo|MixinKyber|MixinMStable|MixinMooniswap|MixinOasis|MixinShell|MixinSushiswap|MixinUniswap|MixinUniswapV2|MixinZeroExBridge|NativeOrdersFeature|OwnableFeature|PayTakerTransformer|SignatureValidatorFeature|SimpleFunctionRegistryFeature|TestBridge|TestCallTarget|TestDelegateCaller|TestFillQuoteTransformerBridge|TestFillQuoteTransformerExchange|TestFillQuoteTransformerHost|TestFixinProtocolFees|TestFixinTokenSpender|TestFullMigration|TestInitialMigration|TestLibNativeOrder|TestLibSignature|TestLiquidityProvider|TestMetaTransactionsTransformERC20Feature|TestMigrator|TestMintTokenERC20Transformer|TestMintableERC20Token|TestNativeOrdersFeature|TestSimpleFunctionRegistryFeatureImpl1|TestSimpleFunctionRegistryFeatureImpl2|TestStaking|TestTokenSpender|TestTokenSpenderERC20Token|TestTransformERC20|TestTransformerBase|TestTransformerDeployerTransformer|TestTransformerHost|TestWeth|TestWethTransformerHost|TestZeroExFeature|TokenSpenderFeature|TransformERC20Feature|Transformer|TransformerDeployer|UniswapFeature|WethTransformer|ZeroEx|ZeroExOptimized).json" }, "repository": { "type": "git", diff --git a/contracts/zero-ex/src/bloom_filter_utils.ts b/contracts/zero-ex/src/bloom_filter_utils.ts new file mode 100644 index 0000000000..bd4cb2e18e --- /dev/null +++ b/contracts/zero-ex/src/bloom_filter_utils.ts @@ -0,0 +1,41 @@ +import { BigNumber, hexUtils } from '@0x/utils'; + +/** + * Compute the bloom filter for a list of tokens. + * Used to filter greedy tokens in the exchange proxy. + */ +export function getTokenListBloomFilter(tokens: string[]): string { + let filter = hexUtils.leftPad(0); + for (const token of tokens) { + // (1 << (keccak256(token) % 256)) | (1 << (token % 256)) + const a = hexUtils.toHex(new BigNumber(2).pow(new BigNumber(hexUtils.hash(hexUtils.leftPad(token))).mod(256))); + const b = hexUtils.toHex(new BigNumber(2).pow(new BigNumber(token).mod(256))); + filter = bitwiseOrWords(filter, bitwiseOrWords(a, b)); + } + return filter; +} + +// Bitwise OR two hex words. +function bitwiseOrWords(a: string, b: string): string { + const aBits = hexWordToBitArray(a); + const bBits = hexWordToBitArray(b); + const resultBits = aBits.slice(); + for (let i = 0; i < 256; ++i) { + // tslint:disable-next-line: no-bitwise + resultBits[i] |= bBits[i]; + } + return bitArrayToHexWord(resultBits); +} + +function hexWordToBitArray(hexWord: string): number[] { + // Covnert to a binary string. + const bin = new BigNumber(hexWord).toString(2); + // Convert to integers. + const bits = bin.split('').map(s => parseInt(s, 10)); + // Left the binary string pad with zeroes. + return new Array(256 - bits.length).fill(0).concat(bits); +} + +function bitArrayToHexWord(bits: number[]): string { + return hexUtils.leftPad(new BigNumber(`0b${bits.map(b => b.toString()).join('')}`)); +} diff --git a/contracts/zero-ex/src/constants.ts b/contracts/zero-ex/src/constants.ts new file mode 100644 index 0000000000..1a0736b311 --- /dev/null +++ b/contracts/zero-ex/src/constants.ts @@ -0,0 +1,21 @@ +export const ZERO_BYTES32 = '0x0000000000000000000000000000000000000000000000000000000000000000'; + +// List of tokens that consume all gas when they fail. +export const GREEDY_TOKENS = [ + // USDT + '0xdac17f958d2ee523a2206206994597c13d831ec7', + // LINK + '0x514910771af9ca656af840dff83e8264ecf986ca', + // ORAI + '0x4c11249814f11b9346808179cf06e71ac328c1b5', + // BNT + '0x1f573d6fb3f13d689ff844b4ce37794d79a7ff1c', + // LIT + '0x763fa6806e1acf68130d2d0f0df754c93cc546b2', + // KNC + '0xdd974d5c2e2928dea5f71b9825b8b646686bd200', + // DIP + '0xc719d010b63e5bbf2c0551872cd5316ed26acd83', + // MINI + '0x4d953cf077c0c95ba090226e59a18fcf97db44ec', +]; diff --git a/contracts/zero-ex/src/index.ts b/contracts/zero-ex/src/index.ts index dcc34bc76f..1dfd811f2f 100644 --- a/contracts/zero-ex/src/index.ts +++ b/contracts/zero-ex/src/index.ts @@ -36,6 +36,8 @@ export * from './signature_utils'; export * from './orders'; export * from './eip712_utils'; export * from './revert_errors'; +export * from './bloom_filter_utils'; +export { GREEDY_TOKENS } from './constants'; export { AffiliateFeeTransformerContract, BridgeAdapterContract, diff --git a/contracts/zero-ex/src/migration.ts b/contracts/zero-ex/src/migration.ts index 6e27624b94..a8c19ec84d 100644 --- a/contracts/zero-ex/src/migration.ts +++ b/contracts/zero-ex/src/migration.ts @@ -5,6 +5,7 @@ import { TxData } from 'ethereum-types'; import * as _ from 'lodash'; import { artifacts } from './artifacts'; +import { ZERO_BYTES32 } from './constants'; import { FullMigrationContract, InitialMigrationContract, @@ -133,6 +134,7 @@ export interface FullFeaturesDeployConfig { wethAddress: string; stakingAddress: string; protocolFeeMultiplier: number; + greedyTokensBloomFilter: string; } /** @@ -147,6 +149,7 @@ const DEFAULT_FULL_FEATURES_DEPLOY_CONFIG = { wethAddress: NULL_ADDRESS, stakingAddress: NULL_ADDRESS, protocolFeeMultiplier: 70e3, + greedyTokensBloomFilter: ZERO_BYTES32, }; const DEFAULT_FULL_FEATURES_ARTIFACTS = { @@ -189,6 +192,7 @@ export async function deployFullFeaturesAsync( provider, txDefaults, artifacts, + _config.greedyTokensBloomFilter, )).address, signatureValidator: features.signatureValidator || @@ -206,6 +210,7 @@ export async function deployFullFeaturesAsync( txDefaults, artifacts, _config.zeroExAddress, + _config.greedyTokensBloomFilter, )).address, nativeOrders: features.nativeOrders || @@ -218,6 +223,7 @@ export async function deployFullFeaturesAsync( _config.wethAddress, _config.stakingAddress, _config.protocolFeeMultiplier, + _config.greedyTokensBloomFilter, )).address, }; } diff --git a/contracts/zero-ex/test/artifacts.ts b/contracts/zero-ex/test/artifacts.ts index b34cdf9d86..7949e132f8 100644 --- a/contracts/zero-ex/test/artifacts.ts +++ b/contracts/zero-ex/test/artifacts.ts @@ -15,6 +15,7 @@ import * as FixinCommon from '../test/generated-artifacts/FixinCommon.json'; import * as FixinEIP712 from '../test/generated-artifacts/FixinEIP712.json'; import * as FixinProtocolFees from '../test/generated-artifacts/FixinProtocolFees.json'; import * as FixinReentrancyGuard from '../test/generated-artifacts/FixinReentrancyGuard.json'; +import * as FixinTokenSpender from '../test/generated-artifacts/FixinTokenSpender.json'; import * as FlashWallet from '../test/generated-artifacts/FlashWallet.json'; import * as FullMigration from '../test/generated-artifacts/FullMigration.json'; import * as IAllowanceTarget from '../test/generated-artifacts/IAllowanceTarget.json'; @@ -64,7 +65,6 @@ import * as LibSimpleFunctionRegistryRichErrors from '../test/generated-artifact import * as LibSimpleFunctionRegistryStorage from '../test/generated-artifacts/LibSimpleFunctionRegistryStorage.json'; import * as LibSpenderRichErrors from '../test/generated-artifacts/LibSpenderRichErrors.json'; import * as LibStorage from '../test/generated-artifacts/LibStorage.json'; -import * as LibTokenSpender from '../test/generated-artifacts/LibTokenSpender.json'; import * as LibTokenSpenderStorage from '../test/generated-artifacts/LibTokenSpenderStorage.json'; import * as LibTransformERC20RichErrors from '../test/generated-artifacts/LibTransformERC20RichErrors.json'; import * as LibTransformERC20Storage from '../test/generated-artifacts/LibTransformERC20Storage.json'; @@ -98,11 +98,11 @@ import * as TestFillQuoteTransformerBridge from '../test/generated-artifacts/Tes import * as TestFillQuoteTransformerExchange from '../test/generated-artifacts/TestFillQuoteTransformerExchange.json'; import * as TestFillQuoteTransformerHost from '../test/generated-artifacts/TestFillQuoteTransformerHost.json'; import * as TestFixinProtocolFees from '../test/generated-artifacts/TestFixinProtocolFees.json'; +import * as TestFixinTokenSpender from '../test/generated-artifacts/TestFixinTokenSpender.json'; import * as TestFullMigration from '../test/generated-artifacts/TestFullMigration.json'; import * as TestInitialMigration from '../test/generated-artifacts/TestInitialMigration.json'; import * as TestLibNativeOrder from '../test/generated-artifacts/TestLibNativeOrder.json'; import * as TestLibSignature from '../test/generated-artifacts/TestLibSignature.json'; -import * as TestLibTokenSpender from '../test/generated-artifacts/TestLibTokenSpender.json'; import * as TestLiquidityProvider from '../test/generated-artifacts/TestLiquidityProvider.json'; import * as TestMetaTransactionsTransformERC20Feature from '../test/generated-artifacts/TestMetaTransactionsTransformERC20Feature.json'; import * as TestMigrator from '../test/generated-artifacts/TestMigrator.json'; @@ -128,9 +128,11 @@ import * as TransformerDeployer from '../test/generated-artifacts/TransformerDep import * as UniswapFeature from '../test/generated-artifacts/UniswapFeature.json'; import * as WethTransformer from '../test/generated-artifacts/WethTransformer.json'; import * as ZeroEx from '../test/generated-artifacts/ZeroEx.json'; +import * as ZeroExOptimized from '../test/generated-artifacts/ZeroExOptimized.json'; export const artifacts = { IZeroEx: IZeroEx as ContractArtifact, ZeroEx: ZeroEx as ContractArtifact, + ZeroExOptimized: ZeroExOptimized as ContractArtifact, LibCommonRichErrors: LibCommonRichErrors as ContractArtifact, LibLiquidityProviderRichErrors: LibLiquidityProviderRichErrors as ContractArtifact, LibMetaTransactionsRichErrors: LibMetaTransactionsRichErrors as ContractArtifact, @@ -174,11 +176,11 @@ export const artifacts = { LibNativeOrder: LibNativeOrder as ContractArtifact, LibSignature: LibSignature as ContractArtifact, LibSignedCallData: LibSignedCallData as ContractArtifact, - LibTokenSpender: LibTokenSpender as ContractArtifact, FixinCommon: FixinCommon as ContractArtifact, FixinEIP712: FixinEIP712 as ContractArtifact, FixinProtocolFees: FixinProtocolFees as ContractArtifact, FixinReentrancyGuard: FixinReentrancyGuard as ContractArtifact, + FixinTokenSpender: FixinTokenSpender as ContractArtifact, FullMigration: FullMigration as ContractArtifact, InitialMigration: InitialMigration as ContractArtifact, LibBootstrap: LibBootstrap as ContractArtifact, @@ -229,11 +231,11 @@ export const artifacts = { TestFillQuoteTransformerExchange: TestFillQuoteTransformerExchange as ContractArtifact, TestFillQuoteTransformerHost: TestFillQuoteTransformerHost as ContractArtifact, TestFixinProtocolFees: TestFixinProtocolFees as ContractArtifact, + TestFixinTokenSpender: TestFixinTokenSpender as ContractArtifact, TestFullMigration: TestFullMigration as ContractArtifact, TestInitialMigration: TestInitialMigration as ContractArtifact, TestLibNativeOrder: TestLibNativeOrder as ContractArtifact, TestLibSignature: TestLibSignature as ContractArtifact, - TestLibTokenSpender: TestLibTokenSpender as ContractArtifact, TestLiquidityProvider: TestLiquidityProvider as ContractArtifact, TestMetaTransactionsTransformERC20Feature: TestMetaTransactionsTransformERC20Feature as ContractArtifact, TestMigrator: TestMigrator as ContractArtifact, diff --git a/contracts/zero-ex/test/features/liquidity_provider_test.ts b/contracts/zero-ex/test/features/liquidity_provider_test.ts index 3d7345a7c3..88a4368c67 100644 --- a/contracts/zero-ex/test/features/liquidity_provider_test.ts +++ b/contracts/zero-ex/test/features/liquidity_provider_test.ts @@ -2,6 +2,7 @@ import { artifacts as erc20Artifacts, DummyERC20TokenContract } from '@0x/contra import { blockchainTests, constants, expect, verifyEventsFromLogs } from '@0x/contracts-test-utils'; import { BigNumber, OwnableRevertErrors, ZeroExRevertErrors } from '@0x/utils'; +import { ZERO_BYTES32 } from '../../src/constants'; import { IOwnableFeatureContract, IZeroExContract, LiquidityProviderFeatureContract } from '../../src/wrappers'; import { artifacts } from '../artifacts'; import { abis } from '../utils/abis'; @@ -64,6 +65,7 @@ blockchainTests('LiquidityProvider feature', env => { env.txDefaults, artifacts, sandbox.address, + ZERO_BYTES32, ); await new IOwnableFeatureContract(zeroEx.address, env.provider, env.txDefaults, abis) .migrate(featureImpl.address, featureImpl.migrate().getABIEncodedTransactionData(), owner) diff --git a/contracts/zero-ex/test/features/limit_orders_feature_test.ts b/contracts/zero-ex/test/features/native_orders_feature_test.ts similarity index 99% rename from contracts/zero-ex/test/features/limit_orders_feature_test.ts rename to contracts/zero-ex/test/features/native_orders_feature_test.ts index 219ec66ec8..298e282e8a 100644 --- a/contracts/zero-ex/test/features/limit_orders_feature_test.ts +++ b/contracts/zero-ex/test/features/native_orders_feature_test.ts @@ -10,7 +10,7 @@ import { fullMigrateAsync } from '../utils/migration'; import { getRandomLimitOrder, getRandomRfqOrder } from '../utils/orders'; import { TestMintableERC20TokenContract } from '../wrappers'; -blockchainTests.resets('LimitOrdersFeature', env => { +blockchainTests.resets('NativeOrdersFeature', env => { const { NULL_ADDRESS, MAX_UINT256, ZERO_AMOUNT } = constants; const GAS_PRICE = new BigNumber('123e9'); const PROTOCOL_FEE_MULTIPLIER = 1337e3; diff --git a/contracts/zero-ex/test/features/simple_function_registry_test.ts b/contracts/zero-ex/test/features/simple_function_registry_test.ts index 00c03e8fa8..53d0ec99b0 100644 --- a/contracts/zero-ex/test/features/simple_function_registry_test.ts +++ b/contracts/zero-ex/test/features/simple_function_registry_test.ts @@ -66,7 +66,7 @@ blockchainTests.resets('SimpleFunctionRegistry feature', env => { it('`rollback()` to zero impl succeeds for unregistered function', async () => { await registry.rollback(testFnSelector, NULL_ADDRESS).awaitTransactionSuccessAsync(); - const impl = await registry.getFunctionImplementation(testFnSelector).callAsync(); + const impl = await zeroEx.getFunctionImplementation(testFnSelector).callAsync(); expect(impl).to.eq(NULL_ADDRESS); }); diff --git a/contracts/zero-ex/test/features/transform_erc20_test.ts b/contracts/zero-ex/test/features/transform_erc20_test.ts index 3fc6994956..50627193ab 100644 --- a/contracts/zero-ex/test/features/transform_erc20_test.ts +++ b/contracts/zero-ex/test/features/transform_erc20_test.ts @@ -24,6 +24,7 @@ import { TestMintTokenERC20TransformerContract, TestMintTokenERC20TransformerEvents, TestMintTokenERC20TransformerMintTransformEventArgs, + TestTransformERC20Contract, TransformERC20FeatureEvents, } from '../wrappers'; @@ -49,7 +50,7 @@ blockchainTests.resets('TransformERC20 feature', env => { env.provider, env.txDefaults, { - transformERC20: (await TransformERC20FeatureContract.deployFrom0xArtifactAsync( + transformERC20: (await TestTransformERC20Contract.deployFrom0xArtifactAsync( artifacts.TestTransformERC20, env.provider, env.txDefaults, diff --git a/contracts/zero-ex/test/lib_token_spender_test.ts b/contracts/zero-ex/test/fixin_token_spender_test.ts similarity index 54% rename from contracts/zero-ex/test/lib_token_spender_test.ts rename to contracts/zero-ex/test/fixin_token_spender_test.ts index 0da14b4ac2..c504803e87 100644 --- a/contracts/zero-ex/test/lib_token_spender_test.ts +++ b/contracts/zero-ex/test/fixin_token_spender_test.ts @@ -7,34 +7,49 @@ import { } from '@0x/contracts-test-utils'; import { BigNumber, hexUtils, StringRevertError, ZeroExRevertErrors } from '@0x/utils'; +import { getTokenListBloomFilter } from '../src/bloom_filter_utils'; + import { artifacts } from './artifacts'; import { - TestLibTokenSpenderContract, - TestLibTokenSpenderEvents, + TestFixinTokenSpenderContract, + TestFixinTokenSpenderEvents, TestTokenSpenderERC20TokenContract, TestTokenSpenderERC20TokenEvents, } from './wrappers'; -blockchainTests.resets('LibTokenSpender library', env => { - let tokenSpender: TestLibTokenSpenderContract; +blockchainTests.resets('FixinTokenSpender', env => { + let tokenSpender: TestFixinTokenSpenderContract; let token: TestTokenSpenderERC20TokenContract; + let greedyToken: TestTokenSpenderERC20TokenContract; + let greedyTokensBloomFilter: string; before(async () => { - tokenSpender = await TestLibTokenSpenderContract.deployFrom0xArtifactAsync( - artifacts.TestLibTokenSpender, - env.provider, - env.txDefaults, - artifacts, - ); token = await TestTokenSpenderERC20TokenContract.deployFrom0xArtifactAsync( artifacts.TestTokenSpenderERC20Token, env.provider, env.txDefaults, artifacts, ); + greedyToken = await TestTokenSpenderERC20TokenContract.deployFrom0xArtifactAsync( + artifacts.TestTokenSpenderERC20Token, + env.provider, + env.txDefaults, + artifacts, + ); + await greedyToken.setGreedyRevert(true).awaitTransactionSuccessAsync(); + + greedyTokensBloomFilter = getTokenListBloomFilter([greedyToken.address]); + + tokenSpender = await TestFixinTokenSpenderContract.deployFrom0xArtifactAsync( + artifacts.TestFixinTokenSpender, + env.provider, + env.txDefaults, + artifacts, + greedyTokensBloomFilter, + ); }); - describe('spendERC20Tokens()', () => { + describe('transferERC20Tokens()', () => { const EMPTY_RETURN_AMOUNT = 1337; const FALSE_RETURN_AMOUNT = 1338; const REVERT_RETURN_AMOUNT = 1339; @@ -42,12 +57,12 @@ blockchainTests.resets('LibTokenSpender library', env => { const EXTRA_RETURN_TRUE_AMOUNT = 1341; const EXTRA_RETURN_FALSE_AMOUNT = 1342; - it('spendERC20Tokens() successfully calls compliant ERC20 token', async () => { + it('transferERC20Tokens() successfully calls compliant ERC20 token', async () => { const tokenFrom = randomAddress(); const tokenTo = randomAddress(); const tokenAmount = new BigNumber(123456); const receipt = await tokenSpender - .spendERC20Tokens(token.address, tokenFrom, tokenTo, tokenAmount) + .transferERC20Tokens(token.address, tokenFrom, tokenTo, tokenAmount) .awaitTransactionSuccessAsync(); verifyEventsFromLogs( receipt.logs, @@ -63,12 +78,12 @@ blockchainTests.resets('LibTokenSpender library', env => { ); }); - it('spendERC20Tokens() successfully calls non-compliant ERC20 token', async () => { + it('transferERC20Tokens() successfully calls non-compliant ERC20 token', async () => { const tokenFrom = randomAddress(); const tokenTo = randomAddress(); const tokenAmount = new BigNumber(EMPTY_RETURN_AMOUNT); const receipt = await tokenSpender - .spendERC20Tokens(token.address, tokenFrom, tokenTo, tokenAmount) + .transferERC20Tokens(token.address, tokenFrom, tokenTo, tokenAmount) .awaitTransactionSuccessAsync(); verifyEventsFromLogs( receipt.logs, @@ -84,12 +99,12 @@ blockchainTests.resets('LibTokenSpender library', env => { ); }); - it('spendERC20Tokens() reverts if ERC20 token reverts', async () => { + it('transferERC20Tokens() reverts if ERC20 token reverts', async () => { const tokenFrom = randomAddress(); const tokenTo = randomAddress(); const tokenAmount = new BigNumber(REVERT_RETURN_AMOUNT); const tx = tokenSpender - .spendERC20Tokens(token.address, tokenFrom, tokenTo, tokenAmount) + .transferERC20Tokens(token.address, tokenFrom, tokenTo, tokenAmount) .awaitTransactionSuccessAsync(); const expectedError = new ZeroExRevertErrors.Spender.SpenderERC20TransferFromFailedError( token.address, @@ -101,12 +116,12 @@ blockchainTests.resets('LibTokenSpender library', env => { return expect(tx).to.revertWith(expectedError); }); - it('spendERC20Tokens() reverts if ERC20 token returns false', async () => { + it('transferERC20Tokens() reverts if ERC20 token returns false', async () => { const tokenFrom = randomAddress(); const tokenTo = randomAddress(); const tokenAmount = new BigNumber(FALSE_RETURN_AMOUNT); const tx = tokenSpender - .spendERC20Tokens(token.address, tokenFrom, tokenTo, tokenAmount) + .transferERC20Tokens(token.address, tokenFrom, tokenTo, tokenAmount) .awaitTransactionSuccessAsync(); return expect(tx).to.revertWith( new ZeroExRevertErrors.Spender.SpenderERC20TransferFromFailedError( @@ -119,12 +134,12 @@ blockchainTests.resets('LibTokenSpender library', env => { ); }); - it('spendERC20Tokens() falls back successfully to TokenSpender._spendERC20Tokens()', async () => { + it('transferERC20Tokens() falls back successfully to TokenSpender._spendERC20Tokens()', async () => { const tokenFrom = randomAddress(); const tokenTo = randomAddress(); const tokenAmount = new BigNumber(TRIGGER_FALLBACK_SUCCESS_AMOUNT); const receipt = await tokenSpender - .spendERC20Tokens(token.address, tokenFrom, tokenTo, tokenAmount) + .transferERC20Tokens(token.address, tokenFrom, tokenTo, tokenAmount) .awaitTransactionSuccessAsync(); verifyEventsFromLogs( receipt.logs, @@ -136,17 +151,17 @@ blockchainTests.resets('LibTokenSpender library', env => { amount: tokenAmount, }, ], - TestLibTokenSpenderEvents.FallbackCalled, + TestFixinTokenSpenderEvents.FallbackCalled, ); }); - it('spendERC20Tokens() allows extra data after true', async () => { + it('transferERC20Tokens() allows extra data after true', async () => { const tokenFrom = randomAddress(); const tokenTo = randomAddress(); const tokenAmount = new BigNumber(EXTRA_RETURN_TRUE_AMOUNT); const receipt = await tokenSpender - .spendERC20Tokens(token.address, tokenFrom, tokenTo, tokenAmount) + .transferERC20Tokens(token.address, tokenFrom, tokenTo, tokenAmount) .awaitTransactionSuccessAsync(); verifyEventsFromLogs( receipt.logs, @@ -162,13 +177,13 @@ blockchainTests.resets('LibTokenSpender library', env => { ); }); - it("spendERC20Tokens() reverts when there's extra data after false", async () => { + it("transferERC20Tokens() reverts when there's extra data after false", async () => { const tokenFrom = randomAddress(); const tokenTo = randomAddress(); const tokenAmount = new BigNumber(EXTRA_RETURN_FALSE_AMOUNT); const tx = tokenSpender - .spendERC20Tokens(token.address, tokenFrom, tokenTo, tokenAmount) + .transferERC20Tokens(token.address, tokenFrom, tokenTo, tokenAmount) .awaitTransactionSuccessAsync(); return expect(tx).to.revertWith( new ZeroExRevertErrors.Spender.SpenderERC20TransferFromFailedError( @@ -181,15 +196,82 @@ blockchainTests.resets('LibTokenSpender library', env => { ); }); - it('spendERC20Tokens() cannot call self', async () => { + it('transferERC20Tokens() cannot call self', async () => { const tokenFrom = randomAddress(); const tokenTo = randomAddress(); const tokenAmount = new BigNumber(123456); const tx = tokenSpender - .spendERC20Tokens(tokenSpender.address, tokenFrom, tokenTo, tokenAmount) + .transferERC20Tokens(tokenSpender.address, tokenFrom, tokenTo, tokenAmount) .awaitTransactionSuccessAsync(); - return expect(tx).to.revertWith('LibTokenSpender/CANNOT_INVOKE_SELF'); + return expect(tx).to.revertWith('FixinTokenSpender/CANNOT_INVOKE_SELF'); + }); + + it('calls transferFrom() directly for a greedy token with allowance', async () => { + const tokenFrom = randomAddress(); + const tokenTo = randomAddress(); + const tokenAmount = new BigNumber(123456); + await greedyToken.approveAs(tokenFrom, tokenSpender.address, tokenAmount).awaitTransactionSuccessAsync(); + + const receipt = await tokenSpender + .transferERC20Tokens(greedyToken.address, tokenFrom, tokenTo, tokenAmount) + .awaitTransactionSuccessAsync(); + // Because there is an allowance set, we will call transferFrom() + // directly, which succeds and emits an event. + verifyEventsFromLogs( + receipt.logs, + [ + { + sender: tokenSpender.address, + from: tokenFrom, + to: tokenTo, + amount: tokenAmount, + }, + ], // No events. + TestTokenSpenderERC20TokenEvents.TransferFromCalled, + ); + }); + + it('calls only the fallback for a greedy token with no allowance', async () => { + const tokenFrom = randomAddress(); + const tokenTo = randomAddress(); + const tokenAmount = new BigNumber(TRIGGER_FALLBACK_SUCCESS_AMOUNT); + + const receipt = await tokenSpender + .transferERC20Tokens(greedyToken.address, tokenFrom, tokenTo, tokenAmount) + .awaitTransactionSuccessAsync(); + // Because this is a greedy token and there is no allowance set, transferFrom() + // will not be called before hitting the fallback, which only emits an event + // in the test contract. + verifyEventsFromLogs( + receipt.logs, + [], // No events. + TestTokenSpenderERC20TokenEvents.TransferFromCalled, + ); + verifyEventsFromLogs( + receipt.logs, + [ + { + token: greedyToken.address, + owner: tokenFrom, + to: tokenTo, + amount: tokenAmount, + }, + ], + TestFixinTokenSpenderEvents.FallbackCalled, + ); + }); + }); + + describe('isTokenPossiblyGreedy()', () => { + it('returns true for greedy token', async () => { + const isGreedy = await tokenSpender.isTokenPossiblyGreedy(greedyToken.address).callAsync(); + expect(isGreedy).to.eq(true); + }); + + it('returns false for non-greedy token', async () => { + const isGreedy = await tokenSpender.isTokenPossiblyGreedy(token.address).callAsync(); + expect(isGreedy).to.eq(false); }); }); diff --git a/contracts/zero-ex/test/full_migration_test.ts b/contracts/zero-ex/test/full_migration_test.ts index 1461280a5f..0c32bd72b8 100644 --- a/contracts/zero-ex/test/full_migration_test.ts +++ b/contracts/zero-ex/test/full_migration_test.ts @@ -13,7 +13,6 @@ import { INativeOrdersFeatureContract, IOwnableFeatureContract, ISignatureValidatorFeatureContract, - ISimpleFunctionRegistryFeatureContract, ITokenSpenderFeatureContract, ITransformERC20FeatureContract, TestFullMigrationContract, @@ -27,7 +26,6 @@ blockchainTests.resets('Full migration', env => { let zeroEx: ZeroExContract; let features: FullFeatures; let migrator: TestFullMigrationContract; - let registry: ISimpleFunctionRegistryFeatureContract; const transformerDeployer = randomAddress(); before(async () => { @@ -50,7 +48,6 @@ blockchainTests.resets('Full migration', env => { await migrator .migrateZeroEx(owner, zeroEx.address, features, { transformerDeployer }) .awaitTransactionSuccessAsync(); - registry = new ISimpleFunctionRegistryFeatureContract(zeroEx.address, env.provider, env.txDefaults); }); it('ZeroEx has the correct owner', async () => { @@ -191,7 +188,7 @@ blockchainTests.resets('Full migration', env => { for (const fn of featureInfo.fns) { it(`${fn} is registered`, async () => { const selector = contract.getSelector(fn); - const impl = await registry.getFunctionImplementation(selector).callAsync(); + const impl = await zeroEx.getFunctionImplementation(selector).callAsync(); expect(impl).to.not.eq(NULL_ADDRESS); }); diff --git a/contracts/zero-ex/test/wrappers.ts b/contracts/zero-ex/test/wrappers.ts index 14b14f3cf3..c645125d83 100644 --- a/contracts/zero-ex/test/wrappers.ts +++ b/contracts/zero-ex/test/wrappers.ts @@ -13,6 +13,7 @@ export * from '../test/generated-wrappers/fixin_common'; export * from '../test/generated-wrappers/fixin_e_i_p712'; export * from '../test/generated-wrappers/fixin_protocol_fees'; export * from '../test/generated-wrappers/fixin_reentrancy_guard'; +export * from '../test/generated-wrappers/fixin_token_spender'; export * from '../test/generated-wrappers/flash_wallet'; export * from '../test/generated-wrappers/full_migration'; export * from '../test/generated-wrappers/i_allowance_target'; @@ -62,7 +63,6 @@ export * from '../test/generated-wrappers/lib_simple_function_registry_rich_erro export * from '../test/generated-wrappers/lib_simple_function_registry_storage'; export * from '../test/generated-wrappers/lib_spender_rich_errors'; export * from '../test/generated-wrappers/lib_storage'; -export * from '../test/generated-wrappers/lib_token_spender'; export * from '../test/generated-wrappers/lib_token_spender_storage'; export * from '../test/generated-wrappers/lib_transform_erc20_rich_errors'; export * from '../test/generated-wrappers/lib_transform_erc20_storage'; @@ -96,11 +96,11 @@ export * from '../test/generated-wrappers/test_fill_quote_transformer_bridge'; export * from '../test/generated-wrappers/test_fill_quote_transformer_exchange'; export * from '../test/generated-wrappers/test_fill_quote_transformer_host'; export * from '../test/generated-wrappers/test_fixin_protocol_fees'; +export * from '../test/generated-wrappers/test_fixin_token_spender'; export * from '../test/generated-wrappers/test_full_migration'; export * from '../test/generated-wrappers/test_initial_migration'; export * from '../test/generated-wrappers/test_lib_native_order'; export * from '../test/generated-wrappers/test_lib_signature'; -export * from '../test/generated-wrappers/test_lib_token_spender'; export * from '../test/generated-wrappers/test_liquidity_provider'; export * from '../test/generated-wrappers/test_meta_transactions_transform_erc20_feature'; export * from '../test/generated-wrappers/test_migrator'; @@ -126,3 +126,4 @@ export * from '../test/generated-wrappers/transformer_deployer'; export * from '../test/generated-wrappers/uniswap_feature'; export * from '../test/generated-wrappers/weth_transformer'; export * from '../test/generated-wrappers/zero_ex'; +export * from '../test/generated-wrappers/zero_ex_optimized'; diff --git a/contracts/zero-ex/test/zero_ex_test.ts b/contracts/zero-ex/test/zero_ex_test.ts index 99c72de78d..566504f157 100644 --- a/contracts/zero-ex/test/zero_ex_test.ts +++ b/contracts/zero-ex/test/zero_ex_test.ts @@ -83,7 +83,7 @@ blockchainTests.resets('ZeroEx contract', env => { // registry.getSelector('extendSelf'), ]; const selectors = [...ownableSelectors, ...registrySelectors]; - const impls = await Promise.all(selectors.map(s => registry.getFunctionImplementation(s).callAsync())); + const impls = await Promise.all(selectors.map(s => zeroEx.getFunctionImplementation(s).callAsync())); for (let i = 0; i < impls.length; ++i) { const selector = selectors[i]; const impl = impls[i]; diff --git a/contracts/zero-ex/tsconfig.json b/contracts/zero-ex/tsconfig.json index da1dce5609..2a66e650c7 100644 --- a/contracts/zero-ex/tsconfig.json +++ b/contracts/zero-ex/tsconfig.json @@ -40,6 +40,7 @@ "test/generated-artifacts/FixinEIP712.json", "test/generated-artifacts/FixinProtocolFees.json", "test/generated-artifacts/FixinReentrancyGuard.json", + "test/generated-artifacts/FixinTokenSpender.json", "test/generated-artifacts/FlashWallet.json", "test/generated-artifacts/FullMigration.json", "test/generated-artifacts/IAllowanceTarget.json", @@ -89,7 +90,6 @@ "test/generated-artifacts/LibSimpleFunctionRegistryStorage.json", "test/generated-artifacts/LibSpenderRichErrors.json", "test/generated-artifacts/LibStorage.json", - "test/generated-artifacts/LibTokenSpender.json", "test/generated-artifacts/LibTokenSpenderStorage.json", "test/generated-artifacts/LibTransformERC20RichErrors.json", "test/generated-artifacts/LibTransformERC20Storage.json", @@ -123,11 +123,11 @@ "test/generated-artifacts/TestFillQuoteTransformerExchange.json", "test/generated-artifacts/TestFillQuoteTransformerHost.json", "test/generated-artifacts/TestFixinProtocolFees.json", + "test/generated-artifacts/TestFixinTokenSpender.json", "test/generated-artifacts/TestFullMigration.json", "test/generated-artifacts/TestInitialMigration.json", "test/generated-artifacts/TestLibNativeOrder.json", "test/generated-artifacts/TestLibSignature.json", - "test/generated-artifacts/TestLibTokenSpender.json", "test/generated-artifacts/TestLiquidityProvider.json", "test/generated-artifacts/TestMetaTransactionsTransformERC20Feature.json", "test/generated-artifacts/TestMigrator.json", @@ -152,7 +152,8 @@ "test/generated-artifacts/TransformerDeployer.json", "test/generated-artifacts/UniswapFeature.json", "test/generated-artifacts/WethTransformer.json", - "test/generated-artifacts/ZeroEx.json" + "test/generated-artifacts/ZeroEx.json", + "test/generated-artifacts/ZeroExOptimized.json" ], "exclude": ["./deploy/solc/solc_bin"] } diff --git a/docs/basics/orders.rst b/docs/basics/orders.rst index 5f92dfe200..c595665304 100644 --- a/docs/basics/orders.rst +++ b/docs/basics/orders.rst @@ -282,7 +282,6 @@ RFQ orders are a stripped down version of standard limit orders, supporting fewe Some notable differences from regular limit orders are: -* RFQ orders can only be filled once. Even a partial fill will mark the order as ``FILLED``. * The only fill restrictions that can be placed on an RFQ order is on the ``tx.origin`` of the transaction. * There are no taker token fees.