Merge pull request #986 from 0xProject/feature/contracts/assertions

Add more assertions to assertValidFill

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