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

Always validate callback signatures & add support for EIP-1271 signatures #1885

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
05c6024
Update generated wrappers for coordinator and exchange.
dorothy-zbornak Mar 26, 2019
d835071
`@0x/base-contract`: Make `PromiseWithTransactionHash` fully compatib…
dorothy-zbornak Jun 19, 2019
4e86c53
`@0x/contracts-exchange`: Always check `OrderValidator` and `WalletOr…
dorothy-zbornak Jun 21, 2019
b1439cd
`@0x/utils`: Add `toStringTag` symbol to `RevertError`
dorothy-zbornak Jun 21, 2019
a9e1a26
`@0x/contracts-exchange`: Update changelog
dorothy-zbornak Jun 21, 2019
ce6f447
`@0x/order-utils`: Add `validator` field to `SignatureOrderValidatorE…
dorothy-zbornak Jun 21, 2019
18cc4b7
`@0x/contracts-exchange`: Have `TestValidatorWallet` always accept `W…
dorothy-zbornak Jun 21, 2019
cad5dd8
`@0x/types`: Rename `WalletOrderValidator` to `OrderWallet` in `Signa…
dorothy-zbornak Jun 22, 2019
89ca1e0
`@0x/order-utils`: Rename `SignatureWalletOrderValidatorError` to `Si…
dorothy-zbornak Jun 22, 2019
577a721
`@0x/contracts-exchange`: Rename `WalletOrderValidator` to `OrderWall…
dorothy-zbornak Jun 22, 2019
81f6265
Run prettier/linter
dorothy-zbornak Jun 22, 2019
56dbaa5
Update changelogs
dorothy-zbornak Jun 22, 2019
0e9d1cf
Rebase against 3.0
dorothy-zbornak Jun 24, 2019
d5623cf
`@0x/contracts-utils`: Add LibEIP1271.sol
dorothy-zbornak Jun 25, 2019
e3c4442
`@0x/contracts-exchange`: Fix comments in `test/signature_validator.ts`
dorothy-zbornak Jun 25, 2019
c6761b2
`@0x/contracts-exchange`: Refactor out `EIP1271_MAGIC_VALUE` into a `…
dorothy-zbornak Jun 25, 2019
6093a73
`@0x/contracts-exchange`: Add comments about preserving signatures in…
dorothy-zbornak Jun 25, 2019
1186820
Fix linter errors
dorothy-zbornak Jun 25, 2019
24b21ed
`@0x/contracts-exchange`: Consolidate signature types.
dorothy-zbornak Jun 25, 2019
acdcb1b
`@0x/contracts-exchange`: Fix linearization issues.
dorothy-zbornak Jun 25, 2019
c0aa42b
`@0x/types`: Consolidate signature types.
dorothy-zbornak Jun 25, 2019
c08a618
`@0x/order-utils`: Remove unused exchange revert errors
dorothy-zbornak Jun 25, 2019
721de3c
`@0x/contracts-exchange`: Add `isValidHashSignature()` back.
dorothy-zbornak Jun 26, 2019
51638fc
`@0x/contracts-exchange`: Remove `isValidOrderSignature()` from `IWal…
dorothy-zbornak Jun 26, 2019
1f28d67
`@0x/contracts-test-utils`: Add `hexConcat()` in `hex_utils.ts`.
dorothy-zbornak Jun 26, 2019
13e0433
`@0x/contracts-exchange`: Update tests for new/consolidated signature…
dorothy-zbornak Jun 26, 2019
3ab8eab
`@0x/contracts-exchange`: Update CHANGELOG
dorothy-zbornak Jun 26, 2019
8e9c185
`@0x/order-utils`: Update CHANGELOG
dorothy-zbornak Jun 26, 2019
a099f05
`@0x/contracts-exchange`: Run `contracts:gen`
dorothy-zbornak Jun 26, 2019
e7bde04
`@0x/contracts-exchange`: Make `assertValidFill` and `calculateMatche…
dorothy-zbornak Jun 26, 2019
a966abd
`@0x/contracts-exchange`: Cosmetic changes to contracts.
dorothy-zbornak Jun 26, 2019
c2f4d45
`@0x/contracts-exchange`: Make `assertValidFill()` internal again.
dorothy-zbornak Jul 1, 2019
896cd51
`@0x/contracts-exchange`: REALLY make `assertValidFill()` internal ag…
dorothy-zbornak Jul 1, 2019
d85e9c7
`@0x/contracts-exchange`: Use named return values in `MixinSignatureV…
dorothy-zbornak Jul 1, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
@0x/contracts-exchange: Make assertValidFill and `calculateMatche…
…dFillResults` public
  • Loading branch information
dorothy-zbornak committed Jun 26, 2019
commit e7bde04d7f634706c5374e8582490d884149834d
4 changes: 4 additions & 0 deletions contracts/exchange/CHANGELOG.json
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,10 @@
{
"note": "Add `validatorAddress` field to `SignatureValidatorError` rich reverts",
"pr": 1885
},
{
"note": "Make `assertValidFill` and `calculateMatchedFillResults` public",
"pr": 1885
}
]
},
Expand Down
122 changes: 61 additions & 61 deletions contracts/exchange/contracts/src/MixinExchangeCore.sol
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,66 @@ contract MixinExchangeCore is
return orderInfo;
}

