Fix rounding error issues, use different logic when makerAsset is ZRX

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.