From ca20df4752597054f74de2fc8d319819973e2c23 Mon Sep 17 00:00:00 2001 From: Lawrence Forman Date: Tue, 24 Nov 2020 17:52:22 -0500 Subject: [PATCH] `@0x/contracts-zero-ex`: Fix `TransformERC20Feature` reverting when selling taker's entire balance when input token is ETH (#46) Co-authored-by: Lawrence Forman --- contracts/zero-ex/CHANGELOG.json | 4 + .../src/features/TransformERC20Feature.sol | 18 +++-- .../test/TestMintTokenERC20Transformer.sol | 10 ++- .../test/features/transform_erc20_test.ts | 76 +++++++++++++++++++ 4 files changed, 100 insertions(+), 8 deletions(-) diff --git a/contracts/zero-ex/CHANGELOG.json b/contracts/zero-ex/CHANGELOG.json index 9b32fdedd9..24658a09de 100644 --- a/contracts/zero-ex/CHANGELOG.json +++ b/contracts/zero-ex/CHANGELOG.json @@ -33,6 +33,10 @@ { "note": "Require RFQ orders to specify a transaction origin, and allow approved alternative addresses", "pr": 47 + }, + { + "note": "Do not try to pull all tokens if selling all ETH in `TransformERC20Feature`", + "pr": 46 } ] }, diff --git a/contracts/zero-ex/contracts/src/features/TransformERC20Feature.sol b/contracts/zero-ex/contracts/src/features/TransformERC20Feature.sol index f92bc8a261..8f67fe0437 100644 --- a/contracts/zero-ex/contracts/src/features/TransformERC20Feature.sol +++ b/contracts/zero-ex/contracts/src/features/TransformERC20Feature.sol @@ -214,13 +214,19 @@ contract TransformERC20Feature is private returns (uint256 outputTokenAmount) { - // If the input token amount is -1, transform the taker's entire - // spendable balance. + // If the input token amount is -1 and we are not selling ETH, + // transform the taker's entire spendable balance. if (args.inputTokenAmount == uint256(-1)) { - args.inputTokenAmount = _getSpendableERC20BalanceOf( - args.inputToken, - args.taker - ); + if (LibERC20Transformer.isTokenETH(args.inputToken)) { + // We can't pull more ETH from the taker, so we just set the + // input token amount to the value attached to the call. + args.inputTokenAmount = msg.value; + } else { + args.inputTokenAmount = _getSpendableERC20BalanceOf( + args.inputToken, + args.taker + ); + } } TransformERC20PrivateState memory state; diff --git a/contracts/zero-ex/contracts/test/TestMintTokenERC20Transformer.sol b/contracts/zero-ex/contracts/test/TestMintTokenERC20Transformer.sol index bb5fccd464..b6b956ce0a 100644 --- a/contracts/zero-ex/contracts/test/TestMintTokenERC20Transformer.sol +++ b/contracts/zero-ex/contracts/test/TestMintTokenERC20Transformer.sol @@ -60,11 +60,17 @@ contract TestMintTokenERC20Transformer is context.sender, context.taker, context.data, - data.inputToken.balanceOf(address(this)), + LibERC20Transformer.isTokenETH(data.inputToken) + ? address(this).balance + : data.inputToken.balanceOf(address(this)), address(this).balance ); // "Burn" input tokens. - data.inputToken.transfer(address(0), data.burnAmount); + if (LibERC20Transformer.isTokenETH(data.inputToken)) { + address(0).transfer(data.burnAmount); + } else { + data.inputToken.transfer(address(0), data.burnAmount); + } // Mint output tokens. if (LibERC20Transformer.isTokenETH(IERC20TokenV06(address(data.outputToken)))) { context.taker.transfer(data.mintAmount); diff --git a/contracts/zero-ex/test/features/transform_erc20_test.ts b/contracts/zero-ex/test/features/transform_erc20_test.ts index 50627193ab..5e763db3f2 100644 --- a/contracts/zero-ex/test/features/transform_erc20_test.ts +++ b/contracts/zero-ex/test/features/transform_erc20_test.ts @@ -649,6 +649,82 @@ blockchainTests.resets('TransformERC20 feature', env => { const { callDataHash: actualCallDataHash } = (receipt.logs[0] as MintTokenTransformerEvent).args; expect(actualCallDataHash).to.eq(NULL_BYTES32); }); + + it('can sell entire taker balance', async () => { + const startingInputTokenBalance = getRandomInteger(0, '100e18'); + await inputToken.mint(taker, startingInputTokenBalance).awaitTransactionSuccessAsync(); + const minOutputTokenAmount = getRandomInteger(1, '1e18'); + const outputTokenMintAmount = minOutputTokenAmount; + const callValue = getRandomInteger(1, '1e18'); + const callDataHash = hexUtils.random(); + const transformation = createMintTokenTransformation({ + outputTokenMintAmount, + inputTokenBurnAmunt: startingInputTokenBalance, + }); + const receipt = await feature + ._transformERC20({ + taker, + inputToken: inputToken.address, + outputToken: outputToken.address, + inputTokenAmount: MAX_UINT256, + minOutputTokenAmount, + transformations: [transformation], + callDataHash, + callDataSignature: NULL_BYTES, + }) + .awaitTransactionSuccessAsync({ value: callValue }); + verifyEventsFromLogs( + receipt.logs, + [ + { + taker, + inputTokenAmount: startingInputTokenBalance, + outputTokenAmount: outputTokenMintAmount, + inputToken: inputToken.address, + outputToken: outputToken.address, + }, + ], + TransformERC20FeatureEvents.TransformedERC20, + ); + }); + + it('can sell entire taker balance with ETH (but not really)', async () => { + const ethAttchedAmount = getRandomInteger(0, '100e18'); + await inputToken.mint(taker, ethAttchedAmount).awaitTransactionSuccessAsync(); + const minOutputTokenAmount = getRandomInteger(1, '1e18'); + const outputTokenMintAmount = minOutputTokenAmount; + const callDataHash = hexUtils.random(); + const transformation = createMintTokenTransformation({ + outputTokenMintAmount, + inputTokenAddress: ETH_TOKEN_ADDRESS, + inputTokenBurnAmunt: ethAttchedAmount, + }); + const receipt = await feature + ._transformERC20({ + taker, + inputToken: ETH_TOKEN_ADDRESS, + outputToken: outputToken.address, + inputTokenAmount: MAX_UINT256, + minOutputTokenAmount, + transformations: [transformation], + callDataHash, + callDataSignature: NULL_BYTES, + }) + .awaitTransactionSuccessAsync({ value: ethAttchedAmount }); + verifyEventsFromLogs( + receipt.logs, + [ + { + taker, + inputTokenAmount: ethAttchedAmount, + outputTokenAmount: outputTokenMintAmount, + inputToken: ETH_TOKEN_ADDRESS, + outputToken: outputToken.address, + }, + ], + TransformERC20FeatureEvents.TransformedERC20, + ); + }); }); describe('transformERC20()', () => {