Skip to content
This repository has been archived by the owner on Jul 9, 2021. It is now read-only.

Commit

Permalink
Addressed review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@jalextowle
jalextowle committed Jul 17, 2019
1 parent 4462631 commit d469606
Show file tree
Hide file tree
Showing 6 changed files with 137 additions and 15 deletions.
32 changes: 23 additions & 9 deletions contracts/exchange/contracts/src/MixinMatchOrders.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -289,15 +289,15 @@ 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,
leftOrder,
rightMakerAssetAmountRemaining,
rightTakerAssetAmountRemaining
);
} else {
} else if (rightMakerAssetAmountRemaining > leftTakerAssetAmountRemaining) {
// Case 2: Left order is fully filled
matchedFillResults.left.makerAssetFilledAmount = leftMakerAssetAmountRemaining;
matchedFillResults.left.takerAssetFilledAmount = leftTakerAssetAmountRemaining;
Expand All @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
));
}

Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions contracts/exchange/contracts/src/interfaces/IExchangeRichErrors.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
42 changes: 42 additions & 0 deletions contracts/exchange/contracts/test/ReentrantERC20Token.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
)
Expand Down
68 changes: 66 additions & 2 deletions contracts/exchange/test/match_orders.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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
2 changes: 2 additions & 0 deletions contracts/exchange/test/utils/constants.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
4 changes: 2 additions & 2 deletions packages/order-utils/src/exchange_revert_errors.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ import * as _ from 'lodash';
export enum BatchMatchOrdersErrorCodes {
ZeroLeftOrders,
ZeroRightOrders,
IncompatibleLeftOrders,
IncompatibleRightOrders,
InvalidLengthLeftSignatures,
InvalidLengthRightSignatures,
}

export enum FillErrorCode {
Expand Down

0 comments on commit d469606

Please sign in to comment.