/// @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
)
public
dorothy-zbornak marked this conversation as resolved.
Show resolved Hide resolved
pure
{
// Revert if fill amount is invalid
// TODO: reconsider necessity for v2.1
if (takerAssetFillAmount == 0) {
_rrevert(FillError(FillErrorCodes.INVALID_TAKER_AMOUNT, orderInfo.orderHash));
}

// 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.
if (takerAssetFilledAmount > takerAssetFillAmount) {
_rrevert(FillError(FillErrorCodes.TAKER_OVERPAY, orderInfo.orderHash));
}

// Make sure order is not overfilled
// NOTE: This assertion should never fail, it is here
// as an extra defence against potential bugs.
if (_safeAdd(orderInfo.orderTakerAssetFilledAmount, takerAssetFilledAmount)
> order.takerAssetAmount) {
_rrevert(FillError(FillErrorCodes.OVERFILL, orderInfo.orderHash));
}

// 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.
if (_safeMul(makerAssetFilledAmount, order.takerAssetAmount)
> _safeMul(order.makerAssetAmount, takerAssetFilledAmount)) {
_rrevert(FillError(FillErrorCodes.INVALID_FILL_PRICE, orderInfo.orderHash));
}
}

/// @dev Fills the input order.
/// @param order Order struct containing order specifications.
/// @param takerAssetFillAmount Desired amount of takerAsset to sell.
Expand Down Expand Up @@ -203,7 +263,7 @@ contract MixinExchangeCore is
uint256 takerAssetFilledAmount = _min256(takerAssetFillAmount, remainingTakerAssetAmount);

// Validate context
_assertValidFill(
assertValidFill(
order,
orderInfo,
takerAssetFillAmount,
Expand Down Expand Up @@ -372,66 +432,6 @@ contract MixinExchangeCore is
}
}

/// @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
pure
{
// Revert if fill amount is invalid
// TODO: reconsider necessity for v2.1
if (takerAssetFillAmount == 0) {
_rrevert(FillError(FillErrorCodes.INVALID_TAKER_AMOUNT, orderInfo.orderHash));
}

// 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.
if (takerAssetFilledAmount > takerAssetFillAmount) {
_rrevert(FillError(FillErrorCodes.TAKER_OVERPAY, orderInfo.orderHash));
}

// Make sure order is not overfilled
// NOTE: This assertion should never fail, it is here
// as an extra defence against potential bugs.
if (_safeAdd(orderInfo.orderTakerAssetFilledAmount, takerAssetFilledAmount)
> order.takerAssetAmount) {
_rrevert(FillError(FillErrorCodes.OVERFILL, orderInfo.orderHash));
}

// 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.
if (_safeMul(makerAssetFilledAmount, order.takerAssetAmount)
> _safeMul(order.makerAssetAmount, takerAssetFilledAmount)) {
_rrevert(FillError(FillErrorCodes.INVALID_FILL_PRICE, orderInfo.orderHash));
}
}

/// @dev Validates context for cancelOrder. Succeeds or throws.
/// @param order to be cancelled.
/// @param orderInfo OrderStatus, orderHash, and amount already filled of order.
Expand Down
64 changes: 32 additions & 32 deletions contracts/exchange/contracts/src/MixinMatchOrders.sol
Original file line number Diff line number Diff line change
Expand Up @@ -81,22 +81,22 @@ contract MixinMatchOrders is
_assertValidMatch(leftOrder, rightOrder);

