slightly restructure contracts to fix bug in the WETH-fee case

contracts/exchange-forwarder/contracts/src/MixinExchangeWrapper.sol

@@ -89,11 +89,13 @@ contract MixinExchangeWrapper is
         bytes[] memory signatures
     )
         internal
-        returns (FillResults memory totalFillResults)
+        returns (
+            FillResults memory totalFillResults,
+            uint256 wethSpentAmount,
+            uint256 makerAssetAcquiredAmount
+        )
     {
         uint256 ordersLength = orders.length;
-        // The remaining amount of WETH to sell
-        uint256 remainingTakerAssetFillAmount = wethSellAmount;
 
         for (uint256 i = 0; i != ordersLength; i++) {
             require(
@@ -101,34 +103,62 @@ contract MixinExchangeWrapper is
                 "MAKER_ASSET_MISMATCH"
             );
 
+            // The remaining amount of WETH to sell
+            uint256 remainingTakerAssetFillAmount = _safeSub(
+                wethSellAmount,
+                wethSpentAmount
+            );
+            FillResults memory singleFillResults;
+
+            // Update amounts filled and the remaining amount of WETH to sell
+            if (orders[i].makerAssetData.equals(orders[i].takerFeeAssetData)) {
                 // Attempt to sell the remaining amount of WETH
-            FillResults memory singleFillResults = _fillOrderNoThrow(
+                singleFillResults = _fillOrderNoThrow(
                     orders[i],
                     remainingTakerAssetFillAmount,
                     signatures[i]
                 );
 
-            // Update amounts filled and the remaining amount of WETH to sell
+                makerAssetAcquiredAmount = _safeAdd(
-            if (orders[i].makerAssetData.equals(orders[i].takerFeeAssetData)) {
+                    makerAssetAcquiredAmount,
-                _addFillResultsDeductFees(totalFillResults, singleFillResults);
+                    _safeSub(singleFillResults.makerAssetFilledAmount, singleFillResults.takerFeePaid)
-            } else {
-                _addFillResults(totalFillResults, singleFillResults);
-                remainingTakerAssetFillAmount = _safeSub(
-                    remainingTakerAssetFillAmount,
-                    singleFillResults.takerFeePaid
                 );
-            }
+                wethSpentAmount = _safeAdd(
-            remainingTakerAssetFillAmount = _safeSub(
+                    wethSpentAmount,
-                remainingTakerAssetFillAmount,
                     singleFillResults.takerAssetFilledAmount
                 );
+            } else {
+                // We first sell WETH as the takerAsset, then use it to pay the takerFee.
+                // This ensures that we reserve enough to pay the fee.
+                uint256 takerAssetFillAmount = _getPartialAmountCeil(
+                    orders[i].takerAssetAmount,
+                    _safeAdd(orders[i].takerAssetAmount, orders[i].takerFee),
+                    remainingTakerAssetFillAmount
+                );
+                singleFillResults = _fillOrderNoThrow(
+                    orders[i],
+                    takerAssetFillAmount,
+                    signatures[i]
+                );
+
+                wethSpentAmount = _safeAdd(
+                    wethSpentAmount,
+                    _safeAdd(singleFillResults.takerAssetFilledAmount, singleFillResults.takerFeePaid)
+                );
+                makerAssetAcquiredAmount = _safeAdd(
+                    makerAssetAcquiredAmount,
+                    singleFillResults.makerAssetFilledAmount
+                );
+            }
+
+            _addFillResults(totalFillResults, singleFillResults);
 
             // Stop execution if the entire amount of takerAsset has been sold
-            if (totalFillResults.takerAssetFilledAmount >= wethSellAmount) {
+            if (wethSpentAmount >= wethSellAmount) {
                 break;
             }
         }
-        return totalFillResults;
+        return (totalFillResults, wethSpentAmount, makerAssetAcquiredAmount);
     }
 
     /// @dev Synchronously executes multiple fill orders in a single transaction until total amount is bought by taker.
@@ -144,10 +174,13 @@ contract MixinExchangeWrapper is
         bytes[] memory signatures
     )
         internal
-        returns (FillResults memory totalFillResults)
+        returns (
+            FillResults memory totalFillResults,
+            uint256 wethSpentAmount,
+            uint256 makerAssetAcquiredAmount
+        )
     {
         uint256 ordersLength = orders.length;
-        uint256 makerAssetFilledAmount = 0;
 
         for (uint256 i = 0; i != ordersLength; i++) {
             require(
@@ -156,7 +189,7 @@ contract MixinExchangeWrapper is
             );
 
             // Calculate the remaining amount of makerAsset to buy
-            uint256 remainingMakerAssetFillAmount = _safeSub(makerAssetFillAmount, totalFillResults.makerAssetFilledAmount);
+            uint256 remainingMakerAssetFillAmount = _safeSub(makerAssetFillAmount, makerAssetAcquiredAmount);
 
             // Convert the remaining amount of makerAsset to buy into remaining amount
             // of takerAsset to sell, assuming entire amount can be sold in the current order.
@@ -177,22 +210,36 @@ contract MixinExchangeWrapper is
 
             // Update amounts filled and fees paid by maker and taker
             if (orders[i].makerAssetData.equals(orders[i].takerFeeAssetData)) {
-                _addFillResultsDeductFees(totalFillResults, singleFillResults);
+                makerAssetAcquiredAmount = _safeAdd(
+                    makerAssetAcquiredAmount,
+                    _safeSub(singleFillResults.makerAssetFilledAmount, singleFillResults.takerFeePaid)
+                );
+                wethSpentAmount = _safeAdd(
+                    wethSpentAmount,
+                    singleFillResults.takerAssetFilledAmount
+                );
             } else {
-                _addFillResults(totalFillResults, singleFillResults);
+                makerAssetAcquiredAmount = _safeAdd(
+                    makerAssetAcquiredAmount,
+                    _safeSub(singleFillResults.makerAssetFilledAmount, singleFillResults.takerFeePaid)
+                );
+                wethSpentAmount = _safeAdd(
+                    wethSpentAmount,
+                    singleFillResults.takerAssetFilledAmount
+                );
             }
+            _addFillResults(totalFillResults, singleFillResults);
 
             // Stop execution if the entire amount of makerAsset has been bought
-            makerAssetFilledAmount = totalFillResults.makerAssetFilledAmount;
+            if (makerAssetAcquiredAmount >= makerAssetFillAmount) {
-            if (makerAssetFilledAmount >= makerAssetFillAmount) {
                 break;
             }
         }
 
         require(
-            makerAssetFilledAmount >= makerAssetFillAmount,
+            makerAssetAcquiredAmount >= makerAssetFillAmount,
             "COMPLETE_FILL_FAILED"
         );
-        return totalFillResults;
+        return (totalFillResults, wethSpentAmount, makerAssetAcquiredAmount);
     }
 }

contracts/exchange-forwarder/contracts/src/MixinForwarderCore.sol

@@ -88,7 +88,13 @@ contract MixinForwarderCore is
 
         // Market sell 95% of WETH.
         // ZRX fees are payed with this contract's balance.
-        orderFillResults = _marketSellWeth(
+        uint256 wethSpentAmount;
+        uint256 makerAssetAcquiredAmount;
+        (
+            orderFillResults,
+            wethSpentAmount,
+            makerAssetAcquiredAmount
+        ) = _marketSellWeth(
             orders,
             wethSellAmount,
             signatures
@@ -97,7 +103,7 @@ contract MixinForwarderCore is
         // Transfer feePercentage of total ETH spent on primary orders to feeRecipient.
         // Refund remaining ETH to msg.sender.
         _transferEthFeeAndRefund(
-            orderFillResults.takerAssetFilledAmount,
+            wethSpentAmount,
             feePercentage,
             feeRecipient
         );
@@ -105,7 +111,7 @@ contract MixinForwarderCore is
         // Transfer purchased assets to msg.sender.
         _transferAssetToSender(
             orders[0].makerAssetData,
-            orderFillResults.makerAssetFilledAmount
+            makerAssetAcquiredAmount
         );
     }
 
@@ -136,7 +142,13 @@ contract MixinForwarderCore is
 
         // Attempt to purchase desired amount of makerAsset.
         // ZRX fees are payed with this contract's balance.
-        orderFillResults = _marketBuyExactAmountWithWeth(
+        uint256 wethSpentAmount;
+        uint256 makerAssetAcquiredAmount;
+        (
+            orderFillResults,
+            wethSpentAmount,
+            makerAssetAcquiredAmount
+        ) = _marketBuyExactAmountWithWeth(
             orders,
             makerAssetFillAmount,
             signatures
@@ -145,7 +157,7 @@ contract MixinForwarderCore is
         // Transfer feePercentage of total ETH spent on primary orders to feeRecipient.
         // Refund remaining ETH to msg.sender.
         _transferEthFeeAndRefund(
-            orderFillResults.takerAssetFilledAmount,
+            wethSpentAmount,
             feePercentage,
             feeRecipient
         );
@@ -153,7 +165,7 @@ contract MixinForwarderCore is
         // Transfer purchased assets to msg.sender.
         _transferAssetToSender(
             orders[0].makerAssetData,
-            orderFillResults.makerAssetFilledAmount
+            makerAssetAcquiredAmount
         );
     }
 }

contracts/exchange-libs/contracts/src/LibFillResults.sol

@@ -402,19 +402,4 @@ 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
-    {
-        totalFillResults.makerAssetFilledAmount = _safeAdd(totalFillResults.makerAssetFilledAmount, singleFillResults.makerAssetFilledAmount);
-        totalFillResults.takerAssetFilledAmount = _safeAdd(totalFillResults.takerAssetFilledAmount, singleFillResults.takerAssetFilledAmount);
-
-        // The fee amount must be deducted from the amount transfered back to sender.
-        totalFillResults.makerAssetFilledAmount = _safeSub(totalFillResults.makerAssetFilledAmount, singleFillResults.takerFeePaid);
-    }
 }