require txOrigin for RFQ orders, allow origins to register alternate … (#47)

require txOrigin for RFQ orders, allow origins to register alternate addresses that can fill orders

contracts/zero-ex/CHANGELOG.json

@@ -29,6 +29,10 @@
             {
                 "note": "Add metatransaction support for limit orders",
                 "pr": 44
+            },
+            {
+                "note": "Require RFQ orders to specify a transaction origin, and allow approved alternative addresses",
+                "pr": 47
             }
         ]
     },

contracts/zero-ex/contracts/src/features/INativeOrdersFeature.sol

@@ -91,6 +91,17 @@ interface INativeOrdersFeature {
         uint256 minValidSalt
     );
 
+    /// @dev Emitted when a new address is registered or unregistered to fill
+    ///      orders with a given txOrigin.
+    /// @param origin The address doing the registration.
+    /// @param addr The address being registered.
+    /// @param allowed Indicates whether the address should be allowed.
+    event RfqOrderOriginAllowed(
+        address origin,
+        address addr,
+        bool allowed
+    );
+
     /// @dev Transfers protocol fees from the `FeeCollector` pools into
     ///      the staking contract.
     /// @param poolIds Staking pool IDs
@@ -217,6 +228,13 @@ interface INativeOrdersFeature {
     function cancelRfqOrder(LibNativeOrder.RfqOrder calldata order)
         external;
 
+    /// @dev Mark what tx.origin addresses are allowed to fill an order that
+    ///      specifies the message sender as its txOrigin.
+    /// @param origin The origin to update.
+    /// @param allowed True to register, false to unregister.
+    function registerAllowedRfqOrigin(address origin, bool allowed)
+        external;
+
     /// @dev Cancel multiple limit orders. The caller must be the maker.
     ///      Silently succeeds if the order has already been cancelled.
     /// @param orders The limit orders.

contracts/zero-ex/contracts/src/features/NativeOrdersFeature.sol

@@ -146,6 +146,7 @@ contract NativeOrdersFeature is
         _registerFeatureFunction(this.getLimitOrderHash.selector);
         _registerFeatureFunction(this.getRfqOrderHash.selector);
         _registerFeatureFunction(this.getProtocolFeeMultiplier.selector);
+        _registerFeatureFunction(this.registerAllowedRfqOrigin.selector);
         return LibMigrate.MIGRATE_SUCCESS;
     }
 
@@ -554,6 +555,25 @@ contract NativeOrdersFeature is
         );
     }
 
+    /// @dev Mark what tx.origin addresses are allowed to fill an order that
+    ///      specifies the message sender as its txOrigin.
+    /// @param origin The origin to update.
+    /// @param allowed True to register, false to unregister.
+    function registerAllowedRfqOrigin(
+        address origin,
+        bool allowed
+    )
+        external
+        override
+    {
+        LibNativeOrdersStorage.Storage storage stor =
+            LibNativeOrdersStorage.getStorage();
+
+        stor.originRegistry[msg.sender][origin] = allowed;
+
+        emit RfqOrderOriginAllowed(msg.sender, origin, allowed);
+    }
+
     /// @dev Cancel all RFQ orders for a given maker and pair with a salt less
     ///      than the value provided. The caller must be the maker. Subsequent
     ///      calls to this function with the same caller and pair require the
@@ -864,14 +884,19 @@ contract NativeOrdersFeature is
             ).rrevert();
         }
 
