diff options
author | Amir Bandeali <abandeali1@gmail.com> | 2018-08-25 09:15:22 +0800 |
---|---|---|
committer | GitHub <noreply@github.com> | 2018-08-25 09:15:22 +0800 |
commit | f938c989e3e07161de20dd865baf59eecdde872d (patch) | |
tree | 565f92c50b1655347bac61b27a8cbe67f2c1e21f /packages/contracts/src | |
parent | 74ce893f520db6ae4617432975b5c9321ff5b89b (diff) | |
parent | c8500cab10b8ffd175acd760cbd1286577a1c4b1 (diff) | |
download | dexon-0x-contracts-f938c989e3e07161de20dd865baf59eecdde872d.tar.gz dexon-0x-contracts-f938c989e3e07161de20dd865baf59eecdde872d.tar.zst dexon-0x-contracts-f938c989e3e07161de20dd865baf59eecdde872d.zip |
Merge pull request #1002 from 0xProject/feature/contracts/mutex
[contracts] Add mutexes and reentrancy guards
Diffstat (limited to 'packages/contracts/src')
9 files changed, 384 insertions, 50 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 f3a0591b2..291af3792 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, @@ -54,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. @@ -86,43 +89,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; } @@ -131,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); @@ -203,6 +178,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/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/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; } } 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..39fa724cc 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinWrapperFunctions.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinWrapperFunctions.sol @@ -19,18 +19,22 @@ 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, - MExchangeCore + MExchangeCore, + MWrapperFunctions { /// @dev Fills the input order. Reverts if exact takerAssetFillAmount not filled. @@ -43,17 +47,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; } @@ -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,11 +111,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 +138,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 +191,7 @@ contract MixinWrapperFunctions is bytes[] memory signatures ) public + nonReentrant returns (FillResults memory totalFillResults) { bytes memory takerAssetData = orders[0].takerAssetData; @@ -210,7 +207,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 +279,7 @@ contract MixinWrapperFunctions is bytes[] memory signatures ) public + nonReentrant returns (FillResults memory totalFillResults) { bytes memory makerAssetData = orders[0].makerAssetData; @@ -305,7 +303,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 +398,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/test/ReentrantERC20Token/ReentrantERC20Token.sol b/packages/contracts/src/2.0.0/test/ReentrantERC20Token/ReentrantERC20Token.sol new file mode 100644 index 000000000..8bfdd2e66 --- /dev/null +++ b/packages/contracts/src/2.0.0/test/ReentrantERC20Token/ReentrantERC20Token.sol @@ -0,0 +1,182 @@ +/* + + 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 "../../utils/LibBytes/LibBytes.sol"; +import "../../tokens/ERC20Token/ERC20Token.sol"; +import "../../protocol/Exchange/interfaces/IExchange.sol"; +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 + // We do not test any "noThrow" functions because `fillOrderNoThrow` makes a delegatecall to `fillOrder` + enum ExchangeFunction { + FILL_ORDER, + FILL_OR_KILL_ORDER, + BATCH_FILL_ORDERS, + BATCH_FILL_OR_KILL_ORDERS, + MARKET_BUY_ORDERS, + MARKET_SELL_ORDERS, + MATCH_ORDERS, + CANCEL_ORDER, + CANCEL_ORDERS_UP_TO, + SET_SIGNATURE_VALIDATOR_APPROVAL + } + + 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; + + // Initialize remaining null parameters + 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)) { + calldata = abi.encodeWithSelector( + EXCHANGE.fillOrder.selector, + order, + 0, + signature + ); + } else if (currentFunctionId == uint8(ExchangeFunction.FILL_OR_KILL_ORDER)) { + calldata = abi.encodeWithSelector( + EXCHANGE.fillOrKillOrder.selector, + order, + 0, + signature + ); + } else if (currentFunctionId == uint8(ExchangeFunction.BATCH_FILL_ORDERS)) { + calldata = abi.encodeWithSelector( + EXCHANGE.batchFillOrders.selector, + orders, + takerAssetFillAmounts, + signatures + ); + } else if (currentFunctionId == uint8(ExchangeFunction.BATCH_FILL_OR_KILL_ORDERS)) { + calldata = abi.encodeWithSelector( + EXCHANGE.batchFillOrKillOrders.selector, + orders, + takerAssetFillAmounts, + signatures + ); + } else if (currentFunctionId == uint8(ExchangeFunction.MARKET_BUY_ORDERS)) { + calldata = abi.encodeWithSelector( + EXCHANGE.marketBuyOrders.selector, + orders, + 0, + signatures + ); + } else if (currentFunctionId == uint8(ExchangeFunction.MARKET_SELL_ORDERS)) { + calldata = abi.encodeWithSelector( + EXCHANGE.marketSellOrders.selector, + orders, + 0, + signatures + ); + } else if (currentFunctionId == uint8(ExchangeFunction.MATCH_ORDERS)) { + 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 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..1dee512d4 --- /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 modifer cannot be reentered. The mutex will be locked + /// before function execution and unlocked after. + 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; + } +} |