// Compute proportional fill amounts
matchedFillResults = _calculateMatchedFillResults(
matchedFillResults = calculateMatchedFillResults(
leftOrder,
rightOrder,
leftOrderInfo.orderTakerAssetFilledAmount,
rightOrderInfo.orderTakerAssetFilledAmount
);

// Validate fill contexts
_assertValidFill(
assertValidFill(
leftOrder,
leftOrderInfo,
matchedFillResults.left.takerAssetFilledAmount,
matchedFillResults.left.takerAssetFilledAmount,
matchedFillResults.left.makerAssetFilledAmount
);
_assertValidFill(
assertValidFill(
rightOrder,
rightOrderInfo,
matchedFillResults.right.takerAssetFilledAmount,
Expand Down Expand Up @@ -133,33 +133,6 @@ contract MixinMatchOrders is
return matchedFillResults;
}

/// @dev Validates context for matchOrders. Succeeds or throws.
/// @param leftOrder First order to match.
/// @param rightOrder Second order to match.
function _assertValidMatch(
LibOrder.Order memory leftOrder,
LibOrder.Order memory rightOrder
)
internal
view
{
// Make sure there is a profitable spread.
// There is a profitable spread iff the cost per unit bought (OrderA.MakerAmount/OrderA.TakerAmount) for each order is greater
// than the profit per unit sold of the matched order (OrderB.TakerAmount/OrderB.MakerAmount).
// This is satisfied by the equations below:
// <leftOrder.makerAssetAmount> / <leftOrder.takerAssetAmount> >= <rightOrder.takerAssetAmount> / <rightOrder.makerAssetAmount>
// AND
// <rightOrder.makerAssetAmount> / <rightOrder.takerAssetAmount> >= <leftOrder.takerAssetAmount> / <leftOrder.makerAssetAmount>
// These equations can be combined to get the following:
if (_safeMul(leftOrder.makerAssetAmount, rightOrder.makerAssetAmount) <
_safeMul(leftOrder.takerAssetAmount, rightOrder.takerAssetAmount)) {
_rrevert(NegativeSpreadError(
getOrderHash(leftOrder),
getOrderHash(rightOrder)
));
}
}

/// @dev Calculates fill amounts for the matched orders.
/// Each order is filled at their respective price point. However, the calculations are
/// carried out as though the orders are both being filled at the right order's price point.
Expand All @@ -169,13 +142,13 @@ contract MixinMatchOrders is
/// @param leftOrderTakerAssetFilledAmount Amount of left order already filled.
/// @param rightOrderTakerAssetFilledAmount Amount of right order already filled.
/// @param matchedFillResults Amounts to fill and fees to pay by maker and taker of matched orders.
function _calculateMatchedFillResults(
function calculateMatchedFillResults(
LibOrder.Order memory leftOrder,
LibOrder.Order memory rightOrder,
uint256 leftOrderTakerAssetFilledAmount,
uint256 rightOrderTakerAssetFilledAmount
)
internal
public
pure
returns (LibFillResults.MatchedFillResults memory matchedFillResults)
{
Expand Down Expand Up @@ -262,6 +235,33 @@ contract MixinMatchOrders is
return matchedFillResults;
}

/// @dev Validates context for matchOrders. Succeeds or throws.
/// @param leftOrder First order to match.
/// @param rightOrder Second order to match.
function _assertValidMatch(
LibOrder.Order memory leftOrder,
LibOrder.Order memory rightOrder
)
internal
view
{
// Make sure there is a profitable spread.
// There is a profitable spread iff the cost per unit bought (OrderA.MakerAmount/OrderA.TakerAmount) for each order is greater
// than the profit per unit sold of the matched order (OrderB.TakerAmount/OrderB.MakerAmount).
// This is satisfied by the equations below:
// <leftOrder.makerAssetAmount> / <leftOrder.takerAssetAmount> >= <rightOrder.takerAssetAmount> / <rightOrder.makerAssetAmount>
// AND
// <rightOrder.makerAssetAmount> / <rightOrder.takerAssetAmount> >= <leftOrder.takerAssetAmount> / <leftOrder.makerAssetAmount>
// These equations can be combined to get the following:
if (_safeMul(leftOrder.makerAssetAmount, rightOrder.makerAssetAmount) <
_safeMul(leftOrder.takerAssetAmount, rightOrder.takerAssetAmount)) {
_rrevert(NegativeSpreadError(
getOrderHash(leftOrder),
getOrderHash(rightOrder)
));
}
}

/// @dev Settles matched order by transferring appropriate funds between order makers, taker, and fee recipient.
/// @param leftOrderHash First matched order hash.
/// @param rightOrderHash Second matched order hash.
Expand Down
16 changes: 16 additions & 0 deletions contracts/exchange/contracts/src/interfaces/IExchangeCore.sol
Original file line number Diff line number Diff line change
Expand Up @@ -99,4 +99,20 @@ contract IExchangeCore {
public
view
returns (bytes32 orderHash);

/// @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(
LibOrder.Order memory order,
LibOrder.OrderInfo memory orderInfo,
uint256 takerAssetFillAmount, // TODO: use FillResults
uint256 takerAssetFilledAmount,
uint256 makerAssetFilledAmount
)
public
pure;
}
19 changes: 19 additions & 0 deletions contracts/exchange/contracts/src/interfaces/IMatchOrders.sol
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,23 @@ contract IMatchOrders {
)
public
returns (LibFillResults.MatchedFillResults memory matchedFillResults);

/// @dev Calculates fill amounts for the matched orders.
/// Each order is filled at their respective price point. However, the calculations are
/// carried out as though the orders are both being filled at the right order's price point.
/// The profit made by the leftOrder order goes to the taker (who matched the two orders).
/// @param leftOrder First order to match.
/// @param rightOrder Second order to match.
/// @param leftOrderTakerAssetFilledAmount Amount of left order already filled.
/// @param rightOrderTakerAssetFilledAmount Amount of right order already filled.
/// @param matchedFillResults Amounts to fill and fees to pay by maker and taker of matched orders.
function calculateMatchedFillResults(
LibOrder.Order memory leftOrder,
LibOrder.Order memory rightOrder,
uint256 leftOrderTakerAssetFilledAmount,
uint256 rightOrderTakerAssetFilledAmount
)
public
pure
returns (LibFillResults.MatchedFillResults memory matchedFillResults);
}