From ad337271d344d40c1e8181a4f6acc6e820ae3349 Mon Sep 17 00:00:00 2001 From: Steve Marx Date: Wed, 25 Nov 2020 14:08:15 -0500 Subject: [PATCH] Miscellaneous fixes to RFQ origin registration (#48) * Contracts can no longer call registerAllowedRfqOrigins() * registerAllowedRfqOrigins() is now plural, registers an array of addresses --- .../src/features/INativeOrdersFeature.sol | 14 ++++---- .../src/features/NativeOrdersFeature.sol | 21 +++++++----- .../test/TestRfqOriginRegistration.sol | 34 +++++++++++++++++++ contracts/zero-ex/package.json | 2 +- contracts/zero-ex/test/artifacts.ts | 2 ++ .../features/native_orders_feature_test.ts | 32 ++++++++++++----- contracts/zero-ex/test/wrappers.ts | 1 + contracts/zero-ex/tsconfig.json | 1 + 8 files changed, 83 insertions(+), 24 deletions(-) create mode 100644 contracts/zero-ex/contracts/test/TestRfqOriginRegistration.sol diff --git a/contracts/zero-ex/contracts/src/features/INativeOrdersFeature.sol b/contracts/zero-ex/contracts/src/features/INativeOrdersFeature.sol index b5c239adcf..8e65340956 100644 --- a/contracts/zero-ex/contracts/src/features/INativeOrdersFeature.sol +++ b/contracts/zero-ex/contracts/src/features/INativeOrdersFeature.sol @@ -89,14 +89,14 @@ interface INativeOrdersFeature { uint256 minValidSalt ); - /// @dev Emitted when a new address is registered or unregistered to fill + /// @dev Emitted when new addresses are allowed or disallowed to fill /// orders with a given txOrigin. - /// @param origin The address doing the registration. - /// @param addr The address being registered. + /// @param origin The address doing the allowing. + /// @param addrs The address being allowed/disallowed. /// @param allowed Indicates whether the address should be allowed. - event RfqOrderOriginAllowed( + event RfqOrderOriginsAllowed( address origin, - address addr, + address[] addrs, bool allowed ); @@ -220,9 +220,9 @@ interface INativeOrdersFeature { /// @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 origins An array of origin addresses to update. /// @param allowed True to register, false to unregister. - function registerAllowedRfqOrigin(address origin, bool allowed) + function registerAllowedRfqOrigins(address[] memory origins, bool allowed) external; /// @dev Cancel multiple limit orders. The caller must be the maker. diff --git a/contracts/zero-ex/contracts/src/features/NativeOrdersFeature.sol b/contracts/zero-ex/contracts/src/features/NativeOrdersFeature.sol index a75e2d576d..c48c40bfdc 100644 --- a/contracts/zero-ex/contracts/src/features/NativeOrdersFeature.sol +++ b/contracts/zero-ex/contracts/src/features/NativeOrdersFeature.sol @@ -146,7 +146,7 @@ contract NativeOrdersFeature is _registerFeatureFunction(this.getLimitOrderHash.selector); _registerFeatureFunction(this.getRfqOrderHash.selector); _registerFeatureFunction(this.getProtocolFeeMultiplier.selector); - _registerFeatureFunction(this.registerAllowedRfqOrigin.selector); + _registerFeatureFunction(this.registerAllowedRfqOrigins.selector); return LibMigrate.MIGRATE_SUCCESS; } @@ -493,7 +493,7 @@ contract NativeOrdersFeature is require( makerTokens.length == takerTokens.length && makerTokens.length == minValidSalts.length, - "LimitOrdersFeature/MISMATCHED_PAIR_ORDERS_ARRAY_LENGTHS" + "NativeOrdersFeature/MISMATCHED_PAIR_ORDERS_ARRAY_LENGTHS" ); for (uint256 i = 0; i < makerTokens.length; ++i) { @@ -551,21 +551,26 @@ 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 origins An array of origin addresses to update. /// @param allowed True to register, false to unregister. - function registerAllowedRfqOrigin( - address origin, + function registerAllowedRfqOrigins( + address[] memory origins, bool allowed ) external override { + require(msg.sender == tx.origin, + "NativeOrdersFeature/NO_CONTRACT_ORIGINS"); + LibNativeOrdersStorage.Storage storage stor = LibNativeOrdersStorage.getStorage(); - stor.originRegistry[msg.sender][origin] = allowed; + for (uint256 i = 0; i < origins.length; i++) { + stor.originRegistry[msg.sender][origins[i]] = allowed; + } - emit RfqOrderOriginAllowed(msg.sender, origin, allowed); + emit RfqOrderOriginsAllowed(msg.sender, origins, allowed); } /// @dev Cancel all RFQ orders for a given maker and pair with a salt less @@ -586,7 +591,7 @@ contract NativeOrdersFeature is require( makerTokens.length == takerTokens.length && makerTokens.length == minValidSalts.length, - "LimitOrdersFeature/MISMATCHED_PAIR_ORDERS_ARRAY_LENGTHS" + "NativeOrdersFeature/MISMATCHED_PAIR_ORDERS_ARRAY_LENGTHS" ); for (uint256 i = 0; i < makerTokens.length; ++i) { diff --git a/contracts/zero-ex/contracts/test/TestRfqOriginRegistration.sol b/contracts/zero-ex/contracts/test/TestRfqOriginRegistration.sol new file mode 100644 index 0000000000..45b121644f --- /dev/null +++ b/contracts/zero-ex/contracts/test/TestRfqOriginRegistration.sol @@ -0,0 +1,34 @@ +/* + + Copyright 2020 ZeroEx Intl. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + +*/ + +pragma solidity ^0.6.5; +pragma experimental ABIEncoderV2; + +import "../src/features/INativeOrdersFeature.sol"; + +contract TestRfqOriginRegistration { + function registerAllowedRfqOrigins( + INativeOrdersFeature feature, + address[] memory origins, + bool allowed + ) + external + { + feature.registerAllowedRfqOrigins(origins, allowed); + } +} diff --git a/contracts/zero-ex/package.json b/contracts/zero-ex/package.json index 134392de3f..fb05b09f35 100644 --- a/contracts/zero-ex/package.json +++ b/contracts/zero-ex/package.json @@ -42,7 +42,7 @@ "config": { "publicInterfaceContracts": "IZeroEx,ZeroEx,FullMigration,InitialMigration,IFlashWallet,IAllowanceTarget,IERC20Transformer,IOwnableFeature,ISimpleFunctionRegistryFeature,ITokenSpenderFeature,ITransformERC20Feature,FillQuoteTransformer,PayTakerTransformer,WethTransformer,OwnableFeature,SimpleFunctionRegistryFeature,TransformERC20Feature,TokenSpenderFeature,AffiliateFeeTransformer,SignatureValidatorFeature,MetaTransactionsFeature,LogMetadataTransformer,BridgeAdapter,LiquidityProviderFeature,ILiquidityProviderFeature,NativeOrdersFeature,INativeOrdersFeature", "abis:comment": "This list is auto-generated by contracts-gen. Don't edit manually.", - "abis": "./test/generated-artifacts/@(AffiliateFeeTransformer|AllowanceTarget|BootstrapFeature|BridgeAdapter|FeeCollector|FillQuoteTransformer|FixinCommon|FixinEIP712|FixinProtocolFees|FixinReentrancyGuard|FixinTokenSpender|FlashWallet|FullMigration|IAllowanceTarget|IBootstrapFeature|IBridgeAdapter|IERC20Bridge|IERC20Transformer|IExchange|IFeature|IFlashWallet|IGasToken|ILiquidityProvider|ILiquidityProviderFeature|ILiquidityProviderSandbox|IMetaTransactionsFeature|INativeOrdersFeature|IOwnableFeature|ISignatureValidatorFeature|ISimpleFunctionRegistryFeature|IStaking|ITestSimpleFunctionRegistryFeature|ITokenSpenderFeature|ITransformERC20Feature|IUniswapFeature|IZeroEx|InitialMigration|LibBootstrap|LibCommonRichErrors|LibERC20Transformer|LibLiquidityProviderRichErrors|LibMetaTransactionsRichErrors|LibMetaTransactionsStorage|LibMigrate|LibNativeOrder|LibNativeOrdersRichErrors|LibNativeOrdersStorage|LibOrderHash|LibOwnableRichErrors|LibOwnableStorage|LibProxyRichErrors|LibProxyStorage|LibReentrancyGuardStorage|LibSignature|LibSignatureRichErrors|LibSignedCallData|LibSimpleFunctionRegistryRichErrors|LibSimpleFunctionRegistryStorage|LibSpenderRichErrors|LibStorage|LibTokenSpenderStorage|LibTransformERC20RichErrors|LibTransformERC20Storage|LibWalletRichErrors|LiquidityProviderFeature|LiquidityProviderSandbox|LogMetadataTransformer|MetaTransactionsFeature|MixinAdapterAddresses|MixinBalancer|MixinCurve|MixinDodo|MixinKyber|MixinMStable|MixinMooniswap|MixinOasis|MixinShell|MixinSushiswap|MixinUniswap|MixinUniswapV2|MixinZeroExBridge|NativeOrdersFeature|OwnableFeature|PayTakerTransformer|SignatureValidatorFeature|SimpleFunctionRegistryFeature|TestBridge|TestCallTarget|TestDelegateCaller|TestFillQuoteTransformerBridge|TestFillQuoteTransformerExchange|TestFillQuoteTransformerHost|TestFixinProtocolFees|TestFixinTokenSpender|TestFullMigration|TestInitialMigration|TestLibNativeOrder|TestLibSignature|TestLiquidityProvider|TestMetaTransactionsNativeOrdersFeature|TestMetaTransactionsTransformERC20Feature|TestMigrator|TestMintTokenERC20Transformer|TestMintableERC20Token|TestNativeOrdersFeature|TestSimpleFunctionRegistryFeatureImpl1|TestSimpleFunctionRegistryFeatureImpl2|TestStaking|TestTokenSpender|TestTokenSpenderERC20Token|TestTransformERC20|TestTransformerBase|TestTransformerDeployerTransformer|TestTransformerHost|TestWeth|TestWethTransformerHost|TestZeroExFeature|TokenSpenderFeature|TransformERC20Feature|Transformer|TransformerDeployer|UniswapFeature|WethTransformer|ZeroEx|ZeroExOptimized).json" + "abis": "./test/generated-artifacts/@(AffiliateFeeTransformer|AllowanceTarget|BootstrapFeature|BridgeAdapter|FeeCollector|FillQuoteTransformer|FixinCommon|FixinEIP712|FixinProtocolFees|FixinReentrancyGuard|FixinTokenSpender|FlashWallet|FullMigration|IAllowanceTarget|IBootstrapFeature|IBridgeAdapter|IERC20Bridge|IERC20Transformer|IExchange|IFeature|IFlashWallet|IGasToken|ILiquidityProvider|ILiquidityProviderFeature|ILiquidityProviderSandbox|IMetaTransactionsFeature|INativeOrdersFeature|IOwnableFeature|ISignatureValidatorFeature|ISimpleFunctionRegistryFeature|IStaking|ITestSimpleFunctionRegistryFeature|ITokenSpenderFeature|ITransformERC20Feature|IUniswapFeature|IZeroEx|InitialMigration|LibBootstrap|LibCommonRichErrors|LibERC20Transformer|LibLiquidityProviderRichErrors|LibMetaTransactionsRichErrors|LibMetaTransactionsStorage|LibMigrate|LibNativeOrder|LibNativeOrdersRichErrors|LibNativeOrdersStorage|LibOrderHash|LibOwnableRichErrors|LibOwnableStorage|LibProxyRichErrors|LibProxyStorage|LibReentrancyGuardStorage|LibSignature|LibSignatureRichErrors|LibSignedCallData|LibSimpleFunctionRegistryRichErrors|LibSimpleFunctionRegistryStorage|LibSpenderRichErrors|LibStorage|LibTokenSpenderStorage|LibTransformERC20RichErrors|LibTransformERC20Storage|LibWalletRichErrors|LiquidityProviderFeature|LiquidityProviderSandbox|LogMetadataTransformer|MetaTransactionsFeature|MixinAdapterAddresses|MixinBalancer|MixinCurve|MixinDodo|MixinKyber|MixinMStable|MixinMooniswap|MixinOasis|MixinShell|MixinSushiswap|MixinUniswap|MixinUniswapV2|MixinZeroExBridge|NativeOrdersFeature|OwnableFeature|PayTakerTransformer|SignatureValidatorFeature|SimpleFunctionRegistryFeature|TestBridge|TestCallTarget|TestDelegateCaller|TestFillQuoteTransformerBridge|TestFillQuoteTransformerExchange|TestFillQuoteTransformerHost|TestFixinProtocolFees|TestFixinTokenSpender|TestFullMigration|TestInitialMigration|TestLibNativeOrder|TestLibSignature|TestLiquidityProvider|TestMetaTransactionsNativeOrdersFeature|TestMetaTransactionsTransformERC20Feature|TestMigrator|TestMintTokenERC20Transformer|TestMintableERC20Token|TestNativeOrdersFeature|TestRfqOriginRegistration|TestSimpleFunctionRegistryFeatureImpl1|TestSimpleFunctionRegistryFeatureImpl2|TestStaking|TestTokenSpender|TestTokenSpenderERC20Token|TestTransformERC20|TestTransformerBase|TestTransformerDeployerTransformer|TestTransformerHost|TestWeth|TestWethTransformerHost|TestZeroExFeature|TokenSpenderFeature|TransformERC20Feature|Transformer|TransformerDeployer|UniswapFeature|WethTransformer|ZeroEx|ZeroExOptimized).json" }, "repository": { "type": "git", diff --git a/contracts/zero-ex/test/artifacts.ts b/contracts/zero-ex/test/artifacts.ts index 1ddfded855..1d7b60d218 100644 --- a/contracts/zero-ex/test/artifacts.ts +++ b/contracts/zero-ex/test/artifacts.ts @@ -110,6 +110,7 @@ import * as TestMigrator from '../test/generated-artifacts/TestMigrator.json'; import * as TestMintableERC20Token from '../test/generated-artifacts/TestMintableERC20Token.json'; import * as TestMintTokenERC20Transformer from '../test/generated-artifacts/TestMintTokenERC20Transformer.json'; import * as TestNativeOrdersFeature from '../test/generated-artifacts/TestNativeOrdersFeature.json'; +import * as TestRfqOriginRegistration from '../test/generated-artifacts/TestRfqOriginRegistration.json'; import * as TestSimpleFunctionRegistryFeatureImpl1 from '../test/generated-artifacts/TestSimpleFunctionRegistryFeatureImpl1.json'; import * as TestSimpleFunctionRegistryFeatureImpl2 from '../test/generated-artifacts/TestSimpleFunctionRegistryFeatureImpl2.json'; import * as TestStaking from '../test/generated-artifacts/TestStaking.json'; @@ -244,6 +245,7 @@ export const artifacts = { TestMintTokenERC20Transformer: TestMintTokenERC20Transformer as ContractArtifact, TestMintableERC20Token: TestMintableERC20Token as ContractArtifact, TestNativeOrdersFeature: TestNativeOrdersFeature as ContractArtifact, + TestRfqOriginRegistration: TestRfqOriginRegistration as ContractArtifact, TestSimpleFunctionRegistryFeatureImpl1: TestSimpleFunctionRegistryFeatureImpl1 as ContractArtifact, TestSimpleFunctionRegistryFeatureImpl2: TestSimpleFunctionRegistryFeatureImpl2 as ContractArtifact, TestStaking: TestStaking as ContractArtifact, 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 11ce6ea181..20cf623012 100644 --- a/contracts/zero-ex/test/features/native_orders_feature_test.ts +++ b/contracts/zero-ex/test/features/native_orders_feature_test.ts @@ -8,7 +8,7 @@ import { IZeroExContract, IZeroExEvents } from '../../src/wrappers'; import { artifacts } from '../artifacts'; import { fullMigrateAsync } from '../utils/migration'; import { getRandomLimitOrder, getRandomRfqOrder } from '../utils/orders'; -import { TestMintableERC20TokenContract } from '../wrappers'; +import { TestMintableERC20TokenContract, TestRfqOriginRegistrationContract } from '../wrappers'; blockchainTests.resets('NativeOrdersFeature', env => { const { NULL_ADDRESS, MAX_UINT256, ZERO_AMOUNT } = constants; @@ -24,6 +24,7 @@ blockchainTests.resets('NativeOrdersFeature', env => { let makerToken: TestMintableERC20TokenContract; let takerToken: TestMintableERC20TokenContract; let wethToken: TestMintableERC20TokenContract; + let testRfqOriginRegistration: TestRfqOriginRegistrationContract; before(async () => { let owner; @@ -57,6 +58,12 @@ blockchainTests.resets('NativeOrdersFeature', env => { takerToken.approve(zeroEx.address, MAX_UINT256).awaitTransactionSuccessAsync({ from: a }), ), ); + testRfqOriginRegistration = await TestRfqOriginRegistrationContract.deployFrom0xArtifactAsync( + artifacts.TestRfqOriginRegistration, + env.provider, + env.txDefaults, + artifacts, + ); }); function getTestLimitOrder(fields: Partial = {}): LimitOrder { @@ -1048,6 +1055,15 @@ blockchainTests.resets('NativeOrdersFeature', env => { expect(takerBalance).to.bignumber.eq(makerTokenFilledAmount); } + describe('registerAllowedRfqOrigins()', () => { + it('cannot register through a contract', async () => { + const tx = testRfqOriginRegistration + .registerAllowedRfqOrigins(zeroEx.address, [], true) + .awaitTransactionSuccessAsync(); + expect(tx).to.revertWith('NativeOrdersFeature/NO_CONTRACT_ORIGINS'); + }); + }); + describe('fillRfqOrder()', () => { it('can fully fill an order', async () => { const order = getTestRfqOrder(); @@ -1155,18 +1171,18 @@ blockchainTests.resets('NativeOrdersFeature', env => { const order = getTestRfqOrder(); const receipt = await zeroEx - .registerAllowedRfqOrigin(notTaker, true) + .registerAllowedRfqOrigins([notTaker], true) .awaitTransactionSuccessAsync({ from: taker }); verifyEventsFromLogs( receipt.logs, [ { origin: taker, - addr: notTaker, + addrs: [notTaker], allowed: true, }, ], - IZeroExEvents.RfqOrderOriginAllowed, + IZeroExEvents.RfqOrderOriginsAllowed, ); return fillRfqOrderAsync(order, order.takerAmount, notTaker); }); @@ -1174,20 +1190,20 @@ blockchainTests.resets('NativeOrdersFeature', env => { it('cannot fill an order with registered then unregistered tx.origin', async () => { const order = getTestRfqOrder(); - await zeroEx.registerAllowedRfqOrigin(notTaker, true).awaitTransactionSuccessAsync({ from: taker }); + await zeroEx.registerAllowedRfqOrigins([notTaker], true).awaitTransactionSuccessAsync({ from: taker }); const receipt = await zeroEx - .registerAllowedRfqOrigin(notTaker, false) + .registerAllowedRfqOrigins([notTaker], false) .awaitTransactionSuccessAsync({ from: taker }); verifyEventsFromLogs( receipt.logs, [ { origin: taker, - addr: notTaker, + addrs: [notTaker], allowed: false, }, ], - IZeroExEvents.RfqOrderOriginAllowed, + IZeroExEvents.RfqOrderOriginsAllowed, ); const tx = fillRfqOrderAsync(order, order.takerAmount, notTaker); diff --git a/contracts/zero-ex/test/wrappers.ts b/contracts/zero-ex/test/wrappers.ts index 47966fb4cc..bb8336591d 100644 --- a/contracts/zero-ex/test/wrappers.ts +++ b/contracts/zero-ex/test/wrappers.ts @@ -108,6 +108,7 @@ export * from '../test/generated-wrappers/test_migrator'; export * from '../test/generated-wrappers/test_mint_token_erc20_transformer'; export * from '../test/generated-wrappers/test_mintable_erc20_token'; export * from '../test/generated-wrappers/test_native_orders_feature'; +export * from '../test/generated-wrappers/test_rfq_origin_registration'; export * from '../test/generated-wrappers/test_simple_function_registry_feature_impl1'; export * from '../test/generated-wrappers/test_simple_function_registry_feature_impl2'; export * from '../test/generated-wrappers/test_staking'; diff --git a/contracts/zero-ex/tsconfig.json b/contracts/zero-ex/tsconfig.json index 862124b2d1..118b75f64d 100644 --- a/contracts/zero-ex/tsconfig.json +++ b/contracts/zero-ex/tsconfig.json @@ -135,6 +135,7 @@ "test/generated-artifacts/TestMintTokenERC20Transformer.json", "test/generated-artifacts/TestMintableERC20Token.json", "test/generated-artifacts/TestNativeOrdersFeature.json", + "test/generated-artifacts/TestRfqOriginRegistration.json", "test/generated-artifacts/TestSimpleFunctionRegistryFeatureImpl1.json", "test/generated-artifacts/TestSimpleFunctionRegistryFeatureImpl2.json", "test/generated-artifacts/TestStaking.json",