From cf12daea2f3a907f6a111c12d1e67c2e8b0a8797 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Tue, 21 Aug 2018 17:53:40 -0700 Subject: Add ReentrantToken --- .../ReentrantERC20Token/ReentrantERC20Token.sol | 173 +++++++++++++++++++++ 1 file changed, 173 insertions(+) create mode 100644 packages/contracts/src/2.0.0/test/ReentrantERC20Token/ReentrantERC20Token.sol (limited to 'packages/contracts/src/2.0.0/test/ReentrantERC20Token') 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 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(-) (limited to 'packages/contracts/src/2.0.0/test/ReentrantERC20Token') 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 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 --- .../ReentrantERC20Token/ReentrantERC20Token.sol | 33 +--------------------- 1 file changed, 1 insertion(+), 32 deletions(-) (limited to 'packages/contracts/src/2.0.0/test/ReentrantERC20Token') 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, -- cgit v1.2.3