From 15c0d622c9959b3873f5b8bf2f1a2d4a73703395 Mon Sep 17 00:00:00 2001 From: Michael Zhu Date: Thu, 11 Jul 2019 17:46:41 -0700 Subject: [PATCH] Update function definitions, require that makerAsset is the same across orders, approve proxy to transfer makerAsset (for percentage-based fees) --- .../contracts/src/MixinAssets.sol | 19 +++++++++++++++++++ .../contracts/src/MixinExchangeWrapper.sol | 18 ++++++++++++++---- .../contracts/src/MixinForwarderCore.sol | 8 ++++---- .../contracts/src/MixinWeth.sol | 3 +-- .../src/interfaces/IForwarderCore.sol | 18 ++---------------- .../contracts/src/LibFillResults.sol | 4 ++++ 6 files changed, 44 insertions(+), 26 deletions(-) diff --git a/contracts/exchange-forwarder/contracts/src/MixinAssets.sol b/contracts/exchange-forwarder/contracts/src/MixinAssets.sol index 1edd299c5e..33fc25ea00 100644 --- a/contracts/exchange-forwarder/contracts/src/MixinAssets.sol +++ b/contracts/exchange-forwarder/contracts/src/MixinAssets.sol @@ -50,6 +50,25 @@ contract MixinAssets is _transferAssetToSender(assetData, amount); } + function _approveMakerAssetProxy(bytes memory assetData) + internal + { + bytes4 proxyId = assetData.readBytes4(0); + + address proxyAddress; + address assetAddress; + if (proxyId == ERC20_DATA_ID) { + proxyAddress = EXCHANGE.getAssetProxy(ERC20_DATA_ID); + } else if (proxyId == ERC721_DATA_ID) { + proxyAddress = EXCHANGE.getAssetProxy(ERC721_DATA_ID); + } else { + revert("UNSUPPORTED_ASSET_PROXY"); + } + + assetAddress = assetData.readAddress(16); + IERC20Token(assetAddress).approve(proxyAddress, MAX_UINT); + } + /// @dev Transfers given amount of asset to sender. /// @param assetData Byte array encoded for the respective asset proxy. /// @param amount Amount of asset to transfer to sender. diff --git a/contracts/exchange-forwarder/contracts/src/MixinExchangeWrapper.sol b/contracts/exchange-forwarder/contracts/src/MixinExchangeWrapper.sol index 42a379c58d..2a4bb8d1b6 100644 --- a/contracts/exchange-forwarder/contracts/src/MixinExchangeWrapper.sol +++ b/contracts/exchange-forwarder/contracts/src/MixinExchangeWrapper.sol @@ -96,6 +96,11 @@ contract MixinExchangeWrapper is uint256 remainingTakerAssetFillAmount = wethSellAmount; for (uint256 i = 0; i != ordersLength; i++) { + require( + orders[i].makerAssetData.equals(orders[0].makerAssetData), + "MAKER_ASSET_MISMATCH" + ); + // Attempt to sell the remaining amount of WETH FillResults memory singleFillResults = _fillOrderNoThrow( orders[i], @@ -105,18 +110,18 @@ contract MixinExchangeWrapper is // Update amounts filled and the remaining amount of WETH to sell if (orders[i].makerAssetData.equals(orders[i].takerFeeAssetData)) { - _addFillResultsDeductFees(totalFillResults, singleFillResults) + _addFillResultsDeductFees(totalFillResults, singleFillResults); } else { _addFillResults(totalFillResults, singleFillResults); remainingTakerAssetFillAmount = _safeSub( remainingTakerAssetFillAmount, singleFillResults.takerFeePaid - ) + ); } remainingTakerAssetFillAmount = _safeSub( remainingTakerAssetFillAmount, singleFillResults.takerAssetFilledAmount - ) + ); // Stop execution if the entire amount of takerAsset has been sold if (totalFillResults.takerAssetFilledAmount >= wethSellAmount) { @@ -145,6 +150,11 @@ contract MixinExchangeWrapper is uint256 makerAssetFilledAmount = 0; for (uint256 i = 0; i != ordersLength; i++) { + require( + orders[i].makerAssetData.equals(orders[0].makerAssetData), + "MAKER_ASSET_MISMATCH" + ); + // Calculate the remaining amount of makerAsset to buy uint256 remainingMakerAssetFillAmount = _safeSub(makerAssetFillAmount, totalFillResults.makerAssetFilledAmount); @@ -167,7 +177,7 @@ contract MixinExchangeWrapper is // Update amounts filled and fees paid by maker and taker if (orders[i].makerAssetData.equals(orders[i].takerFeeAssetData)) { - _addFillResultsDeductFees(totalFillResults, singleFillResults) + _addFillResultsDeductFees(totalFillResults, singleFillResults); } else { _addFillResults(totalFillResults, singleFillResults); } diff --git a/contracts/exchange-forwarder/contracts/src/MixinForwarderCore.sol b/contracts/exchange-forwarder/contracts/src/MixinForwarderCore.sol index e34a66e898..5359bc33ee 100644 --- a/contracts/exchange-forwarder/contracts/src/MixinForwarderCore.sol +++ b/contracts/exchange-forwarder/contracts/src/MixinForwarderCore.sol @@ -61,8 +61,6 @@ contract MixinForwarderCore is /// Any ETH not spent will be refunded to sender. /// @param orders Array of order specifications used containing desired makerAsset and WETH as takerAsset. /// @param signatures Proofs that orders have been created by makers. - /// @param feeOrders Array of order specifications containing ZRX as makerAsset and WETH as takerAsset. Used to purchase ZRX for primary order fees. - /// @param feeSignatures Proofs that feeOrders have been created by makers. /// @param feePercentage Percentage of WETH sold that will payed as fee to forwarding contract feeRecipient. /// @param feeRecipient Address that will receive ETH when orders are filled. /// @return Amounts filled and fees paid by maker and taker for both sets of orders. @@ -86,6 +84,8 @@ contract MixinForwarderCore is msg.value ); + _approveMakerAssetProxy(orders[0].makerAssetData); + // Market sell 95% of WETH. // ZRX fees are payed with this contract's balance. orderFillResults = _marketSellWeth( @@ -115,8 +115,6 @@ contract MixinForwarderCore is /// @param orders Array of order specifications used containing desired makerAsset and WETH as takerAsset. /// @param makerAssetFillAmount Desired amount of makerAsset to purchase. /// @param signatures Proofs that orders have been created by makers. - /// @param feeOrders Array of order specifications containing ZRX as makerAsset and WETH as takerAsset. Used to purchase ZRX for primary order fees. - /// @param feeSignatures Proofs that feeOrders have been created by makers. /// @param feePercentage Percentage of WETH sold that will payed as fee to forwarding contract feeRecipient. /// @param feeRecipient Address that will receive ETH when orders are filled. /// @return Amounts filled and fees paid by maker and taker for both sets of orders. @@ -134,6 +132,8 @@ contract MixinForwarderCore is // Convert ETH to WETH. _convertEthToWeth(); + _approveMakerAssetProxy(orders[0].makerAssetData); + // Attempt to purchase desired amount of makerAsset. // ZRX fees are payed with this contract's balance. orderFillResults = _marketBuyExactAmountWithWeth( diff --git a/contracts/exchange-forwarder/contracts/src/MixinWeth.sol b/contracts/exchange-forwarder/contracts/src/MixinWeth.sol index b5d074e8d6..f431600a26 100644 --- a/contracts/exchange-forwarder/contracts/src/MixinWeth.sol +++ b/contracts/exchange-forwarder/contracts/src/MixinWeth.sol @@ -50,8 +50,7 @@ contract MixinWeth is /// @dev Transfers feePercentage of WETH spent on primary orders to feeRecipient. /// Refunds any excess ETH to msg.sender. - /// @param wethSoldExcludingFeeOrders Amount of WETH sold when filling primary orders. - /// @param wethSoldForZrx Amount of WETH sold when purchasing ZRX required for primary order fees. + /// @param wethSold Amount of WETH sold when filling primary orders. /// @param feePercentage Percentage of WETH sold that will payed as fee to forwarding contract feeRecipient. /// @param feeRecipient Address that will receive ETH when orders are filled. function _transferEthFeeAndRefund( diff --git a/contracts/exchange-forwarder/contracts/src/interfaces/IForwarderCore.sol b/contracts/exchange-forwarder/contracts/src/interfaces/IForwarderCore.sol index 544f9b1954..3d177a578a 100644 --- a/contracts/exchange-forwarder/contracts/src/interfaces/IForwarderCore.sol +++ b/contracts/exchange-forwarder/contracts/src/interfaces/IForwarderCore.sol @@ -31,25 +31,18 @@ contract IForwarderCore { /// Any ETH not spent will be refunded to sender. /// @param orders Array of order specifications used containing desired makerAsset and WETH as takerAsset. /// @param signatures Proofs that orders have been created by makers. - /// @param feeOrders Array of order specifications containing ZRX as makerAsset and WETH as takerAsset. Used to purchase ZRX for primary order fees. - /// @param feeSignatures Proofs that feeOrders have been created by makers. /// @param feePercentage Percentage of WETH sold that will payed as fee to forwarding contract feeRecipient. /// @param feeRecipient Address that will receive ETH when orders are filled. /// @return Amounts filled and fees paid by maker and taker for both sets of orders. function marketSellOrdersWithEth( LibOrder.Order[] memory orders, bytes[] memory signatures, - LibOrder.Order[] memory feeOrders, - bytes[] memory feeSignatures, uint256 feePercentage, address payable feeRecipient ) public payable - returns ( - LibFillResults.FillResults memory orderFillResults, - LibFillResults.FillResults memory feeOrderFillResults - ); + returns (LibFillResults.FillResults memory orderFillResults); /// @dev Attempt to purchase makerAssetFillAmount of makerAsset by selling ETH provided with transaction. /// Any ZRX required to pay fees for primary orders will automatically be purchased by this contract. @@ -57,8 +50,6 @@ contract IForwarderCore { /// @param orders Array of order specifications used containing desired makerAsset and WETH as takerAsset. /// @param makerAssetFillAmount Desired amount of makerAsset to purchase. /// @param signatures Proofs that orders have been created by makers. - /// @param feeOrders Array of order specifications containing ZRX as makerAsset and WETH as takerAsset. Used to purchase ZRX for primary order fees. - /// @param feeSignatures Proofs that feeOrders have been created by makers. /// @param feePercentage Percentage of WETH sold that will payed as fee to forwarding contract feeRecipient. /// @param feeRecipient Address that will receive ETH when orders are filled. /// @return Amounts filled and fees paid by maker and taker for both sets of orders. @@ -66,15 +57,10 @@ contract IForwarderCore { LibOrder.Order[] memory orders, uint256 makerAssetFillAmount, bytes[] memory signatures, - LibOrder.Order[] memory feeOrders, - bytes[] memory feeSignatures, uint256 feePercentage, address payable feeRecipient ) public payable - returns ( - LibFillResults.FillResults memory orderFillResults, - LibFillResults.FillResults memory feeOrderFillResults - ); + returns (LibFillResults.FillResults memory orderFillResults); } diff --git a/contracts/exchange-libs/contracts/src/LibFillResults.sol b/contracts/exchange-libs/contracts/src/LibFillResults.sol index 31d4fe094b..374e35d200 100644 --- a/contracts/exchange-libs/contracts/src/LibFillResults.sol +++ b/contracts/exchange-libs/contracts/src/LibFillResults.sol @@ -403,6 +403,10 @@ library LibFillResults { return matchedFillResults; } + /// @dev Adds properties of both FillResults instances. + /// Modifies the first FillResults instance specified. + /// @param totalFillResults Fill results instance that will be added onto. + /// @param singleFillResults Fill results instance that will be added to totalFillResults. function _addFillResultsDeductFees(FillResults memory totalFillResults, FillResults memory singleFillResults) internal pure