From c28c3db63fb80d5d5e82ccd3b327cb8c408cc6fa Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Thu, 23 Aug 2018 16:43:46 -0700 Subject: Only use one nonReentrant modifier, remove modifier from fillOrderNoThrow variations --- .../2.0.0/protocol/Exchange/MixinExchangeCore.sol | 2 +- .../2.0.0/protocol/Exchange/MixinMatchOrders.sol | 2 +- .../protocol/Exchange/MixinWrapperFunctions.sol | 17 +++++------ .../ReentrantERC20Token/ReentrantERC20Token.sol | 33 +--------------------- .../utils/ReentrancyGuard/ReentrancyGuard.sol | 14 +-------- 5 files changed, 11 insertions(+), 57 deletions(-) (limited to 'packages/contracts/src') 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, -- cgit v1.2.3