From 8dbdffc9b4852d87c35365d847ef112ad2f1a5e5 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Sun, 17 Feb 2019 14:28:21 -0800 Subject: [PATCH] Fix abi decoding of 0x transaction data --- .../tec/contracts/src/MixinTECApprovalVerifier.sol | 12 ++++++------ contracts/tec/contracts/src/MixinTECCore.sol | 9 ++++++++- .../src/interfaces/ITECApprovalVerifier.sol | 2 +- .../interfaces/{IExchange.sol => ITransactions.sol} | 2 +- contracts/tec/contracts/src/libs/LibConstants.sol | 6 +++--- contracts/tec/src/artifacts.ts | 2 +- 6 files changed, 20 insertions(+), 13 deletions(-) rename contracts/tec/contracts/src/interfaces/{IExchange.sol => ITransactions.sol} (97%) diff --git a/contracts/tec/contracts/src/MixinTECApprovalVerifier.sol b/contracts/tec/contracts/src/MixinTECApprovalVerifier.sol index 320efd2057..025b1feff9 100644 --- a/contracts/tec/contracts/src/MixinTECApprovalVerifier.sol +++ b/contracts/tec/contracts/src/MixinTECApprovalVerifier.sol @@ -45,7 +45,7 @@ contract MixinTECApprovalVerifier is /// @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 in the transaction's Exchange calldata. - function assertValidTECApproval( + function assertValidTECApprovals( LibZeroExTransaction.ZeroExTransaction memory transaction, bytes memory transactionSignature, uint256[] memory approvalExpirationTimeSeconds, @@ -67,7 +67,7 @@ contract MixinTECApprovalVerifier is ) { // Decode single order (LibOrder.Order memory order) = abi.decode( - transaction.data, + transaction.data.slice(4, transaction.data.length), (LibOrder.Order) ); @@ -90,7 +90,7 @@ contract MixinTECApprovalVerifier is ) { // Decode all orders (LibOrder.Order[] memory orders) = abi.decode( - transaction.data, + transaction.data.slice(4, transaction.data.length), (LibOrder.Order[]) ); @@ -105,7 +105,7 @@ contract MixinTECApprovalVerifier is } else if (exchangeFunctionSelector == MATCH_ORDERS_SELECTOR) { // Decode left and right orders (LibOrder.Order memory leftOrder, LibOrder.Order memory rightOrder) = abi.decode( - transaction.data, + transaction.data.slice(4, transaction.data.length), (LibOrder.Order, LibOrder.Order) ); @@ -182,7 +182,7 @@ contract MixinTECApprovalVerifier is ); } - /// @dev Validates that the feeRecipient of a single order has approved a 0x transaction. + /// @dev Validates that the feeRecipients of a batch of order have approved a 0x transaction. /// @param orders Array of order structs containing order specifications. /// @param transactionHash EIP712 hash of the 0x transaction. /// @param transactionSignature Proof that the transaction has been signed by the signer. @@ -223,7 +223,7 @@ contract MixinTECApprovalVerifier is address approvalSignerAddress = getSignerAddress(approvalHash, approvalSignatures[i]); // Add approval signer to list of signers - approvalSignerAddresses.append(approvalSignerAddress); + approvalSignerAddresses = approvalSignerAddresses.append(approvalSignerAddress); } uint256 ordersLength = orders.length; diff --git a/contracts/tec/contracts/src/MixinTECCore.sol b/contracts/tec/contracts/src/MixinTECCore.sol index d02e2df6e2..1fb4463444 100644 --- a/contracts/tec/contracts/src/MixinTECCore.sol +++ b/contracts/tec/contracts/src/MixinTECCore.sol @@ -43,8 +43,15 @@ contract MixinTECCore is ) public { + // Ethereum transaction signer must be the same as 0x transaction signer + require( + // solhint-disable-next-line avoid-tx-origin + transaction.signerAddress == tx.origin, + "INVALID_TX_SIGNER" + ); + // Validate that the 0x transaction has been approves by each feeRecipient - assertValidTECApproval( + assertValidTECApprovals( transaction, transactionSignature, approvalExpirationTimeSeconds, diff --git a/contracts/tec/contracts/src/interfaces/ITECApprovalVerifier.sol b/contracts/tec/contracts/src/interfaces/ITECApprovalVerifier.sol index c08bd1afa1..b3dac9e58b 100644 --- a/contracts/tec/contracts/src/interfaces/ITECApprovalVerifier.sol +++ b/contracts/tec/contracts/src/interfaces/ITECApprovalVerifier.sol @@ -31,7 +31,7 @@ contract ITECApprovalVerifier { /// @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 in the transaction's Exchange calldata. - function assertValidTECApproval( + function assertValidTECApprovals( LibZeroExTransaction.ZeroExTransaction memory transaction, bytes memory transactionSignature, uint256[] memory approvalExpirationTimeSeconds, diff --git a/contracts/tec/contracts/src/interfaces/IExchange.sol b/contracts/tec/contracts/src/interfaces/ITransactions.sol similarity index 97% rename from contracts/tec/contracts/src/interfaces/IExchange.sol rename to contracts/tec/contracts/src/interfaces/ITransactions.sol index b038a119b6..2efcdc9d65 100644 --- a/contracts/tec/contracts/src/interfaces/IExchange.sol +++ b/contracts/tec/contracts/src/interfaces/ITransactions.sol @@ -18,7 +18,7 @@ pragma solidity ^0.5.3; -contract IExchange { +contract ITransactions { /// @dev Executes an exchange method call in the context of signer. /// @param salt Arbitrary number to ensure uniqueness of transaction hash. diff --git a/contracts/tec/contracts/src/libs/LibConstants.sol b/contracts/tec/contracts/src/libs/LibConstants.sol index 84119c75c5..dc66eb1821 100644 --- a/contracts/tec/contracts/src/libs/LibConstants.sol +++ b/contracts/tec/contracts/src/libs/LibConstants.sol @@ -18,17 +18,17 @@ pragma solidity ^0.5.3; -import "../interfaces/IExchange.sol"; +import "../interfaces/ITransactions.sol"; contract LibConstants { // solhint-disable-next-line var-name-mixedcase - IExchange internal EXCHANGE; + ITransactions internal EXCHANGE; constructor (address _exchange) public { - EXCHANGE = IExchange(_exchange); + EXCHANGE = ITransactions(_exchange); } } diff --git a/contracts/tec/src/artifacts.ts b/contracts/tec/src/artifacts.ts index 6d007d2b15..d37eb0fff3 100644 --- a/contracts/tec/src/artifacts.ts +++ b/contracts/tec/src/artifacts.ts @@ -9,7 +9,7 @@ import * as TEC from '../generated-artifacts/TEC.json'; import * as TestLibs from '../generated-artifacts/TestLibs.json'; import * as TestMixins from '../generated-artifacts/TestMixins.json'; export const artifacts = { - TestMixins: TestMixins as ContractArtifact, TEC: TEC as ContractArtifact, TestLibs: TestLibs as ContractArtifact, + TestMixins: TestMixins as ContractArtifact, };