+        {
+            LibNativeOrdersStorage.Storage storage stor =
+                LibNativeOrdersStorage.getStorage();
+
             // Must be fillable by the tx.origin.
-        if (order.txOrigin != address(0) && order.txOrigin != tx.origin) {
+            if (order.txOrigin != tx.origin && !stor.originRegistry[order.txOrigin][tx.origin]) {
                 LibNativeOrdersRichErrors.OrderNotFillableByOriginError(
                     orderInfo.orderHash,
                     tx.origin,
                     order.txOrigin
                 ).rrevert();
             }
+        }
 
         // Signature must be valid for the order.
         {

contracts/zero-ex/contracts/src/storage/LibNativeOrdersStorage.sol

@@ -39,6 +39,9 @@ library LibNativeOrdersStorage {
         // for RFQ orders.
         mapping(address => mapping(address => mapping(address => uint256)))
             rfqOrdersMakerToMakerTokenToTakerTokenToMinValidOrderSalt;
+        // For a given order origin, which tx.origin addresses are allowed to
+        // fill the order.
+        mapping(address => mapping(address => bool)) originRegistry;
     }
 
     /// @dev Get the storage bucket for this contract.

contracts/zero-ex/test/features/native_orders_feature_test.ts

@@ -77,7 +77,7 @@ blockchainTests.resets('NativeOrdersFeature', env => {
             verifyingContract,
             takerToken: takerToken.address,
             makerToken: makerToken.address,
-            txOrigin: NULL_ADDRESS,
+            txOrigin: taker,
             ...fields,
         });
     }
@@ -86,10 +86,10 @@ blockchainTests.resets('NativeOrdersFeature', env => {
         await makerToken.mint(maker, order.makerAmount).awaitTransactionSuccessAsync();
         if ('takerTokenFeeAmount' in order) {
             await takerToken
-                .mint(taker, order.takerAmount.plus(order.takerTokenFeeAmount))
+                .mint(_taker, order.takerAmount.plus(order.takerTokenFeeAmount))
                 .awaitTransactionSuccessAsync();
         } else {
-            await takerToken.mint(taker, order.takerAmount).awaitTransactionSuccessAsync();
+            await takerToken.mint(_taker, order.takerAmount).awaitTransactionSuccessAsync();
         }
     }
 
@@ -303,7 +303,7 @@ blockchainTests.resets('NativeOrdersFeature', env => {
         it('filled order', async () => {
             const order = getTestRfqOrder();
             // Fill the order first.
-            await fillRfqOrderAsync(order);
+            await fillRfqOrderAsync(order, order.takerAmount, taker);
             const info = await zeroEx.getRfqOrderInfo(order).callAsync();
             assertOrderInfoEquals(info, {
                 status: OrderStatus.Filled,
@@ -1110,13 +1110,66 @@ blockchainTests.resets('NativeOrdersFeature', env => {
         });
 
         it('cannot fill an order with wrong tx.origin', async () => {
-            const order = getTestRfqOrder({ txOrigin: taker });
+            const order = getTestRfqOrder();
             const tx = fillRfqOrderAsync(order, order.takerAmount, notTaker);
             return expect(tx).to.revertWith(
                 new RevertErrors.OrderNotFillableByOriginError(order.getHash(), notTaker, taker),
             );
         });
 
+        it('can fill an order from a different tx.origin if registered', async () => {
+            const order = getTestRfqOrder();
+
+            const receipt = await zeroEx
+                .registerAllowedRfqOrigin(notTaker, true)
+                .awaitTransactionSuccessAsync({ from: taker });
+            verifyEventsFromLogs(
+                receipt.logs,
+                [
+                    {
+                        origin: taker,
+                        addr: notTaker,
+                        allowed: true,
+                    },
+                ],
+                IZeroExEvents.RfqOrderOriginAllowed,
+            );
+            return fillRfqOrderAsync(order, order.takerAmount, notTaker);
+        });
+
+        it('cannot fill an order with registered then unregistered tx.origin', async () => {
+            const order = getTestRfqOrder();
+
+            await zeroEx.registerAllowedRfqOrigin(notTaker, true).awaitTransactionSuccessAsync({ from: taker });
+            const receipt = await zeroEx
+                .registerAllowedRfqOrigin(notTaker, false)
+                .awaitTransactionSuccessAsync({ from: taker });
+            verifyEventsFromLogs(
+                receipt.logs,
+                [
+                    {
+                        origin: taker,
+                        addr: notTaker,
+                        allowed: false,
+                    },
+                ],
+                IZeroExEvents.RfqOrderOriginAllowed,
+            );
+
+            const tx = fillRfqOrderAsync(order, order.takerAmount, notTaker);
+            return expect(tx).to.revertWith(
+                new RevertErrors.OrderNotFillableByOriginError(order.getHash(), notTaker, taker),
+            );
+        });
+
+        it('cannot fill an order with a zero tx.origin', async () => {
+            const order = getTestRfqOrder({ txOrigin: NULL_ADDRESS });
+            const tx = fillRfqOrderAsync(order, order.takerAmount, notTaker);
+            return expect(tx).to.revertWith(
+                new RevertErrors.OrderNotFillableByOriginError(order.getHash(), notTaker, order.txOrigin),
+            );
+        });
+
         it('cannot fill an expired order', async () => {
             const order = getTestRfqOrder({ expiry: createExpiry(-60) });
             const tx = fillRfqOrderAsync(order);
@@ -1166,7 +1219,7 @@ blockchainTests.resets('NativeOrdersFeature', env => {
                 )
                 .awaitTransactionSuccessAsync({ from: taker, value: ZERO_AMOUNT });
             // The exact revert error depends on whether we are still doing a
-            // token spender fallthroigh, so we won't get too specific.
+            // token spender fallthrough, so we won't get too specific.
             return expect(tx).to.revertWith(new AnyRevertError());
         });
     });