Update transferEthFeeAndRefund, add check to ERC721 transfer

6 files changed, 54 insertions, 26 deletions

diff --git a/packages/contracts/src/2.0.0/forwarder/MixinErrorMessages.sol b/packages/contracts/src/2.0.0/forwarder/LibForwarderErrors.sol
index 91758eadc..69108738a 100644
--- a/packages/contracts/src/2.0.0/forwarder/MixinErrorMessages.sol
+++ b/packages/contracts/src/2.0.0/forwarder/LibForwarderErrors.sol
@@ -21,7 +21,7 @@ pragma solidity 0.4.24;
 
 
 /// This contract is intended to serve as a reference, but is not actually used for efficiency reasons.
-contract MixinErrorMessages {
+contract LibForwarderErrors {
     string constant FEE_PERCENTAGE_TOO_LARGE = "FEE_PROPORTION_TOO_LARGE";                        // Provided fee percentage greater than 5%.
     string constant MAX_FEE_EXCEEDED = "MAX_FEE_EXCEEDED";                                        // Not enough ETH remaining to pay feeRecipient.
     string constant OVERSOLD_WETH = "OVERSOLD_WETH";                                              // More WETH sold than provided with current message call.
@@ -30,4 +30,5 @@ contract MixinErrorMessages {
     string constant UNSUPPORTED_TOKEN_PROXY = "UNSUPPORTED_TOKEN_PROXY";                          // Proxy in assetData not supported.
     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.
 }
diff --git a/packages/contracts/src/2.0.0/forwarder/MixinAssets.sol b/packages/contracts/src/2.0.0/forwarder/MixinAssets.sol
index 325af5518..cd150764e 100644
--- a/packages/contracts/src/2.0.0/forwarder/MixinAssets.sol
+++ b/packages/contracts/src/2.0.0/forwarder/MixinAssets.sol
@@ -95,7 +95,7 @@ contract MixinAssets is
         if (proxyId == ERC20_DATA_ID) {
             transferERC20Token(assetData, amount);
         } else if (proxyId == ERC721_DATA_ID) {
-            transferERC721Token(assetData);
+            transferERC721Token(assetData, amount);
         } else {
             revert("UNSUPPORTED_TOKEN_PROXY");
         }
@@ -149,9 +149,17 @@ contract MixinAssets is
 
     /// @dev Decodes ERC721 assetData and transfers given amount to sender.
     /// @param assetData Byte array encoded for the respective asset proxy.
-    function transferERC721Token(bytes memory assetData)
+    /// @param amount Amount of asset to transfer to sender.
+    function transferERC721Token(
+        bytes memory assetData,
+        uint256 amount
+    )
         internal
     {
+        require(
+            amount == 1,
+            "INVALID_AMOUNT"
+        );
         // Decode asset data.
         address token = assetData.readAddress(16);
         uint256 tokenId = assetData.readUint256(36);
diff --git a/packages/contracts/src/2.0.0/forwarder/MixinForwarderCore.sol b/packages/contracts/src/2.0.0/forwarder/MixinForwarderCore.sol
index 1a0687ab5..88ec5f472 100644
--- a/packages/contracts/src/2.0.0/forwarder/MixinForwarderCore.sol
+++ b/packages/contracts/src/2.0.0/forwarder/MixinForwarderCore.sol
@@ -80,7 +80,7 @@ contract MixinForwarderCore is
 
         // Attempt to sell 95% of WETH.
         // ZRX fees are payed with this contract's balance.
-        marketSellEth(
+        orderFillResults = marketSellEth(
             orders,
             wethAvailable,
             signatures
@@ -258,6 +258,7 @@ contract MixinForwarderCore is
 
             bytes memory zrxAssetData = ZRX_ASSET_DATA;
             bytes memory wethAssetData = WETH_ASSET_DATA;
+            uint256 zrxPurchased = 0;
 
             for (uint256 i = 0; i < orders.length; i++) {
 
@@ -266,10 +267,7 @@ contract MixinForwarderCore is
                 orders[i].takerAssetData = wethAssetData;
 
                 // Calculate the remaining amount of ZRX to buy.
-                uint256 remainingZrxBuyAmount = safeAdd(
-                    safeSub(zrxBuyAmount, totalFillResults.makerAssetFilledAmount),
-                    totalFillResults.takerFeePaid
-                );
+                uint256 remainingZrxBuyAmount = safeSub(zrxBuyAmount, zrxPurchased);
     
                 // Convert the remaining amount of ZRX to buy into remaining amount
                 // of WETH to sell, assuming entire amount can be sold in the current order.
@@ -288,16 +286,17 @@ contract MixinForwarderCore is
 
                 // Update amounts filled and fees paid by maker and taker.
                 addFillResults(totalFillResults, singleFillResult);
+                zrxPurchased = safeSub(totalFillResults.makerAssetFilledAmount, totalFillResults.takerFeePaid);
 
                 // Stop execution if the entire amount of ZRX has been bought.
-                if (totalFillResults.makerAssetFilledAmount == zrxBuyAmount) {
+                if (zrxPurchased == zrxBuyAmount) {
                     break;
                 }
             }
 
             // Ensure that all ZRX spent while filling primary orders has been repurchased.
             require(
-                totalFillResults.makerAssetFilledAmount == 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 cbc81a11c..1d0c315f5 100644
--- a/packages/contracts/src/2.0.0/forwarder/MixinWeth.sol
+++ b/packages/contracts/src/2.0.0/forwarder/MixinWeth.sol
@@ -78,32 +78,46 @@ contract MixinWeth is
     )
         internal
     {
+        // Ensure feePercentage is less than 5%.
+        require(
+            feePercentage <= MAX_FEE_PERCENTAGE,
+            "FEE_PERCENTAGE_TOO_LARGE"
+        );
+
+        // Calculate amount of WETH that hasn't been sold.
         uint256 wethRemaining = safeSub(
             msg.value,
             safeAdd(wethSoldExcludingFeeOrders, wethSoldForZrx)
         );
-        ETHER_TOKEN.withdraw(wethRemaining);
 
-        require(
-            feePercentage <= MAX_FEE_PERCENTAGE,
-            "FEE_PERCENTAGE_TOO_LARGE"
-        );
+        // Calculate ETH fee to pay to feeRecipient.
         uint256 ethFee = getPartialAmount(
             feePercentage,
             PERCENTAGE_DENOMINATOR,
             wethSoldExcludingFeeOrders
         );
+
+        // Ensure fee is less than amount of WETH remaining.
         require(
-            ethFee < wethRemaining,
+            ethFee <= wethRemaining,
             "MAX_FEE_EXCEEDED"
         );
-        if (ethFee > 0) {
-            feeRecipient.transfer(ethFee);
-        }
-
-        uint256 ethRefund = safeSub(wethRemaining, ethFee);
-        if (ethRefund > 0) {
-            msg.sender.transfer(ethRefund);
+    
+        // Do nothing if no WETH remaining
+        if (wethRemaining > 0) {
+            // Convert remaining WETH to ETH
+            ETHER_TOKEN.withdraw(wethRemaining);
+
+            // Pay ETH to feeRecipient
+            if (ethFee > 0) {
+                feeRecipient.transfer(ethFee);
+            }
+
+            // Refund remaining ETH to msg.sender.
+            uint256 ethRefund = safeSub(wethRemaining, ethFee);
+            if (ethRefund > 0) {
+                msg.sender.transfer(ethRefund);
+            }
         }
     }
 }
diff --git a/packages/contracts/src/2.0.0/forwarder/mixins/MAssets.sol b/packages/contracts/src/2.0.0/forwarder/mixins/MAssets.sol
index b69f7482d..340ee0bcb 100644
--- a/packages/contracts/src/2.0.0/forwarder/mixins/MAssets.sol
+++ b/packages/contracts/src/2.0.0/forwarder/mixins/MAssets.sol
@@ -45,6 +45,10 @@ contract MAssets is
 
     /// @dev Decodes ERC721 assetData and transfers given amount to sender.
     /// @param assetData Byte array encoded for the respective asset proxy.
-    function transferERC721Token(bytes memory assetData)
+    /// @param amount Amount of asset to transfer to sender.
+    function transferERC721Token(
+        bytes memory assetData,
+        uint256 amount
+    )
         internal;
 }
diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/libs/LibMath.sol b/packages/contracts/src/2.0.0/protocol/Exchange/libs/LibMath.sol
index 46c13d390..fa09da6ac 100644
--- a/packages/contracts/src/2.0.0/protocol/Exchange/libs/LibMath.sol
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/libs/LibMath.sol
@@ -33,7 +33,8 @@ contract LibMath is
     function getPartialAmount(
         uint256 numerator,
         uint256 denominator,
-        uint256 target)
+        uint256 target
+    )
         internal
         pure
         returns (uint256 partialAmount)
@@ -53,7 +54,8 @@ contract LibMath is
     function isRoundingError(
         uint256 numerator,
         uint256 denominator,
-        uint256 target)
+        uint256 target
+    )
         internal
         pure
         returns (bool isError)