aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorAmir Bandeali <abandeali1@gmail.com>2018-08-25 11:03:28 +0800
committerGitHub <noreply@github.com>2018-08-25 11:03:28 +0800
commit00e7c70b4df04a5a606a674bf13310badd70d527 (patch)
treed97ac9e8230813050cc04860b575cb3941934e70
parent0aa9ed3839b2b3b99967fc98ac92a48656b4ca5b (diff)
parentd652deea232417bbef223bde46d8c12e9922b277 (diff)
downloaddexon-sol-tools-00e7c70b4df04a5a606a674bf13310badd70d527.tar
dexon-sol-tools-00e7c70b4df04a5a606a674bf13310badd70d527.tar.gz
dexon-sol-tools-00e7c70b4df04a5a606a674bf13310badd70d527.tar.bz2
dexon-sol-tools-00e7c70b4df04a5a606a674bf13310badd70d527.tar.lz
dexon-sol-tools-00e7c70b4df04a5a606a674bf13310badd70d527.tar.xz
dexon-sol-tools-00e7c70b4df04a5a606a674bf13310badd70d527.tar.zst
dexon-sol-tools-00e7c70b4df04a5a606a674bf13310badd70d527.zip
Merge pull request #986 from 0xProject/feature/contracts/assertions
Add more assertions to assertValidFill
-rw-r--r--packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol99
-rw-r--r--packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol22
-rw-r--r--packages/contracts/src/2.0.0/protocol/Exchange/mixins/MExchangeCore.sol22
-rw-r--r--packages/types/src/index.ts4
4 files changed, 117 insertions, 30 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 291af3792..be163ec97 100644
--- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol
@@ -196,7 +196,15 @@ contract MixinExchangeCore is
// Fetch taker address
address takerAddress = getCurrentContextAddress();
-
+
+ // Assert that the order is fillable by taker
+ assertFillableOrder(
+ order,
+ orderInfo,
+ takerAddress,
+ signature
+ );
+
// Get amount of takerAsset to fill
uint256 remainingTakerAssetAmount = safeSub(order.takerAssetAmount, orderInfo.orderTakerAssetFilledAmount);
uint256 takerAssetFilledAmount = min256(takerAssetFillAmount, remainingTakerAssetAmount);
@@ -205,10 +213,9 @@ contract MixinExchangeCore is
assertValidFill(
order,
orderInfo,
- takerAddress,
takerAssetFillAmount,
takerAssetFilledAmount,
- signature
+ fillResults.makerAssetFilledAmount
);
// Compute proportional fill amounts
@@ -285,20 +292,16 @@ contract MixinExchangeCore is
order.takerAssetData
);
}
-
+
/// @dev Validates context for fillOrder. Succeeds or throws.
/// @param order to be filled.
/// @param orderInfo OrderStatus, orderHash, and amount already filled of order.
/// @param takerAddress Address of order taker.
- /// @param takerAssetFillAmount Desired amount of order to fill by taker.
- /// @param takerAssetFilledAmount Amount of takerAsset that will be filled.
/// @param signature Proof that the orders was created by its maker.
- function assertValidFill(
+ function assertFillableOrder(
Order memory order,
OrderInfo memory orderInfo,
address takerAddress,
- uint256 takerAssetFillAmount,
- uint256 takerAssetFilledAmount,
bytes memory signature
)
internal
@@ -309,13 +312,7 @@ contract MixinExchangeCore is
orderInfo.orderStatus == uint8(OrderStatus.FILLABLE),
"ORDER_UNFILLABLE"
);
-
- // Revert if fill amount is invalid
- require(
- takerAssetFillAmount != 0,
- "INVALID_TAKER_AMOUNT"
- );
-
+
// Validate sender is allowed to fill this order
if (order.senderAddress != address(0)) {
require(
@@ -323,7 +320,7 @@ contract MixinExchangeCore is
"INVALID_SENDER"
);
}
-
+
// Validate taker is allowed to fill this order
if (order.takerAddress != address(0)) {
require(
@@ -331,7 +328,7 @@ contract MixinExchangeCore is
"INVALID_TAKER"
);
}
-
+
// Validate Maker signature (check only if first time seen)
if (orderInfo.orderTakerAssetFilledAmount == 0) {
require(
@@ -343,7 +340,71 @@ contract MixinExchangeCore is
"INVALID_ORDER_SIGNATURE"
);
}
-
+ }
+
+ /// @dev Validates context for fillOrder. Succeeds or throws.
+ /// @param order to be filled.
+ /// @param orderInfo OrderStatus, orderHash, and amount already filled of order.
+ /// @param takerAssetFillAmount Desired amount of order to fill by taker.
+ /// @param takerAssetFilledAmount Amount of takerAsset that will be filled.
+ /// @param makerAssetFilledAmount Amount of makerAsset that will be transfered.
+ function assertValidFill(
+ Order memory order,
+ OrderInfo memory orderInfo,
+ uint256 takerAssetFillAmount, // TODO: use FillResults
+ uint256 takerAssetFilledAmount,
+ uint256 makerAssetFilledAmount
+ )
+ internal
+ view
+ {
+ // Revert if fill amount is invalid
+ // TODO: reconsider necessity for v2.1
+ require(
+ takerAssetFillAmount != 0,
+ "INVALID_TAKER_AMOUNT"
+ );
+
+ // Make sure taker does not pay more than desired amount
+ // NOTE: This assertion should never fail, it is here
+ // as an extra defence against potential bugs.
+ require(
+ takerAssetFilledAmount <= takerAssetFillAmount,
+ "TAKER_OVERPAY"
+ );
+
+ // Make sure order is not overfilled
+ // NOTE: This assertion should never fail, it is here
+ // as an extra defence against potential bugs.
+ require(
+ safeAdd(orderInfo.orderTakerAssetFilledAmount, takerAssetFilledAmount) <= order.takerAssetAmount,
+ "ORDER_OVERFILL"
+ );
+
+ // Make sure order is filled at acceptable price.
+ // The order has an implied price from the makers perspective:
+ // order price = order.makerAssetAmount / order.takerAssetAmount
+ // i.e. the number of makerAsset maker is paying per takerAsset. The
+ // maker is guaranteed to get this price or a better (lower) one. The
+ // actual price maker is getting in this fill is:
+ // fill price = makerAssetFilledAmount / takerAssetFilledAmount
+ // We need `fill price <= order price` for the fill to be fair to maker.
+ // This amounts to:
+ // makerAssetFilledAmount order.makerAssetAmount
+ // ------------------------ <= -----------------------
+ // takerAssetFilledAmount order.takerAssetAmount
+ // or, equivalently:
+ // makerAssetFilledAmount * order.takerAssetAmount <=
+ // order.makerAssetAmount * takerAssetFilledAmount
+ // NOTE: This assertion should never fail, it is here
+ // as an extra defence against potential bugs.
+ require(
+ safeMul(makerAssetFilledAmount, order.takerAssetAmount)
+ <=
+ safeMul(order.makerAssetAmount, takerAssetFilledAmount),
+ "INVALID_FILL_PRICE"
+ );
+
// Validate fill order rounding
require(
!isRoundingErrorFloor(
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 4b8360c23..075a610b5 100644
--- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol
@@ -64,8 +64,20 @@ contract MixinMatchOrders is
// Fetch taker address
address takerAddress = getCurrentContextAddress();
-
+
// Either our context is valid or we revert
+ assertFillableOrder(
+ leftOrder,
+ leftOrderInfo,
+ takerAddress,
+ leftSignature
+ );
+ assertFillableOrder(
+ rightOrder,
+ rightOrderInfo,
+ takerAddress,
+ rightSignature
+ );
assertValidMatch(leftOrder, rightOrder);
// Compute proportional fill amounts
@@ -80,20 +92,18 @@ contract MixinMatchOrders is
assertValidFill(
leftOrder,
leftOrderInfo,
- takerAddress,
matchedFillResults.left.takerAssetFilledAmount,
matchedFillResults.left.takerAssetFilledAmount,
- leftSignature
+ matchedFillResults.left.makerAssetFilledAmount
);
assertValidFill(
rightOrder,
rightOrderInfo,
- takerAddress,
matchedFillResults.right.takerAssetFilledAmount,
matchedFillResults.right.takerAssetFilledAmount,
- rightSignature
+ matchedFillResults.right.makerAssetFilledAmount
);
-
+
// Update exchange state
updateFilledState(
leftOrder,
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 41832fe4b..d85913e0f 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
@@ -96,21 +96,33 @@ contract MExchangeCore is
bytes32 orderHash
)
internal;
-
+
/// @dev Validates context for fillOrder. Succeeds or throws.
/// @param order to be filled.
- /// @param orderInfo Status, orderHash, and amount already filled of order.
+ /// @param orderInfo OrderStatus, orderHash, and amount already filled of order.
/// @param takerAddress Address of order taker.
+ /// @param signature Proof that the orders was created by its maker.
+ function assertFillableOrder(
+ LibOrder.Order memory order,
+ LibOrder.OrderInfo memory orderInfo,
+ address takerAddress,
+ bytes memory signature
+ )
+ internal
+ view;
+
+ /// @dev Validates context for fillOrder. Succeeds or throws.
+ /// @param order to be filled.
+ /// @param orderInfo Status, orderHash, and amount already filled of order.
/// @param takerAssetFillAmount Desired amount of order to fill by taker.
/// @param takerAssetFilledAmount Amount of takerAsset that will be filled.
- /// @param signature Proof that the orders was created by its maker.
+ /// @param makerAssetFilledAmount Amount of makerAsset that will be transfered.
function assertValidFill(
LibOrder.Order memory order,
LibOrder.OrderInfo memory orderInfo,
- address takerAddress,
uint256 takerAssetFillAmount,
uint256 takerAssetFilledAmount,
- bytes memory signature
+ uint256 makerAssetFilledAmount
)
internal
view;
diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts
index f07d7f756..d8bffccf9 100644
--- a/packages/types/src/index.ts
+++ b/packages/types/src/index.ts
@@ -165,6 +165,7 @@ export interface ERC721AssetData {
tokenId: BigNumber;
}
+// TODO: DRY. These should be extracted from contract code.
export enum RevertReason {
OrderUnfillable = 'ORDER_UNFILLABLE',
InvalidMaker = 'INVALID_MAKER',
@@ -177,6 +178,9 @@ export enum RevertReason {
InvalidSignature = 'INVALID_SIGNATURE',
SignatureIllegal = 'SIGNATURE_ILLEGAL',
SignatureUnsupported = 'SIGNATURE_UNSUPPORTED',
+ TakerOverpay = 'TAKER_OVERPAY',
+ OrderOverfill = 'ORDER_OVERFILL',
+ InvalidFillPrice = 'INVALID_FILL_PRICE',
InvalidNewOrderEpoch = 'INVALID_NEW_ORDER_EPOCH',
CompleteFillFailed = 'COMPLETE_FILL_FAILED',
NegativeSpreadRequired = 'NEGATIVE_SPREAD_REQUIRED',