diff --git a/contracts/coordinator/contracts/src/MixinCoordinatorApprovalVerifier.sol b/contracts/coordinator/contracts/src/MixinCoordinatorApprovalVerifier.sol index 408fa69b4e..bb3be65a73 100644 --- a/contracts/coordinator/contracts/src/MixinCoordinatorApprovalVerifier.sol +++ b/contracts/coordinator/contracts/src/MixinCoordinatorApprovalVerifier.sol @@ -58,7 +58,7 @@ contract MixinCoordinatorApprovalVerifier is view { // Get the orders from the the Exchange calldata in the 0x transaction - LibOrder.Order[] memory orders = decodeFillDataOrders(transaction.data); + LibOrder.Order[] memory orders = decodeOrdersFromFillData(transaction.data); // No approval is required for non-fill methods if (orders.length > 0) { @@ -74,6 +74,57 @@ contract MixinCoordinatorApprovalVerifier is } } + /// @dev Decodes the orders from Exchange calldata representing any fill method. + /// @param data Exchange calldata representing a fill method. + /// @return The orders from the Exchange calldata. + function decodeOrdersFromFillData(bytes memory data) + public + pure + returns (LibOrder.Order[] memory orders) + { + bytes4 selector = data.readBytes4(0); + if ( + selector == FILL_ORDER_SELECTOR || + selector == FILL_ORDER_NO_THROW_SELECTOR || + selector == FILL_OR_KILL_ORDER_SELECTOR + ) { + // Decode single order + (LibOrder.Order memory order) = abi.decode( + data.slice(4, data.length), + (LibOrder.Order) + ); + orders = new LibOrder.Order[](1); + orders[0] = order; + } else if ( + selector == BATCH_FILL_ORDERS_SELECTOR || + selector == BATCH_FILL_ORDERS_NO_THROW_SELECTOR || + selector == BATCH_FILL_OR_KILL_ORDERS_SELECTOR || + selector == MARKET_BUY_ORDERS_SELECTOR || + selector == MARKET_BUY_ORDERS_NO_THROW_SELECTOR || + selector == MARKET_SELL_ORDERS_SELECTOR || + selector == MARKET_SELL_ORDERS_NO_THROW_SELECTOR + ) { + // Decode all orders + // solhint-disable indent + (orders) = abi.decode( + data.slice(4, data.length), + (LibOrder.Order[]) + ); + } else if (selector == MATCH_ORDERS_SELECTOR) { + // Decode left and right orders + (LibOrder.Order memory leftOrder, LibOrder.Order memory rightOrder) = abi.decode( + data.slice(4, data.length), + (LibOrder.Order, LibOrder.Order) + ); + + // Create array of orders + orders = new LibOrder.Order[](2); + orders[0] = leftOrder; + orders[1] = rightOrder; + } + return orders; + } + /// @dev Validates that the feeRecipients of a batch of order have approved a 0x transaction. /// @param transaction 0x transaction containing salt, signerAddress, and data. /// @param orders Array of order structs containing order specifications. @@ -89,7 +140,7 @@ contract MixinCoordinatorApprovalVerifier is uint256[] memory approvalExpirationTimeSeconds, bytes[] memory approvalSignatures ) - public + internal view { // Verify that Ethereum tx signer is the same as the approved txOrigin @@ -149,55 +200,4 @@ contract MixinCoordinatorApprovalVerifier is ); } } - - /// @dev Decodes the orders from Exchange calldata representing any fill method. - /// @param data Exchange calldata representing a fill method. - /// @return The orders from the Exchange calldata. - function decodeFillDataOrders(bytes memory data) - internal - pure - returns (LibOrder.Order[] memory orders) - { - bytes4 selector = data.readBytes4(0); - if ( - selector == FILL_ORDER_SELECTOR || - selector == FILL_ORDER_NO_THROW_SELECTOR || - selector == FILL_OR_KILL_ORDER_SELECTOR - ) { - // Decode single order - (LibOrder.Order memory order) = abi.decode( - data.slice(4, data.length), - (LibOrder.Order) - ); - orders = new LibOrder.Order[](1); - orders[0] = order; - } else if ( - selector == BATCH_FILL_ORDERS_SELECTOR || - selector == BATCH_FILL_ORDERS_NO_THROW_SELECTOR || - selector == BATCH_FILL_OR_KILL_ORDERS_SELECTOR || - selector == MARKET_BUY_ORDERS_SELECTOR || - selector == MARKET_BUY_ORDERS_NO_THROW_SELECTOR || - selector == MARKET_SELL_ORDERS_SELECTOR || - selector == MARKET_SELL_ORDERS_NO_THROW_SELECTOR - ) { - // Decode all orders - // solhint-disable indent - (orders) = abi.decode( - data.slice(4, data.length), - (LibOrder.Order[]) - ); - } else if (selector == MATCH_ORDERS_SELECTOR) { - // Decode left and right orders - (LibOrder.Order memory leftOrder, LibOrder.Order memory rightOrder) = abi.decode( - data.slice(4, data.length), - (LibOrder.Order, LibOrder.Order) - ); - - // Create array of orders - orders = new LibOrder.Order[](2); - orders[0] = leftOrder; - orders[1] = rightOrder; - } - return orders; - } } diff --git a/contracts/coordinator/contracts/src/interfaces/ICoordinatorApprovalVerifier.sol b/contracts/coordinator/contracts/src/interfaces/ICoordinatorApprovalVerifier.sol index c603e7a4fa..c0f8ccf4e0 100644 --- a/contracts/coordinator/contracts/src/interfaces/ICoordinatorApprovalVerifier.sol +++ b/contracts/coordinator/contracts/src/interfaces/ICoordinatorApprovalVerifier.sol @@ -42,21 +42,11 @@ contract ICoordinatorApprovalVerifier { public view; - /// @dev Validates that the feeRecipients of a batch of order have approved a 0x transaction. - /// @param transaction 0x transaction containing salt, signerAddress, and data. - /// @param orders Array of order structs containing order specifications. - /// @param txOrigin Required signer of Ethereum transaction calling this function. - /// @param transactionSignature Proof that the transaction has been signed by the signer. - /// @param approvalExpirationTimeSeconds Array of expiration times in seconds for which each corresponding approval signature expires. - /// @param approvalSignatures Array of signatures that correspond to the feeRecipients of each order. - function assertValidTransactionOrdersApproval( - LibZeroExTransaction.ZeroExTransaction memory transaction, - LibOrder.Order[] memory orders, - address txOrigin, - bytes memory transactionSignature, - uint256[] memory approvalExpirationTimeSeconds, - bytes[] memory approvalSignatures - ) + /// @dev Decodes the orders from Exchange calldata representing any fill method. + /// @param data Exchange calldata representing a fill method. + /// @return The orders from the Exchange calldata. + function decodeOrdersFromFillData(bytes memory data) public - view; + pure + returns (LibOrder.Order[] memory orders); } diff --git a/contracts/coordinator/contracts/src/mixins/MCoordinatorApprovalVerifier.sol b/contracts/coordinator/contracts/src/mixins/MCoordinatorApprovalVerifier.sol index d8e217279b..302350c357 100644 --- a/contracts/coordinator/contracts/src/mixins/MCoordinatorApprovalVerifier.sol +++ b/contracts/coordinator/contracts/src/mixins/MCoordinatorApprovalVerifier.sol @@ -26,11 +26,21 @@ import "../interfaces/ICoordinatorApprovalVerifier.sol"; contract MCoordinatorApprovalVerifier is ICoordinatorApprovalVerifier { - /// @dev Decodes the orders from Exchange calldata representing any fill method. - /// @param data Exchange calldata representing a fill method. - /// @return The orders from the Exchange calldata. - function decodeFillDataOrders(bytes memory data) + /// @dev Validates that the feeRecipients of a batch of order have approved a 0x transaction. + /// @param transaction 0x transaction containing salt, signerAddress, and data. + /// @param orders Array of order structs containing order specifications. + /// @param txOrigin Required signer of Ethereum transaction calling this function. + /// @param transactionSignature Proof that the transaction has been signed by the signer. + /// @param approvalExpirationTimeSeconds Array of expiration times in seconds for which each corresponding approval signature expires. + /// @param approvalSignatures Array of signatures that correspond to the feeRecipients of each order. + function assertValidTransactionOrdersApproval( + LibZeroExTransaction.ZeroExTransaction memory transaction, + LibOrder.Order[] memory orders, + address txOrigin, + bytes memory transactionSignature, + uint256[] memory approvalExpirationTimeSeconds, + bytes[] memory approvalSignatures + ) internal - pure - returns (LibOrder.Order[] memory orders); + view; } diff --git a/contracts/coordinator/test/mixins.ts b/contracts/coordinator/test/mixins.ts index 2e79423e1b..1fe3c89955 100644 --- a/contracts/coordinator/test/mixins.ts +++ b/contracts/coordinator/test/mixins.ts @@ -148,15 +148,6 @@ describe('Mixins tests', () => { transactionSignerAddress, approvalExpirationTimeSeconds, ); - await mixins.assertValidTransactionOrdersApproval.callAsync( - transaction, - orders, - transactionSignerAddress, - transaction.signature, - [approvalExpirationTimeSeconds], - [approval.signature], - { from: transactionSignerAddress }, - ); await mixins.assertValidCoordinatorApprovals.callAsync( transaction, transactionSignerAddress, @@ -181,15 +172,6 @@ describe('Mixins tests', () => { transactionSignerAddress, approvalExpirationTimeSeconds, ); - await mixins.assertValidTransactionOrdersApproval.callAsync( - transaction, - orders, - transactionSignerAddress, - transaction.signature, - [approvalExpirationTimeSeconds], - [approval.signature], - { from: transactionSignerAddress }, - ); await mixins.assertValidCoordinatorApprovals.callAsync( transaction, transactionSignerAddress, @@ -203,15 +185,6 @@ describe('Mixins tests', () => { const orders = [defaultOrder]; const data = exchangeDataEncoder.encodeOrdersToExchangeData(fnName, orders); const transaction = transactionFactory.newSignedTransaction(data); - await mixins.assertValidTransactionOrdersApproval.callAsync( - transaction, - orders, - approvalSignerAddress1, - transaction.signature, - [], - [], - { from: approvalSignerAddress1 }, - ); await mixins.assertValidCoordinatorApprovals.callAsync( transaction, approvalSignerAddress1, @@ -234,15 +207,6 @@ describe('Mixins tests', () => { transactionSignerAddress, approvalExpirationTimeSeconds, ); - await mixins.assertValidTransactionOrdersApproval.callAsync( - transaction, - orders, - approvalSignerAddress1, - transaction.signature, - [approvalExpirationTimeSeconds], - [approval.signature], - { from: approvalSignerAddress1 }, - ); await mixins.assertValidCoordinatorApprovals.callAsync( transaction, approvalSignerAddress1, @@ -256,15 +220,6 @@ describe('Mixins tests', () => { const orders = [defaultOrder]; const data = exchangeDataEncoder.encodeOrdersToExchangeData(fnName, orders); const transaction = transactionFactory.newSignedTransaction(data); - await mixins.assertValidTransactionOrdersApproval.callAsync( - transaction, - orders, - approvalSignerAddress1, - transaction.signature, - [], - [], - { from: approvalSignerAddress1 }, - ); await mixins.assertValidCoordinatorApprovals.callAsync( transaction, approvalSignerAddress1, @@ -288,18 +243,6 @@ describe('Mixins tests', () => { approvalExpirationTimeSeconds, ); const signature = `${approval.signature.slice(0, 4)}FFFFFFFF${approval.signature.slice(12)}`; - expectContractCallFailedAsync( - mixins.assertValidTransactionOrdersApproval.callAsync( - transaction, - orders, - transactionSignerAddress, - transaction.signature, - [approvalExpirationTimeSeconds], - [signature], - { from: transactionSignerAddress }, - ), - RevertReason.InvalidApprovalSignature, - ); expectContractCallFailedAsync( mixins.assertValidCoordinatorApprovals.callAsync( transaction, @@ -323,18 +266,6 @@ describe('Mixins tests', () => { transactionSignerAddress, approvalExpirationTimeSeconds, ); - expectContractCallFailedAsync( - mixins.assertValidTransactionOrdersApproval.callAsync( - transaction, - orders, - transactionSignerAddress, - transaction.signature, - [approvalExpirationTimeSeconds], - [approval.signature], - { from: transactionSignerAddress }, - ), - RevertReason.ApprovalExpired, - ); expectContractCallFailedAsync( mixins.assertValidCoordinatorApprovals.callAsync( transaction, @@ -358,18 +289,6 @@ describe('Mixins tests', () => { transactionSignerAddress, approvalExpirationTimeSeconds, ); - expectContractCallFailedAsync( - mixins.assertValidTransactionOrdersApproval.callAsync( - transaction, - orders, - transactionSignerAddress, - transaction.signature, - [approvalExpirationTimeSeconds], - [approval.signature], - { from: approvalSignerAddress2 }, - ), - RevertReason.InvalidOrigin, - ); expectContractCallFailedAsync( mixins.assertValidCoordinatorApprovals.callAsync( transaction, @@ -401,15 +320,6 @@ describe('Mixins tests', () => { transactionSignerAddress, approvalExpirationTimeSeconds, ); - await mixins.assertValidTransactionOrdersApproval.callAsync( - transaction, - orders, - transactionSignerAddress, - transaction.signature, - [approvalExpirationTimeSeconds], - [approval.signature], - { from: transactionSignerAddress }, - ); await mixins.assertValidCoordinatorApprovals.callAsync( transaction, transactionSignerAddress, @@ -433,15 +343,6 @@ describe('Mixins tests', () => { transactionSignerAddress, approvalExpirationTimeSeconds, ); - await mixins.assertValidTransactionOrdersApproval.callAsync( - transaction, - orders, - transactionSignerAddress, - transaction.signature, - [approvalExpirationTimeSeconds], - [approval.signature], - { from: transactionSignerAddress }, - ); await mixins.assertValidCoordinatorApprovals.callAsync( transaction, transactionSignerAddress, @@ -458,15 +359,6 @@ describe('Mixins tests', () => { })); const data = exchangeDataEncoder.encodeOrdersToExchangeData(fnName, orders); const transaction = transactionFactory.newSignedTransaction(data); - await mixins.assertValidTransactionOrdersApproval.callAsync( - transaction, - orders, - transactionSignerAddress, - transaction.signature, - [], - [], - { from: transactionSignerAddress }, - ); await mixins.assertValidCoordinatorApprovals.callAsync( transaction, transactionSignerAddress, @@ -487,15 +379,6 @@ describe('Mixins tests', () => { transactionSignerAddress, approvalExpirationTimeSeconds, ); - await mixins.assertValidTransactionOrdersApproval.callAsync( - transaction, - orders, - transactionSignerAddress, - transaction.signature, - [approvalExpirationTimeSeconds], - [approval.signature], - { from: transactionSignerAddress }, - ); await mixins.assertValidCoordinatorApprovals.callAsync( transaction, transactionSignerAddress, @@ -521,15 +404,6 @@ describe('Mixins tests', () => { transactionSignerAddress, approvalExpirationTimeSeconds, ); - await mixins.assertValidTransactionOrdersApproval.callAsync( - transaction, - orders, - transactionSignerAddress, - transaction.signature, - [approvalExpirationTimeSeconds, approvalExpirationTimeSeconds], - [approval1.signature, approval2.signature], - { from: transactionSignerAddress }, - ); await mixins.assertValidCoordinatorApprovals.callAsync( transaction, transactionSignerAddress, @@ -543,15 +417,6 @@ describe('Mixins tests', () => { const orders = [defaultOrder, defaultOrder]; const data = exchangeDataEncoder.encodeOrdersToExchangeData(fnName, orders); const transaction = transactionFactory.newSignedTransaction(data); - await mixins.assertValidTransactionOrdersApproval.callAsync( - transaction, - orders, - approvalSignerAddress1, - transaction.signature, - [], - [], - { from: approvalSignerAddress1 }, - ); await mixins.assertValidCoordinatorApprovals.callAsync( transaction, approvalSignerAddress1, @@ -572,18 +437,6 @@ describe('Mixins tests', () => { transactionSignerAddress, approvalExpirationTimeSeconds, ); - expectContractCallFailedAsync( - mixins.assertValidTransactionOrdersApproval.callAsync( - transaction, - orders, - transactionSignerAddress, - transaction.signature, - [approvalExpirationTimeSeconds], - [approval2.signature], - { from: approvalSignerAddress1 }, - ), - RevertReason.InvalidOrigin, - ); expectContractCallFailedAsync( mixins.assertValidCoordinatorApprovals.callAsync( transaction, @@ -600,18 +453,6 @@ describe('Mixins tests', () => { const orders = [defaultOrder, defaultOrder]; const data = exchangeDataEncoder.encodeOrdersToExchangeData(fnName, orders); const transaction = transactionFactory.newSignedTransaction(data); - expectContractCallFailedAsync( - mixins.assertValidTransactionOrdersApproval.callAsync( - transaction, - orders, - transactionSignerAddress, - transaction.signature, - [], - [], - { from: transactionSignerAddress }, - ), - RevertReason.InvalidApprovalSignature, - ); expectContractCallFailedAsync( mixins.assertValidCoordinatorApprovals.callAsync( transaction, @@ -636,18 +477,6 @@ describe('Mixins tests', () => { approvalExpirationTimeSeconds, ); const signature = `${approval.signature.slice(0, 4)}FFFFFFFF${approval.signature.slice(12)}`; - expectContractCallFailedAsync( - mixins.assertValidTransactionOrdersApproval.callAsync( - transaction, - orders, - transactionSignerAddress, - transaction.signature, - [approvalExpirationTimeSeconds], - [signature], - { from: transactionSignerAddress }, - ), - RevertReason.InvalidApprovalSignature, - ); expectContractCallFailedAsync( mixins.assertValidCoordinatorApprovals.callAsync( transaction, @@ -677,18 +506,6 @@ describe('Mixins tests', () => { approvalExpirationTimeSeconds, ); const approvalSignature2 = `${approval2.signature.slice(0, 4)}FFFFFFFF${approval2.signature.slice(12)}`; - expectContractCallFailedAsync( - mixins.assertValidTransactionOrdersApproval.callAsync( - transaction, - orders, - transactionSignerAddress, - transaction.signature, - [approvalExpirationTimeSeconds, approvalExpirationTimeSeconds], - [approval1.signature, approvalSignature2], - { from: transactionSignerAddress }, - ), - RevertReason.InvalidApprovalSignature, - ); expectContractCallFailedAsync( mixins.assertValidCoordinatorApprovals.callAsync( transaction, @@ -713,18 +530,6 @@ describe('Mixins tests', () => { approvalExpirationTimeSeconds, ); const approvalSignature2 = `${approval2.signature.slice(0, 4)}FFFFFFFF${approval2.signature.slice(12)}`; - expectContractCallFailedAsync( - mixins.assertValidTransactionOrdersApproval.callAsync( - transaction, - orders, - approvalSignerAddress1, - transaction.signature, - [approvalExpirationTimeSeconds], - [approvalSignature2], - { from: approvalSignerAddress1 }, - ), - RevertReason.InvalidApprovalSignature, - ); expectContractCallFailedAsync( mixins.assertValidCoordinatorApprovals.callAsync( transaction, @@ -754,18 +559,6 @@ describe('Mixins tests', () => { transactionSignerAddress, approvalExpirationTimeSeconds2, ); - expectContractCallFailedAsync( - mixins.assertValidTransactionOrdersApproval.callAsync( - transaction, - orders, - transactionSignerAddress, - transaction.signature, - [approvalExpirationTimeSeconds1, approvalExpirationTimeSeconds2], - [approval1.signature, approval2.signature], - { from: transactionSignerAddress }, - ), - RevertReason.ApprovalExpired, - ); expectContractCallFailedAsync( mixins.assertValidCoordinatorApprovals.callAsync( transaction, @@ -789,18 +582,6 @@ describe('Mixins tests', () => { transactionSignerAddress, approvalExpirationTimeSeconds, ); - expectContractCallFailedAsync( - mixins.assertValidTransactionOrdersApproval.callAsync( - transaction, - orders, - approvalSignerAddress1, - transaction.signature, - [approvalExpirationTimeSeconds], - [approval2.signature], - { from: approvalSignerAddress1 }, - ), - RevertReason.ApprovalExpired, - ); expectContractCallFailedAsync( mixins.assertValidCoordinatorApprovals.callAsync( transaction, @@ -824,18 +605,6 @@ describe('Mixins tests', () => { transactionSignerAddress, approvalExpirationTimeSeconds, ); - expectContractCallFailedAsync( - mixins.assertValidTransactionOrdersApproval.callAsync( - transaction, - orders, - transactionSignerAddress, - transaction.signature, - [approvalExpirationTimeSeconds], - [approval1.signature], - { from: approvalSignerAddress2 }, - ), - RevertReason.InvalidOrigin, - ); expectContractCallFailedAsync( mixins.assertValidCoordinatorApprovals.callAsync( transaction,