From 861871134b138055bb1fdb5e79d2485af269df7b Mon Sep 17 00:00:00 2001 From: Steve Marx Date: Fri, 23 Oct 2020 10:45:46 -0400 Subject: [PATCH] add LibTokenSpender and convert to using that (#3) add LibTokenSpender and convert to using that This skips the allowance target. Allowances are instead just set on the exchange proxy itself. There is a fallback, though, to try spending from the allowance target if the original transfer fails. --- contracts/zero-ex/CHANGELOG.json | 9 + .../src/features/LiquidityProviderFeature.sol | 4 +- .../src/features/MetaTransactionsFeature.sol | 8 +- .../src/features/TransformERC20Feature.sol | 15 +- .../contracts/src/features/UniswapFeature.sol | 54 ++++- .../src/features/libs/LibTokenSpender.sol | 128 +++++++++++ .../contracts/test/TestLibTokenSpender.sol | 72 ++++++ .../test/TestTokenSpenderERC20Token.sol | 16 ++ contracts/zero-ex/package.json | 2 +- contracts/zero-ex/test/artifacts.ts | 4 + .../test/features/liquidity_provider_test.ts | 20 +- .../test/features/meta_transactions_test.ts | 7 +- .../test/features/transform_erc20_test.ts | 7 +- .../zero-ex/test/lib_token_spender_test.ts | 210 ++++++++++++++++++ contracts/zero-ex/test/wrappers.ts | 2 + contracts/zero-ex/tsconfig.json | 2 + 16 files changed, 504 insertions(+), 56 deletions(-) create mode 100644 contracts/zero-ex/contracts/src/features/libs/LibTokenSpender.sol create mode 100644 contracts/zero-ex/contracts/test/TestLibTokenSpender.sol create mode 100644 contracts/zero-ex/test/lib_token_spender_test.ts diff --git a/contracts/zero-ex/CHANGELOG.json b/contracts/zero-ex/CHANGELOG.json index 919ac5f365..71b928bbe5 100644 --- a/contracts/zero-ex/CHANGELOG.json +++ b/contracts/zero-ex/CHANGELOG.json @@ -1,4 +1,13 @@ [ + { + "version": "0.4.0", + "changes": [ + { + "note": "Use the exchange proxy as the primary allowance target", + "pr": 3 + } + ] + }, { "version": "0.3.0", "changes": [ diff --git a/contracts/zero-ex/contracts/src/features/LiquidityProviderFeature.sol b/contracts/zero-ex/contracts/src/features/LiquidityProviderFeature.sol index 61bb080866..83b375e630 100644 --- a/contracts/zero-ex/contracts/src/features/LiquidityProviderFeature.sol +++ b/contracts/zero-ex/contracts/src/features/LiquidityProviderFeature.sol @@ -31,7 +31,7 @@ import "../storage/LibLiquidityProviderStorage.sol"; import "../vendor/v3/IERC20Bridge.sol"; import "./IFeature.sol"; import "./ILiquidityProviderFeature.sol"; -import "./ITokenSpenderFeature.sol"; +import "./libs/LibTokenSpender.sol"; contract LiquidityProviderFeature is @@ -97,7 +97,7 @@ contract LiquidityProviderFeature is weth.deposit{value: sellAmount}(); weth.transfer(providerAddress, sellAmount); } else { - ITokenSpenderFeature(address(this))._spendERC20Tokens( + LibTokenSpender.spendERC20Tokens( IERC20TokenV06(takerToken), msg.sender, providerAddress, diff --git a/contracts/zero-ex/contracts/src/features/MetaTransactionsFeature.sol b/contracts/zero-ex/contracts/src/features/MetaTransactionsFeature.sol index 6ac4475d8d..fe481ed1eb 100644 --- a/contracts/zero-ex/contracts/src/features/MetaTransactionsFeature.sol +++ b/contracts/zero-ex/contracts/src/features/MetaTransactionsFeature.sol @@ -32,8 +32,8 @@ import "./libs/LibSignedCallData.sol"; import "./IMetaTransactionsFeature.sol"; import "./ITransformERC20Feature.sol"; import "./ISignatureValidatorFeature.sol"; -import "./ITokenSpenderFeature.sol"; import "./IFeature.sol"; +import "./libs/LibTokenSpender.sol"; /// @dev MetaTransactions feature. @@ -279,10 +279,10 @@ contract MetaTransactionsFeature is // Pay the fee to the sender. if (mtx.feeAmount > 0) { - ITokenSpenderFeature(address(this))._spendERC20Tokens( + LibTokenSpender.spendERC20Tokens( mtx.feeToken, - mtx.signer, // From the signer. - sender, // To the sender. + mtx.signer, + sender, mtx.feeAmount ); } diff --git a/contracts/zero-ex/contracts/src/features/TransformERC20Feature.sol b/contracts/zero-ex/contracts/src/features/TransformERC20Feature.sol index fa77fba85a..99b63bacae 100644 --- a/contracts/zero-ex/contracts/src/features/TransformERC20Feature.sol +++ b/contracts/zero-ex/contracts/src/features/TransformERC20Feature.sol @@ -33,9 +33,9 @@ import "../transformers/IERC20Transformer.sol"; import "../transformers/LibERC20Transformer.sol"; import "./libs/LibSignedCallData.sol"; import "./ITransformERC20Feature.sol"; -import "./ITokenSpenderFeature.sol"; import "./IFeature.sol"; import "./ISignatureValidatorFeature.sol"; +import "./libs/LibTokenSpender.sol"; /// @dev Feature to composably transform between ERC20 tokens. @@ -211,8 +211,10 @@ contract TransformERC20Feature is // If the input token amount is -1, transform the taker's entire // spendable balance. if (args.inputTokenAmount == uint256(-1)) { - args.inputTokenAmount = ITokenSpenderFeature(address(this)) - .getSpendableERC20BalanceOf(args.inputToken, args.taker); + args.inputTokenAmount = LibTokenSpender.getSpendableERC20BalanceOf( + args.inputToken, + args.taker + ); } TransformERC20PrivateState memory state; @@ -315,12 +317,7 @@ contract TransformERC20Feature is // Transfer input tokens. if (!LibERC20Transformer.isTokenETH(inputToken)) { // Token is not ETH, so pull ERC20 tokens. - ITokenSpenderFeature(address(this))._spendERC20Tokens( - inputToken, - from, - to, - amount - ); + LibTokenSpender.spendERC20Tokens(inputToken, from, to, amount); } else if (msg.value < amount) { // Token is ETH, so the caller must attach enough ETH to the call. LibTransformERC20RichErrors.InsufficientEthAttachedError( diff --git a/contracts/zero-ex/contracts/src/features/UniswapFeature.sol b/contracts/zero-ex/contracts/src/features/UniswapFeature.sol index 9aec0c018d..2ac4f8aee0 100644 --- a/contracts/zero-ex/contracts/src/features/UniswapFeature.sol +++ b/contracts/zero-ex/contracts/src/features/UniswapFeature.sol @@ -157,17 +157,49 @@ contract UniswapFeature is switch eq(sellToken, ETH_TOKEN_ADDRESS_32) case 0 { // For the first pair we need to transfer sellTokens into the - // pair contract using `AllowanceTarget.executeCall()` - 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() + // 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() + } } } default { diff --git a/contracts/zero-ex/contracts/src/features/libs/LibTokenSpender.sol b/contracts/zero-ex/contracts/src/features/libs/LibTokenSpender.sol new file mode 100644 index 0000000000..22bc725fa2 --- /dev/null +++ b/contracts/zero-ex/contracts/src/features/libs/LibTokenSpender.sol @@ -0,0 +1,128 @@ +/* + + 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 "@0x/contracts-erc20/contracts/src/v06/IERC20TokenV06.sol"; +import "@0x/contracts-utils/contracts/src/v06/LibSafeMathV06.sol"; +import "../../errors/LibSpenderRichErrors.sol"; +import "../ITokenSpenderFeature.sol"; + +library LibTokenSpender { + using LibRichErrorsV06 for bytes; + + /// @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. + function spendERC20Tokens( + IERC20TokenV06 token, + address owner, + address to, + uint256 amount + ) + internal + { + bool success; + bytes memory revertData; + + require(address(token) != address(this), "LibTokenSpender/CANNOT_INVOKE_SELF"); + + assembly { + let ptr := mload(0x40) // free memory pointer + + // selector for transferFrom(address,address,uint256) + mstore(ptr, 0x23b872dd00000000000000000000000000000000000000000000000000000000) + mstore(add(ptr, 0x04), owner) + mstore(add(ptr, 0x24), to) + mstore(add(ptr, 0x44), amount) + + success := call(gas(), token, 0, ptr, 0x64, 0, 0) + + let rdsize := returndatasize() + + returndatacopy(add(ptr, 0x20), 0, rdsize) // reuse memory + + // Check for ERC20 success. ERC20 tokens should return a boolean, + // but some don't. 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(add(ptr, 0x20)), 1) // starts with uint256(1) + ) + ) + ) + + if iszero(success) { + // revertData is a bytes, so length-prefixed data + mstore(ptr, rdsize) + revertData := ptr + + // update free memory pointer (ptr + 32-byte length + return data) + mstore(0x40, add(add(ptr, 0x20), rdsize)) + } + } + + 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(); + } + } + } + + /// @dev Gets the maximum amount of an ERC20 token `token` that can be + /// pulled from `owner` by this address. + /// @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( + IERC20TokenV06 token, + address owner + ) + internal + view + returns (uint256) + { + return LibSafeMathV06.min256( + token.allowance(owner, address(this)), + token.balanceOf(owner) + ); + } +} diff --git a/contracts/zero-ex/contracts/test/TestLibTokenSpender.sol b/contracts/zero-ex/contracts/test/TestLibTokenSpender.sol new file mode 100644 index 0000000000..e7fbff3af4 --- /dev/null +++ b/contracts/zero-ex/contracts/test/TestLibTokenSpender.sol @@ -0,0 +1,72 @@ +/* + + 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 "@0x/contracts-erc20/contracts/src/v06/IERC20TokenV06.sol"; + +import "../src/features/libs/LibTokenSpender.sol"; + +contract TestLibTokenSpender { + uint256 constant private TRIGGER_FALLBACK_SUCCESS_AMOUNT = 1340; + + function spendERC20Tokens( + IERC20TokenV06 token, + address owner, + address to, + uint256 amount + ) + external + { + LibTokenSpender.spendERC20Tokens(token, owner, to, amount); + } + + event FallbackCalled( + address token, + address owner, + address to, + uint256 amount + ); + + // This is called as a fallback when the original transferFrom() fails. + function _spendERC20Tokens( + IERC20TokenV06 token, + address owner, + address to, + uint256 amount + ) + external + { + require(amount == TRIGGER_FALLBACK_SUCCESS_AMOUNT, + "TokenSpenderFallback/FAILURE_AMOUNT"); + + emit FallbackCalled(address(token), owner, to, amount); + } + + function getSpendableERC20BalanceOf( + IERC20TokenV06 token, + address owner + ) + external + view + returns (uint256) + { + return LibTokenSpender.getSpendableERC20BalanceOf(token, owner); + } +} diff --git a/contracts/zero-ex/contracts/test/TestTokenSpenderERC20Token.sol b/contracts/zero-ex/contracts/test/TestTokenSpenderERC20Token.sol index 86cd367c4b..d77f2394b9 100644 --- a/contracts/zero-ex/contracts/test/TestTokenSpenderERC20Token.sol +++ b/contracts/zero-ex/contracts/test/TestTokenSpenderERC20Token.sol @@ -37,6 +37,9 @@ contract TestTokenSpenderERC20Token is uint256 constant private EMPTY_RETURN_AMOUNT = 1337; uint256 constant private FALSE_RETURN_AMOUNT = 1338; uint256 constant private REVERT_RETURN_AMOUNT = 1339; + uint256 constant private TRIGGER_FALLBACK_SUCCESS_AMOUNT = 1340; + uint256 constant private EXTRA_RETURN_TRUE_AMOUNT = 1341; + uint256 constant private EXTRA_RETURN_FALSE_AMOUNT = 1342; function transferFrom(address from, address to, uint256 amount) public @@ -53,6 +56,19 @@ contract TestTokenSpenderERC20Token is if (amount == REVERT_RETURN_AMOUNT) { revert("TestTokenSpenderERC20Token/Revert"); } + if (amount == TRIGGER_FALLBACK_SUCCESS_AMOUNT) { + return false; + } + if (amount == EXTRA_RETURN_TRUE_AMOUNT + || amount == EXTRA_RETURN_FALSE_AMOUNT) { + bool ret = amount == EXTRA_RETURN_TRUE_AMOUNT; + + assembly { + mstore(0x00, ret) + mstore(0x20, amount) // just something extra to return + return(0, 0x40) + } + } return true; } diff --git a/contracts/zero-ex/package.json b/contracts/zero-ex/package.json index 7ddb40e5da..4aadb5333f 100644 --- a/contracts/zero-ex/package.json +++ b/contracts/zero-ex/package.json @@ -41,7 +41,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", "abis:comment": "This list is auto-generated by contracts-gen. Don't edit manually.", - "abis": "./test/generated-artifacts/@(AffiliateFeeTransformer|AllowanceTarget|BootstrapFeature|BridgeAdapter|FillQuoteTransformer|FixinCommon|FixinEIP712|FixinReentrancyGuard|FlashWallet|FullMigration|IAllowanceTarget|IBootstrapFeature|IBridgeAdapter|IERC20Bridge|IERC20Transformer|IExchange|IFeature|IFlashWallet|IGasToken|ILiquidityProviderFeature|IMetaTransactionsFeature|IOwnableFeature|ISignatureValidatorFeature|ISimpleFunctionRegistryFeature|ITestSimpleFunctionRegistryFeature|ITokenSpenderFeature|ITransformERC20Feature|IUniswapFeature|IZeroEx|InitialMigration|LibBootstrap|LibCommonRichErrors|LibERC20Transformer|LibLiquidityProviderRichErrors|LibLiquidityProviderStorage|LibMetaTransactionsRichErrors|LibMetaTransactionsStorage|LibMigrate|LibOwnableRichErrors|LibOwnableStorage|LibProxyRichErrors|LibProxyStorage|LibReentrancyGuardStorage|LibSignatureRichErrors|LibSignedCallData|LibSimpleFunctionRegistryRichErrors|LibSimpleFunctionRegistryStorage|LibSpenderRichErrors|LibStorage|LibTokenSpenderStorage|LibTransformERC20RichErrors|LibTransformERC20Storage|LibWalletRichErrors|LiquidityProviderFeature|LogMetadataTransformer|MetaTransactionsFeature|MixinAdapterAddresses|MixinBalancer|MixinCurve|MixinKyber|MixinMStable|MixinMooniswap|MixinOasis|MixinShell|MixinUniswap|MixinUniswapV2|MixinZeroExBridge|OwnableFeature|PayTakerTransformer|SignatureValidatorFeature|SimpleFunctionRegistryFeature|TestBridge|TestCallTarget|TestDelegateCaller|TestFillQuoteTransformerBridge|TestFillQuoteTransformerExchange|TestFillQuoteTransformerHost|TestFullMigration|TestInitialMigration|TestMetaTransactionsTransformERC20Feature|TestMigrator|TestMintTokenERC20Transformer|TestMintableERC20Token|TestSimpleFunctionRegistryFeatureImpl1|TestSimpleFunctionRegistryFeatureImpl2|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|FillQuoteTransformer|FixinCommon|FixinEIP712|FixinReentrancyGuard|FlashWallet|FullMigration|IAllowanceTarget|IBootstrapFeature|IBridgeAdapter|IERC20Bridge|IERC20Transformer|IExchange|IFeature|IFlashWallet|IGasToken|ILiquidityProviderFeature|IMetaTransactionsFeature|IOwnableFeature|ISignatureValidatorFeature|ISimpleFunctionRegistryFeature|ITestSimpleFunctionRegistryFeature|ITokenSpenderFeature|ITransformERC20Feature|IUniswapFeature|IZeroEx|InitialMigration|LibBootstrap|LibCommonRichErrors|LibERC20Transformer|LibLiquidityProviderRichErrors|LibLiquidityProviderStorage|LibMetaTransactionsRichErrors|LibMetaTransactionsStorage|LibMigrate|LibOwnableRichErrors|LibOwnableStorage|LibProxyRichErrors|LibProxyStorage|LibReentrancyGuardStorage|LibSignatureRichErrors|LibSignedCallData|LibSimpleFunctionRegistryRichErrors|LibSimpleFunctionRegistryStorage|LibSpenderRichErrors|LibStorage|LibTokenSpender|LibTokenSpenderStorage|LibTransformERC20RichErrors|LibTransformERC20Storage|LibWalletRichErrors|LiquidityProviderFeature|LogMetadataTransformer|MetaTransactionsFeature|MixinAdapterAddresses|MixinBalancer|MixinCurve|MixinKyber|MixinMStable|MixinMooniswap|MixinOasis|MixinShell|MixinUniswap|MixinUniswapV2|MixinZeroExBridge|OwnableFeature|PayTakerTransformer|SignatureValidatorFeature|SimpleFunctionRegistryFeature|TestBridge|TestCallTarget|TestDelegateCaller|TestFillQuoteTransformerBridge|TestFillQuoteTransformerExchange|TestFillQuoteTransformerHost|TestFullMigration|TestInitialMigration|TestLibTokenSpender|TestMetaTransactionsTransformERC20Feature|TestMigrator|TestMintTokenERC20Transformer|TestMintableERC20Token|TestSimpleFunctionRegistryFeatureImpl1|TestSimpleFunctionRegistryFeatureImpl2|TestTokenSpender|TestTokenSpenderERC20Token|TestTransformERC20|TestTransformerBase|TestTransformerDeployerTransformer|TestTransformerHost|TestWeth|TestWethTransformerHost|TestZeroExFeature|TokenSpenderFeature|TransformERC20Feature|Transformer|TransformerDeployer|UniswapFeature|WethTransformer|ZeroEx).json" }, "repository": { "type": "git", diff --git a/contracts/zero-ex/test/artifacts.ts b/contracts/zero-ex/test/artifacts.ts index ee85274846..df53980f76 100644 --- a/contracts/zero-ex/test/artifacts.ts +++ b/contracts/zero-ex/test/artifacts.ts @@ -54,6 +54,7 @@ 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'; @@ -84,6 +85,7 @@ import * as TestFillQuoteTransformerExchange from '../test/generated-artifacts/T import * as TestFillQuoteTransformerHost from '../test/generated-artifacts/TestFillQuoteTransformerHost.json'; import * as TestFullMigration from '../test/generated-artifacts/TestFullMigration.json'; import * as TestInitialMigration from '../test/generated-artifacts/TestInitialMigration.json'; +import * as TestLibTokenSpender from '../test/generated-artifacts/TestLibTokenSpender.json'; import * as TestMetaTransactionsTransformERC20Feature from '../test/generated-artifacts/TestMetaTransactionsTransformERC20Feature.json'; import * as TestMigrator from '../test/generated-artifacts/TestMigrator.json'; import * as TestMintableERC20Token from '../test/generated-artifacts/TestMintableERC20Token.json'; @@ -144,6 +146,7 @@ export const artifacts = { TransformERC20Feature: TransformERC20Feature as ContractArtifact, UniswapFeature: UniswapFeature as ContractArtifact, LibSignedCallData: LibSignedCallData as ContractArtifact, + LibTokenSpender: LibTokenSpender as ContractArtifact, FixinCommon: FixinCommon as ContractArtifact, FixinEIP712: FixinEIP712 as ContractArtifact, FixinReentrancyGuard: FixinReentrancyGuard as ContractArtifact, @@ -193,6 +196,7 @@ export const artifacts = { TestFillQuoteTransformerHost: TestFillQuoteTransformerHost as ContractArtifact, TestFullMigration: TestFullMigration as ContractArtifact, TestInitialMigration: TestInitialMigration as ContractArtifact, + TestLibTokenSpender: TestLibTokenSpender as ContractArtifact, TestMetaTransactionsTransformERC20Feature: TestMetaTransactionsTransformERC20Feature as ContractArtifact, TestMigrator: TestMigrator as ContractArtifact, TestMintTokenERC20Transformer: TestMintTokenERC20Transformer 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 12fbfc0e9a..520075aa28 100644 --- a/contracts/zero-ex/test/features/liquidity_provider_test.ts +++ b/contracts/zero-ex/test/features/liquidity_provider_test.ts @@ -2,12 +2,7 @@ import { artifacts as erc20Artifacts, DummyERC20TokenContract } from '@0x/contra import { blockchainTests, constants, expect, randomAddress, verifyEventsFromLogs } from '@0x/contracts-test-utils'; import { BigNumber, OwnableRevertErrors, ZeroExRevertErrors } from '@0x/utils'; -import { - IOwnableFeatureContract, - IZeroExContract, - LiquidityProviderFeatureContract, - TokenSpenderFeatureContract, -} from '../../src/wrappers'; +import { IOwnableFeatureContract, IZeroExContract, LiquidityProviderFeatureContract } from '../../src/wrappers'; import { artifacts } from '../artifacts'; import { abis } from '../utils/abis'; import { fullMigrateAsync } from '../utils/migration'; @@ -23,16 +18,7 @@ blockchainTests('LiquidityProvider feature', env => { before(async () => { [owner, taker] = await env.getAccountAddressesAsync(); - zeroEx = await fullMigrateAsync(owner, env.provider, env.txDefaults, { - tokenSpender: (await TokenSpenderFeatureContract.deployFrom0xArtifactAsync( - artifacts.TestTokenSpender, - env.provider, - env.txDefaults, - artifacts, - )).address, - }); - const tokenSpender = new TokenSpenderFeatureContract(zeroEx.address, env.provider, env.txDefaults, abis); - const allowanceTarget = await tokenSpender.getAllowanceTarget().callAsync(); + zeroEx = await fullMigrateAsync(owner, env.provider, env.txDefaults, {}); token = await DummyERC20TokenContract.deployFrom0xArtifactAsync( erc20Artifacts.DummyERC20Token, @@ -52,7 +38,7 @@ blockchainTests('LiquidityProvider feature', env => { artifacts, ); await token - .approve(allowanceTarget, constants.INITIAL_ERC20_ALLOWANCE) + .approve(zeroEx.address, constants.INITIAL_ERC20_ALLOWANCE) .awaitTransactionSuccessAsync({ from: taker }); feature = new LiquidityProviderFeatureContract(zeroEx.address, env.provider, env.txDefaults, abis); diff --git a/contracts/zero-ex/test/features/meta_transactions_test.ts b/contracts/zero-ex/test/features/meta_transactions_test.ts index 8e08b02d45..2ffb3127c4 100644 --- a/contracts/zero-ex/test/features/meta_transactions_test.ts +++ b/contracts/zero-ex/test/features/meta_transactions_test.ts @@ -17,7 +17,6 @@ import { artifacts } from '../artifacts'; import { abis } from '../utils/abis'; import { fullMigrateAsync } from '../utils/migration'; import { - ITokenSpenderFeatureContract, TestMetaTransactionsTransformERC20FeatureContract, TestMetaTransactionsTransformERC20FeatureEvents, TestMintableERC20TokenContract, @@ -33,7 +32,6 @@ blockchainTests.resets('MetaTransactions feature', env => { let feature: MetaTransactionsFeatureContract; let feeToken: TestMintableERC20TokenContract; let transformERC20Feature: TestMetaTransactionsTransformERC20FeatureContract; - let allowanceTarget: string; const MAX_FEE_AMOUNT = new BigNumber('1e18'); const TRANSFORM_ERC20_FAILING_VALUE = new BigNumber(666); @@ -64,14 +62,11 @@ blockchainTests.resets('MetaTransactions feature', env => { env.txDefaults, {}, ); - allowanceTarget = await new ITokenSpenderFeatureContract(zeroEx.address, env.provider, env.txDefaults) - .getAllowanceTarget() - .callAsync(); // Fund signers with fee tokens. await Promise.all( signers.map(async signer => { await feeToken.mint(signer, MAX_FEE_AMOUNT).awaitTransactionSuccessAsync(); - await feeToken.approve(allowanceTarget, MAX_FEE_AMOUNT).awaitTransactionSuccessAsync({ from: signer }); + await feeToken.approve(zeroEx.address, MAX_FEE_AMOUNT).awaitTransactionSuccessAsync({ from: signer }); }), ); }); diff --git a/contracts/zero-ex/test/features/transform_erc20_test.ts b/contracts/zero-ex/test/features/transform_erc20_test.ts index 77e44a3a6b..3fc6994956 100644 --- a/contracts/zero-ex/test/features/transform_erc20_test.ts +++ b/contracts/zero-ex/test/features/transform_erc20_test.ts @@ -20,7 +20,6 @@ import { abis } from '../utils/abis'; import { fullMigrateAsync } from '../utils/migration'; import { FlashWalletContract, - ITokenSpenderFeatureContract, TestMintableERC20TokenContract, TestMintTokenERC20TransformerContract, TestMintTokenERC20TransformerEvents, @@ -42,7 +41,6 @@ blockchainTests.resets('TransformERC20 feature', env => { let zeroEx: IZeroExContract; let feature: TransformERC20FeatureContract; let wallet: FlashWalletContract; - let allowanceTarget: string; before(async () => { [owner, taker, sender, transformerDeployer] = await env.getAccountAddressesAsync(); @@ -67,9 +65,6 @@ blockchainTests.resets('TransformERC20 feature', env => { abis, ); wallet = new FlashWalletContract(await feature.getTransformWallet().callAsync(), env.provider, env.txDefaults); - allowanceTarget = await new ITokenSpenderFeatureContract(zeroEx.address, env.provider, env.txDefaults) - .getAllowanceTarget() - .callAsync(); await feature.setQuoteSigner(callDataSigner).awaitTransactionSuccessAsync({ from: owner }); }); @@ -173,7 +168,7 @@ blockchainTests.resets('TransformERC20 feature', env => { }, artifacts, ); - await inputToken.approve(allowanceTarget, MAX_UINT256).awaitTransactionSuccessAsync({ from: taker }); + await inputToken.approve(zeroEx.address, MAX_UINT256).awaitTransactionSuccessAsync({ from: taker }); }); interface Transformation { diff --git a/contracts/zero-ex/test/lib_token_spender_test.ts b/contracts/zero-ex/test/lib_token_spender_test.ts new file mode 100644 index 0000000000..0da14b4ac2 --- /dev/null +++ b/contracts/zero-ex/test/lib_token_spender_test.ts @@ -0,0 +1,210 @@ +import { + blockchainTests, + expect, + getRandomInteger, + randomAddress, + verifyEventsFromLogs, +} from '@0x/contracts-test-utils'; +import { BigNumber, hexUtils, StringRevertError, ZeroExRevertErrors } from '@0x/utils'; + +import { artifacts } from './artifacts'; +import { + TestLibTokenSpenderContract, + TestLibTokenSpenderEvents, + TestTokenSpenderERC20TokenContract, + TestTokenSpenderERC20TokenEvents, +} from './wrappers'; + +blockchainTests.resets('LibTokenSpender library', env => { + let tokenSpender: TestLibTokenSpenderContract; + let token: TestTokenSpenderERC20TokenContract; + + before(async () => { + tokenSpender = await TestLibTokenSpenderContract.deployFrom0xArtifactAsync( + artifacts.TestLibTokenSpender, + env.provider, + env.txDefaults, + artifacts, + ); + token = await TestTokenSpenderERC20TokenContract.deployFrom0xArtifactAsync( + artifacts.TestTokenSpenderERC20Token, + env.provider, + env.txDefaults, + artifacts, + ); + }); + + describe('spendERC20Tokens()', () => { + const EMPTY_RETURN_AMOUNT = 1337; + const FALSE_RETURN_AMOUNT = 1338; + const REVERT_RETURN_AMOUNT = 1339; + const TRIGGER_FALLBACK_SUCCESS_AMOUNT = 1340; + const EXTRA_RETURN_TRUE_AMOUNT = 1341; + const EXTRA_RETURN_FALSE_AMOUNT = 1342; + + it('spendERC20Tokens() 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) + .awaitTransactionSuccessAsync(); + verifyEventsFromLogs( + receipt.logs, + [ + { + sender: tokenSpender.address, + from: tokenFrom, + to: tokenTo, + amount: tokenAmount, + }, + ], + TestTokenSpenderERC20TokenEvents.TransferFromCalled, + ); + }); + + it('spendERC20Tokens() 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) + .awaitTransactionSuccessAsync(); + verifyEventsFromLogs( + receipt.logs, + [ + { + sender: tokenSpender.address, + from: tokenFrom, + to: tokenTo, + amount: tokenAmount, + }, + ], + TestTokenSpenderERC20TokenEvents.TransferFromCalled, + ); + }); + + it('spendERC20Tokens() 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) + .awaitTransactionSuccessAsync(); + const expectedError = new ZeroExRevertErrors.Spender.SpenderERC20TransferFromFailedError( + token.address, + tokenFrom, + tokenTo, + tokenAmount, + new StringRevertError('TestTokenSpenderERC20Token/Revert').encode(), + ); + return expect(tx).to.revertWith(expectedError); + }); + + it('spendERC20Tokens() 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) + .awaitTransactionSuccessAsync(); + return expect(tx).to.revertWith( + new ZeroExRevertErrors.Spender.SpenderERC20TransferFromFailedError( + token.address, + tokenFrom, + tokenTo, + tokenAmount, + hexUtils.leftPad(0), + ), + ); + }); + + it('spendERC20Tokens() 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) + .awaitTransactionSuccessAsync(); + verifyEventsFromLogs( + receipt.logs, + [ + { + token: token.address, + owner: tokenFrom, + to: tokenTo, + amount: tokenAmount, + }, + ], + TestLibTokenSpenderEvents.FallbackCalled, + ); + }); + + it('spendERC20Tokens() 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) + .awaitTransactionSuccessAsync(); + verifyEventsFromLogs( + receipt.logs, + [ + { + sender: tokenSpender.address, + from: tokenFrom, + to: tokenTo, + amount: tokenAmount, + }, + ], + TestTokenSpenderERC20TokenEvents.TransferFromCalled, + ); + }); + + it("spendERC20Tokens() 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) + .awaitTransactionSuccessAsync(); + return expect(tx).to.revertWith( + new ZeroExRevertErrors.Spender.SpenderERC20TransferFromFailedError( + token.address, + tokenFrom, + tokenTo, + tokenAmount, + hexUtils.leftPad(EXTRA_RETURN_FALSE_AMOUNT, 64), + ), + ); + }); + + it('spendERC20Tokens() cannot call self', async () => { + const tokenFrom = randomAddress(); + const tokenTo = randomAddress(); + const tokenAmount = new BigNumber(123456); + + const tx = tokenSpender + .spendERC20Tokens(tokenSpender.address, tokenFrom, tokenTo, tokenAmount) + .awaitTransactionSuccessAsync(); + return expect(tx).to.revertWith('LibTokenSpender/CANNOT_INVOKE_SELF'); + }); + }); + + describe('getSpendableERC20BalanceOf()', () => { + it("returns the minimum of the owner's balance and allowance", async () => { + const balance = getRandomInteger(1, '1e18'); + const allowance = getRandomInteger(1, '1e18'); + const tokenOwner = randomAddress(); + await token + .setBalanceAndAllowanceOf(tokenOwner, balance, tokenSpender.address, allowance) + .awaitTransactionSuccessAsync(); + const spendableBalance = await tokenSpender + .getSpendableERC20BalanceOf(token.address, tokenOwner) + .callAsync(); + expect(spendableBalance).to.bignumber.eq(BigNumber.min(balance, allowance)); + }); + }); +}); diff --git a/contracts/zero-ex/test/wrappers.ts b/contracts/zero-ex/test/wrappers.ts index 30812a7248..adc00601e5 100644 --- a/contracts/zero-ex/test/wrappers.ts +++ b/contracts/zero-ex/test/wrappers.ts @@ -52,6 +52,7 @@ 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'; @@ -82,6 +83,7 @@ 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_full_migration'; export * from '../test/generated-wrappers/test_initial_migration'; +export * from '../test/generated-wrappers/test_lib_token_spender'; export * from '../test/generated-wrappers/test_meta_transactions_transform_erc20_feature'; export * from '../test/generated-wrappers/test_migrator'; export * from '../test/generated-wrappers/test_mint_token_erc20_transformer'; diff --git a/contracts/zero-ex/tsconfig.json b/contracts/zero-ex/tsconfig.json index 457630a96d..e75fcce22e 100644 --- a/contracts/zero-ex/tsconfig.json +++ b/contracts/zero-ex/tsconfig.json @@ -76,6 +76,7 @@ "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", @@ -106,6 +107,7 @@ "test/generated-artifacts/TestFillQuoteTransformerHost.json", "test/generated-artifacts/TestFullMigration.json", "test/generated-artifacts/TestInitialMigration.json", + "test/generated-artifacts/TestLibTokenSpender.json", "test/generated-artifacts/TestMetaTransactionsTransformERC20Feature.json", "test/generated-artifacts/TestMigrator.json", "test/generated-artifacts/TestMintTokenERC20Transformer.json",