Only use one nonReentrant modifier, remove modifier from fillOrderNoThrow variations

6 files changed, 11 insertions, 61 deletions

diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol
index f02514fc6..291af3792 100644
--- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol
@@ -89,7 +89,7 @@ contract MixinExchangeCore is
         bytes memory signature
     )
         public
-        lockMutex
+        nonReentrant
         returns (FillResults memory fillResults)
     {
         fillResults = fillOrderInternal(
diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol
index 39c47e6f9..25220a673 100644
--- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol
@@ -50,7 +50,7 @@ contract MixinMatchOrders is
         bytes memory rightSignature
     )
         public
-        lockMutex
+        nonReentrant
         returns (LibFillResults.MatchedFillResults memory matchedFillResults)
     {
         // We assume that rightOrder.takerAssetData == leftOrder.makerAssetData and rightOrder.makerAssetData == leftOrder.takerAssetData.
diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinWrapperFunctions.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinWrapperFunctions.sol
index b0474b110..39fa724cc 100644
--- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinWrapperFunctions.sol
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinWrapperFunctions.sol
@@ -33,7 +33,8 @@ contract MixinWrapperFunctions is
     LibMath,
     LibFillResults,
     LibAbiEncoder,
-    MExchangeCore
+    MExchangeCore,
+    MWrapperFunctions
 {
 
     /// @dev Fills the input order. Reverts if exact takerAssetFillAmount not filled.
@@ -46,7 +47,7 @@ contract MixinWrapperFunctions is
         bytes memory signature
     )
         public
-        lockMutex
+        nonReentrant
         returns (FillResults memory fillResults)
     {
         fillResults = fillOrKillOrderInternal(
@@ -69,7 +70,6 @@ contract MixinWrapperFunctions is
         bytes memory signature
     )
         public
-        nonReentrant
         returns (FillResults memory fillResults)
     {
         // ABI encode calldata for `fillOrder`
@@ -111,7 +111,7 @@ contract MixinWrapperFunctions is
         bytes[] memory signatures
     )
         public
-        lockMutex
+        nonReentrant
         returns (FillResults memory totalFillResults)
     {
         uint256 ordersLength = orders.length;
@@ -138,7 +138,7 @@ contract MixinWrapperFunctions is
         bytes[] memory signatures
     )
         public
-        lockMutex
+        nonReentrant
         returns (FillResults memory totalFillResults)
     {
         uint256 ordersLength = orders.length;
@@ -166,7 +166,6 @@ contract MixinWrapperFunctions is
         bytes[] memory signatures
     )
         public
-        nonReentrant
         returns (FillResults memory totalFillResults)
     {
         uint256 ordersLength = orders.length;
@@ -192,7 +191,7 @@ contract MixinWrapperFunctions is
         bytes[] memory signatures
     )
         public
-        lockMutex
+        nonReentrant
         returns (FillResults memory totalFillResults)
     {
         bytes memory takerAssetData = orders[0].takerAssetData;
@@ -237,7 +236,6 @@ contract MixinWrapperFunctions is
         bytes[] memory signatures
     )
         public
-        nonReentrant
         returns (FillResults memory totalFillResults)
     {
         bytes memory takerAssetData = orders[0].takerAssetData;
@@ -281,7 +279,7 @@ contract MixinWrapperFunctions is
         bytes[] memory signatures
     )
         public
-        lockMutex
+        nonReentrant
         returns (FillResults memory totalFillResults)
     {
         bytes memory makerAssetData = orders[0].makerAssetData;
@@ -334,7 +332,6 @@ contract MixinWrapperFunctions is
         bytes[] memory signatures
     )
         public
-        nonReentrant
         returns (FillResults memory totalFillResults)
     {
         bytes memory makerAssetData = orders[0].makerAssetData;
diff --git a/packages/contracts/src/2.0.0/test/ReentrantERC20Token/ReentrantERC20Token.sol b/packages/contracts/src/2.0.0/test/ReentrantERC20Token/ReentrantERC20Token.sol
index 8d575d09a..8bfdd2e66 100644
--- a/packages/contracts/src/2.0.0/test/ReentrantERC20Token/ReentrantERC20Token.sol
+++ b/packages/contracts/src/2.0.0/test/ReentrantERC20Token/ReentrantERC20Token.sol
@@ -40,17 +40,14 @@ contract ReentrantERC20Token is
     );
 
     // All of these functions are potentially vulnerable to reentrancy
+    // We do not test any "noThrow" functions because `fillOrderNoThrow` makes a delegatecall to `fillOrder`
     enum ExchangeFunction {
         FILL_ORDER,
         FILL_OR_KILL_ORDER,
-        FILL_ORDER_NO_THROW,
         BATCH_FILL_ORDERS,
         BATCH_FILL_OR_KILL_ORDERS,
-        BATCH_FILL_ORDERS_NO_THROW,
         MARKET_BUY_ORDERS,
-        MARKET_BUY_ORDERS_NO_THROW,
         MARKET_SELL_ORDERS,
-        MARKET_SELL_ORDERS_NO_THROW,
         MATCH_ORDERS,
         CANCEL_ORDER,
         CANCEL_ORDERS_UP_TO,
@@ -111,13 +108,6 @@ contract ReentrantERC20Token is
                 0,
                 signature
             );
-        } else if (currentFunctionId == uint8(ExchangeFunction.FILL_ORDER_NO_THROW)) {
-            calldata = abi.encodeWithSelector(
-                EXCHANGE.fillOrderNoThrow.selector,
-                order,
-                0,
-                signature
-            );
         } else if (currentFunctionId == uint8(ExchangeFunction.BATCH_FILL_ORDERS)) {
             calldata = abi.encodeWithSelector(
                 EXCHANGE.batchFillOrders.selector,
@@ -132,13 +122,6 @@ contract ReentrantERC20Token is
                 takerAssetFillAmounts,
                 signatures
             );
-        } else if (currentFunctionId == uint8(ExchangeFunction.BATCH_FILL_ORDERS_NO_THROW)) {
-            calldata = abi.encodeWithSelector(
-                EXCHANGE.batchFillOrdersNoThrow.selector,
-                orders,
-                takerAssetFillAmounts,
-                signatures
-            );
         } else if (currentFunctionId == uint8(ExchangeFunction.MARKET_BUY_ORDERS)) {
             calldata = abi.encodeWithSelector(
                 EXCHANGE.marketBuyOrders.selector,
@@ -146,13 +129,6 @@ contract ReentrantERC20Token is
                 0,
                 signatures
             );
-        } else if (currentFunctionId == uint8(ExchangeFunction.MARKET_BUY_ORDERS_NO_THROW)) {
-            calldata = abi.encodeWithSelector(
-                EXCHANGE.marketBuyOrdersNoThrow.selector,
-                orders,
-                0,
-                signatures
-            );
         } else if (currentFunctionId == uint8(ExchangeFunction.MARKET_SELL_ORDERS)) {
             calldata = abi.encodeWithSelector(
                 EXCHANGE.marketSellOrders.selector,
@@ -160,13 +136,6 @@ contract ReentrantERC20Token is
                 0,
                 signatures
             );
-        } else if (currentFunctionId == uint8(ExchangeFunction.MARKET_SELL_ORDERS_NO_THROW)) {
-            calldata = abi.encodeWithSelector(
-                EXCHANGE.marketSellOrdersNoThrow.selector,
-                orders,
-                0,
-                signatures
-            );
         } else if (currentFunctionId == uint8(ExchangeFunction.MATCH_ORDERS)) {
             calldata = abi.encodeWithSelector(
                 EXCHANGE.matchOrders.selector,
diff --git a/packages/contracts/src/2.0.0/utils/ReentrancyGuard/ReentrancyGuard.sol b/packages/contracts/src/2.0.0/utils/ReentrancyGuard/ReentrancyGuard.sol
index 2b980c7ca..1dee512d4 100644
--- a/packages/contracts/src/2.0.0/utils/ReentrancyGuard/ReentrancyGuard.sol
+++ b/packages/contracts/src/2.0.0/utils/ReentrancyGuard/ReentrancyGuard.sol
@@ -23,21 +23,9 @@ contract ReentrancyGuard {
     // Locked state of mutex
     bool private locked = false;
 
-    /// @dev Functions with this modifier cannot be reentered.
-    modifier nonReentrant() {
-        // Ensure mutex is unlocked
-        require(
-            !locked,
-            "REENTRANCY_ILLEGAL"
-        );
-
-        // Perform function call
-        _;
-    }
-
     /// @dev Functions with this modifer cannot be reentered. The mutex will be locked
     ///      before function execution and unlocked after.
-    modifier lockMutex() {
+    modifier nonReentrant() {
         // Ensure mutex is unlocked
         require(
             !locked,
diff --git a/packages/contracts/test/utils/constants.ts b/packages/contracts/test/utils/constants.ts
index 7060b5091..ee4378d2e 100644
--- a/packages/contracts/test/utils/constants.ts
+++ b/packages/contracts/test/utils/constants.ts
@@ -54,14 +54,10 @@ export const constants = {
     FUNCTIONS_WITH_MUTEX: [
         'FILL_ORDER',
         'FILL_OR_KILL_ORDER',
-        'FILL_ORDER_NO_THROW',
         'BATCH_FILL_ORDERS',
         'BATCH_FILL_OR_KILL_ORDERS',
-        'BATCH_FILL_ORDERS_NO_THROW',
         'MARKET_BUY_ORDERS',
-        'MARKET_BUY_ORDERS_NO_THROW',
         'MARKET_SELL_ORDERS',
-        'MARKET_SELL_ORDERS_NO_THROW',
         'MATCH_ORDERS',
         'CANCEL_ORDER',
         'CANCEL_ORDERS_UP_TO',