From 799ff2a5c392383c8b245ae53057593acc2534ce Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Tue, 17 Jul 2018 11:32:44 -0700 Subject: Fix rounding error issues, use different logic when makerAsset is ZRX --- .../src/2.0.0/forwarder/LibForwarderErrors.sol | 1 + .../src/2.0.0/forwarder/MixinConstants.sol | 5 +- .../src/2.0.0/forwarder/MixinForwarderCore.sol | 121 +++++++++++++++------ .../contracts/src/2.0.0/forwarder/MixinWeth.sol | 13 --- .../src/2.0.0/forwarder/mixins/MConstants.sol | 3 + .../contracts/src/2.0.0/forwarder/mixins/MWeth.sol | 4 +- 6 files changed, 95 insertions(+), 52 deletions(-) diff --git a/packages/contracts/src/2.0.0/forwarder/LibForwarderErrors.sol b/packages/contracts/src/2.0.0/forwarder/LibForwarderErrors.sol index 69108738a..af85c75e3 100644 --- a/packages/contracts/src/2.0.0/forwarder/LibForwarderErrors.sol +++ b/packages/contracts/src/2.0.0/forwarder/LibForwarderErrors.sol @@ -31,4 +31,5 @@ contract LibForwarderErrors { string constant DEFAULT_FUNCTION_WETH_CONTRACT_ONLY = "DEFAULT_FUNCTION_WETH_CONTRACT_ONLY"; // Fallback function may only be used for WETH withdrawals. string constant INVALID_MSG_VALUE = "INVALID_MSG_VALUE"; // msg.value must be greater than 0. string constant INVALID_AMOUNT = "INVALID_AMOUNT"; // Amount must equal 1. + string constant INVALID_ORDERS_LENGTH = "INVALID_ORDERS_LENGTH"; // Length of orders must be greater than 1. } diff --git a/packages/contracts/src/2.0.0/forwarder/MixinConstants.sol b/packages/contracts/src/2.0.0/forwarder/MixinConstants.sol index 4f95c796b..e430aba41 100644 --- a/packages/contracts/src/2.0.0/forwarder/MixinConstants.sol +++ b/packages/contracts/src/2.0.0/forwarder/MixinConstants.sol @@ -28,7 +28,10 @@ contract MixinConstants is bytes4 constant internal ERC20_DATA_ID = bytes4(keccak256("ERC20Token(address)")); bytes4 constant internal ERC721_DATA_ID = bytes4(keccak256("ERC721Token(address,uint256,bytes)")); uint256 constant internal MAX_UINT = 2**256 - 1; - + uint256 constant internal PERCENTAGE_DENOMINATOR = 10**18; + uint256 constant internal MAX_FEE_PERCENTAGE = 5 * PERCENTAGE_DENOMINATOR / 100; // 5% + uint256 constant internal MAX_WETH_FILL_PERCENTAGE = 95 * PERCENTAGE_DENOMINATOR / 100; // 95% + constructor ( address _exchange, address _etherToken, diff --git a/packages/contracts/src/2.0.0/forwarder/MixinForwarderCore.sol b/packages/contracts/src/2.0.0/forwarder/MixinForwarderCore.sol index 19b1c357b..9dc203373 100644 --- a/packages/contracts/src/2.0.0/forwarder/MixinForwarderCore.sol +++ b/packages/contracts/src/2.0.0/forwarder/MixinForwarderCore.sol @@ -23,6 +23,7 @@ import "./mixins/MWeth.sol"; import "./mixins/MAssets.sol"; import "./mixins/MConstants.sol"; import "./mixins/MForwarderCore.sol"; +import "../utils/LibBytes/LibBytes.sol"; import "../protocol/Exchange/libs/LibOrder.sol"; import "../protocol/Exchange/libs/LibFillResults.sol"; import "../protocol/Exchange/libs/LibMath.sol"; @@ -37,6 +38,8 @@ contract MixinForwarderCore is MForwarderCore { + using LibBytes for bytes; + /// @dev Constructor approves ERC20 proxy to transfer ZRX and WETH on this contract's behalf. constructor () public @@ -74,24 +77,54 @@ contract MixinForwarderCore is FillResults memory feeOrderFillResults ) { - // Convert ETH to WETH. - // 5% of WETH is reserved for filling feeOrders and paying feeRecipient. - uint256 wethAvailable = convertEthToWeth(); - - // Attempt to sell 95% of WETH. - // ZRX fees are payed with this contract's balance. - orderFillResults = marketSellWeth( - orders, - wethAvailable, - signatures + require( + orders.length > 0, + "INVALID_ORDERS_LENGTH" ); - // Buy back all ZRX spent on fees. - feeOrderFillResults = marketBuyZrx( - feeOrders, - orderFillResults.takerFeePaid, - feeSignatures - ); + // Convert ETH to WETH. + convertEthToWeth(); + + uint256 makerAssetAmountPurchased; + uint256 wethAvailable; + if (orders[0].makerAssetData.equals(ZRX_ASSET_DATA)) { + // Calculate amount of WETH that won't be spent on ETH fees. + wethAvailable = getPartialAmount( + PERCENTAGE_DENOMINATOR, + safeAdd(PERCENTAGE_DENOMINATOR, feePercentage), + msg.value + ); + // Market sell available WETH. + // ZRX fees are paid with this contract's balance. + orderFillResults = marketSellWeth( + orders, + wethAvailable, + signatures + ); + // The fee amount must be deducted from the amount transfered back to sender. + makerAssetAmountPurchased = safeSub(orderFillResults.makerAssetFilledAmount, orderFillResults.takerFeePaid); + } else { + // 5% of WETH is reserved for filling feeOrders and paying feeRecipient. + wethAvailable = getPartialAmount( + MAX_WETH_FILL_PERCENTAGE, + PERCENTAGE_DENOMINATOR, + msg.value + ); + // Market sell 95% of WETH. + // ZRX fees are payed with this contract's balance. + orderFillResults = marketSellWeth( + orders, + wethAvailable, + signatures + ); + // Buy back all ZRX spent on fees. + feeOrderFillResults = marketBuyZrx( + feeOrders, + orderFillResults.takerFeePaid, + feeSignatures + ); + makerAssetAmountPurchased = orderFillResults.makerAssetFilledAmount; + } // Ensure that no extra WETH owned by this contract has been sold. uint256 totalWethSold = safeAdd(orderFillResults.takerAssetFilledAmount, feeOrderFillResults.takerAssetFilledAmount); @@ -110,7 +143,7 @@ contract MixinForwarderCore is ); // Transfer purchased assets to msg.sender. - transferPurchasedAssetToSender(orders[0].makerAssetData, orderFillResults.makerAssetFilledAmount); + transferPurchasedAssetToSender(orders[0].makerAssetData, makerAssetAmountPurchased); } /// @dev Attempt to purchase makerAssetFillAmount of makerAsset by selling ETH provided with transaction. @@ -140,23 +173,41 @@ contract MixinForwarderCore is FillResults memory feeOrderFillResults ) { + require( + orders.length > 0, + "INVALID_ORDERS_LENGTH" + ); + // Convert ETH to WETH. convertEthToWeth(); - // Attemp to purchase desired amount of makerAsset. - // ZRX fees are payed with this contract's balance. - orderFillResults = marketBuyAsset( - orders, - makerAssetFillAmount, - signatures - ); - - // Buy back all ZRX spent on fees. - feeOrderFillResults = marketBuyZrx( - feeOrders, - orderFillResults.takerFeePaid, - feeSignatures - ); + uint256 makerAssetAmountPurchased; + if (orders[0].makerAssetData.equals(ZRX_ASSET_DATA)) { + // If the makerAsset is ZRX, it is not necessary to pay fees out of this + // contracts's ZRX balance because fees are factored into the price of the order. + orderFillResults = marketBuyZrx( + orders, + makerAssetFillAmount, + signatures + ); + // The fee amount must be deducted from the amount transfered back to sender. + makerAssetAmountPurchased = safeSub(orderFillResults.makerAssetFilledAmount, orderFillResults.takerFeePaid); + } else { + // Attemp to purchase desired amount of makerAsset. + // ZRX fees are payed with this contract's balance. + orderFillResults = marketBuyAsset( + orders, + makerAssetFillAmount, + signatures + ); + // Buy back all ZRX spent on fees. + feeOrderFillResults = marketBuyZrx( + feeOrders, + orderFillResults.takerFeePaid, + feeSignatures + ); + makerAssetAmountPurchased = orderFillResults.makerAssetFilledAmount; + } // Ensure that no extra WETH owned by this contract has been sold. uint256 totalWethSold = safeAdd(orderFillResults.takerAssetFilledAmount, feeOrderFillResults.takerAssetFilledAmount); @@ -175,7 +226,7 @@ contract MixinForwarderCore is ); // Transfer purchased assets to msg.sender. - transferPurchasedAssetToSender(orders[0].makerAssetData, orderFillResults.makerAssetFilledAmount); + transferPurchasedAssetToSender(orders[0].makerAssetData, makerAssetAmountPurchased); } /// @param orders Array of order specifications used containing desired makerAsset and WETH as takerAsset. @@ -280,7 +331,7 @@ contract MixinForwarderCore is // Attempt to sell the remaining amount of WETH. FillResults memory singleFillResult = EXCHANGE.fillOrderNoThrow( orders[i], - remainingWethSellAmount, + safeAdd(remainingWethSellAmount, 1), signatures[i] ); @@ -289,14 +340,14 @@ contract MixinForwarderCore is zrxPurchased = safeSub(totalFillResults.makerAssetFilledAmount, totalFillResults.takerFeePaid); // Stop execution if the entire amount of ZRX has been bought. - if (zrxPurchased == zrxBuyAmount) { + if (zrxPurchased >= zrxBuyAmount) { break; } } // Ensure that all ZRX spent while filling primary orders has been repurchased. require( - zrxPurchased == zrxBuyAmount, + zrxPurchased >= zrxBuyAmount, "COMPLETE_FILL_FAILED" ); } diff --git a/packages/contracts/src/2.0.0/forwarder/MixinWeth.sol b/packages/contracts/src/2.0.0/forwarder/MixinWeth.sol index 44cd9cdf9..b566c3ef6 100644 --- a/packages/contracts/src/2.0.0/forwarder/MixinWeth.sol +++ b/packages/contracts/src/2.0.0/forwarder/MixinWeth.sol @@ -29,10 +29,6 @@ contract MixinWeth is MWeth { - uint256 constant internal PERCENTAGE_DENOMINATOR = 10**18; - uint256 constant internal MAX_FEE_PERCENTAGE = 5 * PERCENTAGE_DENOMINATOR / 100; // 5% - uint256 constant internal MAX_WETH_FILL_PERCENTAGE = 95 * PERCENTAGE_DENOMINATOR / 100; // 95% - /// @dev Default payabale function, this allows us to withdraw WETH function () public @@ -45,23 +41,14 @@ contract MixinWeth is } /// @dev Converts message call's ETH value into WETH. - /// @return 95% of ETH converted to WETH. function convertEthToWeth() internal - returns (uint256 wethAvailable) { require( msg.value > 0, "INVALID_MSG_VALUE" ); - ETHER_TOKEN.deposit.value(msg.value)(); - wethAvailable = getPartialAmount( - MAX_WETH_FILL_PERCENTAGE, - PERCENTAGE_DENOMINATOR, - msg.value - ); - return wethAvailable; } /// @dev Transfers feePercentage of WETH spent on primary orders to feeRecipient. diff --git a/packages/contracts/src/2.0.0/forwarder/mixins/MConstants.sol b/packages/contracts/src/2.0.0/forwarder/mixins/MConstants.sol index bbc4969d6..712a11c5d 100644 --- a/packages/contracts/src/2.0.0/forwarder/mixins/MConstants.sol +++ b/packages/contracts/src/2.0.0/forwarder/mixins/MConstants.sol @@ -28,6 +28,9 @@ contract MConstants { bytes4 constant internal ERC20_DATA_ID = bytes4(keccak256("ERC20Token(address)")); bytes4 constant internal ERC721_DATA_ID = bytes4(keccak256("ERC721Token(address,uint256,bytes)")); uint256 constant internal MAX_UINT = 2**256 - 1; + uint256 constant internal PERCENTAGE_DENOMINATOR = 10**18; + uint256 constant internal MAX_FEE_PERCENTAGE = 5 * PERCENTAGE_DENOMINATOR / 100; // 5% + uint256 constant internal MAX_WETH_FILL_PERCENTAGE = 95 * PERCENTAGE_DENOMINATOR / 100; // 95% // solhint-disable var-name-mixedcase IExchange internal EXCHANGE; diff --git a/packages/contracts/src/2.0.0/forwarder/mixins/MWeth.sol b/packages/contracts/src/2.0.0/forwarder/mixins/MWeth.sol index 7cbc115e9..88e77be4e 100644 --- a/packages/contracts/src/2.0.0/forwarder/mixins/MWeth.sol +++ b/packages/contracts/src/2.0.0/forwarder/mixins/MWeth.sol @@ -22,10 +22,8 @@ pragma solidity 0.4.24; contract MWeth { /// @dev Converts message call's ETH value into WETH. - /// @return 95% of ETH converted to WETH. function convertEthToWeth() - internal - returns (uint256 wethAvailable); + internal; /// @dev Transfers feePercentage of WETH spent on primary orders to feeRecipient. /// Refunds any excess ETH to msg.sender. -- cgit v1.2.3