Reordered fund transfers for matched orders, plus added an extra sanity check to order matching calculations

3 files changed, 49 insertions, 35 deletions

diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol
index 77ba0a089..ab4768d02 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol
@@ -84,8 +84,8 @@ contract MixinMatchOrders is
         validateMatchOrThrow(leftOrder, rightOrder);
 
         // Compute proportional fill amounts
-        uint8 matchedFillAmountsStatus;
-        (   matchedFillAmountsStatus,
+        uint8 matchedFillResultsStatus;
+        (   matchedFillResultsStatus,
             matchedFillResults
         ) = calculateMatchedFillResults(
             leftOrder,
@@ -94,7 +94,7 @@ contract MixinMatchOrders is
             rightOrderInfo.orderStatus,
             leftOrderInfo.orderFilledAmount,
             rightOrderInfo.orderFilledAmount);
-        if (matchedFillAmountsStatus != uint8(Status.SUCCESS)) {
+        if (matchedFillResultsStatus != uint8(Status.SUCCESS)) {
             return matchedFillResults;
         }
 
@@ -181,6 +181,27 @@ contract MixinMatchOrders is
     function validateMatchOrThrow(MatchedFillResults memory matchedFillResults)
         internal
     {
+        // The left order must spend at least as much as we're sending to the combined
+        // amounts being sent to the right order and taker
+        uint256 amountSpentByLeft = safeAdd(
+            matchedFillResults.right.takerAssetFilledAmount,
+            matchedFillResults.takerFillAmount
+        );
+        require(
+            matchedFillResults.left.makerAssetFilledAmount >=
+            amountSpentByLeft,
+            MISCALCULATED_TRANSFER_AMOUNTS
+        );
+        // If the amount transferred from the left order is different than what is transferred, it is a rounding error amount.
+        // Ensure this difference is negligible by dividing the values with each other. The result should equal to ~1.
+        require(
+            !isRoundingError(
+                matchedFillResults.left.makerAssetFilledAmount,
+                amountSpentByLeft,
+                1),
+            ROUNDING_ERROR_TRANSFER_AMOUNTS
+        );
+
         // The right order must spend at least as much as we're transferring to the left order.
         require(
             matchedFillResults.right.makerAssetFilledAmount >=
@@ -283,6 +304,12 @@ contract MixinMatchOrders is
             return (status, matchedFillResults);
         }
 
+        // Calculate amount given to taker
+        matchedFillResults.takerFillAmount = safeSub(
+            matchedFillResults.left.makerAssetFilledAmount,
+            matchedFillResults.right.takerAssetFilledAmount
+        );
+
         // Validate the fill results
         validateMatchOrThrow(matchedFillResults);
 
diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSettlement.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSettlement.sol
index cb96bbc3e..4911c62b5 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSettlement.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSettlement.sol
@@ -105,57 +105,41 @@ contract MixinSettlement is
         address takerAddress)
         internal
     {
-        // Optimized for:
-        // * leftOrder.feeRecipient =?= rightOrder.feeRecipient
-
-        // Not optimized for:
-        // * {left, right}.{MakerAsset, TakerAsset} == ZRX
-        // * {left, right}.maker, takerAddress == {left, right}.feeRecipient
-
-        // leftOrder.MakerAsset == rightOrder.TakerAsset
-        // Taker should be left with a positive balance (the spread)
+        // Order makers and taker
         dispatchTransferFrom(
             leftOrder.makerAssetData,
             leftOrder.makerAddress,
-            takerAddress,
-            matchedFillResults.left.makerAssetFilledAmount);
-        dispatchTransferFrom(
-            leftOrder.makerAssetData,
-            takerAddress,
             rightOrder.makerAddress,
-            matchedFillResults.right.takerAssetFilledAmount);
-
-        // rightOrder.MakerAsset == leftOrder.TakerAsset
-        // leftOrder.takerAssetFilledAmount ~ rightOrder.makerAssetFilledAmount
-        // The change goes to right, not to taker.
-        require(
-            matchedFillResults.right.makerAssetFilledAmount >=
-            matchedFillResults.left.takerAssetFilledAmount,
-            MISCALCULATED_TRANSFER_AMOUNTS
+            matchedFillResults.right.takerAssetFilledAmount
         );
         dispatchTransferFrom(
             rightOrder.makerAssetData,
             rightOrder.makerAddress,
             leftOrder.makerAddress,
-            matchedFillResults.right.makerAssetFilledAmount);
+            matchedFillResults.left.takerAssetFilledAmount
+        );
+        dispatchTransferFrom(
+            leftOrder.makerAssetData,
+            leftOrder.makerAddress,
+            takerAddress,
+            matchedFillResults.takerFillAmount
+        );
 
         // Maker fees
         dispatchTransferFrom(
             ZRX_PROXY_DATA,
             leftOrder.makerAddress,
             leftOrder.feeRecipientAddress,
-            matchedFillResults.left.makerFeePaid);
+            matchedFillResults.left.makerFeePaid
+        );
         dispatchTransferFrom(
             ZRX_PROXY_DATA,
             rightOrder.makerAddress,
             rightOrder.feeRecipientAddress,
-            matchedFillResults.right.makerFeePaid);
+            matchedFillResults.right.makerFeePaid
+        );
 
         // Taker fees
-        // If we assume distinct(left, right, takerAddress) and
-        // distinct(MakerAsset, TakerAsset, zrx) then the only remaining
-        // opportunity for optimization is when both feeRecipientAddress' are
-        // the same.
         if (leftOrder.feeRecipientAddress == rightOrder.feeRecipientAddress) {
             dispatchTransferFrom(
                 ZRX_PROXY_DATA,
@@ -171,12 +155,14 @@ contract MixinSettlement is
                 ZRX_PROXY_DATA,
                 takerAddress,
                 leftOrder.feeRecipientAddress,
-                matchedFillResults.left.takerFeePaid);
+                matchedFillResults.left.takerFeePaid
+            );
             dispatchTransferFrom(
                 ZRX_PROXY_DATA,
                 takerAddress,
                 rightOrder.feeRecipientAddress,
-                matchedFillResults.right.takerFeePaid);
+                matchedFillResults.right.takerFeePaid
+            );
         }
     }
 }
diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MMatchOrders.sol b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MMatchOrders.sol
index a1e3f28a1..17b6cd92d 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MMatchOrders.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MMatchOrders.sol
@@ -27,6 +27,7 @@ contract MMatchOrders {
     struct MatchedFillResults {
         LibFillResults.FillResults left;
         LibFillResults.FillResults right;
+        uint256 takerFillAmount;
     }
 
     /// This struct exists solely to avoid the stack limit constraint