crowetic/protocol
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Addressed review comments

Browse Source
This commit is contained in:
Alex Towle
2019-07-17 11:02:15 -05:00
committed by Amir Bandeali
parent 4ef8b7f733
commit 50b22c673e
6 changed files with 138 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files

32
contracts/exchange/contracts/src/MixinMatchOrders.sol
View File

@@ -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;

4
contracts/exchange/contracts/src/interfaces/IExchangeRichErrors.sol
View File

@@ -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 {

42
contracts/exchange/contracts/test/ReentrantERC20Token.sol
View File

@@ -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
)

69
contracts/exchange/test/match_orders.ts
View File

@@ -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,39 @@ 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
contracts/exchange/test/utils/constants.ts
View File

@@ -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',

4
packages/order-utils/src/exchange_revert_errors.ts
View File

@@ -7,8 +7,8 @@ import * as _ from 'lodash';
export enum BatchMatchOrdersErrorCodes {
ZeroLeftOrders,
ZeroRightOrders,
IncompatibleLeftOrders,
IncompatibleRightOrders,
InvalidLengthLeftSignatures,
InvalidLengthRightSignatures,
}
export enum FillErrorCode {