Fixed the bug and added tests that fail without the patch

contracts/dev-utils/contracts/src/OrderTransferSimulationUtils.sol

@@ -31,8 +31,6 @@ import "@0x/contracts-utils/contracts/src/LibBytes.sol";
 contract OrderTransferSimulationUtils is
     LibExchangeRichErrorDecoder
 {
-    bool second;
-
     using LibBytes for bytes;
 
     enum OrderTransferResults {
@@ -69,12 +67,11 @@ contract OrderTransferSimulationUtils is
         public
         returns (OrderTransferResults orderTransferResults)
     {
-        LibFillResults.FillResults memory fillResults = LibFillResults.calculateFillResultsTrace(
+        LibFillResults.FillResults memory fillResults = LibFillResults.calculateFillResults(
             order,
             takerAssetFillAmount,
             _EXCHANGE.protocolFeeMultiplier(),
-            tx.gasprice,
+            tx.gasprice
-            second
         );
 
         bytes[] memory assetData = new bytes[](2);

contracts/dev-utils/contracts/src/OrderValidationUtils.sol

@@ -16,7 +16,7 @@
 
 */
 
-pragma solidity ^0.5.5;
+pragma solidity ^0.5.9;
 pragma experimental ABIEncoderV2;
 
 import "@0x/contracts-exchange/contracts/src/interfaces/IExchange.sol";
@@ -127,6 +127,11 @@ contract OrderValidationUtils is
             fillableTakerAssetAmount
         ) == OrderTransferResults.TransfersSuccessful ? fillableTakerAssetAmount : 0;
 
+        /*
+        // Perform taker asset validation
+        fillableTakerAssetAmount = _isTakerAssetDataValid(order) ? fillableTakerAssetAmount : 0;
+        */
+
         return (orderInfo, fillableTakerAssetAmount, isValidSignature);
     }
 
@@ -154,10 +159,6 @@ contract OrderValidationUtils is
         isValidSignature = new bool[](length);
 
         for (uint256 i = 0; i != length; i++) {
-            if (i == 1) {
-                second = true;
-            }
-
             (ordersInfo[i], fillableTakerAssetAmounts[i], isValidSignature[i]) = getOrderRelevantState(
                 orders[i],
                 signatures[i]
@@ -184,4 +185,49 @@ contract OrderValidationUtils is
         transferableAssetAmount = LibSafeMath.min256(balance, allowance);
         return transferableAssetAmount;
     }
+
+    /// @dev This function handles the edge cases around taker validation. This function
+    ///      currently attempts to find duplicate ERC721 token's in the taker
+    ///      multiAssetData.
+    /// @param order The order that should be validated.
+    /// @return Whether or not the order should be considered valid.
+    function _isTakerAssetDataValid(LibOrder.Order memory order)
+        internal
+        pure
+        returns (bool)
+    {
+        bytes memory takerAssetData = order.takerAssetData;
+
+        // Only process the taker asset data if it is multiAssetData.
+        bytes4 assetProxyId = takerAssetData.readBytes4(0);
+        if (assetProxyId != IAssetData(address(0)).MultiAsset.selector) {
+            return true;
+        }
+
+        // Get array of values and array of assetDatas
+        (, uint256[] memory assetAmounts, bytes[] memory nestedAssetData) = decodeMultiAssetData(takerAssetData);
+
+        uint256 length = nestedAssetData.length;
+        for (uint256 i = 0; i != length; i++) {
+            // NOTE(jalextowle): As an optimization, we will break out of this function
+            // as soon as it is determined that there are no possible ERC721 duplicates.
+            bool hasSeenERC721 = false;
+
+            bytes4 nestedAssetProxyId = nestedAssetData[i].readBytes4(0);
+            if (nestedAssetProxyId == IAssetData(address(0)).ERC721Token.selector) {
+                hasSeenERC721 = true;
+                for (uint256 j = i; j != length; j++) {
+                    if (nestedAssetData[i].equals(nestedAssetData[j])) {
+                        return false;
+                    }
+                }
+            }
+
+            if (!hasSeenERC721) {
+                break;
+            }
+        }
+
+        return true;
+    }
 }

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

@@ -49,51 +49,6 @@ library LibFillResults {
         uint256 profitInRightMakerAsset; // Profit taken from the right maker
     }
 
-    event TestEvent(string file_name, string value_name, uint256 value);
-
-    /// FIXME: Remove this function after investigation
-    /// @dev Calculates amounts filled and fees paid by maker and taker.
-    /// @param order to be filled.
-    /// @param takerAssetFilledAmount Amount of takerAsset that will be filled.
-    /// @param protocolFeeMultiplier The current protocol fee of the exchange contract.
-    /// @param gasPrice The gasprice of the transaction. This is provided so that the function call can continue
-    ///        to be pure rather than view.
-    /// @return fillResults Amounts filled and fees paid by maker and taker.
-    function calculateFillResultsTrace(
-        LibOrder.Order memory order,
-        uint256 takerAssetFilledAmount,
-        uint256 protocolFeeMultiplier,
-        uint256 gasPrice,
-        bool shouldTrace
-    )
-        internal
-        pure
-        returns (FillResults memory fillResults)
-    {
-        // Compute proportional transfer amounts
-        fillResults.takerAssetFilledAmount = takerAssetFilledAmount;
-        fillResults.makerAssetFilledAmount = LibMath.safeGetPartialAmountFloor(
-            takerAssetFilledAmount,
-            order.takerAssetAmount,
-            order.makerAssetAmount
-        );
-        fillResults.makerFeePaid = LibMath.safeGetPartialAmountFloor(
-            takerAssetFilledAmount,
-            order.takerAssetAmount,
-            order.makerFee
-        );
-        fillResults.takerFeePaid = LibMath.safeGetPartialAmountFloor(
-            takerAssetFilledAmount,
-            order.takerAssetAmount,
-            order.takerFee
-        );
-
-        // Compute the protocol fee that should be paid for a single fill.
-        fillResults.protocolFeePaid = gasPrice.safeMul(protocolFeeMultiplier);
-
-        return fillResults;
-    }
-
     /// @dev Calculates amounts filled and fees paid by maker and taker.
     /// @param order to be filled.
     /// @param takerAssetFilledAmount Amount of takerAsset that will be filled.

contracts/integrations/test/dev-utils/order_validation_utils.ts

@@ -22,6 +22,7 @@ blockchainTests.resets.only('OrderValidationUtils/OrderTransferSimulatorUtils',
     let feeErc20Token: DummyERC20TokenContract;
     let erc20Proxy: ERC20ProxyContract;
     let exchange: ExchangeContract;
+    let deployment: DeploymentManager;
 
     let erc20AssetData: string;
     let erc20AssetData2: string;
@@ -33,7 +34,7 @@ blockchainTests.resets.only('OrderValidationUtils/OrderTransferSimulatorUtils',
     before(async () => {
         [takerAddress, owner] = await env.getAccountAddressesAsync();
 
-        const deployment = await DeploymentManager.deployAsync(env, {
+        deployment = await DeploymentManager.deployAsync(env, {
             numErc20TokensToDeploy: 3,
             numErc721TokensToDeploy: 1,
             owner,
@@ -60,9 +61,6 @@ blockchainTests.resets.only('OrderValidationUtils/OrderTransferSimulatorUtils',
                 feeRecipientAddress: constants.NULL_ADDRESS,
             },
         });
-        /*
-        await Promise.all(deployment.tokens.erc20.map(async token => maker.configureERC20TokenAsync(token)));
-        */
         const [tokenID] = await maker.configureERC721TokenAsync(deployment.tokens.erc721[0]);
         erc721AssetData = assetDataUtils.encodeERC721AssetData(deployment.tokens.erc721[0].address, tokenID);
     });
@@ -165,11 +163,35 @@ blockchainTests.resets.only('OrderValidationUtils/OrderTransferSimulatorUtils',
                 .callAsync();
             expect(fillableTakerAssetAmount).to.bignumber.equal(constants.ZERO_AMOUNT);
         });
-        it('should return a fillableTakerAssetAmount of 0 when an erc721 asset is duplicated in a multi-asset proxy order', async () => {
+        it('should return a fillableTakerAssetAmount of 0 when an erc721 asset is duplicated in the maker side of a multi-asset proxy order', async () => {
             const multiAssetData = await devUtils
                 .encodeMultiAssetData([new BigNumber(1), new BigNumber(1)], [erc721AssetData, erc721AssetData])
                 .callAsync();
-            signedOrder = await maker.signOrderAsync({ makerAssetData: multiAssetData });
+            signedOrder = await maker.signOrderAsync({
+                makerAssetData: multiAssetData,
+                makerAssetAmount: new BigNumber(1),
+                takerAssetData: erc721AssetData,
+                takerAssetAmount: new BigNumber(1),
+                makerFee: constants.ZERO_AMOUNT,
+                takerFee: constants.ZERO_AMOUNT,
+            });
+            const [, fillableTakerAssetAmount] = await devUtils
+                .getOrderRelevantState(signedOrder, signedOrder.signature)
+                .callAsync();
+            expect(fillableTakerAssetAmount).to.bignumber.equal(constants.ZERO_AMOUNT);
+        });
+        it('should return a fillableTakerAssetAmount of 0 when an erc721 asset is duplicated in the taker side of a multi-asset proxy order', async () => {
+            const multiAssetData = await devUtils
+                .encodeMultiAssetData([new BigNumber(1), new BigNumber(1)], [erc721AssetData, erc721AssetData])
+                .callAsync();
+            signedOrder = await maker.signOrderAsync({
+                makerAssetData: erc721AssetData,
+                makerAssetAmount: new BigNumber(1),
+                takerAssetData: multiAssetData,
+                takerAssetAmount: new BigNumber(1),
+                makerFee: constants.ZERO_AMOUNT,
+                takerFee: constants.ZERO_AMOUNT,
+            });
             const [, fillableTakerAssetAmount] = await devUtils
                 .getOrderRelevantState(signedOrder, signedOrder.signature)
                 .callAsync();