From 044415e23d3a079e6301d8fb838da60456794c4f Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Mon, 20 Aug 2018 17:37:23 -0700 Subject: Remove redundant sload from getCurrentContextAddress --- packages/contracts/src/2.0.0/protocol/Exchange/MixinTransactions.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinTransactions.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinTransactions.sol index 821d30279..4a59b6c0f 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinTransactions.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinTransactions.sol @@ -155,7 +155,8 @@ contract MixinTransactions is view returns (address) { - address contextAddress = currentContextAddress == address(0) ? msg.sender : currentContextAddress; + address currentContextAddress_ = currentContextAddress; + address contextAddress = currentContextAddress_ == address(0) ? msg.sender : currentContextAddress_; return contextAddress; } } -- cgit v1.2.3 From d8cb56caa33885397f557afac5a4ccb24efb47d0 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Mon, 20 Aug 2018 17:38:25 -0700 Subject: Add ReentrancyGuard contract --- .../utils/ReentrancyGuard/ReentrancyGuard.sol | 44 ++++++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 packages/contracts/src/2.0.0/utils/ReentrancyGuard/ReentrancyGuard.sol diff --git a/packages/contracts/src/2.0.0/utils/ReentrancyGuard/ReentrancyGuard.sol b/packages/contracts/src/2.0.0/utils/ReentrancyGuard/ReentrancyGuard.sol new file mode 100644 index 000000000..70fc4ca6b --- /dev/null +++ b/packages/contracts/src/2.0.0/utils/ReentrancyGuard/ReentrancyGuard.sol @@ -0,0 +1,44 @@ +/* + + Copyright 2018 ZeroEx Intl. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + +*/ +pragma solidity 0.4.24; + + +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" + ); + + // Lock mutex before function call + locked = true; + + // Perform function call + _; + + // Unlock mutex after function call + locked = false; + } + +} -- cgit v1.2.3 From 6f88e9bdbdd8f44c45053b8c17c84411f9499084 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Tue, 21 Aug 2018 16:06:21 -0700 Subject: Add internal fill functions, add reentrancy guard to public functions that make external calls --- .../2.0.0/protocol/Exchange/MixinExchangeCore.sol | 86 ++++++++++++++-------- .../2.0.0/protocol/Exchange/MixinMatchOrders.sol | 3 + .../protocol/Exchange/MixinWrapperFunctions.sol | 46 +++++++++--- .../protocol/Exchange/mixins/MExchangeCore.sol | 13 ++++ .../protocol/Exchange/mixins/MWrapperFunctions.sol | 40 ++++++++++ .../utils/ReentrancyGuard/ReentrancyGuard.sol | 1 - 6 files changed, 148 insertions(+), 41 deletions(-) create mode 100644 packages/contracts/src/2.0.0/protocol/Exchange/mixins/MWrapperFunctions.sol 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 f3a0591b2..d00323b15 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol @@ -19,6 +19,7 @@ pragma solidity 0.4.24; pragma experimental ABIEncoderV2; +import "../../utils/ReentrancyGuard/ReentrancyGuard.sol"; import "./libs/LibConstants.sol"; import "./libs/LibFillResults.sol"; import "./libs/LibOrder.sol"; @@ -30,6 +31,7 @@ import "./mixins/MAssetProxyDispatcher.sol"; contract MixinExchangeCore is + ReentrancyGuard, LibConstants, LibMath, LibOrder, @@ -86,43 +88,14 @@ contract MixinExchangeCore is bytes memory signature ) public + nonReentrant returns (FillResults memory fillResults) { - // Fetch order info - OrderInfo memory orderInfo = getOrderInfo(order); - - // Fetch taker address - address takerAddress = getCurrentContextAddress(); - - // Get amount of takerAsset to fill - uint256 remainingTakerAssetAmount = safeSub(order.takerAssetAmount, orderInfo.orderTakerAssetFilledAmount); - uint256 takerAssetFilledAmount = min256(takerAssetFillAmount, remainingTakerAssetAmount); - - // Validate context - assertValidFill( + fillResults = fillOrderInternal( order, - orderInfo, - takerAddress, takerAssetFillAmount, - takerAssetFilledAmount, signature ); - - // Compute proportional fill amounts - fillResults = calculateFillResults(order, takerAssetFilledAmount); - - // Update exchange internal state - updateFilledState( - order, - takerAddress, - orderInfo.orderHash, - orderInfo.orderTakerAssetFilledAmount, - fillResults - ); - - // Settle order - settleOrder(order, takerAddress, fillResults); - return fillResults; } @@ -203,6 +176,57 @@ contract MixinExchangeCore is return orderInfo; } + /// @dev Fills the input order. + /// @param order Order struct containing order specifications. + /// @param takerAssetFillAmount Desired amount of takerAsset to sell. + /// @param signature Proof that order has been created by maker. + /// @return Amounts filled and fees paid by maker and taker. + function fillOrderInternal( + Order memory order, + uint256 takerAssetFillAmount, + bytes memory signature + ) + internal + returns (FillResults memory fillResults) + { + // Fetch order info + OrderInfo memory orderInfo = getOrderInfo(order); + + // Fetch taker address + address takerAddress = getCurrentContextAddress(); + + // Get amount of takerAsset to fill + uint256 remainingTakerAssetAmount = safeSub(order.takerAssetAmount, orderInfo.orderTakerAssetFilledAmount); + uint256 takerAssetFilledAmount = min256(takerAssetFillAmount, remainingTakerAssetAmount); + + // Validate context + assertValidFill( + order, + orderInfo, + takerAddress, + takerAssetFillAmount, + takerAssetFilledAmount, + signature + ); + + // Compute proportional fill amounts + fillResults = calculateFillResults(order, takerAssetFilledAmount); + + // Update exchange internal state + updateFilledState( + order, + takerAddress, + orderInfo.orderHash, + orderInfo.orderTakerAssetFilledAmount, + fillResults + ); + + // Settle order + settleOrder(order, takerAddress, fillResults); + + return fillResults; + } + /// @dev Updates state with results of a fill order. /// @param order that was filled. /// @param takerAddress Address of taker who filled the order. 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 20f4b1c33..25220a673 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol @@ -14,6 +14,7 @@ pragma solidity 0.4.24; pragma experimental ABIEncoderV2; +import "../../utils/ReentrancyGuard/ReentrancyGuard.sol"; import "./libs/LibConstants.sol"; import "./libs/LibMath.sol"; import "./libs/LibOrder.sol"; @@ -25,6 +26,7 @@ import "./mixins/MAssetProxyDispatcher.sol"; contract MixinMatchOrders is + ReentrancyGuard, LibConstants, LibMath, MAssetProxyDispatcher, @@ -48,6 +50,7 @@ contract MixinMatchOrders is bytes memory rightSignature ) public + 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 1b77cfbcb..ff1cb6995 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinWrapperFunctions.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinWrapperFunctions.sol @@ -19,14 +19,17 @@ pragma solidity 0.4.24; pragma experimental ABIEncoderV2; +import "../../utils/ReentrancyGuard/ReentrancyGuard.sol"; import "./libs/LibMath.sol"; import "./libs/LibOrder.sol"; import "./libs/LibFillResults.sol"; import "./libs/LibAbiEncoder.sol"; import "./mixins/MExchangeCore.sol"; +import "./mixins/MWrapperFunctions.sol"; contract MixinWrapperFunctions is + ReentrancyGuard, LibMath, LibFillResults, LibAbiEncoder, @@ -43,17 +46,14 @@ contract MixinWrapperFunctions is bytes memory signature ) public + nonReentrant returns (FillResults memory fillResults) { - fillResults = fillOrder( + fillResults = fillOrKillOrderInternal( order, takerAssetFillAmount, signature ); - require( - fillResults.takerAssetFilledAmount == takerAssetFillAmount, - "COMPLETE_FILL_FAILED" - ); return fillResults; } @@ -117,11 +117,12 @@ contract MixinWrapperFunctions is bytes[] memory signatures ) public + nonReentrant returns (FillResults memory totalFillResults) { uint256 ordersLength = orders.length; for (uint256 i = 0; i != ordersLength; i++) { - FillResults memory singleFillResults = fillOrder( + FillResults memory singleFillResults = fillOrderInternal( orders[i], takerAssetFillAmounts[i], signatures[i] @@ -143,11 +144,12 @@ contract MixinWrapperFunctions is bytes[] memory signatures ) public + nonReentrant returns (FillResults memory totalFillResults) { uint256 ordersLength = orders.length; for (uint256 i = 0; i != ordersLength; i++) { - FillResults memory singleFillResults = fillOrKillOrder( + FillResults memory singleFillResults = fillOrKillOrderInternal( orders[i], takerAssetFillAmounts[i], signatures[i] @@ -195,6 +197,7 @@ contract MixinWrapperFunctions is bytes[] memory signatures ) public + nonReentrant returns (FillResults memory totalFillResults) { bytes memory takerAssetData = orders[0].takerAssetData; @@ -210,7 +213,7 @@ contract MixinWrapperFunctions is uint256 remainingTakerAssetFillAmount = safeSub(takerAssetFillAmount, totalFillResults.takerAssetFilledAmount); // Attempt to sell the remaining amount of takerAsset - FillResults memory singleFillResults = fillOrder( + FillResults memory singleFillResults = fillOrderInternal( orders[i], remainingTakerAssetFillAmount, signatures[i] @@ -282,6 +285,7 @@ contract MixinWrapperFunctions is bytes[] memory signatures ) public + nonReentrant returns (FillResults memory totalFillResults) { bytes memory makerAssetData = orders[0].makerAssetData; @@ -305,7 +309,7 @@ contract MixinWrapperFunctions is ); // Attempt to sell the remaining amount of takerAsset - FillResults memory singleFillResults = fillOrder( + FillResults memory singleFillResults = fillOrderInternal( orders[i], remainingTakerAssetFillAmount, signatures[i] @@ -400,4 +404,28 @@ contract MixinWrapperFunctions is } return ordersInfo; } + + /// @dev Fills the input order. Reverts if exact takerAssetFillAmount not filled. + /// @param order Order struct containing order specifications. + /// @param takerAssetFillAmount Desired amount of takerAsset to sell. + /// @param signature Proof that order has been created by maker. + function fillOrKillOrderInternal( + LibOrder.Order memory order, + uint256 takerAssetFillAmount, + bytes memory signature + ) + internal + returns (FillResults memory fillResults) + { + fillResults = fillOrderInternal( + order, + takerAssetFillAmount, + signature + ); + require( + fillResults.takerAssetFilledAmount == takerAssetFillAmount, + "COMPLETE_FILL_FAILED" + ); + return fillResults; + } } diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MExchangeCore.sol b/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MExchangeCore.sol index c165b647c..41832fe4b 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MExchangeCore.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MExchangeCore.sol @@ -59,6 +59,19 @@ contract MExchangeCore is uint256 orderEpoch // Orders with specified makerAddress and senderAddress with a salt less than this value are considered cancelled. ); + /// @dev Fills the input order. + /// @param order Order struct containing order specifications. + /// @param takerAssetFillAmount Desired amount of takerAsset to sell. + /// @param signature Proof that order has been created by maker. + /// @return Amounts filled and fees paid by maker and taker. + function fillOrderInternal( + LibOrder.Order memory order, + uint256 takerAssetFillAmount, + bytes memory signature + ) + internal + returns (LibFillResults.FillResults memory fillResults); + /// @dev Updates state with results of a fill order. /// @param order that was filled. /// @param takerAddress Address of taker who filled the order. diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MWrapperFunctions.sol b/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MWrapperFunctions.sol new file mode 100644 index 000000000..e04d4a429 --- /dev/null +++ b/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MWrapperFunctions.sol @@ -0,0 +1,40 @@ +/* + + Copyright 2018 ZeroEx Intl. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + +*/ + +pragma solidity 0.4.24; +pragma experimental ABIEncoderV2; + +import "../libs/LibOrder.sol"; +import "../libs/LibFillResults.sol"; +import "../interfaces/IWrapperFunctions.sol"; + + +contract MWrapperFunctions { + + /// @dev Fills the input order. Reverts if exact takerAssetFillAmount not filled. + /// @param order LibOrder.Order struct containing order specifications. + /// @param takerAssetFillAmount Desired amount of takerAsset to sell. + /// @param signature Proof that order has been created by maker. + function fillOrKillOrderInternal( + LibOrder.Order memory order, + uint256 takerAssetFillAmount, + bytes memory signature + ) + internal + returns (LibFillResults.FillResults memory fillResults); +} 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 70fc4ca6b..5d6f6970d 100644 --- a/packages/contracts/src/2.0.0/utils/ReentrancyGuard/ReentrancyGuard.sol +++ b/packages/contracts/src/2.0.0/utils/ReentrancyGuard/ReentrancyGuard.sol @@ -40,5 +40,4 @@ contract ReentrancyGuard { // Unlock mutex after function call locked = false; } - } -- cgit v1.2.3 From cf12daea2f3a907f6a111c12d1e67c2e8b0a8797 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Tue, 21 Aug 2018 17:53:40 -0700 Subject: Add ReentrantToken --- packages/contracts/compiler.json | 1 + packages/contracts/package.json | 2 +- .../ReentrantERC20Token/ReentrantERC20Token.sol | 173 +++++++++++++++++++++ 3 files changed, 175 insertions(+), 1 deletion(-) create mode 100644 packages/contracts/src/2.0.0/test/ReentrantERC20Token/ReentrantERC20Token.sol diff --git a/packages/contracts/compiler.json b/packages/contracts/compiler.json index 27bb69a36..f66114e87 100644 --- a/packages/contracts/compiler.json +++ b/packages/contracts/compiler.json @@ -40,6 +40,7 @@ "MultiSigWallet", "MultiSigWalletWithTimeLock", "OrderValidator", + "ReentrantERC20Token", "TestAssetProxyOwner", "TestAssetProxyDispatcher", "TestConstants", diff --git a/packages/contracts/package.json b/packages/contracts/package.json index 0ead2f43f..8d75eb4ce 100644 --- a/packages/contracts/package.json +++ b/packages/contracts/package.json @@ -34,7 +34,7 @@ }, "config": { "abis": - "../migrations/artifacts/2.0.0/@(AssetProxyOwner|DummyERC20Token|DummyERC721Receiver|DummyERC721Token|DummyNoReturnERC20Token|ERC20Proxy|ERC721Proxy|Forwarder|Exchange|ExchangeWrapper|IAssetData|IAssetProxy|InvalidERC721Receiver|MixinAuthorizable|MultiSigWallet|MultiSigWalletWithTimeLock|OrderValidator|TestAssetProxyOwner|TestAssetProxyDispatcher|TestConstants|TestExchangeInternals|TestLibBytes|TestLibs|TestSignatureValidator|TestStaticCallReceiver|Validator|Wallet|TokenRegistry|Whitelist|WETH9|ZRXToken).json" + "../migrations/artifacts/2.0.0/@(AssetProxyOwner|DummyERC20Token|DummyERC721Receiver|DummyERC721Token|DummyNoReturnERC20Token|ERC20Proxy|ERC721Proxy|Forwarder|Exchange|ExchangeWrapper|IAssetData|IAssetProxy|InvalidERC721Receiver|MixinAuthorizable|MultiSigWallet|MultiSigWalletWithTimeLock|OrderValidator|ReentrantERC20Token|TestAssetProxyOwner|TestAssetProxyDispatcher|TestConstants|TestExchangeInternals|TestLibBytes|TestLibs|TestSignatureValidator|Validator|Wallet|TokenRegistry|Whitelist|WETH9|ZRXToken).json" }, "repository": { "type": "git", diff --git a/packages/contracts/src/2.0.0/test/ReentrantERC20Token/ReentrantERC20Token.sol b/packages/contracts/src/2.0.0/test/ReentrantERC20Token/ReentrantERC20Token.sol new file mode 100644 index 000000000..f5c98177f --- /dev/null +++ b/packages/contracts/src/2.0.0/test/ReentrantERC20Token/ReentrantERC20Token.sol @@ -0,0 +1,173 @@ +/* + + Copyright 2018 ZeroEx Intl. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + +*/ + +pragma solidity 0.4.24; +pragma experimental ABIEncoderV2; + +import "../../tokens/ERC20Token/ERC20Token.sol"; +import "../../protocol/Exchange/interfaces/IExchange.sol"; +import "../../protocol/Exchange/libs/LibOrder.sol"; + + +contract ReentrantERC20Token is + ERC20Token +{ + // solhint-disable-next-line var-name-mixedcase + IExchange internal EXCHANGE; + + // All of these functions are potentially vulnerable to reentrancy + 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 + } + + uint8 internal currentFunctionId = 0; + + constructor (address _exchange) + public + { + EXCHANGE = IExchange(_exchange); + } + + /// @dev Set the current function that will be called when `transferFrom` is called. + /// @param _currentFunctionId Id that corresponds to function name. + function setCurrentFunction(uint8 _currentFunctionId) + external + { + currentFunctionId = _currentFunctionId; + } + + /// @dev A version of `transferFrom` that attempts to reenter the Exchange contract. + /// @param _from The address of the sender + /// @param _to The address of the recipient + /// @param _value The amount of token to be transferred + function transferFrom( + address _from, + address _to, + uint256 _value + ) + external + returns (bool) + { + // This order would normally be invalid, but it will be used strictly for testing reentrnacy. + // Any reentrancy checks will happen before any other checks that invalidate the order. + LibOrder.Order memory order = LibOrder.Order({ + makerAddress: address(0), + takerAddress: address(0), + feeRecipientAddress: address(0), + senderAddress: address(0), + makerAssetAmount: 0, + takerAssetAmount: 0, + makerFee: 0, + takerFee: 0, + expirationTimeSeconds: 0, + salt: 0, + makerAssetData: "", + takerAssetData: "" + }); + + // Initialize remaining null parameters + bytes memory signature = ""; + LibOrder.Order[] memory orders = new LibOrder.Order[](1); + orders[0] = order; + uint256[] memory takerAssetFillAmounts = new uint256[](1); + takerAssetFillAmounts[0] = 0; + bytes[] memory signatures = new bytes[](1); + signatures[0] = signature; + + // Attempt to reenter Exchange by calling function with currentFunctionId + if (currentFunctionId == uint8(ExchangeFunction.FILL_ORDER)) { + EXCHANGE.fillOrder( + order, + 0, + signature + ); + } else if (currentFunctionId == uint8(ExchangeFunction.FILL_OR_KILL_ORDER)) { + EXCHANGE.fillOrKillOrder( + order, + 0, + signature + ); + } else if (currentFunctionId == uint8(ExchangeFunction.FILL_ORDER_NO_THROW)) { + EXCHANGE.fillOrderNoThrow( + order, + 0, + signature + ); + } else if (currentFunctionId == uint8(ExchangeFunction.BATCH_FILL_ORDERS)) { + EXCHANGE.batchFillOrders( + orders, + takerAssetFillAmounts, + signatures + ); + } else if (currentFunctionId == uint8(ExchangeFunction.BATCH_FILL_OR_KILL_ORDERS)) { + EXCHANGE.batchFillOrKillOrders( + orders, + takerAssetFillAmounts, + signatures + ); + } else if (currentFunctionId == uint8(ExchangeFunction.BATCH_FILL_ORDERS_NO_THROW)) { + EXCHANGE.batchFillOrdersNoThrow( + orders, + takerAssetFillAmounts, + signatures + ); + } else if (currentFunctionId == uint8(ExchangeFunction.MARKET_BUY_ORDERS)) { + EXCHANGE.marketBuyOrders( + orders, + 0, + signatures + ); + } else if (currentFunctionId == uint8(ExchangeFunction.MARKET_BUY_ORDERS_NO_THROW)) { + EXCHANGE.marketBuyOrdersNoThrow( + orders, + 0, + signatures + ); + } else if (currentFunctionId == uint8(ExchangeFunction.MARKET_SELL_ORDERS)) { + EXCHANGE.marketSellOrders( + orders, + 0, + signatures + ); + } else if (currentFunctionId == uint8(ExchangeFunction.MARKET_SELL_ORDERS_NO_THROW)) { + EXCHANGE.marketSellOrdersNoThrow( + orders, + 0, + signatures + ); + } else if (currentFunctionId == uint8(ExchangeFunction.MATCH_ORDERS)) { + EXCHANGE.matchOrders( + order, + order, + signature, + signature + ); + } + return true; + } +} \ No newline at end of file -- cgit v1.2.3 From 6d0dedc62cb1be54dbdd287fd59d7b568a199007 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Wed, 22 Aug 2018 17:57:57 -0700 Subject: Split modifiers into check only and check, lock, unlock --- .../src/2.0.0/utils/ReentrancyGuard/ReentrancyGuard.sol | 13 +++++++++++++ 1 file changed, 13 insertions(+) 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 5d6f6970d..2b980c7ca 100644 --- a/packages/contracts/src/2.0.0/utils/ReentrancyGuard/ReentrancyGuard.sol +++ b/packages/contracts/src/2.0.0/utils/ReentrancyGuard/ReentrancyGuard.sol @@ -31,6 +31,19 @@ contract ReentrancyGuard { "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() { + // Ensure mutex is unlocked + require( + !locked, + "REENTRANCY_ILLEGAL" + ); + // Lock mutex before function call locked = true; -- cgit v1.2.3 From c75212bef0b3801ecda09a89d5e30e359dcf1b63 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Wed, 22 Aug 2018 17:58:47 -0700 Subject: Add nonReentrant modifiers on functions that use getCurrentContextAddress only, add lockMutex modifier on functions that make external calls --- .../2.0.0/protocol/Exchange/MixinExchangeCore.sol | 4 +++- .../2.0.0/protocol/Exchange/MixinMatchOrders.sol | 2 +- .../protocol/Exchange/MixinSignatureValidator.sol | 3 +++ .../protocol/Exchange/MixinWrapperFunctions.sol | 23 ++++++++++------------ 4 files changed, 17 insertions(+), 15 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 d00323b15..f02514fc6 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol @@ -56,6 +56,7 @@ contract MixinExchangeCore is /// @param targetOrderEpoch Orders created with a salt less or equal to this value will be cancelled. function cancelOrdersUpTo(uint256 targetOrderEpoch) external + nonReentrant { address makerAddress = getCurrentContextAddress(); // If this function is called via `executeTransaction`, we only update the orderEpoch for the makerAddress/msg.sender combination. @@ -88,7 +89,7 @@ contract MixinExchangeCore is bytes memory signature ) public - nonReentrant + lockMutex returns (FillResults memory fillResults) { fillResults = fillOrderInternal( @@ -104,6 +105,7 @@ contract MixinExchangeCore is /// @param order Order to cancel. Order must be OrderStatus.FILLABLE. function cancelOrder(Order memory order) public + nonReentrant { // Fetch current order status OrderInfo memory orderInfo = getOrderInfo(order); 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 25220a673..39c47e6f9 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 - nonReentrant + lockMutex 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/MixinSignatureValidator.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinSignatureValidator.sol index f30adcdb8..4eb6a2fa6 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinSignatureValidator.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinSignatureValidator.sol @@ -19,6 +19,7 @@ pragma solidity 0.4.24; import "../../utils/LibBytes/LibBytes.sol"; +import "../../utils/ReentrancyGuard/ReentrancyGuard.sol"; import "./mixins/MSignatureValidator.sol"; import "./mixins/MTransactions.sol"; import "./interfaces/IWallet.sol"; @@ -26,6 +27,7 @@ import "./interfaces/IValidator.sol"; contract MixinSignatureValidator is + ReentrancyGuard, MSignatureValidator, MTransactions { @@ -69,6 +71,7 @@ contract MixinSignatureValidator is bool approval ) external + nonReentrant { address signerAddress = getCurrentContextAddress(); allowedValidators[signerAddress][validatorAddress] = approval; 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 ff1cb6995..b0474b110 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinWrapperFunctions.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinWrapperFunctions.sol @@ -46,7 +46,7 @@ contract MixinWrapperFunctions is bytes memory signature ) public - nonReentrant + lockMutex returns (FillResults memory fillResults) { fillResults = fillOrKillOrderInternal( @@ -69,6 +69,7 @@ contract MixinWrapperFunctions is bytes memory signature ) public + nonReentrant returns (FillResults memory fillResults) { // ABI encode calldata for `fillOrder` @@ -88,14 +89,7 @@ contract MixinWrapperFunctions is fillOrderCalldata, // write output over input 128 // output size is 128 bytes ) - switch success - case 0 { - mstore(fillResults, 0) - mstore(add(fillResults, 32), 0) - mstore(add(fillResults, 64), 0) - mstore(add(fillResults, 96), 0) - } - case 1 { + if success { mstore(fillResults, mload(fillOrderCalldata)) mstore(add(fillResults, 32), mload(add(fillOrderCalldata, 32))) mstore(add(fillResults, 64), mload(add(fillOrderCalldata, 64))) @@ -117,7 +111,7 @@ contract MixinWrapperFunctions is bytes[] memory signatures ) public - nonReentrant + lockMutex returns (FillResults memory totalFillResults) { uint256 ordersLength = orders.length; @@ -144,7 +138,7 @@ contract MixinWrapperFunctions is bytes[] memory signatures ) public - nonReentrant + lockMutex returns (FillResults memory totalFillResults) { uint256 ordersLength = orders.length; @@ -172,6 +166,7 @@ contract MixinWrapperFunctions is bytes[] memory signatures ) public + nonReentrant returns (FillResults memory totalFillResults) { uint256 ordersLength = orders.length; @@ -197,7 +192,7 @@ contract MixinWrapperFunctions is bytes[] memory signatures ) public - nonReentrant + lockMutex returns (FillResults memory totalFillResults) { bytes memory takerAssetData = orders[0].takerAssetData; @@ -242,6 +237,7 @@ contract MixinWrapperFunctions is bytes[] memory signatures ) public + nonReentrant returns (FillResults memory totalFillResults) { bytes memory takerAssetData = orders[0].takerAssetData; @@ -285,7 +281,7 @@ contract MixinWrapperFunctions is bytes[] memory signatures ) public - nonReentrant + lockMutex returns (FillResults memory totalFillResults) { bytes memory makerAssetData = orders[0].makerAssetData; @@ -338,6 +334,7 @@ contract MixinWrapperFunctions is bytes[] memory signatures ) public + nonReentrant returns (FillResults memory totalFillResults) { bytes memory makerAssetData = orders[0].makerAssetData; -- cgit v1.2.3 From 56c3c29febe6264eee7f1c3f1ed8f1dc7c4685c6 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Wed, 22 Aug 2018 18:00:01 -0700 Subject: Update ReentrantERC20Token with new functions and check that revert is occuring for correct reason --- .../ReentrantERC20Token/ReentrantERC20Token.sol | 110 ++++++++++++++------- 1 file changed, 75 insertions(+), 35 deletions(-) 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 f5c98177f..8d575d09a 100644 --- a/packages/contracts/src/2.0.0/test/ReentrantERC20Token/ReentrantERC20Token.sol +++ b/packages/contracts/src/2.0.0/test/ReentrantERC20Token/ReentrantERC20Token.sol @@ -19,6 +19,7 @@ pragma solidity 0.4.24; pragma experimental ABIEncoderV2; +import "../../utils/LibBytes/LibBytes.sol"; import "../../tokens/ERC20Token/ERC20Token.sol"; import "../../protocol/Exchange/interfaces/IExchange.sol"; import "../../protocol/Exchange/libs/LibOrder.sol"; @@ -27,9 +28,17 @@ import "../../protocol/Exchange/libs/LibOrder.sol"; contract ReentrantERC20Token is ERC20Token { + + using LibBytes for bytes; + // solhint-disable-next-line var-name-mixedcase IExchange internal EXCHANGE; + bytes internal constant REENTRANCY_ILLEGAL_REVERT_REASON = abi.encodeWithSelector( + bytes4(keccak256("Error(string)")), + "REENTRANCY_ILLEGAL" + ); + // All of these functions are potentially vulnerable to reentrancy enum ExchangeFunction { FILL_ORDER, @@ -42,7 +51,10 @@ contract ReentrantERC20Token is MARKET_BUY_ORDERS_NO_THROW, MARKET_SELL_ORDERS, MARKET_SELL_ORDERS_NO_THROW, - MATCH_ORDERS + MATCH_ORDERS, + CANCEL_ORDER, + CANCEL_ORDERS_UP_TO, + SET_SIGNATURE_VALIDATOR_APPROVAL } uint8 internal currentFunctionId = 0; @@ -75,99 +87,127 @@ contract ReentrantERC20Token is { // This order would normally be invalid, but it will be used strictly for testing reentrnacy. // Any reentrancy checks will happen before any other checks that invalidate the order. - LibOrder.Order memory order = LibOrder.Order({ - makerAddress: address(0), - takerAddress: address(0), - feeRecipientAddress: address(0), - senderAddress: address(0), - makerAssetAmount: 0, - takerAssetAmount: 0, - makerFee: 0, - takerFee: 0, - expirationTimeSeconds: 0, - salt: 0, - makerAssetData: "", - takerAssetData: "" - }); + LibOrder.Order memory order; // Initialize remaining null parameters - bytes memory signature = ""; - LibOrder.Order[] memory orders = new LibOrder.Order[](1); - orders[0] = order; - uint256[] memory takerAssetFillAmounts = new uint256[](1); - takerAssetFillAmounts[0] = 0; - bytes[] memory signatures = new bytes[](1); - signatures[0] = signature; - - // Attempt to reenter Exchange by calling function with currentFunctionId + bytes memory signature; + LibOrder.Order[] memory orders; + uint256[] memory takerAssetFillAmounts; + bytes[] memory signatures; + bytes memory calldata; + + // Create calldata for function that corresponds to currentFunctionId if (currentFunctionId == uint8(ExchangeFunction.FILL_ORDER)) { - EXCHANGE.fillOrder( + calldata = abi.encodeWithSelector( + EXCHANGE.fillOrder.selector, order, 0, signature ); } else if (currentFunctionId == uint8(ExchangeFunction.FILL_OR_KILL_ORDER)) { - EXCHANGE.fillOrKillOrder( + calldata = abi.encodeWithSelector( + EXCHANGE.fillOrKillOrder.selector, order, 0, signature ); } else if (currentFunctionId == uint8(ExchangeFunction.FILL_ORDER_NO_THROW)) { - EXCHANGE.fillOrderNoThrow( + calldata = abi.encodeWithSelector( + EXCHANGE.fillOrderNoThrow.selector, order, 0, signature ); } else if (currentFunctionId == uint8(ExchangeFunction.BATCH_FILL_ORDERS)) { - EXCHANGE.batchFillOrders( + calldata = abi.encodeWithSelector( + EXCHANGE.batchFillOrders.selector, orders, takerAssetFillAmounts, signatures ); } else if (currentFunctionId == uint8(ExchangeFunction.BATCH_FILL_OR_KILL_ORDERS)) { - EXCHANGE.batchFillOrKillOrders( + calldata = abi.encodeWithSelector( + EXCHANGE.batchFillOrKillOrders.selector, orders, takerAssetFillAmounts, signatures ); } else if (currentFunctionId == uint8(ExchangeFunction.BATCH_FILL_ORDERS_NO_THROW)) { - EXCHANGE.batchFillOrdersNoThrow( + calldata = abi.encodeWithSelector( + EXCHANGE.batchFillOrdersNoThrow.selector, orders, takerAssetFillAmounts, signatures ); } else if (currentFunctionId == uint8(ExchangeFunction.MARKET_BUY_ORDERS)) { - EXCHANGE.marketBuyOrders( + calldata = abi.encodeWithSelector( + EXCHANGE.marketBuyOrders.selector, orders, 0, signatures ); } else if (currentFunctionId == uint8(ExchangeFunction.MARKET_BUY_ORDERS_NO_THROW)) { - EXCHANGE.marketBuyOrdersNoThrow( + calldata = abi.encodeWithSelector( + EXCHANGE.marketBuyOrdersNoThrow.selector, orders, 0, signatures ); } else if (currentFunctionId == uint8(ExchangeFunction.MARKET_SELL_ORDERS)) { - EXCHANGE.marketSellOrders( + calldata = abi.encodeWithSelector( + EXCHANGE.marketSellOrders.selector, orders, 0, signatures ); } else if (currentFunctionId == uint8(ExchangeFunction.MARKET_SELL_ORDERS_NO_THROW)) { - EXCHANGE.marketSellOrdersNoThrow( + calldata = abi.encodeWithSelector( + EXCHANGE.marketSellOrdersNoThrow.selector, orders, 0, signatures ); } else if (currentFunctionId == uint8(ExchangeFunction.MATCH_ORDERS)) { - EXCHANGE.matchOrders( + calldata = abi.encodeWithSelector( + EXCHANGE.matchOrders.selector, order, order, signature, signature ); + } else if (currentFunctionId == uint8(ExchangeFunction.CANCEL_ORDER)) { + calldata = abi.encodeWithSelector( + EXCHANGE.cancelOrder.selector, + order + ); + } else if (currentFunctionId == uint8(ExchangeFunction.CANCEL_ORDERS_UP_TO)) { + calldata = abi.encodeWithSelector( + EXCHANGE.cancelOrdersUpTo.selector, + 0 + ); + } else if (currentFunctionId == uint8(ExchangeFunction.SET_SIGNATURE_VALIDATOR_APPROVAL)) { + calldata = abi.encodeWithSelector( + EXCHANGE.setSignatureValidatorApproval.selector, + address(0), + false + ); } + + // Call Exchange function, swallow error + address(EXCHANGE).call(calldata); + + // Revert reason is 100 bytes + bytes memory returnData = new bytes(100); + + // Copy return data + assembly { + returndatacopy(add(returnData, 32), 0, 100) + } + + // Revert if function reverted with REENTRANCY_ILLEGAL error + require(!REENTRANCY_ILLEGAL_REVERT_REASON.equals(returnData)); + + // Transfer will return true if function failed for any other reason return true; } } \ No newline at end of file -- cgit v1.2.3 From 7d5a23969d5514ca3c942f11a48a8e858b9d0188 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Wed, 22 Aug 2018 18:01:45 -0700 Subject: Add reentrancy tests for fillOrder and wrapper functions --- packages/contracts/test/exchange/core.ts | 28 ++++ packages/contracts/test/exchange/wrapper.ts | 193 ++++++++++++++++++++++++++++ packages/contracts/test/utils/artifacts.ts | 2 + packages/contracts/test/utils/constants.ts | 16 +++ 4 files changed, 239 insertions(+) diff --git a/packages/contracts/test/exchange/core.ts b/packages/contracts/test/exchange/core.ts index f9d8b7783..8ba55a0ab 100644 --- a/packages/contracts/test/exchange/core.ts +++ b/packages/contracts/test/exchange/core.ts @@ -14,6 +14,7 @@ import { DummyNoReturnERC20TokenContract } from '../../generated_contract_wrappe import { ERC20ProxyContract } from '../../generated_contract_wrappers/erc20_proxy'; import { ERC721ProxyContract } from '../../generated_contract_wrappers/erc721_proxy'; import { ExchangeCancelEventArgs, ExchangeContract } from '../../generated_contract_wrappers/exchange'; +import { ReentrantERC20TokenContract } from '../../generated_contract_wrappers/reentrant_erc20_token'; import { TestStaticCallReceiverContract } from '../../generated_contract_wrappers/test_static_call_receiver'; import { artifacts } from '../utils/artifacts'; import { expectTransactionFailedAsync } from '../utils/assertions'; @@ -42,6 +43,7 @@ describe('Exchange core', () => { let zrxToken: DummyERC20TokenContract; let erc721Token: DummyERC721TokenContract; let noReturnErc20Token: DummyNoReturnERC20TokenContract; + let reentrantErc20Token: ReentrantERC20TokenContract; let exchange: ExchangeContract; let erc20Proxy: ERC20ProxyContract; let erc721Proxy: ERC721ProxyContract; @@ -117,6 +119,12 @@ describe('Exchange core', () => { provider, txDefaults, ); + reentrantErc20Token = await ReentrantERC20TokenContract.deployFrom0xArtifactAsync( + artifacts.ReentrantERC20Token, + provider, + txDefaults, + exchange.address, + ); defaultMakerAssetAddress = erc20TokenA.address; defaultTakerAssetAddress = erc20TokenB.address; @@ -144,6 +152,26 @@ describe('Exchange core', () => { signedOrder = await orderFactory.newSignedOrderAsync(); }); + const reentrancyTest = (functionNames: string[]) => { + _.forEach(functionNames, async (functionName: string, functionId: number) => { + const description = `should not allow fillOrder to reenter the Exchange contract via ${functionName}`; + it(description, async () => { + signedOrder = await orderFactory.newSignedOrderAsync({ + makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + }); + await web3Wrapper.awaitTransactionSuccessAsync( + await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + await expectTransactionFailedAsync( + exchangeWrapper.fillOrderAsync(signedOrder, takerAddress), + RevertReason.TransferFailed, + ); + }); + }); + }; + describe('fillOrder reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX)); + it('should throw if signature is invalid', async () => { signedOrder = await orderFactory.newSignedOrderAsync({ makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), diff --git a/packages/contracts/test/exchange/wrapper.ts b/packages/contracts/test/exchange/wrapper.ts index d48441dca..aadb5ab59 100644 --- a/packages/contracts/test/exchange/wrapper.ts +++ b/packages/contracts/test/exchange/wrapper.ts @@ -11,6 +11,7 @@ import { DummyERC721TokenContract } from '../../generated_contract_wrappers/dumm import { ERC20ProxyContract } from '../../generated_contract_wrappers/erc20_proxy'; import { ERC721ProxyContract } from '../../generated_contract_wrappers/erc721_proxy'; import { ExchangeContract } from '../../generated_contract_wrappers/exchange'; +import { ReentrantERC20TokenContract } from '../../generated_contract_wrappers/reentrant_erc20_token'; import { artifacts } from '../utils/artifacts'; import { expectTransactionFailedAsync } from '../utils/assertions'; import { getLatestBlockTimestampAsync, increaseTimeAndMineBlockAsync } from '../utils/block_timestamp'; @@ -40,6 +41,7 @@ describe('Exchange wrappers', () => { let exchange: ExchangeContract; let erc20Proxy: ERC20ProxyContract; let erc721Proxy: ERC721ProxyContract; + let reentrantErc20Token: ReentrantERC20TokenContract; let exchangeWrapper: ExchangeWrapper; let erc20Wrapper: ERC20Wrapper; @@ -104,6 +106,13 @@ describe('Exchange wrappers', () => { constants.AWAIT_TRANSACTION_MINED_MS, ); + reentrantErc20Token = await ReentrantERC20TokenContract.deployFrom0xArtifactAsync( + artifacts.ReentrantERC20Token, + provider, + txDefaults, + exchange.address, + ); + defaultMakerAssetAddress = erc20TokenA.address; defaultTakerAssetAddress = erc20TokenB.address; @@ -126,6 +135,26 @@ describe('Exchange wrappers', () => { await blockchainLifecycle.revertAsync(); }); describe('fillOrKillOrder', () => { + const reentrancyTest = (functionNames: string[]) => { + _.forEach(functionNames, async (functionName: string, functionId: number) => { + const description = `should not allow fillOrKillOrder to reenter the Exchange contract via ${functionName}`; + it(description, async () => { + const signedOrder = await orderFactory.newSignedOrderAsync({ + makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + }); + await web3Wrapper.awaitTransactionSuccessAsync( + await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + await expectTransactionFailedAsync( + exchangeWrapper.fillOrKillOrderAsync(signedOrder, takerAddress), + RevertReason.TransferFailed, + ); + }); + }); + }; + describe('fillOrKillOrder reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX)); + it('should transfer the correct amounts', async () => { const signedOrder = await orderFactory.newSignedOrderAsync({ makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18), @@ -197,6 +226,25 @@ describe('Exchange wrappers', () => { }); describe('fillOrderNoThrow', () => { + const reentrancyTest = (functionNames: string[]) => { + _.forEach(functionNames, async (functionName: string, functionId: number) => { + const description = `should not allow fillOrderNoThrow to reenter the Exchange contract via ${functionName}`; + it(description, async () => { + const signedOrder = await orderFactory.newSignedOrderAsync({ + makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + }); + await web3Wrapper.awaitTransactionSuccessAsync( + await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + await exchangeWrapper.fillOrderNoThrowAsync(signedOrder, takerAddress); + const newBalances = await erc20Wrapper.getBalancesAsync(); + expect(erc20Balances).to.deep.equal(newBalances); + }); + }); + }; + describe('fillOrderNoThrow reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX)); + it('should transfer the correct amounts', async () => { const signedOrder = await orderFactory.newSignedOrderAsync({ makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18), @@ -397,6 +445,26 @@ describe('Exchange wrappers', () => { }); describe('batchFillOrders', () => { + const reentrancyTest = (functionNames: string[]) => { + _.forEach(functionNames, async (functionName: string, functionId: number) => { + const description = `should not allow batchFillOrders to reenter the Exchange contract via ${functionName}`; + it(description, async () => { + const signedOrder = await orderFactory.newSignedOrderAsync({ + makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + }); + await web3Wrapper.awaitTransactionSuccessAsync( + await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + await expectTransactionFailedAsync( + exchangeWrapper.batchFillOrdersAsync([signedOrder], takerAddress), + RevertReason.TransferFailed, + ); + }); + }); + }; + describe('batchFillOrders reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX)); + it('should transfer the correct amounts', async () => { const takerAssetFillAmounts: BigNumber[] = []; const makerAssetAddress = erc20TokenA.address; @@ -446,6 +514,26 @@ describe('Exchange wrappers', () => { }); describe('batchFillOrKillOrders', () => { + const reentrancyTest = (functionNames: string[]) => { + _.forEach(functionNames, async (functionName: string, functionId: number) => { + const description = `should not allow batchFillOrKillOrders to reenter the Exchange contract via ${functionName}`; + it(description, async () => { + const signedOrder = await orderFactory.newSignedOrderAsync({ + makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + }); + await web3Wrapper.awaitTransactionSuccessAsync( + await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + await expectTransactionFailedAsync( + exchangeWrapper.batchFillOrKillOrdersAsync([signedOrder], takerAddress), + RevertReason.TransferFailed, + ); + }); + }); + }; + describe('batchFillOrKillOrders reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX)); + it('should transfer the correct amounts', async () => { const takerAssetFillAmounts: BigNumber[] = []; const makerAssetAddress = erc20TokenA.address; @@ -512,6 +600,25 @@ describe('Exchange wrappers', () => { }); describe('batchFillOrdersNoThrow', async () => { + const reentrancyTest = (functionNames: string[]) => { + _.forEach(functionNames, async (functionName: string, functionId: number) => { + const description = `should not allow batchFillOrdersNoThrow to reenter the Exchange contract via ${functionName}`; + it(description, async () => { + const signedOrder = await orderFactory.newSignedOrderAsync({ + makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + }); + await web3Wrapper.awaitTransactionSuccessAsync( + await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + await exchangeWrapper.batchFillOrdersNoThrowAsync([signedOrder], takerAddress); + const newBalances = await erc20Wrapper.getBalancesAsync(); + expect(erc20Balances).to.deep.equal(newBalances); + }); + }); + }; + describe('batchFillOrdersNoThrow reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX)); + it('should transfer the correct amounts', async () => { const takerAssetFillAmounts: BigNumber[] = []; const makerAssetAddress = erc20TokenA.address; @@ -625,6 +732,28 @@ describe('Exchange wrappers', () => { }); describe('marketSellOrders', () => { + const reentrancyTest = (functionNames: string[]) => { + _.forEach(functionNames, async (functionName: string, functionId: number) => { + const description = `should not allow marketSellOrders to reenter the Exchange contract via ${functionName}`; + it(description, async () => { + const signedOrder = await orderFactory.newSignedOrderAsync({ + makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + }); + await web3Wrapper.awaitTransactionSuccessAsync( + await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + await expectTransactionFailedAsync( + exchangeWrapper.marketSellOrdersAsync([signedOrder], takerAddress, { + takerAssetFillAmount: signedOrder.takerAssetAmount, + }), + RevertReason.TransferFailed, + ); + }); + }); + }; + describe('marketSellOrders reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX)); + it('should stop when the entire takerAssetFillAmount is filled', async () => { const takerAssetFillAmount = signedOrders[0].takerAssetAmount.plus( signedOrders[1].takerAssetAmount.div(2), @@ -717,6 +846,27 @@ describe('Exchange wrappers', () => { }); describe('marketSellOrdersNoThrow', () => { + const reentrancyTest = (functionNames: string[]) => { + _.forEach(functionNames, async (functionName: string, functionId: number) => { + const description = `should not allow marketSellOrdersNoThrow to reenter the Exchange contract via ${functionName}`; + it(description, async () => { + const signedOrder = await orderFactory.newSignedOrderAsync({ + makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + }); + await web3Wrapper.awaitTransactionSuccessAsync( + await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + await exchangeWrapper.marketSellOrdersNoThrowAsync([signedOrder], takerAddress, { + takerAssetFillAmount: signedOrder.takerAssetAmount, + }); + const newBalances = await erc20Wrapper.getBalancesAsync(); + expect(erc20Balances).to.deep.equal(newBalances); + }); + }); + }; + describe('marketSellOrdersNoThrow reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX)); + it('should stop when the entire takerAssetFillAmount is filled', async () => { const takerAssetFillAmount = signedOrders[0].takerAssetAmount.plus( signedOrders[1].takerAssetAmount.div(2), @@ -843,6 +993,28 @@ describe('Exchange wrappers', () => { }); describe('marketBuyOrders', () => { + const reentrancyTest = (functionNames: string[]) => { + _.forEach(functionNames, async (functionName: string, functionId: number) => { + const description = `should not allow marketBuyOrders to reenter the Exchange contract via ${functionName}`; + it(description, async () => { + const signedOrder = await orderFactory.newSignedOrderAsync({ + makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + }); + await web3Wrapper.awaitTransactionSuccessAsync( + await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + await expectTransactionFailedAsync( + exchangeWrapper.marketBuyOrdersAsync([signedOrder], takerAddress, { + makerAssetFillAmount: signedOrder.makerAssetAmount, + }), + RevertReason.TransferFailed, + ); + }); + }); + }; + describe('marketBuyOrders reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX)); + it('should stop when the entire makerAssetFillAmount is filled', async () => { const makerAssetFillAmount = signedOrders[0].makerAssetAmount.plus( signedOrders[1].makerAssetAmount.div(2), @@ -933,6 +1105,27 @@ describe('Exchange wrappers', () => { }); describe('marketBuyOrdersNoThrow', () => { + const reentrancyTest = (functionNames: string[]) => { + _.forEach(functionNames, async (functionName: string, functionId: number) => { + const description = `should not allow marketBuyOrdersNoThrow to reenter the Exchange contract via ${functionName}`; + it(description, async () => { + const signedOrder = await orderFactory.newSignedOrderAsync({ + makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + }); + await web3Wrapper.awaitTransactionSuccessAsync( + await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + await exchangeWrapper.marketBuyOrdersNoThrowAsync([signedOrder], takerAddress, { + makerAssetFillAmount: signedOrder.makerAssetAmount, + }); + const newBalances = await erc20Wrapper.getBalancesAsync(); + expect(erc20Balances).to.deep.equal(newBalances); + }); + }); + }; + describe('marketBuyOrdersNoThrow reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX)); + it('should stop when the entire makerAssetFillAmount is filled', async () => { const makerAssetFillAmount = signedOrders[0].makerAssetAmount.plus( signedOrders[1].makerAssetAmount.div(2), diff --git a/packages/contracts/test/utils/artifacts.ts b/packages/contracts/test/utils/artifacts.ts index 2f6fcef71..5ddb5cc7f 100644 --- a/packages/contracts/test/utils/artifacts.ts +++ b/packages/contracts/test/utils/artifacts.ts @@ -16,6 +16,7 @@ import * as MixinAuthorizable from '../../artifacts/MixinAuthorizable.json'; import * as MultiSigWallet from '../../artifacts/MultiSigWallet.json'; import * as MultiSigWalletWithTimeLock from '../../artifacts/MultiSigWalletWithTimeLock.json'; import * as OrderValidator from '../../artifacts/OrderValidator.json'; +import * as ReentrantERC20Token from '../../artifacts/ReentrantERC20Token.json'; import * as TestAssetProxyDispatcher from '../../artifacts/TestAssetProxyDispatcher.json'; import * as TestAssetProxyOwner from '../../artifacts/TestAssetProxyOwner.json'; import * as TestConstants from '../../artifacts/TestConstants.json'; @@ -49,6 +50,7 @@ export const artifacts = { MultiSigWallet: (MultiSigWallet as any) as ContractArtifact, MultiSigWalletWithTimeLock: (MultiSigWalletWithTimeLock as any) as ContractArtifact, OrderValidator: (OrderValidator as any) as ContractArtifact, + ReentrantERC20Token: (ReentrantERC20Token as any) as ContractArtifact, TestAssetProxyOwner: (TestAssetProxyOwner as any) as ContractArtifact, TestAssetProxyDispatcher: (TestAssetProxyDispatcher as any) as ContractArtifact, TestConstants: (TestConstants as any) as ContractArtifact, diff --git a/packages/contracts/test/utils/constants.ts b/packages/contracts/test/utils/constants.ts index 65eaee398..7060b5091 100644 --- a/packages/contracts/test/utils/constants.ts +++ b/packages/contracts/test/utils/constants.ts @@ -51,4 +51,20 @@ export const constants = { WORD_LENGTH: 32, ZERO_AMOUNT: new BigNumber(0), PERCENTAGE_DENOMINATOR: new BigNumber(10).pow(18), + 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', + 'SET_SIGNATURE_VALIDATOR_APPROVAL', + ], }; -- cgit v1.2.3 From a09ee907396d35468e0ad632fb990cb1c8870b49 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Wed, 22 Aug 2018 23:27:43 -0700 Subject: Add tests for matchOrders --- packages/contracts/test/exchange/core.ts | 2 +- packages/contracts/test/exchange/match_orders.ts | 173 +++++++---------------- 2 files changed, 53 insertions(+), 122 deletions(-) diff --git a/packages/contracts/test/exchange/core.ts b/packages/contracts/test/exchange/core.ts index 8ba55a0ab..3bb71b58f 100644 --- a/packages/contracts/test/exchange/core.ts +++ b/packages/contracts/test/exchange/core.ts @@ -530,7 +530,7 @@ describe('Exchange core', () => { // HACK(albrow): We need to hardcode the gas estimate here because // the Geth gas estimator doesn't work with the way we use // delegatecall and swallow errors. - gas: 490000, + gas: 600000, }); const newBalances = await erc20Wrapper.getBalancesAsync(); diff --git a/packages/contracts/test/exchange/match_orders.ts b/packages/contracts/test/exchange/match_orders.ts index 46b3569bd..8cd873a85 100644 --- a/packages/contracts/test/exchange/match_orders.ts +++ b/packages/contracts/test/exchange/match_orders.ts @@ -11,6 +11,7 @@ import { DummyERC721TokenContract } from '../../generated_contract_wrappers/dumm import { ERC20ProxyContract } from '../../generated_contract_wrappers/erc20_proxy'; import { ERC721ProxyContract } from '../../generated_contract_wrappers/erc721_proxy'; import { ExchangeContract } from '../../generated_contract_wrappers/exchange'; +import { ReentrantERC20TokenContract } from '../../generated_contract_wrappers/reentrant_erc20_token'; import { artifacts } from '../utils/artifacts'; import { expectTransactionFailedAsync } from '../utils/assertions'; import { chaiSetup } from '../utils/chai_setup'; @@ -39,6 +40,7 @@ describe('matchOrders', () => { let erc20TokenB: DummyERC20TokenContract; let zrxToken: DummyERC20TokenContract; let erc721Token: DummyERC721TokenContract; + let reentrantErc20Token: ReentrantERC20TokenContract; let exchange: ExchangeContract; let erc20Proxy: ERC20ProxyContract; let erc721Proxy: ERC721ProxyContract; @@ -127,21 +129,39 @@ describe('matchOrders', () => { }), constants.AWAIT_TRANSACTION_MINED_MS, ); + + reentrantErc20Token = await ReentrantERC20TokenContract.deployFrom0xArtifactAsync( + artifacts.ReentrantERC20Token, + provider, + txDefaults, + exchange.address, + ); + // Set default addresses defaultERC20MakerAssetAddress = erc20TokenA.address; defaultERC20TakerAssetAddress = erc20TokenB.address; defaultERC721AssetAddress = erc721Token.address; // Create default order parameters - const defaultOrderParams = { + const defaultOrderParamsLeft = { ...constants.STATIC_ORDER_PARAMS, + makerAddress: makerAddressLeft, exchangeAddress: exchange.address, makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), + feeRecipientAddress: feeRecipientAddressLeft, + }; + const defaultOrderParamsRight = { + ...constants.STATIC_ORDER_PARAMS, + makerAddress: makerAddressRight, + exchangeAddress: exchange.address, + makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), + takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), + feeRecipientAddress: feeRecipientAddressRight, }; const privateKeyLeft = constants.TESTRPC_PRIVATE_KEYS[accounts.indexOf(makerAddressLeft)]; - orderFactoryLeft = new OrderFactory(privateKeyLeft, defaultOrderParams); + orderFactoryLeft = new OrderFactory(privateKeyLeft, defaultOrderParamsLeft); const privateKeyRight = constants.TESTRPC_PRIVATE_KEYS[accounts.indexOf(makerAddressRight)]; - orderFactoryRight = new OrderFactory(privateKeyRight, defaultOrderParams); + orderFactoryRight = new OrderFactory(privateKeyRight, defaultOrderParamsRight); // Set match order tester matchOrderTester = new MatchOrderTester(exchangeWrapper, erc20Wrapper, erc721Wrapper, zrxToken.address); }); @@ -157,21 +177,44 @@ describe('matchOrders', () => { erc721TokenIdsByOwner = await erc721Wrapper.getBalancesAsync(); }); + const reentrancyTest = (functionNames: string[]) => { + _.forEach(functionNames, async (functionName: string, functionId: number) => { + const description = `should not allow matchOrders to reenter the Exchange contract via ${functionName}`; + it(description, async () => { + const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ + makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + }); + const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ + makerAddress: makerAddressRight, + takerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), + feeRecipientAddress: feeRecipientAddressRight, + }); + await web3Wrapper.awaitTransactionSuccessAsync( + await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + await expectTransactionFailedAsync( + exchangeWrapper.matchOrdersAsync(signedOrderLeft, signedOrderRight, takerAddress), + RevertReason.TransferFailed, + ); + }); + }); + }; + describe('matchOrders reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX)); + it('should transfer the correct amounts when orders completely fill each other', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ - makerAddress: makerAddressLeft, makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), - feeRecipientAddress: feeRecipientAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ - makerAddress: makerAddressRight, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), - takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), - feeRecipientAddress: feeRecipientAddressRight, }); // Match signedOrderLeft with signedOrderRight await matchOrderTester.matchOrdersAndVerifyBalancesAsync( @@ -192,18 +235,12 @@ describe('matchOrders', () => { it('should transfer the correct amounts when orders completely fill each other and taker doesnt take a profit', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ - makerAddress: makerAddressLeft, makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), - feeRecipientAddress: feeRecipientAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ - makerAddress: makerAddressRight, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), - takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), - feeRecipientAddress: feeRecipientAddressRight, }); // Store original taker balance const takerInitialBalances = _.cloneDeep(erc20BalancesByOwner[takerAddress][defaultERC20MakerAssetAddress]); @@ -237,18 +274,12 @@ describe('matchOrders', () => { it('should transfer the correct amounts when left order is completely filled and right order is partially filled', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ - makerAddress: makerAddressLeft, makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), - feeRecipientAddress: feeRecipientAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ - makerAddress: makerAddressRight, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), - takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(20), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(4), 18), - feeRecipientAddress: feeRecipientAddressRight, }); // Match orders await matchOrderTester.matchOrdersAndVerifyBalancesAsync( @@ -269,18 +300,12 @@ describe('matchOrders', () => { it('should transfer the correct amounts when right order is completely filled and left order is partially filled', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ - makerAddress: makerAddressLeft, makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(50), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18), - feeRecipientAddress: feeRecipientAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ - makerAddress: makerAddressRight, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), - takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), - feeRecipientAddress: feeRecipientAddressRight, }); // Match orders await matchOrderTester.matchOrdersAndVerifyBalancesAsync( @@ -301,18 +326,12 @@ describe('matchOrders', () => { it('should transfer the correct amounts when consecutive calls are used to completely fill the left order', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ - makerAddress: makerAddressLeft, makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(50), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18), - feeRecipientAddress: feeRecipientAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ - makerAddress: makerAddressRight, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), - takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), - feeRecipientAddress: feeRecipientAddressRight, }); // Match orders let newERC20BalancesByOwner: ERC20BalancesByOwner; @@ -340,12 +359,8 @@ describe('matchOrders', () => { // However, we use 100/50 to ensure a partial fill as we want to go down the "left fill" // branch in the contract twice for this test. const signedOrderRight2 = await orderFactoryRight.newSignedOrderAsync({ - makerAddress: makerAddressRight, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), - takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(50), 18), - feeRecipientAddress: feeRecipientAddressRight, }); // Match signedOrderLeft with signedOrderRight2 const leftTakerAssetFilledAmount = signedOrderRight.makerAssetAmount; @@ -370,19 +385,13 @@ describe('matchOrders', () => { it('should transfer the correct amounts when consecutive calls are used to completely fill the right order', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ - makerAddress: makerAddressLeft, makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), - feeRecipientAddress: feeRecipientAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ - makerAddress: makerAddressRight, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), - takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(50), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18), - feeRecipientAddress: feeRecipientAddressRight, }); // Match orders let newERC20BalancesByOwner: ERC20BalancesByOwner; @@ -410,10 +419,8 @@ describe('matchOrders', () => { // However, we use 100/50 to ensure a partial fill as we want to go down the "right fill" // branch in the contract twice for this test. const signedOrderLeft2 = await orderFactoryLeft.newSignedOrderAsync({ - makerAddress: makerAddressLeft, makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(50), 18), - feeRecipientAddress: feeRecipientAddressLeft, }); // Match signedOrderLeft2 with signedOrderRight const leftTakerAssetFilledAmount = new BigNumber(0); @@ -441,15 +448,11 @@ describe('matchOrders', () => { it('should transfer the correct amounts if fee recipient is the same across both matched orders', async () => { const feeRecipientAddress = feeRecipientAddressLeft; const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ - makerAddress: makerAddressLeft, makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), feeRecipientAddress, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ - makerAddress: makerAddressRight, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), - takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), feeRecipientAddress, @@ -467,18 +470,12 @@ describe('matchOrders', () => { it('should transfer the correct amounts if taker is also the left order maker', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ - makerAddress: makerAddressLeft, makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), - feeRecipientAddress: feeRecipientAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ - makerAddress: makerAddressRight, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), - takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), - feeRecipientAddress: feeRecipientAddressRight, }); // Match orders takerAddress = signedOrderLeft.makerAddress; @@ -494,18 +491,12 @@ describe('matchOrders', () => { it('should transfer the correct amounts if taker is also the right order maker', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ - makerAddress: makerAddressLeft, makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), - feeRecipientAddress: feeRecipientAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ - makerAddress: makerAddressRight, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), - takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), - feeRecipientAddress: feeRecipientAddressRight, }); // Match orders takerAddress = signedOrderRight.makerAddress; @@ -521,18 +512,12 @@ describe('matchOrders', () => { it('should transfer the correct amounts if taker is also the left fee recipient', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ - makerAddress: makerAddressLeft, makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), - feeRecipientAddress: feeRecipientAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ - makerAddress: makerAddressRight, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), - takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), - feeRecipientAddress: feeRecipientAddressRight, }); // Match orders takerAddress = feeRecipientAddressLeft; @@ -548,18 +533,12 @@ describe('matchOrders', () => { it('should transfer the correct amounts if taker is also the right fee recipient', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ - makerAddress: makerAddressLeft, makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), - feeRecipientAddress: feeRecipientAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ - makerAddress: makerAddressRight, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), - takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), - feeRecipientAddress: feeRecipientAddressRight, }); // Match orders takerAddress = feeRecipientAddressRight; @@ -575,18 +554,12 @@ describe('matchOrders', () => { it('should transfer the correct amounts if left maker is the left fee recipient and right maker is the right fee recipient', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ - makerAddress: makerAddressLeft, makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), - feeRecipientAddress: makerAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ - makerAddress: makerAddressRight, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), - takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), - feeRecipientAddress: makerAddressRight, }); // Match orders await matchOrderTester.matchOrdersAndVerifyBalancesAsync( @@ -601,18 +574,12 @@ describe('matchOrders', () => { it('Should throw if left order is not fillable', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ - makerAddress: makerAddressLeft, makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), - feeRecipientAddress: feeRecipientAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ - makerAddress: makerAddressRight, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), - takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), - feeRecipientAddress: feeRecipientAddressRight, }); // Cancel left order await exchangeWrapper.cancelOrderAsync(signedOrderLeft, signedOrderLeft.makerAddress); @@ -626,18 +593,12 @@ describe('matchOrders', () => { it('Should throw if right order is not fillable', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ - makerAddress: makerAddressLeft, makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), - feeRecipientAddress: feeRecipientAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ - makerAddress: makerAddressRight, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), - takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), - feeRecipientAddress: feeRecipientAddressRight, }); // Cancel right order await exchangeWrapper.cancelOrderAsync(signedOrderRight, signedOrderRight.makerAddress); @@ -651,18 +612,12 @@ describe('matchOrders', () => { it('should throw if there is not a positive spread', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ - makerAddress: makerAddressLeft, makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18), - feeRecipientAddress: feeRecipientAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ - makerAddress: makerAddressRight, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), - takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(1), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(200), 18), - feeRecipientAddress: feeRecipientAddressRight, }); // Match orders return expectTransactionFailedAsync( @@ -674,18 +629,13 @@ describe('matchOrders', () => { it('should throw if the left maker asset is not equal to the right taker asset ', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ - makerAddress: makerAddressLeft, makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), - feeRecipientAddress: feeRecipientAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ - makerAddress: makerAddressRight, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), - feeRecipientAddress: feeRecipientAddressRight, }); // Match orders return expectTransactionFailedAsync( @@ -701,20 +651,13 @@ describe('matchOrders', () => { it('should throw if the right maker asset is not equal to the left taker asset', async () => { // Create orders to match const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ - makerAddress: makerAddressLeft, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(5), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), - feeRecipientAddress: feeRecipientAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ - makerAddress: makerAddressRight, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), - takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(2), 18), - feeRecipientAddress: feeRecipientAddressRight, }); // Match orders return expectTransactionFailedAsync( @@ -727,20 +670,14 @@ describe('matchOrders', () => { // Create orders to match const erc721TokenToTransfer = erc721LeftMakerAssetIds[0]; const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ - makerAddress: makerAddressLeft, makerAssetData: assetDataUtils.encodeERC721AssetData(defaultERC721AssetAddress, erc721TokenToTransfer), - takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), makerAssetAmount: new BigNumber(1), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), - feeRecipientAddress: feeRecipientAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ - makerAddress: makerAddressRight, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20TakerAssetAddress), takerAssetData: assetDataUtils.encodeERC721AssetData(defaultERC721AssetAddress, erc721TokenToTransfer), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), takerAssetAmount: new BigNumber(1), - feeRecipientAddress: feeRecipientAddressRight, }); // Match orders await matchOrderTester.matchOrdersAndVerifyBalancesAsync( @@ -762,20 +699,14 @@ describe('matchOrders', () => { // Create orders to match const erc721TokenToTransfer = erc721RightMakerAssetIds[0]; const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({ - makerAddress: makerAddressLeft, - makerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), takerAssetData: assetDataUtils.encodeERC721AssetData(defaultERC721AssetAddress, erc721TokenToTransfer), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), takerAssetAmount: new BigNumber(1), - feeRecipientAddress: feeRecipientAddressLeft, }); const signedOrderRight = await orderFactoryRight.newSignedOrderAsync({ - makerAddress: makerAddressRight, makerAssetData: assetDataUtils.encodeERC721AssetData(defaultERC721AssetAddress, erc721TokenToTransfer), - takerAssetData: assetDataUtils.encodeERC20AssetData(defaultERC20MakerAssetAddress), makerAssetAmount: new BigNumber(1), takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), - feeRecipientAddress: feeRecipientAddressRight, }); // Match orders await matchOrderTester.matchOrdersAndVerifyBalancesAsync( -- cgit v1.2.3 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 +-------- packages/contracts/test/utils/constants.ts | 4 --- 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', -- cgit v1.2.3 From c8500cab10b8ffd175acd760cbd1286577a1c4b1 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Fri, 24 Aug 2018 16:35:43 -0700 Subject: Fix build --- packages/contracts/package.json | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/contracts/package.json b/packages/contracts/package.json index 8d75eb4ce..1c1ac81da 100644 --- a/packages/contracts/package.json +++ b/packages/contracts/package.json @@ -20,11 +20,14 @@ "test:coverage": "SOLIDITY_COVERAGE=true run-s build run_mocha coverage:report:text coverage:report:lcov", "test:profiler": "SOLIDITY_PROFILER=true run-s build run_mocha profiler:report:html", "test:trace": "SOLIDITY_REVERT_TRACE=true run-s build run_mocha", - "run_mocha": "mocha --require source-map-support/register --require make-promises-safe 'lib/test/**/*.js' --timeout 100000 --bail --exit", + "run_mocha": + "mocha --require source-map-support/register --require make-promises-safe 'lib/test/**/*.js' --timeout 100000 --bail --exit", "compile": "sol-compiler --contracts-dir src", "clean": "shx rm -rf lib generated_contract_wrappers", - "generate_contract_wrappers": "abi-gen --abis ${npm_package_config_abis} --template ../contract_templates/contract.handlebars --partials '../contract_templates/partials/**/*.handlebars' --output generated_contract_wrappers --backend ethers", - "lint": "tslint --project . --exclude **/src/generated_contract_wrappers/**/* --exclude **/lib/**/* && yarn lint-contracts", + "generate_contract_wrappers": + "abi-gen --abis ${npm_package_config_abis} --template ../contract_templates/contract.handlebars --partials '../contract_templates/partials/**/*.handlebars' --output generated_contract_wrappers --backend ethers", + "lint": + "tslint --project . --exclude **/src/generated_contract_wrappers/**/* --exclude **/lib/**/* && yarn lint-contracts", "coverage:report:text": "istanbul report text", "coverage:report:html": "istanbul report html && open coverage/index.html", "profiler:report:html": "istanbul report html && open coverage/index.html", @@ -34,7 +37,7 @@ }, "config": { "abis": - "../migrations/artifacts/2.0.0/@(AssetProxyOwner|DummyERC20Token|DummyERC721Receiver|DummyERC721Token|DummyNoReturnERC20Token|ERC20Proxy|ERC721Proxy|Forwarder|Exchange|ExchangeWrapper|IAssetData|IAssetProxy|InvalidERC721Receiver|MixinAuthorizable|MultiSigWallet|MultiSigWalletWithTimeLock|OrderValidator|ReentrantERC20Token|TestAssetProxyOwner|TestAssetProxyDispatcher|TestConstants|TestExchangeInternals|TestLibBytes|TestLibs|TestSignatureValidator|Validator|Wallet|TokenRegistry|Whitelist|WETH9|ZRXToken).json" + "../migrations/artifacts/2.0.0/@(AssetProxyOwner|DummyERC20Token|DummyERC721Receiver|DummyERC721Token|DummyNoReturnERC20Token|ERC20Proxy|ERC721Proxy|Forwarder|Exchange|ExchangeWrapper|IAssetData|IAssetProxy|InvalidERC721Receiver|MixinAuthorizable|MultiSigWallet|MultiSigWalletWithTimeLock|OrderValidator|ReentrantERC20Token|TestAssetProxyOwner|TestAssetProxyDispatcher|TestConstants|TestExchangeInternals|TestLibBytes|TestLibs|TestSignatureValidator|TestStaticCallReceiver|Validator|Wallet|TokenRegistry|Whitelist|WETH9|ZRXToken).json" }, "repository": { "type": "git", -- cgit v1.2.3