From d469606d724f0987462d82a7c86f44cddc0f70f1 Mon Sep 17 00:00:00 2001 From: Alex Towle Date: Wed, 17 Jul 2019 11:02:15 -0500 Subject: [PATCH] Addressed review comments --- .../contracts/src/MixinMatchOrders.sol | 32 ++++++--- .../src/interfaces/IExchangeRichErrors.sol | 4 +- .../contracts/test/ReentrantERC20Token.sol | 42 ++++++++++++ contracts/exchange/test/match_orders.ts | 68 ++++++++++++++++++- contracts/exchange/test/utils/constants.ts | 2 + .../order-utils/src/exchange_revert_errors.ts | 4 +- 6 files changed, 137 insertions(+), 15 deletions(-) diff --git a/contracts/exchange/contracts/src/MixinMatchOrders.sol b/contracts/exchange/contracts/src/MixinMatchOrders.sol index 09861e6d62..bdbb8a4835 100644 --- a/contracts/exchange/contracts/src/MixinMatchOrders.sol +++ b/contracts/exchange/contracts/src/MixinMatchOrders.sol @@ -289,7 +289,7 @@ contract MixinMatchOrders is // If the left maker can buy exactly what the right maker can sell, then both orders are fully filled. // Case 2. // If the left maker cannot buy more than the right maker can sell, then only the left order is fully filled. - if (leftTakerAssetAmountRemaining >= rightMakerAssetAmountRemaining) { + if (leftTakerAssetAmountRemaining > rightMakerAssetAmountRemaining) { // Case 1: Right order is fully filled _calculateCompleteRightFill( matchedFillResults, @@ -297,7 +297,7 @@ contract MixinMatchOrders is rightMakerAssetAmountRemaining, rightTakerAssetAmountRemaining ); - } else { + } else if (rightMakerAssetAmountRemaining > leftTakerAssetAmountRemaining) { // Case 2: Left order is fully filled matchedFillResults.left.makerAssetFilledAmount = leftMakerAssetAmountRemaining; matchedFillResults.left.takerAssetFilledAmount = leftTakerAssetAmountRemaining; @@ -309,6 +309,13 @@ contract MixinMatchOrders is rightOrder.makerAssetAmount, leftTakerAssetAmountRemaining // matchedFillResults.right.makerAssetFilledAmount ); + } else { + // Case 3: Both orders are fully filled. Technically, this could be captured by the above cases, but + // this calculation will be more precise since it does not include rounding. + matchedFillResults.left.makerAssetFilledAmount = leftMakerAssetAmountRemaining; + matchedFillResults.left.takerAssetFilledAmount = leftTakerAssetAmountRemaining; + matchedFillResults.right.makerAssetFilledAmount = rightMakerAssetAmountRemaining; + matchedFillResults.right.takerAssetFilledAmount = rightTakerAssetAmountRemaining; } // Calculate amount given to taker @@ -411,6 +418,13 @@ contract MixinMatchOrders is } } + /// @dev Calculates the fill results for the maker and taker in the order matching and writes the results + /// to the fillResults that are being collected on the order. + /// @param matchedFillResults The fill results object to populate with calculations. + /// @param leftOrder The left order that is being maximally filled. All of the information about fill amounts + /// can be derived from this order and the right asset remaining fields. + /// @param rightMakerAssetAmountRemaining The amount of the right maker asset that is remaining to be filled. + /// @param rightTakerAssetAmountRemaining The amount of the right taker asset that is remaining to be filled. function _calculateCompleteRightFill( MatchedFillResults memory matchedFillResults, LibOrder.Order memory leftOrder, @@ -469,12 +483,12 @@ contract MixinMatchOrders is // Ensure that the left and right arrays are compatible. if (leftOrders.length != leftSignatures.length) { LibRichErrors._rrevert(LibExchangeRichErrors.BatchMatchOrdersError( - BatchMatchOrdersErrorCodes.INCOMPATIBLE_LEFT_ORDERS + BatchMatchOrdersErrorCodes.INVALID_LENGTH_LEFT_SIGNATURES )); } if (rightOrders.length != rightSignatures.length) { LibRichErrors._rrevert(LibExchangeRichErrors.BatchMatchOrdersError( - BatchMatchOrdersErrorCodes.INCOMPATIBLE_RIGHT_ORDERS + BatchMatchOrdersErrorCodes.INVALID_LENGTH_RIGHT_SIGNATURES )); } @@ -540,13 +554,13 @@ contract MixinMatchOrders is // or break out of the loop if there are no more leftOrders to match. if (leftOrderInfo.orderTakerAssetFilledAmount >= leftOrder.takerAssetAmount) { // Update the batched fill results once the leftIdx is updated. - batchMatchedFillResults.left[leftIdx] = leftFillResults; + batchMatchedFillResults.left[leftIdx++] = leftFillResults; // Clear the intermediate fill results value. leftFillResults = LibFillResults.FillResults(0, 0, 0, 0); - // If all of the right orders have been filled, break out of the loop. + // If all of the left orders have been filled, break out of the loop. // Otherwise, update the current right order. - if (++leftIdx == leftOrders.length) { + if (leftIdx == leftOrders.length) { // Update the right batched fill results batchMatchedFillResults.right[rightIdx] = rightFillResults; break; @@ -560,13 +574,13 @@ contract MixinMatchOrders is // or break out of the loop if there are no more rightOrders to match. if (rightOrderInfo.orderTakerAssetFilledAmount >= rightOrder.takerAssetAmount) { // Update the batched fill results once the rightIdx is updated. - batchMatchedFillResults.right[rightIdx] = rightFillResults; + batchMatchedFillResults.right[rightIdx++] = rightFillResults; // Clear the intermediate fill results value. rightFillResults = LibFillResults.FillResults(0, 0, 0, 0); // If all of the right orders have been filled, break out of the loop. // Otherwise, update the current right order. - if (++rightIdx == rightOrders.length) { + if (rightIdx == rightOrders.length) { // Update the left batched fill results batchMatchedFillResults.left[leftIdx] = leftFillResults; break; diff --git a/contracts/exchange/contracts/src/interfaces/IExchangeRichErrors.sol b/contracts/exchange/contracts/src/interfaces/IExchangeRichErrors.sol index 931ed700a5..63c122a472 100644 --- a/contracts/exchange/contracts/src/interfaces/IExchangeRichErrors.sol +++ b/contracts/exchange/contracts/src/interfaces/IExchangeRichErrors.sol @@ -11,8 +11,8 @@ contract IExchangeRichErrors { enum BatchMatchOrdersErrorCodes { ZERO_LEFT_ORDERS, ZERO_RIGHT_ORDERS, - INCOMPATIBLE_LEFT_ORDERS, - INCOMPATIBLE_RIGHT_ORDERS + INVALID_LENGTH_LEFT_SIGNATURES, + INVALID_LENGTH_RIGHT_SIGNATURES } enum FillErrorCodes { diff --git a/contracts/exchange/contracts/test/ReentrantERC20Token.sol b/contracts/exchange/contracts/test/ReentrantERC20Token.sol index c9dc7e04e8..90fe36dc2f 100644 --- a/contracts/exchange/contracts/test/ReentrantERC20Token.sol +++ b/contracts/exchange/contracts/test/ReentrantERC20Token.sol @@ -48,6 +48,8 @@ contract ReentrantERC20Token is MARKET_SELL_ORDERS, MATCH_ORDERS, MATCH_ORDERS_WITH_MAXIMAL_FILL, + BATCH_MATCH_ORDERS, + BATCH_MATCH_ORDERS_WITH_MAXIMAL_FILL, CANCEL_ORDER, BATCH_CANCEL_ORDERS, CANCEL_ORDERS_UP_TO, @@ -158,6 +160,32 @@ contract ReentrantERC20Token is signatures[0], signatures[1] ); + } else if (currentFunctionId == uint8(ExchangeFunction.BATCH_MATCH_ORDERS)) { + LibOrder.Order[] memory leftOrders; + LibOrder.Order[] memory rightOrders; + (leftOrders, rightOrders) = _createBatchMatchedOrders(); + bytes[] memory leftSignatures = _createWalletSignatures(1); + bytes[] memory rightSignatures = _createWalletSignatures(1); + callData = abi.encodeWithSelector( + exchange.batchMatchOrders.selector, + leftOrders, + rightOrders, + leftSignatures, + rightSignatures + ); + } else if (currentFunctionId == uint8(ExchangeFunction.BATCH_MATCH_ORDERS_WITH_MAXIMAL_FILL)) { + LibOrder.Order[] memory leftOrders; + LibOrder.Order[] memory rightOrders; + (leftOrders, rightOrders) = _createBatchMatchedOrders(); + bytes[] memory leftSignatures = _createWalletSignatures(1); + bytes[] memory rightSignatures = _createWalletSignatures(1); + callData = abi.encodeWithSelector( + exchange.batchMatchOrders.selector, + leftOrders, + rightOrders, + leftSignatures, + rightSignatures + ); } else if (currentFunctionId == uint8(ExchangeFunction.CANCEL_ORDER)) { callData = abi.encodeWithSelector( exchange.cancelOrder.selector, @@ -249,6 +277,20 @@ contract ReentrantERC20Token is orders[1].makerAssetAmount = orders[0].takerAssetAmount; } + function _createBatchMatchedOrders() + internal + view + returns (LibOrder.Order[] memory leftOrders, LibOrder.Order[] memory rightOrders) + { + LibOrder.Order[] memory _orders = _createOrders(2); + leftOrders = new LibOrder.Order[](1); + rightOrders = new LibOrder.Order[](1); + leftOrders[0] = _orders[0]; + rightOrders[0] = _orders[1]; + rightOrders[0].takerAssetAmount = rightOrders[0].makerAssetAmount; + rightOrders[0].makerAssetAmount = leftOrders[0].takerAssetAmount; + } + function _getTakerFillAmounts( LibOrder.Order[] memory orders ) diff --git a/contracts/exchange/test/match_orders.ts b/contracts/exchange/test/match_orders.ts index 708a64fbbf..d9d438dead 100644 --- a/contracts/exchange/test/match_orders.ts +++ b/contracts/exchange/test/match_orders.ts @@ -3142,7 +3142,7 @@ describe('matchOrders', () => { // Set params left signatures to only include the first left signature params.leftSignatures = [params.leftSignatures[0]]; const expectedError = new ExchangeRevertErrors.BatchMatchOrdersError( - ExchangeRevertErrors.BatchMatchOrdersErrorCodes.IncompatibleLeftOrders, + ExchangeRevertErrors.BatchMatchOrdersErrorCodes.InvalidLengthLeftSignatures, ); let tx = exchangeWrapper.batchMatchOrdersRawAsync(params, takerAddress); await expect(tx).to.revertWith(expectedError); @@ -3182,7 +3182,7 @@ describe('matchOrders', () => { // Set params right signatures to only include the first right signature params.rightSignatures = [params.rightSignatures[0]]; const expectedError = new ExchangeRevertErrors.BatchMatchOrdersError( - ExchangeRevertErrors.BatchMatchOrdersErrorCodes.IncompatibleRightOrders, + ExchangeRevertErrors.BatchMatchOrdersErrorCodes.InvalidLengthRightSignatures, ); let tx = exchangeWrapper.batchMatchOrdersRawAsync(params, takerAddress); await expect(tx).to.revertWith(expectedError); @@ -3525,6 +3525,38 @@ describe('matchOrders', () => { false, ); }); + + const reentrancyTest = (functionNames: string[]) => { + _.forEach(functionNames, async (functionName: string, functionId: number) => { + const description = `should not allow batchMatchOrders to reenter the Exchange contract via ${functionName}`; + it(description, async () => { + const leftOrders = [ + await orderFactoryLeft.newSignedOrderAsync({ + makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(5, 18), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(10, 18), + }), + ]; + const rightOrders = [ + await orderFactoryRight.newSignedOrderAsync({ + makerAddress: makerAddressRight, + takerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(10, 18), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(2, 18), + feeRecipientAddress: feeRecipientAddressRight, + }), + ]; + await web3Wrapper.awaitTransactionSuccessAsync( + await reentrantErc20Token.setReentrantFunction.sendTransactionAsync(functionId), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + const expectedError = new ReentrancyGuardRevertErrors.IllegalReentrancyError(); + const tx = exchangeWrapper.batchMatchOrdersAsync(leftOrders, rightOrders, takerAddress); + return expect(tx).to.revertWith(expectedError); + }); + }); + }; + describe('batchMatchOrders reentrancy tests', () => reentrancyTest(exchangeConstants.FUNCTIONS_WITH_MUTEX)); }); describe('batchMatchOrdersWithMaximalFill', () => { it('should fully fill the the right order and pay the profit denominated in the left maker asset', async () => { @@ -3786,6 +3818,38 @@ describe('matchOrders', () => { true, ); }); + + const reentrancyTest = (functionNames: string[]) => { + _.forEach(functionNames, async (functionName: string, functionId: number) => { + const description = `should not allow batchMatchOrdersWithMaximalFill to reenter the Exchange contract via ${functionName}`; + it(description, async () => { + const leftOrders = [ + await orderFactoryLeft.newSignedOrderAsync({ + makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(5, 18), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(10, 18), + }), + ]; + const rightOrders = [ + await orderFactoryRight.newSignedOrderAsync({ + makerAddress: makerAddressRight, + takerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(10, 18), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(2, 18), + feeRecipientAddress: feeRecipientAddressRight, + }), + ]; + await web3Wrapper.awaitTransactionSuccessAsync( + await reentrantErc20Token.setReentrantFunction.sendTransactionAsync(functionId), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + const expectedError = new ReentrancyGuardRevertErrors.IllegalReentrancyError(); + const tx = exchangeWrapper.batchMatchOrdersAsync(leftOrders, rightOrders, takerAddress); + return expect(tx).to.revertWith(expectedError); + }); + }); + }; + describe('batchMatchOrdersWithMaximalFill reentrancy tests', () => reentrancyTest(exchangeConstants.FUNCTIONS_WITH_MUTEX)); }); }); // tslint:disable-line:max-file-line-count diff --git a/contracts/exchange/test/utils/constants.ts b/contracts/exchange/test/utils/constants.ts index 9e46f9b476..817ad27acf 100644 --- a/contracts/exchange/test/utils/constants.ts +++ b/contracts/exchange/test/utils/constants.ts @@ -10,6 +10,8 @@ export const constants = { 'MARKET_SELL_ORDERS', 'MATCH_ORDERS', 'MATCH_ORDERS_WITH_MAXIMAL_FILL', + 'BATCH_MATCH_ORDERS', + 'BATCH_MATCH_ORDERS_WITH_MAXIMAL_FILL', 'CANCEL_ORDER', 'BATCH_CANCEL_ORDERS', 'CANCEL_ORDERS_UP_TO', diff --git a/packages/order-utils/src/exchange_revert_errors.ts b/packages/order-utils/src/exchange_revert_errors.ts index 7dd5e94eee..4ef59c6706 100644 --- a/packages/order-utils/src/exchange_revert_errors.ts +++ b/packages/order-utils/src/exchange_revert_errors.ts @@ -7,8 +7,8 @@ import * as _ from 'lodash'; export enum BatchMatchOrdersErrorCodes { ZeroLeftOrders, ZeroRightOrders, - IncompatibleLeftOrders, - IncompatibleRightOrders, + InvalidLengthLeftSignatures, + InvalidLengthRightSignatures, } export enum FillErrorCode {