From 841e4ee666fde649a51b682a4cfbb15fb8945af3 Mon Sep 17 00:00:00 2001 From: Steve Marx Date: Tue, 24 Nov 2020 16:53:32 -0500 Subject: [PATCH] =?UTF-8?q?require=20txOrigin=20for=20RFQ=20orders,=20allo?= =?UTF-8?q?w=20origins=20to=20register=20alternate=20=E2=80=A6=20(#47)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit require txOrigin for RFQ orders, allow origins to register alternate addresses that can fill orders --- contracts/zero-ex/CHANGELOG.json | 4 ++ .../src/features/INativeOrdersFeature.sol | 18 +++++ .../src/features/NativeOrdersFeature.sol | 39 +++++++++-- .../src/storage/LibNativeOrdersStorage.sol | 3 + .../features/native_orders_feature_test.ts | 65 +++++++++++++++++-- 5 files changed, 116 insertions(+), 13 deletions(-) diff --git a/contracts/zero-ex/CHANGELOG.json b/contracts/zero-ex/CHANGELOG.json index d491bbd3ec..9b32fdedd9 100644 --- a/contracts/zero-ex/CHANGELOG.json +++ b/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 } ] }, diff --git a/contracts/zero-ex/contracts/src/features/INativeOrdersFeature.sol b/contracts/zero-ex/contracts/src/features/INativeOrdersFeature.sol index 0df85ab6be..8b83f9e414 100644 --- a/contracts/zero-ex/contracts/src/features/INativeOrdersFeature.sol +++ b/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. diff --git a/contracts/zero-ex/contracts/src/features/NativeOrdersFeature.sol b/contracts/zero-ex/contracts/src/features/NativeOrdersFeature.sol index ca75d3ef0e..0108aef96f 100644 --- a/contracts/zero-ex/contracts/src/features/NativeOrdersFeature.sol +++ b/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,13 +884,18 @@ contract NativeOrdersFeature is ).rrevert(); } - // Must be fillable by the tx.origin. - if (order.txOrigin != address(0) && order.txOrigin != tx.origin) { - LibNativeOrdersRichErrors.OrderNotFillableByOriginError( - orderInfo.orderHash, - tx.origin, - order.txOrigin - ).rrevert(); + { + LibNativeOrdersStorage.Storage storage stor = + LibNativeOrdersStorage.getStorage(); + + // Must be fillable by the 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. diff --git a/contracts/zero-ex/contracts/src/storage/LibNativeOrdersStorage.sol b/contracts/zero-ex/contracts/src/storage/LibNativeOrdersStorage.sol index 093dc79607..5cd43d5461 100644 --- a/contracts/zero-ex/contracts/src/storage/LibNativeOrdersStorage.sol +++ b/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. diff --git a/contracts/zero-ex/test/features/native_orders_feature_test.ts b/contracts/zero-ex/test/features/native_orders_feature_test.ts index 298e282e8a..ba04feb165 100644 --- a/contracts/zero-ex/test/features/native_orders_feature_test.ts +++ b/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()); }); });