From 74b9ad5536e1f9e2985eb53099534c0149aab35f Mon Sep 17 00:00:00 2001 From: Alex Towle Date: Mon, 12 Aug 2019 17:12:00 -0700 Subject: [PATCH 01/34] `@0x:contracts-exchange` Added unit tests for getCurrentContextAddress --- .../contracts/test/TestTransactions.sol | 47 +++++++++++++++++++ contracts/exchange/package.json | 2 +- contracts/exchange/src/artifacts.ts | 2 + contracts/exchange/src/wrappers.ts | 1 + contracts/exchange/test/transactions.ts | 30 ++++++++++++ .../exchange/test/transactions_unit_tests.ts | 43 +++++++++++++++++ contracts/exchange/tsconfig.json | 1 + 7 files changed, 125 insertions(+), 1 deletion(-) create mode 100644 contracts/exchange/contracts/test/TestTransactions.sol create mode 100644 contracts/exchange/test/transactions_unit_tests.ts diff --git a/contracts/exchange/contracts/test/TestTransactions.sol b/contracts/exchange/contracts/test/TestTransactions.sol new file mode 100644 index 0000000000..754e9f1cfe --- /dev/null +++ b/contracts/exchange/contracts/test/TestTransactions.sol @@ -0,0 +1,47 @@ +/* + + Copyright 2019 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.5.9; +pragma experimental ABIEncoderV2; + +import "../src/Exchange.sol"; +import "../src/MixinTransactions.sol"; + + +contract TestTransactions is + Exchange +{ + constructor () + public + Exchange(1337) + {} // solhint-disable-line no-empty-blocks + + function setCurrentContextAddress(address context) + external + { + currentContextAddress = context; + } + + function getCurrentContextAddress() + external + view + returns (address) + { + return _getCurrentContextAddress(); + } +} diff --git a/contracts/exchange/package.json b/contracts/exchange/package.json index 8a55693bd6..de09b6a28b 100644 --- a/contracts/exchange/package.json +++ b/contracts/exchange/package.json @@ -35,7 +35,7 @@ "compile:truffle": "truffle compile" }, "config": { - "abis": "./generated-artifacts/@(Exchange|ExchangeWrapper|IAssetProxy|IAssetProxyDispatcher|IEIP1271Wallet|IExchange|IExchangeCore|IMatchOrders|ISignatureValidator|ITransactions|ITransferSimulator|IWallet|IWrapperFunctions|IsolatedExchange|LibExchangeRichErrorDecoder|MixinAssetProxyDispatcher|MixinExchangeCore|MixinMatchOrders|MixinSignatureValidator|MixinTransactions|MixinTransferSimulator|MixinWrapperFunctions|ReentrancyTester|TestAssetProxyDispatcher|TestExchangeInternals|TestLibExchangeRichErrorDecoder|TestSignatureValidator|TestValidatorWallet|TestWrapperFunctions|Whitelist).json", + "abis": "./generated-artifacts/@(Exchange|ExchangeWrapper|IAssetProxy|IAssetProxyDispatcher|IEIP1271Wallet|IExchange|IExchangeCore|IMatchOrders|ISignatureValidator|ITransactions|ITransferSimulator|IWallet|IWrapperFunctions|IsolatedExchange|LibExchangeRichErrorDecoder|MixinAssetProxyDispatcher|MixinExchangeCore|MixinMatchOrders|MixinSignatureValidator|MixinTransactions|MixinTransferSimulator|MixinWrapperFunctions|ReentrancyTester|TestAssetProxyDispatcher|TestExchangeInternals|TestLibExchangeRichErrorDecoder|TestSignatureValidator|TestTransactions|TestValidatorWallet|TestWrapperFunctions|Whitelist).json", "abis:comment": "This list is auto-generated by contracts-gen. Don't edit manually." }, "repository": { diff --git a/contracts/exchange/src/artifacts.ts b/contracts/exchange/src/artifacts.ts index 8c1aedae3e..a5551815fd 100644 --- a/contracts/exchange/src/artifacts.ts +++ b/contracts/exchange/src/artifacts.ts @@ -32,6 +32,7 @@ import * as TestAssetProxyDispatcher from '../generated-artifacts/TestAssetProxy import * as TestExchangeInternals from '../generated-artifacts/TestExchangeInternals.json'; import * as TestLibExchangeRichErrorDecoder from '../generated-artifacts/TestLibExchangeRichErrorDecoder.json'; import * as TestSignatureValidator from '../generated-artifacts/TestSignatureValidator.json'; +import * as TestTransactions from '../generated-artifacts/TestTransactions.json'; import * as TestValidatorWallet from '../generated-artifacts/TestValidatorWallet.json'; import * as TestWrapperFunctions from '../generated-artifacts/TestWrapperFunctions.json'; import * as Whitelist from '../generated-artifacts/Whitelist.json'; @@ -64,6 +65,7 @@ export const artifacts = { TestExchangeInternals: TestExchangeInternals as ContractArtifact, TestLibExchangeRichErrorDecoder: TestLibExchangeRichErrorDecoder as ContractArtifact, TestSignatureValidator: TestSignatureValidator as ContractArtifact, + TestTransactions: TestTransactions as ContractArtifact, TestValidatorWallet: TestValidatorWallet as ContractArtifact, TestWrapperFunctions: TestWrapperFunctions as ContractArtifact, }; diff --git a/contracts/exchange/src/wrappers.ts b/contracts/exchange/src/wrappers.ts index 171f93ba3e..f64c78b5cf 100644 --- a/contracts/exchange/src/wrappers.ts +++ b/contracts/exchange/src/wrappers.ts @@ -30,6 +30,7 @@ export * from '../generated-wrappers/test_asset_proxy_dispatcher'; export * from '../generated-wrappers/test_exchange_internals'; export * from '../generated-wrappers/test_lib_exchange_rich_error_decoder'; export * from '../generated-wrappers/test_signature_validator'; +export * from '../generated-wrappers/test_transactions'; export * from '../generated-wrappers/test_validator_wallet'; export * from '../generated-wrappers/test_wrapper_functions'; export * from '../generated-wrappers/whitelist'; diff --git a/contracts/exchange/test/transactions.ts b/contracts/exchange/test/transactions.ts index d8321aeecc..9ad594cd65 100644 --- a/contracts/exchange/test/transactions.ts +++ b/contracts/exchange/test/transactions.ts @@ -37,6 +37,7 @@ import { ExchangeTransactionExecutionEventArgs, ExchangeWrapper, ExchangeWrapperContract, + TestTransactionsContract, WhitelistContract, } from '../src/'; @@ -52,6 +53,7 @@ blockchainTests.resets('Exchange transactions', env => { let feeRecipientAddress: string; let validatorAddress: string; let taker2Address: string; + let transactionsContract: TestTransactionsContract; let erc20TokenA: DummyERC20TokenContract; let erc20TokenB: DummyERC20TokenContract; @@ -138,6 +140,13 @@ blockchainTests.resets('Exchange transactions', env => { makerTransactionFactory = new TransactionFactory(makerPrivateKey, exchangeInstance.address, chainId); takerTransactionFactory = new TransactionFactory(takerPrivateKey, exchangeInstance.address, chainId); taker2TransactionFactory = new TransactionFactory(taker2PrivateKey, exchangeInstance.address, chainId); + + transactionsContract = await TestTransactionsContract.deployFrom0xArtifactAsync( + artifacts.TestTransactions, + env.provider, + env.txDefaults, + {}, + ); }); describe('executeTransaction', () => { describe('general functionality', () => { @@ -1189,4 +1198,25 @@ blockchainTests.resets('Exchange transactions', env => { }); }); }); + describe('getCurrentContext', () => { + it('should return the sender address when there is not a saved context address', async () => { + const currentContextAddress = await transactionsContract.getCurrentContextAddress.callAsync({ + from: makerAddress, + }); + expect(currentContextAddress).to.be.eq(makerAddress); + }); + + it('should return the sender address when there is a saved context address', async () => { + // Set the current context address to the taker address + await transactionsContract.setCurrentContextAddress.sendTransactionAsync(takerAddress, { + from: makerAddress, + }); + + // Ensure that the queried current context address is the same as the address that was set. + const currentContextAddress = await transactionsContract.getCurrentContextAddress.callAsync({ + from: makerAddress, + }); + expect(currentContextAddress).to.be.eq(takerAddress); + }); + }); }); diff --git a/contracts/exchange/test/transactions_unit_tests.ts b/contracts/exchange/test/transactions_unit_tests.ts new file mode 100644 index 0000000000..8079f67df0 --- /dev/null +++ b/contracts/exchange/test/transactions_unit_tests.ts @@ -0,0 +1,43 @@ +import { blockchainTests, chaiSetup, describe } from '@0x/contracts-test-utils'; +import * as chai from 'chai'; +import * as _ from 'lodash'; + +import { artifacts, TestTransactionsContract } from '../src'; + +chaiSetup.configure(); +const expect = chai.expect; +blockchainTests.resets('FillOrder Tests', ({ provider, web3Wrapper, txDefaults }) => { + let transactionsContract: TestTransactionsContract; + let accounts: string[]; + + before(async () => { + accounts = await web3Wrapper.getAvailableAddressesAsync(); + + transactionsContract = await TestTransactionsContract.deployFrom0xArtifactAsync( + artifacts.TestTransactions, + provider, + txDefaults, + {}, + ); + }); + + describe('getCurrentContext', () => { + it('should return the sender address when there is not a saved context address', async () => { + const currentContextAddress = await transactionsContract.getCurrentContextAddress.callAsync({ + from: accounts[0], + }); + expect(currentContextAddress).to.be.eq(accounts[0]); + }); + + it('should return the sender address when there is a saved context address', async () => { + // Set the current context address to the taker address + await transactionsContract.setCurrentContextAddress.sendTransactionAsync(accounts[1]); + + // Ensure that the queried current context address is the same as the address that was set. + const currentContextAddress = await transactionsContract.getCurrentContextAddress.callAsync({ + from: accounts[0], + }); + expect(currentContextAddress).to.be.eq(accounts[1]); + }); + }); +}); diff --git a/contracts/exchange/tsconfig.json b/contracts/exchange/tsconfig.json index beff07f648..63279898fc 100644 --- a/contracts/exchange/tsconfig.json +++ b/contracts/exchange/tsconfig.json @@ -30,6 +30,7 @@ "generated-artifacts/TestExchangeInternals.json", "generated-artifacts/TestLibExchangeRichErrorDecoder.json", "generated-artifacts/TestSignatureValidator.json", + "generated-artifacts/TestTransactions.json", "generated-artifacts/TestValidatorWallet.json", "generated-artifacts/TestWrapperFunctions.json", "generated-artifacts/Whitelist.json" From 1724ecd4c3225d47e0a4db395afd5bc7432c5329 Mon Sep 17 00:00:00 2001 From: Alex Towle Date: Tue, 13 Aug 2019 10:45:07 -0700 Subject: [PATCH 02/34] `@0x:contracts-exchange` Added unit tests for executeTransaction --- .../contracts/test/TestTransactions.sol | 63 +++++ .../exchange/test/transactions_unit_tests.ts | 246 +++++++++++++++++- 2 files changed, 303 insertions(+), 6 deletions(-) diff --git a/contracts/exchange/contracts/test/TestTransactions.sol b/contracts/exchange/contracts/test/TestTransactions.sol index 754e9f1cfe..78de63069d 100644 --- a/contracts/exchange/contracts/test/TestTransactions.sol +++ b/contracts/exchange/contracts/test/TestTransactions.sol @@ -26,17 +26,67 @@ import "../src/MixinTransactions.sol"; contract TestTransactions is Exchange { + // Indicates whether or not the overridden _isValidTransactionWithHashSignature should return true or false. + bool public shouldBeValid; + + // Indicates whether or not the fallback function should succeed. + bool public shouldSucceedCall; + + // The returndata of the fallback function. + bytes public fallbackReturnData; + constructor () public Exchange(1337) {} // solhint-disable-line no-empty-blocks + // This fallback function will succeed if the bool `shouldSucceedCall` has been set + // to true, and will fail otherwise. It will return returndata `fallbackReturnData` + // in either case. + function () + external + { + // Circumvent the compiler to return data through the fallback + bool success = shouldSucceedCall; + bytes memory returnData = fallbackReturnData; + assembly { + if iszero(success) { + revert(add(0x20, returnData), mload(returnData)) + } + return(add(0x20, returnData), mload(returnData)) + } + } + function setCurrentContextAddress(address context) external { currentContextAddress = context; } + function setFallbackReturnData(bytes calldata returnData) + external + { + fallbackReturnData = returnData; + } + + function setShouldBeValid(bool isValid) + external + { + shouldBeValid = isValid; + } + + function setShouldCallSucceed(bool shouldSucceed) + external + { + shouldSucceedCall = shouldSucceed; + } + + function setTransactionHash(bytes32 hash) + external + { + transactionsExecuted[hash] = true; + } + function getCurrentContextAddress() external view @@ -44,4 +94,17 @@ contract TestTransactions is { return _getCurrentContextAddress(); } + + function _isValidTransactionWithHashSignature( + ZeroExTransaction memory, + bytes32, + address, + bytes memory + ) + internal + view + returns (bool) + { + return shouldBeValid; + } } diff --git a/contracts/exchange/test/transactions_unit_tests.ts b/contracts/exchange/test/transactions_unit_tests.ts index 8079f67df0..f5ffde70c6 100644 --- a/contracts/exchange/test/transactions_unit_tests.ts +++ b/contracts/exchange/test/transactions_unit_tests.ts @@ -1,24 +1,258 @@ -import { blockchainTests, chaiSetup, describe } from '@0x/contracts-test-utils'; -import * as chai from 'chai'; +import { + blockchainTests, + constants, + describe, + expect, + getLatestBlockTimestampAsync, + hexRandom, + LogDecoder, +} from '@0x/contracts-test-utils'; +import { ExchangeRevertErrors, transactionHashUtils } from '@0x/order-utils'; +import { EIP712DomainWithDefaultSchema } from '@0x/types'; +import { BigNumber } from '@0x/utils'; +import { LogWithDecodedArgs } from 'ethereum-types'; import * as _ from 'lodash'; -import { artifacts, TestTransactionsContract } from '../src'; +import { artifacts, TestTransactionsContract, TestTransactionsTransactionExecutionEventArgs } from '../src'; -chaiSetup.configure(); -const expect = chai.expect; -blockchainTests.resets('FillOrder Tests', ({ provider, web3Wrapper, txDefaults }) => { +blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDefaults }) => { let transactionsContract: TestTransactionsContract; let accounts: string[]; + let domain: EIP712DomainWithDefaultSchema; + let logDecoder: LogDecoder; + + const randomSignature = () => hexRandom(66); + + const EMPTY_ZERO_EX_TRANSACTION = { + salt: constants.ZERO_AMOUNT, + expirationTimeSeconds: constants.ZERO_AMOUNT, + signerAddress: constants.NULL_ADDRESS, + data: constants.NULL_BYTES, + domain: { + verifyingContractAddress: constants.NULL_ADDRESS, + chainId: 0, + }, + }; before(async () => { + // A list of available addresses to use during testing. accounts = await web3Wrapper.getAvailableAddressesAsync(); + // Insantiate a LogDecoder instance + logDecoder = new LogDecoder(web3Wrapper, artifacts); + + // Deploy the transaction test contract. transactionsContract = await TestTransactionsContract.deployFrom0xArtifactAsync( artifacts.TestTransactions, provider, txDefaults, {}, ); + + // Set the default domain. + domain = { + verifyingContractAddress: transactionsContract.address, + chainId: 1337, + }; + }); + + describe('executeTransaction', () => { + it('should revert if the current time is past the expiration time of the metatransaction', async () => { + const currentTimestamp = await getLatestBlockTimestampAsync(); + const transaction = { + ...EMPTY_ZERO_EX_TRANSACTION, + expirationTimeSeconds: new BigNumber(currentTimestamp).minus(10), // Set the expiration time to before the current timestamp + domain, + }; + const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); + const expectedError = new ExchangeRevertErrors.TransactionError( + ExchangeRevertErrors.TransactionErrorCode.Expired, + transactionHash, + ); + const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction, randomSignature()); + return expect(tx).to.revertWith(expectedError); + }); + + it('should revert if the current context address is not address zero', async () => { + // Set the current context address to a nonzero address before the call to `executeTransaction()` + expect(transactionsContract.setCurrentContextAddress.sendTransactionAsync(accounts[0])).to.be.fulfilled(''); + + // Run the transaction with an updated current context address + const currentTimestamp = await getLatestBlockTimestampAsync(); + const transaction = { + ...EMPTY_ZERO_EX_TRANSACTION, + expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), + domain, + }; + const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); + const expectedError = new ExchangeRevertErrors.TransactionError( + ExchangeRevertErrors.TransactionErrorCode.NoReentrancy, + transactionHash, + ); + const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction, randomSignature()); + return expect(tx).to.revertWith(expectedError); + }); + + it('should revert if the transaction has been executed previously', async () => { + const currentTimestamp = await getLatestBlockTimestampAsync(); + const transaction = { + ...EMPTY_ZERO_EX_TRANSACTION, + expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), + domain, + }; + const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); + + // Make it seem like the transaction has already been executed by setting it's value in the transactionsExecuted + // mapping to true. + await expect(transactionsContract.setTransactionHash.sendTransactionAsync(transactionHash)).to.be.fulfilled( + '', + ); + + // Run the transaction with an updated current context address + const expectedError = new ExchangeRevertErrors.TransactionError( + ExchangeRevertErrors.TransactionErrorCode.AlreadyExecuted, + transactionHash, + ); + const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction, randomSignature()); + return expect(tx).to.revertWith(expectedError); + }); + + it('should revert if the signer != msg.sender and the signature is not valid', async () => { + // Set the contract to not accept signatures. Note: This doesn't need to be called but is kept for readability. + // await expect(transactionsContract.setShouldBeValid.sendTransactionAsync(false)).to.be.fulfilled(''); + + const currentTimestamp = await getLatestBlockTimestampAsync(); + const transaction = { + ...EMPTY_ZERO_EX_TRANSACTION, + expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), + signerAddress: accounts[1], // This is different than the account that will be used to send. + domain, + }; + const signature = randomSignature(); + const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); + const expectedError = new ExchangeRevertErrors.TransactionSignatureError( + transactionHash, + accounts[1], + signature, + ); + const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction, signature, { + from: accounts[0], + }); + return expect(tx).to.revertWith(expectedError); + }); + + it('should revert if the signer == msg.sender but the delegatecall fails', async () => { + // Set the contract to fail on calls to the fallback function. Note: This call is unnecessary but is kept for readability. + // await expect(transactionsContract.setShouldBeValid.sendTransactionAsync(false)).to.be.fulfilled(''); + + const currentTimestamp = await getLatestBlockTimestampAsync(); + const transaction = { + ...EMPTY_ZERO_EX_TRANSACTION, + expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), + signerAddress: accounts[1], + domain, + }; + const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); + const expectedError = new ExchangeRevertErrors.TransactionExecutionError( + transactionHash, + constants.NULL_BYTES, + ); + const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction, randomSignature(), { + from: accounts[1], + }); + return expect(tx).to.revertWith(expectedError); + }); + + it('should revert if the signer != msg.sender and the signature is valid but the delegatecall fails', async () => { + // Set the contract to accept signatures. + await expect(transactionsContract.setShouldBeValid.sendTransactionAsync(true)).to.be.fulfilled(''); + + // Set the contract to fail on calls to the fallback function. Note: This call is unnecessary but is kept for readability. + // await expect(transactionsContract.setShouldSucceedCall.sendTransactionAsync(false)).to.be.fulfilled(''); + + const currentTimestamp = await getLatestBlockTimestampAsync(); + const transaction = { + ...EMPTY_ZERO_EX_TRANSACTION, + expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), + signerAddress: accounts[1], // This is different than the account that will be used to send. + domain, + }; + const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); + const expectedError = new ExchangeRevertErrors.TransactionExecutionError( + transactionHash, + constants.NULL_BYTES, + ); + const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction, randomSignature(), { + from: accounts[0], + }); + return expect(tx).to.revertWith(expectedError); + }); + + it('should revert with the correct return data if the signer != msg.sender and the signature is valid but the delegatecall fails', async () => { + // Set the contract to accept signatures. + await expect(transactionsContract.setShouldBeValid.sendTransactionAsync(true)).to.be.fulfilled(''); + + // Set the contract to fail on calls to the fallback function. Note: This call is unnecessary but is kept for readability. + // await expect(transactionsContract.setShouldCallSucceed.sendTransactionAsync(false)).to.be.fulfilled(''); + + // Set the contract fallbackReturnData to the recognizable string of bytes 0xDEADBEEF + await expect(transactionsContract.setFallbackReturnData.sendTransactionAsync('0xdeadbeef')).to.be.fulfilled( + '', + ); + + const currentTimestamp = await getLatestBlockTimestampAsync(); + const transaction = { + ...EMPTY_ZERO_EX_TRANSACTION, + expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), + signerAddress: accounts[1], // This is different than the account that will be used to send. + domain, + }; + const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); + const expectedError = new ExchangeRevertErrors.TransactionExecutionError(transactionHash, '0xdeadbeef'); + const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction, randomSignature(), { + from: accounts[0], + }); + return expect(tx).to.revertWith(expectedError); + }); + + it('should succeed with the correct return hash and event emitted', async () => { + // Set the contract to accept signatures. + await expect(transactionsContract.setShouldBeValid.sendTransactionAsync(true)).to.be.fulfilled(''); + + // Set the contract to fail on calls to the fallback function. Note: This call is unnecessary but is kept for readability. + await expect(transactionsContract.setShouldCallSucceed.sendTransactionAsync(true)).to.be.fulfilled(''); + + // Set the contract fallbackReturnData to the recognizable string of bytes 0xDEADBEEF + await expect(transactionsContract.setFallbackReturnData.sendTransactionAsync('0xdeadbeef')).to.be.fulfilled( + '', + ); + + // Set up the necessary data for the transactions and tests + const currentTimestamp = await getLatestBlockTimestampAsync(); + const transaction = { + ...EMPTY_ZERO_EX_TRANSACTION, + expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), + signerAddress: accounts[1], // This is different than the account that will be used to send. + domain, + }; + const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); + + // Verify that the returndata of the transaction is 0xDEADBEEF + const result = await transactionsContract.executeTransaction.callAsync(transaction, randomSignature(), { + from: accounts[0], + }); + expect(result === '0xdeadbeef').to.be.true(); + + // Verify that the logs returned from the call are correct. + const receipt = await logDecoder.getTxWithDecodedLogsAsync( + await transactionsContract.executeTransaction.sendTransactionAsync(transaction, randomSignature(), { + from: accounts[0], + }), + ); + const logs = receipt.logs as Array>; + expect(logs[0].event === 'TransactionExecution').to.be.true(); + expect(logs[0].args.transactionHash).to.eq(transactionHash); + }); }); describe('getCurrentContext', () => { From 907771f084400f02c5737f75627db81eb6e49a9a Mon Sep 17 00:00:00 2001 From: Alex Towle Date: Thu, 15 Aug 2019 14:37:47 -0700 Subject: [PATCH 03/34] `@0x:contracts-exchange` Added unit tests for batchExecuteTransactions --- .../contracts/src/MixinTransferSimulator.sol | 2 +- .../contracts/test/TestTransactions.sol | 2 +- .../exchange/test/transactions_unit_tests.ts | 240 +++++++++++++++++- 3 files changed, 241 insertions(+), 3 deletions(-) diff --git a/contracts/exchange/contracts/src/MixinTransferSimulator.sol b/contracts/exchange/contracts/src/MixinTransferSimulator.sol index 36fec94e03..9518cca14e 100644 --- a/contracts/exchange/contracts/src/MixinTransferSimulator.sol +++ b/contracts/exchange/contracts/src/MixinTransferSimulator.sol @@ -57,4 +57,4 @@ contract MixinTransferSimulator is } revert("TRANSFERS_SUCCESSFUL"); } -} \ No newline at end of file +} diff --git a/contracts/exchange/contracts/test/TestTransactions.sol b/contracts/exchange/contracts/test/TestTransactions.sol index 78de63069d..62f33fdb32 100644 --- a/contracts/exchange/contracts/test/TestTransactions.sol +++ b/contracts/exchange/contracts/test/TestTransactions.sol @@ -50,7 +50,7 @@ contract TestTransactions is bool success = shouldSucceedCall; bytes memory returnData = fallbackReturnData; assembly { - if iszero(success) { + if or(iszero(success), gt(calldatasize, 0x0)) { revert(add(0x20, returnData), mload(returnData)) } return(add(0x20, returnData), mload(returnData)) diff --git a/contracts/exchange/test/transactions_unit_tests.ts b/contracts/exchange/test/transactions_unit_tests.ts index f5ffde70c6..e3a98a2cd2 100644 --- a/contracts/exchange/test/transactions_unit_tests.ts +++ b/contracts/exchange/test/transactions_unit_tests.ts @@ -56,8 +56,244 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef }; }); + describe('batchExecuteTransaction', () => { + it('should revert if the only call to executeTransaction fails', async () => { + const currentTimestamp = await getLatestBlockTimestampAsync(); + const transaction = { + ...EMPTY_ZERO_EX_TRANSACTION, + expirationTimeSeconds: new BigNumber(currentTimestamp).minus(10), // Set the expiration time to before the current timestamp + domain, + }; + const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); + const expectedError = new ExchangeRevertErrors.TransactionError( + ExchangeRevertErrors.TransactionErrorCode.Expired, + transactionHash, + ); + const tx = transactionsContract.batchExecuteTransactions.sendTransactionAsync( + [transaction], + [randomSignature()], + ); + return expect(tx).to.revertWith(expectedError); + }); + + it('should revert if the second call to executeTransaction fails', async () => { + // Set the contract to accept signatures. + await expect(transactionsContract.setShouldBeValid.sendTransactionAsync(true)).to.be.fulfilled(''); + + // Set the contract to fail on calls to the fallback function. Note: This call is unnecessary but is kept for readability. + await expect(transactionsContract.setShouldCallSucceed.sendTransactionAsync(true)).to.be.fulfilled(''); + + const currentTimestamp = await getLatestBlockTimestampAsync(); + const transaction1 = { + ...EMPTY_ZERO_EX_TRANSACTION, + data: '0x', // This call should succeed + expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), + domain, + }; + const transaction2 = { + ...EMPTY_ZERO_EX_TRANSACTION, + data: '0x32', // This call should fail because the calldata is invalid + expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), + domain, + }; + const transactionHash = transactionHashUtils.getTransactionHashHex(transaction2); + const expectedError = new ExchangeRevertErrors.TransactionExecutionError( + transactionHash, + constants.NULL_BYTES, + ); + const tx = transactionsContract.batchExecuteTransactions.sendTransactionAsync( + [transaction1, transaction2], + [randomSignature(), randomSignature()], + ); + return expect(tx).to.revertWith(expectedError); + }); + + it('should revert if the first call to executeTransaction fails', async () => { + // Set the contract to accept signatures. + await expect(transactionsContract.setShouldBeValid.sendTransactionAsync(true)).to.be.fulfilled(''); + + // Set the contract to fail on calls to the fallback function. Note: This call is unnecessary but is kept for readability. + await expect(transactionsContract.setShouldCallSucceed.sendTransactionAsync(true)).to.be.fulfilled(''); + + const currentTimestamp = await getLatestBlockTimestampAsync(); + const transaction1 = { + ...EMPTY_ZERO_EX_TRANSACTION, + data: '0x32', // This call should fail because the calldata is invalid + expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), + domain, + }; + const transaction2 = { + ...EMPTY_ZERO_EX_TRANSACTION, + data: '0x', // This call should succeed + expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), + domain, + }; + const transactionHash = transactionHashUtils.getTransactionHashHex(transaction1); + const expectedError = new ExchangeRevertErrors.TransactionExecutionError( + transactionHash, + constants.NULL_BYTES, + ); + const tx = transactionsContract.batchExecuteTransactions.sendTransactionAsync( + [transaction1, transaction2], + [randomSignature(), randomSignature()], + ); + return expect(tx).to.revertWith(expectedError); + }); + + it('should revert if the same transaction is executed twice in a batch', async () => { + // Set the contract to accept signatures. + await expect(transactionsContract.setShouldBeValid.sendTransactionAsync(true)).to.be.fulfilled(''); + + // Set the contract to fail on calls to the fallback function. Note: This call is unnecessary but is kept for readability. + await expect(transactionsContract.setShouldCallSucceed.sendTransactionAsync(true)).to.be.fulfilled(''); + + // Set the contract fallbackReturnData to the recognizable string of bytes 0xDEADBEEF + await expect(transactionsContract.setFallbackReturnData.sendTransactionAsync('0xdeadbeef')).to.be.fulfilled( + '', + ); + + // Set up the necessary data for the transactions and tests + const currentTimestamp = await getLatestBlockTimestampAsync(); + const transaction1 = { + ...EMPTY_ZERO_EX_TRANSACTION, + expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), + signerAddress: accounts[1], // This is different than the account that will be used to send. + domain, + }; + const transaction2 = { + ...EMPTY_ZERO_EX_TRANSACTION, + expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), + signerAddress: accounts[1], // This is different than the account that will be used to send. + domain, + }; + const transactionHash2 = transactionHashUtils.getTransactionHashHex(transaction2); + + // Verify that the transaction reverts + const expectedError = new ExchangeRevertErrors.TransactionError( + ExchangeRevertErrors.TransactionErrorCode.AlreadyExecuted, + transactionHash2, + ); + const tx = transactionsContract.batchExecuteTransactions.sendTransactionAsync( + [transaction1, transaction2], + [randomSignature(), randomSignature()], + { + from: accounts[0], + }, + ); + return expect(tx).to.revertWith(expectedError); + }); + + it('should succeed if the only call to executeTransaction succeeds', async () => { + // Set the contract to accept signatures. + await expect(transactionsContract.setShouldBeValid.sendTransactionAsync(true)).to.be.fulfilled(''); + + // Set the contract to fail on calls to the fallback function. Note: This call is unnecessary but is kept for readability. + await expect(transactionsContract.setShouldCallSucceed.sendTransactionAsync(true)).to.be.fulfilled(''); + + // Set the contract fallbackReturnData to the recognizable string of bytes 0xDEADBEEF + await expect(transactionsContract.setFallbackReturnData.sendTransactionAsync('0xdeadbeef')).to.be.fulfilled( + '', + ); + + // Set up the necessary data for the transactions and tests + const currentTimestamp = await getLatestBlockTimestampAsync(); + const transaction = { + ...EMPTY_ZERO_EX_TRANSACTION, + expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), + signerAddress: accounts[1], // This is different than the account that will be used to send. + domain, + }; + const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); + + // Verify that the returndata of the transaction is 0xDEADBEEF + const result = await transactionsContract.batchExecuteTransactions.callAsync( + [transaction], + [randomSignature()], + { + from: accounts[0], + }, + ); + expect(result.length).to.be.eq(1); + expect(result[0] === '0xdeadbeef').to.be.true(); + + // Verify that the logs returned from the call are correct. + const receipt = await logDecoder.getTxWithDecodedLogsAsync( + await transactionsContract.batchExecuteTransactions.sendTransactionAsync( + [transaction], + [randomSignature()], + { + from: accounts[0], + }, + ), + ); + const logs = receipt.logs as Array>; + expect(logs.length).to.be.eq(1); + expect(logs[0].event === 'TransactionExecution').to.be.true(); + expect(logs[0].args.transactionHash).to.eq(transactionHash); + }); + + it('should succeed if the both calls to executeTransaction succeed', async () => { + // Set the contract to accept signatures. + await expect(transactionsContract.setShouldBeValid.sendTransactionAsync(true)).to.be.fulfilled(''); + + // Set the contract to fail on calls to the fallback function. Note: This call is unnecessary but is kept for readability. + await expect(transactionsContract.setShouldCallSucceed.sendTransactionAsync(true)).to.be.fulfilled(''); + + // Set the contract fallbackReturnData to the recognizable string of bytes 0xDEADBEEF + await expect(transactionsContract.setFallbackReturnData.sendTransactionAsync('0xdeadbeef')).to.be.fulfilled( + '', + ); + + // Set up the necessary data for the transactions and tests + const currentTimestamp = await getLatestBlockTimestampAsync(); + const transaction1 = { + ...EMPTY_ZERO_EX_TRANSACTION, + expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), + signerAddress: accounts[0], // This is different than the account that will be used to send. + domain, + }; + const transaction2 = { + ...EMPTY_ZERO_EX_TRANSACTION, + expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), + signerAddress: accounts[1], // This is different than the account that will be used to send. + domain, + }; + const transactionHash1 = transactionHashUtils.getTransactionHashHex(transaction1); + const transactionHash2 = transactionHashUtils.getTransactionHashHex(transaction2); + + // Verify that the returndata of the transaction is 0xDEADBEEF + const result = await transactionsContract.batchExecuteTransactions.callAsync( + [transaction1, transaction2], + [randomSignature(), randomSignature()], + { + from: accounts[0], + }, + ); + expect(result.length).to.be.eq(2); + expect(result[0] === '0xdeadbeef').to.be.true(); + expect(result[1] === '0xdeadbeef').to.be.true(); + + // Verify that the logs returned from the call are correct. + const receipt = await logDecoder.getTxWithDecodedLogsAsync( + await transactionsContract.batchExecuteTransactions.sendTransactionAsync( + [transaction1, transaction2], + [randomSignature(), randomSignature()], + { + from: accounts[0], + }, + ), + ); + + const logs = receipt.logs as Array>; + expect(logs.length).to.be.eq(2); + logs.map(log => expect(log.event === 'TransactionExecution').to.be.true()); + expect(logs[0].args.transactionHash).to.eq(transactionHash1); + expect(logs[1].args.transactionHash).to.eq(transactionHash2); + }); + }); + describe('executeTransaction', () => { - it('should revert if the current time is past the expiration time of the metatransaction', async () => { + it('should revert if the current time is past the expiration time', async () => { const currentTimestamp = await getLatestBlockTimestampAsync(); const transaction = { ...EMPTY_ZERO_EX_TRANSACTION, @@ -250,6 +486,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef }), ); const logs = receipt.logs as Array>; + expect(logs.length).to.be.eq(1); expect(logs[0].event === 'TransactionExecution').to.be.true(); expect(logs[0].args.transactionHash).to.eq(transactionHash); }); @@ -275,3 +512,4 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef }); }); }); +// tslint:disable-line:max-file-line-count From 4b970905cf5f88d62f880817dbfc73196f757ea0 Mon Sep 17 00:00:00 2001 From: Alex Towle Date: Thu, 15 Aug 2019 15:31:42 -0700 Subject: [PATCH 04/34] `@0x:contracts-exchange` Removed code written to transactions --- contracts/exchange/test/transactions.ts | 30 ------------------------- 1 file changed, 30 deletions(-) diff --git a/contracts/exchange/test/transactions.ts b/contracts/exchange/test/transactions.ts index 9ad594cd65..d8321aeecc 100644 --- a/contracts/exchange/test/transactions.ts +++ b/contracts/exchange/test/transactions.ts @@ -37,7 +37,6 @@ import { ExchangeTransactionExecutionEventArgs, ExchangeWrapper, ExchangeWrapperContract, - TestTransactionsContract, WhitelistContract, } from '../src/'; @@ -53,7 +52,6 @@ blockchainTests.resets('Exchange transactions', env => { let feeRecipientAddress: string; let validatorAddress: string; let taker2Address: string; - let transactionsContract: TestTransactionsContract; let erc20TokenA: DummyERC20TokenContract; let erc20TokenB: DummyERC20TokenContract; @@ -140,13 +138,6 @@ blockchainTests.resets('Exchange transactions', env => { makerTransactionFactory = new TransactionFactory(makerPrivateKey, exchangeInstance.address, chainId); takerTransactionFactory = new TransactionFactory(takerPrivateKey, exchangeInstance.address, chainId); taker2TransactionFactory = new TransactionFactory(taker2PrivateKey, exchangeInstance.address, chainId); - - transactionsContract = await TestTransactionsContract.deployFrom0xArtifactAsync( - artifacts.TestTransactions, - env.provider, - env.txDefaults, - {}, - ); }); describe('executeTransaction', () => { describe('general functionality', () => { @@ -1198,25 +1189,4 @@ blockchainTests.resets('Exchange transactions', env => { }); }); }); - describe('getCurrentContext', () => { - it('should return the sender address when there is not a saved context address', async () => { - const currentContextAddress = await transactionsContract.getCurrentContextAddress.callAsync({ - from: makerAddress, - }); - expect(currentContextAddress).to.be.eq(makerAddress); - }); - - it('should return the sender address when there is a saved context address', async () => { - // Set the current context address to the taker address - await transactionsContract.setCurrentContextAddress.sendTransactionAsync(takerAddress, { - from: makerAddress, - }); - - // Ensure that the queried current context address is the same as the address that was set. - const currentContextAddress = await transactionsContract.getCurrentContextAddress.callAsync({ - from: makerAddress, - }); - expect(currentContextAddress).to.be.eq(takerAddress); - }); - }); }); From d845b318b9be4ee47b1676f73541e6afb0fdae00 Mon Sep 17 00:00:00 2001 From: Alex Towle Date: Mon, 19 Aug 2019 15:34:35 -0700 Subject: [PATCH 05/34] `@0x:contracts-exchange` Changed the signature validation stub function to not require state --- .../contracts/test/TestTransactions.sol | 14 ++++++---- .../exchange/test/transactions_unit_tests.ts | 26 +++++-------------- 2 files changed, 16 insertions(+), 24 deletions(-) diff --git a/contracts/exchange/contracts/test/TestTransactions.sol b/contracts/exchange/contracts/test/TestTransactions.sol index 62f33fdb32..860fd46390 100644 --- a/contracts/exchange/contracts/test/TestTransactions.sol +++ b/contracts/exchange/contracts/test/TestTransactions.sol @@ -26,9 +26,6 @@ import "../src/MixinTransactions.sol"; contract TestTransactions is Exchange { - // Indicates whether or not the overridden _isValidTransactionWithHashSignature should return true or false. - bool public shouldBeValid; - // Indicates whether or not the fallback function should succeed. bool public shouldSucceedCall; @@ -99,12 +96,19 @@ contract TestTransactions is ZeroExTransaction memory, bytes32, address, - bytes memory + bytes memory signature ) internal view returns (bool) { - return shouldBeValid; + if ( + signature.length == 2 && + signature[0] == 0x0 && + signature[1] == 0x0 + ) { + return false; + } + return true; } } diff --git a/contracts/exchange/test/transactions_unit_tests.ts b/contracts/exchange/test/transactions_unit_tests.ts index e3a98a2cd2..6be924a677 100644 --- a/contracts/exchange/test/transactions_unit_tests.ts +++ b/contracts/exchange/test/transactions_unit_tests.ts @@ -354,9 +354,6 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef }); it('should revert if the signer != msg.sender and the signature is not valid', async () => { - // Set the contract to not accept signatures. Note: This doesn't need to be called but is kept for readability. - // await expect(transactionsContract.setShouldBeValid.sendTransactionAsync(false)).to.be.fulfilled(''); - const currentTimestamp = await getLatestBlockTimestampAsync(); const transaction = { ...EMPTY_ZERO_EX_TRANSACTION, @@ -364,7 +361,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef signerAddress: accounts[1], // This is different than the account that will be used to send. domain, }; - const signature = randomSignature(); + const signature = '0x0000'; // This is the invalid signature const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); const expectedError = new ExchangeRevertErrors.TransactionSignatureError( transactionHash, @@ -378,9 +375,6 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef }); it('should revert if the signer == msg.sender but the delegatecall fails', async () => { - // Set the contract to fail on calls to the fallback function. Note: This call is unnecessary but is kept for readability. - // await expect(transactionsContract.setShouldBeValid.sendTransactionAsync(false)).to.be.fulfilled(''); - const currentTimestamp = await getLatestBlockTimestampAsync(); const transaction = { ...EMPTY_ZERO_EX_TRANSACTION, @@ -400,9 +394,6 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef }); it('should revert if the signer != msg.sender and the signature is valid but the delegatecall fails', async () => { - // Set the contract to accept signatures. - await expect(transactionsContract.setShouldBeValid.sendTransactionAsync(true)).to.be.fulfilled(''); - // Set the contract to fail on calls to the fallback function. Note: This call is unnecessary but is kept for readability. // await expect(transactionsContract.setShouldSucceedCall.sendTransactionAsync(false)).to.be.fulfilled(''); @@ -413,21 +404,19 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef signerAddress: accounts[1], // This is different than the account that will be used to send. domain, }; + const validSignature = randomSignature(); // Valid because length != 2 const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); const expectedError = new ExchangeRevertErrors.TransactionExecutionError( transactionHash, constants.NULL_BYTES, ); - const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction, randomSignature(), { + const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction, validSignature, { from: accounts[0], }); return expect(tx).to.revertWith(expectedError); }); it('should revert with the correct return data if the signer != msg.sender and the signature is valid but the delegatecall fails', async () => { - // Set the contract to accept signatures. - await expect(transactionsContract.setShouldBeValid.sendTransactionAsync(true)).to.be.fulfilled(''); - // Set the contract to fail on calls to the fallback function. Note: This call is unnecessary but is kept for readability. // await expect(transactionsContract.setShouldCallSucceed.sendTransactionAsync(false)).to.be.fulfilled(''); @@ -437,6 +426,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef ); const currentTimestamp = await getLatestBlockTimestampAsync(); + const validSignature = randomSignature(); // Valid because length != 2 const transaction = { ...EMPTY_ZERO_EX_TRANSACTION, expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), @@ -445,16 +435,13 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef }; const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); const expectedError = new ExchangeRevertErrors.TransactionExecutionError(transactionHash, '0xdeadbeef'); - const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction, randomSignature(), { + const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction, validSignature, { from: accounts[0], }); return expect(tx).to.revertWith(expectedError); }); it('should succeed with the correct return hash and event emitted', async () => { - // Set the contract to accept signatures. - await expect(transactionsContract.setShouldBeValid.sendTransactionAsync(true)).to.be.fulfilled(''); - // Set the contract to fail on calls to the fallback function. Note: This call is unnecessary but is kept for readability. await expect(transactionsContract.setShouldCallSucceed.sendTransactionAsync(true)).to.be.fulfilled(''); @@ -465,6 +452,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef // Set up the necessary data for the transactions and tests const currentTimestamp = await getLatestBlockTimestampAsync(); + const validSignature = randomSignature(); // Valid because length != 2 const transaction = { ...EMPTY_ZERO_EX_TRANSACTION, expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), @@ -474,7 +462,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); // Verify that the returndata of the transaction is 0xDEADBEEF - const result = await transactionsContract.executeTransaction.callAsync(transaction, randomSignature(), { + const result = await transactionsContract.executeTransaction.callAsync(transaction, validSignature, { from: accounts[0], }); expect(result === '0xdeadbeef').to.be.true(); From 0253bba83b5f5d93d4713303c964244230b86648 Mon Sep 17 00:00:00 2001 From: Alex Towle Date: Tue, 20 Aug 2019 15:32:21 -0700 Subject: [PATCH 06/34] `@0x:contracts-exchange` Addressed review comments by completely overhauling transaction_unit_tests.ts --- .../contracts/test/TestTransactions.sol | 66 +-- .../exchange/test/transactions_unit_tests.ts | 461 +++++++++++++----- 2 files changed, 350 insertions(+), 177 deletions(-) diff --git a/contracts/exchange/contracts/test/TestTransactions.sol b/contracts/exchange/contracts/test/TestTransactions.sol index 860fd46390..4555dc822a 100644 --- a/contracts/exchange/contracts/test/TestTransactions.sol +++ b/contracts/exchange/contracts/test/TestTransactions.sol @@ -19,65 +19,26 @@ pragma solidity ^0.5.9; pragma experimental ABIEncoderV2; +import "@0x/contracts-exchange-libs/contracts/src/LibZeroExTransaction.sol"; import "../src/Exchange.sol"; -import "../src/MixinTransactions.sol"; contract TestTransactions is Exchange { - // Indicates whether or not the fallback function should succeed. - bool public shouldSucceedCall; - - // The returndata of the fallback function. - bytes public fallbackReturnData; + event ExecutableCalled(bytes data, bytes returnData); constructor () public Exchange(1337) {} // solhint-disable-line no-empty-blocks - // This fallback function will succeed if the bool `shouldSucceedCall` has been set - // to true, and will fail otherwise. It will return returndata `fallbackReturnData` - // in either case. - function () - external - { - // Circumvent the compiler to return data through the fallback - bool success = shouldSucceedCall; - bytes memory returnData = fallbackReturnData; - assembly { - if or(iszero(success), gt(calldatasize, 0x0)) { - revert(add(0x20, returnData), mload(returnData)) - } - return(add(0x20, returnData), mload(returnData)) - } - } - function setCurrentContextAddress(address context) external { currentContextAddress = context; } - function setFallbackReturnData(bytes calldata returnData) - external - { - fallbackReturnData = returnData; - } - - function setShouldBeValid(bool isValid) - external - { - shouldBeValid = isValid; - } - - function setShouldCallSucceed(bool shouldSucceed) - external - { - shouldSucceedCall = shouldSucceed; - } - function setTransactionHash(bytes32 hash) external { @@ -92,8 +53,29 @@ contract TestTransactions is return _getCurrentContextAddress(); } + // This function will execute arbitrary calldata via a delegatecall. This is highly unsafe to use in production, and this + // is only meant to be used during testing. + function executable( + bool shouldSucceed, + bytes memory data, + bytes memory returnData + ) + public + returns (bytes memory) + { + emit ExecutableCalled(data, returnData); + require(shouldSucceed, "EXECUTABLE_FAILED"); + if (data.length != 0) { + (bool didSucceed, bytes memory callResultData) = address(this).delegatecall(data); // This is a delegatecall to preserve the `msg.sender` field + if (!didSucceed) { + assembly { revert(add(callResultData, 0x20), mload(callResultData)) } + } + } + return returnData; + } + function _isValidTransactionWithHashSignature( - ZeroExTransaction memory, + LibZeroExTransaction.ZeroExTransaction memory, bytes32, address, bytes memory signature diff --git a/contracts/exchange/test/transactions_unit_tests.ts b/contracts/exchange/test/transactions_unit_tests.ts index 6be924a677..cbd12b8758 100644 --- a/contracts/exchange/test/transactions_unit_tests.ts +++ b/contracts/exchange/test/transactions_unit_tests.ts @@ -8,8 +8,8 @@ import { LogDecoder, } from '@0x/contracts-test-utils'; import { ExchangeRevertErrors, transactionHashUtils } from '@0x/order-utils'; -import { EIP712DomainWithDefaultSchema } from '@0x/types'; -import { BigNumber } from '@0x/utils'; +import { EIP712DomainWithDefaultSchema, ZeroExTransaction } from '@0x/types'; +import { AbiEncoder, BigNumber, StringRevertError } from '@0x/utils'; import { LogWithDecodedArgs } from 'ethereum-types'; import * as _ from 'lodash'; @@ -56,19 +56,37 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef }; }); + /** + * Generates calldata for a call to `executable()` in the `TestTransactions` contract. + */ + function getExecutableCallData(shouldSucceed: boolean, callData: string, returnData: string): string { + return (transactionsContract as any).executable.getABIEncodedTransactionData( + shouldSucceed, + callData, + returnData, + ); + } + describe('batchExecuteTransaction', () => { it('should revert if the only call to executeTransaction fails', async () => { + // Create an expired transaction that will fail when used to call `batchExecuteTransactions()`. const currentTimestamp = await getLatestBlockTimestampAsync(); const transaction = { ...EMPTY_ZERO_EX_TRANSACTION, expirationTimeSeconds: new BigNumber(currentTimestamp).minus(10), // Set the expiration time to before the current timestamp + data: getExecutableCallData(false, constants.NULL_BYTES, constants.NULL_BYTES), domain, }; const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); + + // We expect a `TransactionError` to be returned because that is the error that will be triggered in the call to + // `executeTransaction`. const expectedError = new ExchangeRevertErrors.TransactionError( ExchangeRevertErrors.TransactionErrorCode.Expired, transactionHash, ); + + // Call the `batchExecuteTransactions()` function and ensure that it reverts with the expected revert error. const tx = transactionsContract.batchExecuteTransactions.sendTransactionAsync( [transaction], [randomSignature()], @@ -77,30 +95,32 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef }); it('should revert if the second call to executeTransaction fails', async () => { - // Set the contract to accept signatures. - await expect(transactionsContract.setShouldBeValid.sendTransactionAsync(true)).to.be.fulfilled(''); - - // Set the contract to fail on calls to the fallback function. Note: This call is unnecessary but is kept for readability. - await expect(transactionsContract.setShouldCallSucceed.sendTransactionAsync(true)).to.be.fulfilled(''); - + // Create a transaction that will succeed when used to call `batchExecuteTransactions()`. const currentTimestamp = await getLatestBlockTimestampAsync(); const transaction1 = { ...EMPTY_ZERO_EX_TRANSACTION, - data: '0x', // This call should succeed + data: getExecutableCallData(true, constants.NULL_BYTES, constants.NULL_BYTES), // This call should succeed expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), domain, }; + + // Create a transaction that will fail when used to call `batchExecuteTransactions()` because the call to executable will fail. const transaction2 = { ...EMPTY_ZERO_EX_TRANSACTION, - data: '0x32', // This call should fail because the calldata is invalid + data: getExecutableCallData(false, constants.NULL_BYTES, constants.NULL_BYTES), // This call should fail expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), domain, }; const transactionHash = transactionHashUtils.getTransactionHashHex(transaction2); + + // Create the StringRevertError that reflects the returndata that will be returned by the failed transaction. + const executableError = new StringRevertError('EXECUTABLE_FAILED'); const expectedError = new ExchangeRevertErrors.TransactionExecutionError( transactionHash, - constants.NULL_BYTES, + executableError.encode(), ); + + // Call the `batchExecuteTransactions()` function and ensure that it reverts with the expected revert error. const tx = transactionsContract.batchExecuteTransactions.sendTransactionAsync( [transaction1, transaction2], [randomSignature(), randomSignature()], @@ -109,30 +129,32 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef }); it('should revert if the first call to executeTransaction fails', async () => { - // Set the contract to accept signatures. - await expect(transactionsContract.setShouldBeValid.sendTransactionAsync(true)).to.be.fulfilled(''); - - // Set the contract to fail on calls to the fallback function. Note: This call is unnecessary but is kept for readability. - await expect(transactionsContract.setShouldCallSucceed.sendTransactionAsync(true)).to.be.fulfilled(''); - + // Create a transaction that will fail when used to call `batchExecuteTransactions()` because the call to executable will fail. const currentTimestamp = await getLatestBlockTimestampAsync(); const transaction1 = { ...EMPTY_ZERO_EX_TRANSACTION, - data: '0x32', // This call should fail because the calldata is invalid + data: getExecutableCallData(false, constants.NULL_BYTES, constants.NULL_BYTES), // This call should fail expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), domain, }; + + // Create a transaction that will succeed when used to call `batchExecuteTransactions()`. const transaction2 = { ...EMPTY_ZERO_EX_TRANSACTION, - data: '0x', // This call should succeed + data: getExecutableCallData(true, constants.NULL_BYTES, constants.NULL_BYTES), expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), domain, }; const transactionHash = transactionHashUtils.getTransactionHashHex(transaction1); + + // Create the StringRevertError that reflects the returndata that will be returned by the failed transaction. + const executableError = new StringRevertError('EXECUTABLE_FAILED'); const expectedError = new ExchangeRevertErrors.TransactionExecutionError( transactionHash, - constants.NULL_BYTES, + executableError.encode(), ); + + // Call the `batchExecuteTransactions()` function and ensure that it reverts with the expected revert error. const tx = transactionsContract.batchExecuteTransactions.sendTransactionAsync( [transaction1, transaction2], [randomSignature(), randomSignature()], @@ -141,34 +163,28 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef }); it('should revert if the same transaction is executed twice in a batch', async () => { - // Set the contract to accept signatures. - await expect(transactionsContract.setShouldBeValid.sendTransactionAsync(true)).to.be.fulfilled(''); - - // Set the contract to fail on calls to the fallback function. Note: This call is unnecessary but is kept for readability. - await expect(transactionsContract.setShouldCallSucceed.sendTransactionAsync(true)).to.be.fulfilled(''); - - // Set the contract fallbackReturnData to the recognizable string of bytes 0xDEADBEEF - await expect(transactionsContract.setFallbackReturnData.sendTransactionAsync('0xdeadbeef')).to.be.fulfilled( - '', - ); - - // Set up the necessary data for the transactions and tests + // Create a transaction that will succeed when used to call `batchExecuteTransactions()`. const currentTimestamp = await getLatestBlockTimestampAsync(); const transaction1 = { ...EMPTY_ZERO_EX_TRANSACTION, expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), - signerAddress: accounts[1], // This is different than the account that will be used to send. + signerAddress: accounts[1], + data: getExecutableCallData(true, constants.NULL_BYTES, constants.NULL_BYTES), domain, }; + + // Duplicate the first transaction. This should cause the call to `batchExecuteTransactions()` to fail + // because this transaction will have the same order hash as transaction1. const transaction2 = { ...EMPTY_ZERO_EX_TRANSACTION, expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), - signerAddress: accounts[1], // This is different than the account that will be used to send. + signerAddress: accounts[1], + data: getExecutableCallData(true, constants.NULL_BYTES, constants.NULL_BYTES), domain, }; const transactionHash2 = transactionHashUtils.getTransactionHashHex(transaction2); - // Verify that the transaction reverts + // Call the `batchExecuteTransactions()` function and ensure that it reverts with the expected revert error. const expectedError = new ExchangeRevertErrors.TransactionError( ExchangeRevertErrors.TransactionErrorCode.AlreadyExecuted, transactionHash2, @@ -184,26 +200,18 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef }); it('should succeed if the only call to executeTransaction succeeds', async () => { - // Set the contract to accept signatures. - await expect(transactionsContract.setShouldBeValid.sendTransactionAsync(true)).to.be.fulfilled(''); - - // Set the contract to fail on calls to the fallback function. Note: This call is unnecessary but is kept for readability. - await expect(transactionsContract.setShouldCallSucceed.sendTransactionAsync(true)).to.be.fulfilled(''); - - // Set the contract fallbackReturnData to the recognizable string of bytes 0xDEADBEEF - await expect(transactionsContract.setFallbackReturnData.sendTransactionAsync('0xdeadbeef')).to.be.fulfilled( - '', - ); - - // Set up the necessary data for the transactions and tests const currentTimestamp = await getLatestBlockTimestampAsync(); + + // Create a transaction that will succeed when used to call `batchExecuteTransactions()`. const transaction = { ...EMPTY_ZERO_EX_TRANSACTION, expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), - signerAddress: accounts[1], // This is different than the account that will be used to send. + signerAddress: accounts[1], + data: getExecutableCallData(true, constants.NULL_BYTES, '0xdeadbeef'), domain, }; const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); + const validSignature = randomSignature(); // Verify that the returndata of the transaction is 0xDEADBEEF const result = await transactionsContract.batchExecuteTransactions.callAsync( @@ -213,49 +221,58 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef from: accounts[0], }, ); + expect(result.length).to.be.eq(1); - expect(result[0] === '0xdeadbeef').to.be.true(); + + // Create an abiEncoder for bytes. This will be used to decode the result and encode what + // is expected. + const abiEncoder = AbiEncoder.create('bytes'); + + // Ensure that the result contains the abi-encoded bytes "0xdeadbeef" + const encodedDeadbeef = abiEncoder.encode('0xdeadbeef'); + expect( + result[0] === + '0x0000000000000000000000000000000000000000000000000000000000000020'.concat( + encodedDeadbeef.slice(2, encodedDeadbeef.length), + ), + ).to.be.true(); // Verify that the logs returned from the call are correct. const receipt = await logDecoder.getTxWithDecodedLogsAsync( await transactionsContract.batchExecuteTransactions.sendTransactionAsync( [transaction], - [randomSignature()], - { - from: accounts[0], - }, + [validSignature], ), ); + + // Ensure that the correct number of events were logged. const logs = receipt.logs as Array>; - expect(logs.length).to.be.eq(1); - expect(logs[0].event === 'TransactionExecution').to.be.true(); - expect(logs[0].args.transactionHash).to.eq(transactionHash); + expect(logs.length).to.be.eq(2); + + // Ensure that the correct events were logged. + expect(logs[0].event).to.be.eq('ExecutableCalled'); + expect(logs[0].args.data).to.be.eq(constants.NULL_BYTES); + expect(logs[0].args.returnData).to.be.eq('0xdeadbeef'); + expect(logs[1].event).to.be.eq('TransactionExecution'); + expect(logs[1].args.transactionHash).to.eq(transactionHash); }); it('should succeed if the both calls to executeTransaction succeed', async () => { - // Set the contract to accept signatures. - await expect(transactionsContract.setShouldBeValid.sendTransactionAsync(true)).to.be.fulfilled(''); - - // Set the contract to fail on calls to the fallback function. Note: This call is unnecessary but is kept for readability. - await expect(transactionsContract.setShouldCallSucceed.sendTransactionAsync(true)).to.be.fulfilled(''); - - // Set the contract fallbackReturnData to the recognizable string of bytes 0xDEADBEEF - await expect(transactionsContract.setFallbackReturnData.sendTransactionAsync('0xdeadbeef')).to.be.fulfilled( - '', - ); - - // Set up the necessary data for the transactions and tests const currentTimestamp = await getLatestBlockTimestampAsync(); + + // Create two transactions that will succeed when used to call `batchExecuteTransactions()`. const transaction1 = { ...EMPTY_ZERO_EX_TRANSACTION, expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), signerAddress: accounts[0], // This is different than the account that will be used to send. + data: getExecutableCallData(true, constants.NULL_BYTES, '0xdeadbeef'), domain, }; const transaction2 = { ...EMPTY_ZERO_EX_TRANSACTION, expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), - signerAddress: accounts[1], // This is different than the account that will be used to send. + signerAddress: accounts[1], // Different than transaction1's signer address + data: getExecutableCallData(true, constants.NULL_BYTES, '0xbeefdead'), domain, }; const transactionHash1 = transactionHashUtils.getTransactionHashHex(transaction1); @@ -269,9 +286,30 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef from: accounts[0], }, ); + + // Create an abiEncoder for bytes. This will be used to decode the result and encode what + // is expected. + const abiEncoder = AbiEncoder.create('bytes'); + + // Ensure that the result contains the abi-encoded bytes "0xdeadbeef" + const encodedDeadbeef = abiEncoder.encode('0xdeadbeef'); + expect(result.length).to.be.eq(2); + expect( + result[0] === + '0x0000000000000000000000000000000000000000000000000000000000000020'.concat( + encodedDeadbeef.slice(2, encodedDeadbeef.length), + ), + ).to.be.true(); + + // Ensure that the result contains the abi-encoded bytes "0xdeadbeef" + const encodedBeefdead = abiEncoder.encode('0xbeefdead'); expect(result.length).to.be.eq(2); - expect(result[0] === '0xdeadbeef').to.be.true(); - expect(result[1] === '0xdeadbeef').to.be.true(); + expect( + result[1] === + '0x0000000000000000000000000000000000000000000000000000000000000020'.concat( + encodedBeefdead.slice(2, encodedBeefdead.length), + ), + ).to.be.true(); // Verify that the logs returned from the call are correct. const receipt = await logDecoder.getTxWithDecodedLogsAsync( @@ -284,15 +322,32 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef ), ); + // Verify that the correct number of events were logged. const logs = receipt.logs as Array>; - expect(logs.length).to.be.eq(2); - logs.map(log => expect(log.event === 'TransactionExecution').to.be.true()); - expect(logs[0].args.transactionHash).to.eq(transactionHash1); - expect(logs[1].args.transactionHash).to.eq(transactionHash2); + expect(logs.length).to.be.eq(4); + + // Ensure that the correct events were logged. + expect(logs[0].event).to.be.eq('ExecutableCalled'); + expect(logs[0].args.data).to.be.eq(constants.NULL_BYTES); + expect(logs[0].args.returnData).to.be.eq('0xdeadbeef'); + expect(logs[1].event).to.be.eq('TransactionExecution'); + expect(logs[1].args.transactionHash).to.eq(transactionHash1); + expect(logs[2].event).to.be.eq('ExecutableCalled'); + expect(logs[2].args.data).to.be.eq(constants.NULL_BYTES); + expect(logs[2].args.returnData).to.be.eq('0xbeefdead'); + expect(logs[3].event).to.be.eq('TransactionExecution'); + expect(logs[3].args.transactionHash).to.eq(transactionHash2); }); }); describe('executeTransaction', () => { + function getExecuteTransactionCallData(transaction: ZeroExTransaction, signature: string): string { + return (transactionsContract as any).executeTransaction.getABIEncodedTransactionData( + transaction, + signature, + ); + } + it('should revert if the current time is past the expiration time', async () => { const currentTimestamp = await getLatestBlockTimestampAsync(); const transaction = { @@ -309,47 +364,190 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef return expect(tx).to.revertWith(expectedError); }); - it('should revert if the current context address is not address zero', async () => { - // Set the current context address to a nonzero address before the call to `executeTransaction()` - expect(transactionsContract.setCurrentContextAddress.sendTransactionAsync(accounts[0])).to.be.fulfilled(''); + // FIXME - This should be unskipped when the contracts have been updated to fix this problem. + it.skip('should revert if reentrancy occurs in the middle of an executeTransaction call and msg.sender == signer for both calls', async () => { + const validSignature = randomSignature(); - // Run the transaction with an updated current context address const currentTimestamp = await getLatestBlockTimestampAsync(); - const transaction = { + const transaction1 = { ...EMPTY_ZERO_EX_TRANSACTION, expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), + data: getExecutableCallData(true, constants.NULL_BYTES, constants.NULL_BYTES), // This should never get called + signerAddress: accounts[0], domain, }; - const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); - const expectedError = new ExchangeRevertErrors.TransactionError( + const transactionHash1 = transactionHashUtils.getTransactionHashHex(transaction1); + + const callData = getExecutableCallData( + true, + getExecuteTransactionCallData(transaction1, validSignature), + '0xdeadbeef', + ); + + const transaction2 = { + ...EMPTY_ZERO_EX_TRANSACTION, + expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), + data: callData, + signerAddress: accounts[0], + domain, + }; + const transactionHash2 = transactionHashUtils.getTransactionHashHex(transaction2); + const abiEncoder = AbiEncoder.createMethod('TransactionError', ['uint8', 'bytes32']); + const errorData = abiEncoder.encode([ ExchangeRevertErrors.TransactionErrorCode.NoReentrancy, - transactionHash, + transactionHash1, + ]); + + const expectedError = new ExchangeRevertErrors.TransactionExecutionError(transactionHash2, errorData); + const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction2, validSignature, { + from: accounts[0], + }); + return expect(tx).to.revertWith(expectedError); + }); + + it('should revert if reentrancy occurs in the middle of an executeTransaction call and msg.sender != signer for both calls', async () => { + const validSignature = randomSignature(); + + const currentTimestamp = await getLatestBlockTimestampAsync(); + const transaction1 = { + ...EMPTY_ZERO_EX_TRANSACTION, + expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), + data: getExecutableCallData(true, constants.NULL_BYTES, constants.NULL_BYTES), // This should never get called + signerAddress: accounts[0], + domain, + }; + const transactionHash1 = transactionHashUtils.getTransactionHashHex(transaction1); + + const callData = getExecutableCallData( + true, + getExecuteTransactionCallData(transaction1, validSignature), + '0xdeadbeef', ); - const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction, randomSignature()); + + const transaction2 = { + ...EMPTY_ZERO_EX_TRANSACTION, + expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), + data: callData, + signerAddress: accounts[0], + domain, + }; + const transactionHash2 = transactionHashUtils.getTransactionHashHex(transaction2); + const abiEncoder = AbiEncoder.createMethod('TransactionError', ['uint8', 'bytes32']); + const errorData = abiEncoder.encode([ + ExchangeRevertErrors.TransactionErrorCode.NoReentrancy, + transactionHash1, + ]); + + const expectedError = new ExchangeRevertErrors.TransactionExecutionError(transactionHash2, errorData); + const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction2, validSignature, { + from: accounts[1], // Different then the signing addresses + }); + return expect(tx).to.revertWith(expectedError); + }); + + it('should revert if reentrancy occurs in the middle of an executeTransaction call and msg.sender != signer and then msg.sender == sender', async () => { + const validSignature = randomSignature(); + + const currentTimestamp = await getLatestBlockTimestampAsync(); + const transaction1 = { + ...EMPTY_ZERO_EX_TRANSACTION, + expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), + data: getExecutableCallData(true, constants.NULL_BYTES, constants.NULL_BYTES), // This should never get called + signerAddress: accounts[1], + domain, + }; + const transactionHash1 = transactionHashUtils.getTransactionHashHex(transaction1); + + const callData = getExecutableCallData( + true, + getExecuteTransactionCallData(transaction1, validSignature), + '0xdeadbeef', + ); + + const transaction2 = { + ...EMPTY_ZERO_EX_TRANSACTION, + expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), + data: callData, + signerAddress: accounts[0], + domain, + }; + const transactionHash2 = transactionHashUtils.getTransactionHashHex(transaction2); + const abiEncoder = AbiEncoder.createMethod('TransactionError', ['uint8', 'bytes32']); + const errorData = abiEncoder.encode([ + ExchangeRevertErrors.TransactionErrorCode.NoReentrancy, + transactionHash1, + ]); + + const expectedError = new ExchangeRevertErrors.TransactionExecutionError(transactionHash2, errorData); + const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction2, validSignature, { + from: accounts[1], // Different then the signing addresses + }); + return expect(tx).to.revertWith(expectedError); + }); + + // FIXME - This should be unskipped when the contracts have been updated to fix this problem. + it.skip('should revert if reentrancy occurs in the middle of an executeTransaction call and msg.sender == signer and then msg.sender != sender', async () => { + const validSignature = randomSignature(); + + const currentTimestamp = await getLatestBlockTimestampAsync(); + const transaction1 = { + ...EMPTY_ZERO_EX_TRANSACTION, + expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), + data: getExecutableCallData(true, constants.NULL_BYTES, constants.NULL_BYTES), // This should never get called + signerAddress: accounts[0], + domain, + }; + const transactionHash1 = transactionHashUtils.getTransactionHashHex(transaction1); + + const callData = getExecutableCallData( + true, + getExecuteTransactionCallData(transaction1, validSignature), + '0xdeadbeef', + ); + + const transaction2 = { + ...EMPTY_ZERO_EX_TRANSACTION, + expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), + data: callData, + signerAddress: accounts[1], + domain, + }; + const transactionHash2 = transactionHashUtils.getTransactionHashHex(transaction2); + const abiEncoder = AbiEncoder.createMethod('TransactionError', ['uint8', 'bytes32']); + const errorData = abiEncoder.encode([ + ExchangeRevertErrors.TransactionErrorCode.NoReentrancy, + transactionHash1, + ]); + + const expectedError = new ExchangeRevertErrors.TransactionExecutionError(transactionHash2, errorData); + const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction2, validSignature, { + from: accounts[1], // Different then the signing addresses + }); return expect(tx).to.revertWith(expectedError); }); it('should revert if the transaction has been executed previously', async () => { + const validSignature = randomSignature(); const currentTimestamp = await getLatestBlockTimestampAsync(); const transaction = { ...EMPTY_ZERO_EX_TRANSACTION, expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), + data: getExecutableCallData(true, constants.NULL_BYTES, constants.NULL_BYTES), domain, }; const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); - // Make it seem like the transaction has already been executed by setting it's value in the transactionsExecuted - // mapping to true. - await expect(transactionsContract.setTransactionHash.sendTransactionAsync(transactionHash)).to.be.fulfilled( - '', - ); + // Use the transaction in execute transaction. + await expect( + transactionsContract.executeTransaction.sendTransactionAsync(transaction, validSignature), + ).to.be.fulfilled(''); - // Run the transaction with an updated current context address + // Use the same transaction to make another call const expectedError = new ExchangeRevertErrors.TransactionError( ExchangeRevertErrors.TransactionErrorCode.AlreadyExecuted, transactionHash, ); - const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction, randomSignature()); + const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction, validSignature); return expect(tx).to.revertWith(expectedError); }); @@ -375,17 +573,21 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef }); it('should revert if the signer == msg.sender but the delegatecall fails', async () => { + // This calldata is encoded to fail when it hits the executable function. + const callData = getExecutableCallData(false, constants.NULL_BYTES, constants.NULL_BYTES); const currentTimestamp = await getLatestBlockTimestampAsync(); const transaction = { ...EMPTY_ZERO_EX_TRANSACTION, expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), signerAddress: accounts[1], + data: callData, domain, }; const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); + const executableError = new StringRevertError('EXECUTABLE_FAILED'); const expectedError = new ExchangeRevertErrors.TransactionExecutionError( transactionHash, - constants.NULL_BYTES, + executableError.encode(), ); const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction, randomSignature(), { from: accounts[1], @@ -394,47 +596,23 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef }); it('should revert if the signer != msg.sender and the signature is valid but the delegatecall fails', async () => { - // Set the contract to fail on calls to the fallback function. Note: This call is unnecessary but is kept for readability. - // await expect(transactionsContract.setShouldSucceedCall.sendTransactionAsync(false)).to.be.fulfilled(''); - + // This calldata is encoded to fail when it hits the executable function. + const callData = getExecutableCallData(false, constants.NULL_BYTES, constants.NULL_BYTES); const currentTimestamp = await getLatestBlockTimestampAsync(); const transaction = { ...EMPTY_ZERO_EX_TRANSACTION, expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), signerAddress: accounts[1], // This is different than the account that will be used to send. + data: callData, domain, }; const validSignature = randomSignature(); // Valid because length != 2 const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); + const executableError = new StringRevertError('EXECUTABLE_FAILED'); const expectedError = new ExchangeRevertErrors.TransactionExecutionError( transactionHash, - constants.NULL_BYTES, - ); - const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction, validSignature, { - from: accounts[0], - }); - return expect(tx).to.revertWith(expectedError); - }); - - it('should revert with the correct return data if the signer != msg.sender and the signature is valid but the delegatecall fails', async () => { - // Set the contract to fail on calls to the fallback function. Note: This call is unnecessary but is kept for readability. - // await expect(transactionsContract.setShouldCallSucceed.sendTransactionAsync(false)).to.be.fulfilled(''); - - // Set the contract fallbackReturnData to the recognizable string of bytes 0xDEADBEEF - await expect(transactionsContract.setFallbackReturnData.sendTransactionAsync('0xdeadbeef')).to.be.fulfilled( - '', + executableError.encode(), ); - - const currentTimestamp = await getLatestBlockTimestampAsync(); - const validSignature = randomSignature(); // Valid because length != 2 - const transaction = { - ...EMPTY_ZERO_EX_TRANSACTION, - expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), - signerAddress: accounts[1], // This is different than the account that will be used to send. - domain, - }; - const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); - const expectedError = new ExchangeRevertErrors.TransactionExecutionError(transactionHash, '0xdeadbeef'); const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction, validSignature, { from: accounts[0], }); @@ -442,21 +620,15 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef }); it('should succeed with the correct return hash and event emitted', async () => { - // Set the contract to fail on calls to the fallback function. Note: This call is unnecessary but is kept for readability. - await expect(transactionsContract.setShouldCallSucceed.sendTransactionAsync(true)).to.be.fulfilled(''); - - // Set the contract fallbackReturnData to the recognizable string of bytes 0xDEADBEEF - await expect(transactionsContract.setFallbackReturnData.sendTransactionAsync('0xdeadbeef')).to.be.fulfilled( - '', - ); - - // Set up the necessary data for the transactions and tests + // This calldata is encoded to succeed when it hits the executable function. + const callData = getExecutableCallData(true, constants.NULL_BYTES, '0xdeadbeef'); const currentTimestamp = await getLatestBlockTimestampAsync(); const validSignature = randomSignature(); // Valid because length != 2 const transaction = { ...EMPTY_ZERO_EX_TRANSACTION, expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), signerAddress: accounts[1], // This is different than the account that will be used to send. + data: callData, domain, }; const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); @@ -465,18 +637,37 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef const result = await transactionsContract.executeTransaction.callAsync(transaction, validSignature, { from: accounts[0], }); - expect(result === '0xdeadbeef').to.be.true(); + + // Create an abiEncoder for bytes. This will be used to decode the result and encode what + // is expected. + const abiEncoder = AbiEncoder.create('bytes'); + + // Ensure that the result contains the abi-encoded bytes "0xdeadbeef" + const encodedDeadbeef = abiEncoder.encode('0xdeadbeef'); + expect( + result === + '0x0000000000000000000000000000000000000000000000000000000000000020'.concat( + encodedDeadbeef.slice(2, encodedDeadbeef.length), + ), + ).to.be.true(); // Verify that the logs returned from the call are correct. const receipt = await logDecoder.getTxWithDecodedLogsAsync( - await transactionsContract.executeTransaction.sendTransactionAsync(transaction, randomSignature(), { + await transactionsContract.executeTransaction.sendTransactionAsync(transaction, validSignature, { from: accounts[0], }), ); + + // Ensure that the correct number of events were logged. const logs = receipt.logs as Array>; - expect(logs.length).to.be.eq(1); - expect(logs[0].event === 'TransactionExecution').to.be.true(); - expect(logs[0].args.transactionHash).to.eq(transactionHash); + expect(logs.length).to.be.eq(2); + + // Ensure that the correct events were logged. + expect(logs[0].event).to.be.eq('ExecutableCalled'); + expect(logs[0].args.data).to.be.eq(constants.NULL_BYTES); + expect(logs[0].args.returnData).to.be.eq('0xdeadbeef'); + expect(logs[1].event).to.be.eq('TransactionExecution'); + expect(logs[1].args.transactionHash).to.eq(transactionHash); }); }); From cd147dbc41267c603469098b6e8c981b8c980b86 Mon Sep 17 00:00:00 2001 From: Alex Towle Date: Tue, 20 Aug 2019 16:12:45 -0700 Subject: [PATCH 07/34] `@0x:contracts-exchange` Fixed issues caused by rebase --- contracts/exchange/contracts/test/TestTransactions.sol | 1 - contracts/exchange/test/transactions_unit_tests.ts | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/contracts/exchange/contracts/test/TestTransactions.sol b/contracts/exchange/contracts/test/TestTransactions.sol index 4555dc822a..af0bb93e73 100644 --- a/contracts/exchange/contracts/test/TestTransactions.sol +++ b/contracts/exchange/contracts/test/TestTransactions.sol @@ -77,7 +77,6 @@ contract TestTransactions is function _isValidTransactionWithHashSignature( LibZeroExTransaction.ZeroExTransaction memory, bytes32, - address, bytes memory signature ) internal diff --git a/contracts/exchange/test/transactions_unit_tests.ts b/contracts/exchange/test/transactions_unit_tests.ts index cbd12b8758..842b73cb2c 100644 --- a/contracts/exchange/test/transactions_unit_tests.ts +++ b/contracts/exchange/test/transactions_unit_tests.ts @@ -15,7 +15,7 @@ import * as _ from 'lodash'; import { artifacts, TestTransactionsContract, TestTransactionsTransactionExecutionEventArgs } from '../src'; -blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDefaults }) => { +blockchainTests.resets.only('Transaction Unit Tests', ({ provider, web3Wrapper, txDefaults }) => { let transactionsContract: TestTransactionsContract; let accounts: string[]; let domain: EIP712DomainWithDefaultSchema; From c1ed836fda512deed3921fdcf73381c43c0c3f01 Mon Sep 17 00:00:00 2001 From: Alex Towle Date: Tue, 20 Aug 2019 17:10:26 -0700 Subject: [PATCH 08/34] CI: Changed the `test-contracts-ganache-3.0` to run in stages to avoid the `Killed` error --- .circleci/config.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 24adec5182..b3e9ac54a5 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -100,7 +100,7 @@ jobs: - restore_cache: keys: - repo-{{ .Environment.CIRCLE_SHA1 }} - - run: yarn wsrun test:circleci @0x/contracts-multisig @0x/contracts-utils @0x/contracts-exchange-libs @0x/contracts-erc20 @0x/contracts-erc721 @0x/contracts-erc1155 @0x/contracts-asset-proxy @0x/contracts-exchange @0x/contracts-exchange-forwarder @0x/contracts-dev-utils @0x/contracts-staking + - run: yarn wsrun --stages test:circleci @0x/contracts-multisig @0x/contracts-utils @0x/contracts-exchange-libs @0x/contracts-erc20 @0x/contracts-erc721 @0x/contracts-erc1155 @0x/contracts-asset-proxy @0x/contracts-exchange @0x/contracts-exchange-forwarder @0x/contracts-dev-utils @0x/contracts-staking # TODO(dorothy-zbornak): Re-enable after updating this package for 3.0. # - run: yarn wsrun test:circleci @0x/contracts-extensions # TODO(abandeali): Re-enable after this package is complete. From 57338059e114f64f296bd5acadeb5099e53409bd Mon Sep 17 00:00:00 2001 From: Alex Towle Date: Tue, 20 Aug 2019 17:19:25 -0700 Subject: [PATCH 09/34] CI: Split contracts tests up into two seperate jobs to attempt to increase speed --- .circleci/config.yml | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index b3e9ac54a5..fb98ccdffd 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -91,7 +91,7 @@ jobs: keys: - repo-{{ .Environment.CIRCLE_SHA1 }} - run: yarn wsrun test:circleci @0x/contracts-multisig @0x/contracts-utils @0x/contracts-exchange-libs @0x/contracts-erc20 @0x/contracts-erc721 @0x/contracts-erc1155 @0x/contracts-extensions @0x/contracts-asset-proxy @0x/contracts-exchange @0x/contracts-exchange-forwarder @0x/contracts-coordinator @0x/contracts-dev-utils @0x/contracts-staking - test-contracts-ganache-3.0: + test-exchange-ganache-3.0: resource_class: medium+ docker: - image: nikolaik/python-nodejs:python3.7-nodejs8 @@ -100,7 +100,17 @@ jobs: - restore_cache: keys: - repo-{{ .Environment.CIRCLE_SHA1 }} - - run: yarn wsrun --stages test:circleci @0x/contracts-multisig @0x/contracts-utils @0x/contracts-exchange-libs @0x/contracts-erc20 @0x/contracts-erc721 @0x/contracts-erc1155 @0x/contracts-asset-proxy @0x/contracts-exchange @0x/contracts-exchange-forwarder @0x/contracts-dev-utils @0x/contracts-staking + - run: yarn wsrun test:circleci @0x/contracts-exchange + test-contracts-rest-ganache-3.0: + resource_class: medium+ + docker: + - image: nikolaik/python-nodejs:python3.7-nodejs8 + working_directory: ~/repo + steps: + - restore_cache: + keys: + - repo-{{ .Environment.CIRCLE_SHA1 }} + - run: yarn wsrun --stages test:circleci @0x/contracts-multisig @0x/contracts-utils @0x/contracts-exchange-libs @0x/contracts-erc20 @0x/contracts-erc721 @0x/contracts-erc1155 @0x/contracts-asset-proxy @0x/contracts-exchange-forwarder @0x/contracts-dev-utils @0x/contracts-staking # TODO(dorothy-zbornak): Re-enable after updating this package for 3.0. # - run: yarn wsrun test:circleci @0x/contracts-extensions # TODO(abandeali): Re-enable after this package is complete. @@ -563,7 +573,10 @@ workflows: # - build-website: # requires: # - build - - test-contracts-ganache-3.0: + - test-exchange-ganache-3.0: + requires: + - build-3.0 + - test-contracts-rest-ganache-3.0: requires: - build-3.0 # Disabled until geth docker image is fixed. @@ -590,7 +603,8 @@ workflows: # - build-3.0 - submit-coverage-3.0: requires: - - test-contracts-ganache-3.0 + - test-contracts-rest-ganache-3.0 + - test-exchange-ganache-3.0 - test-rest-3.0 - static-tests-3.0 # Disabled for 3.0 From 67f91269eef15e5bc756c8c4f2ac3778d58013c3 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Tue, 20 Aug 2019 10:12:07 -0700 Subject: [PATCH 10/34] Add gasPrice to ZeroExTransaction struct --- .../contracts/src/LibZeroExTransaction.sol | 33 +++++++++++-------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/contracts/exchange-libs/contracts/src/LibZeroExTransaction.sol b/contracts/exchange-libs/contracts/src/LibZeroExTransaction.sol index 59def1596a..a2e381d1e9 100644 --- a/contracts/exchange-libs/contracts/src/LibZeroExTransaction.sol +++ b/contracts/exchange-libs/contracts/src/LibZeroExTransaction.sol @@ -30,16 +30,18 @@ library LibZeroExTransaction { // keccak256(abi.encodePacked( // "ZeroExTransaction(", // "uint256 salt,", - // "uint256 expirationTimeSeconds," + // "uint256 expirationTimeSeconds,", + // "uint256 gasPrice,", // "address signerAddress,", // "bytes data", // ")" // )); - bytes32 constant internal _EIP712_ZEROEX_TRANSACTION_SCHEMA_HASH = 0x6b4c70d217b44d0ff0d3bf7aeb18eb8604c5cd06f615a4b497aeefa4f01d2775; + bytes32 constant internal _EIP712_ZEROEX_TRANSACTION_SCHEMA_HASH = 0xec69816980a3a3ca4554410e60253953e9ff375ba4536a98adfa15cc71541508; struct ZeroExTransaction { uint256 salt; // Arbitrary number to ensure uniqueness of transaction hash. uint256 expirationTimeSeconds; // Timestamp in seconds at which transaction expires. + uint256 gasPrice; // gasPrice at which transaction is required to be executed with. address signerAddress; // Address of transaction signer. bytes data; // AbiV2 encoded calldata. } @@ -72,15 +74,17 @@ library LibZeroExTransaction { bytes memory data = transaction.data; uint256 salt = transaction.salt; uint256 expirationTimeSeconds = transaction.expirationTimeSeconds; + uint256 gasPrice = transaction.gasPrice; address signerAddress = transaction.signerAddress; // Assembly for more efficiently computing: - // keccak256(abi.encodePacked( - // EIP712_ZEROEX_TRANSACTION_SCHEMA_HASH, - // transaction.salt, - // transaction.expirationTimeSeconds, - // uint256(transaction.signerAddress), - // keccak256(transaction.data) + // result = keccak256(abi.encodePacked( + // schemaHash, + // salt, + // expirationTimeSeconds, + // gasPrice, + // uint256(signerAddress), + // keccak256(data) // )); assembly { @@ -90,14 +94,15 @@ library LibZeroExTransaction { // Load free memory pointer let memPtr := mload(64) - mstore(memPtr, schemaHash) // hash of schema - mstore(add(memPtr, 32), salt) // salt - mstore(add(memPtr, 64), expirationTimeSeconds) // expirationTimeSeconds - mstore(add(memPtr, 96), and(signerAddress, 0xffffffffffffffffffffffffffffffffffffffff)) // signerAddress - mstore(add(memPtr, 128), dataHash) // hash of data + mstore(memPtr, schemaHash) // hash of schema + mstore(add(memPtr, 32), salt) // salt + mstore(add(memPtr, 64), expirationTimeSeconds) // expirationTimeSeconds + mstore(add(memPtr, 96), gasPrice) // gasPrice + mstore(add(memPtr, 128), and(signerAddress, 0xffffffffffffffffffffffffffffffffffffffff)) // signerAddress + mstore(add(memPtr, 160), dataHash) // hash of data // Compute hash - result := keccak256(memPtr, 160) + result := keccak256(memPtr, 192) } return result; } From 2134537bc37ae724aa4a48b4964f2bd25fafb19f Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Tue, 20 Aug 2019 12:46:50 -0700 Subject: [PATCH 11/34] Add TransactionGasPriceError to LibExchangeRichErrors --- .../contracts/src/LibExchangeRichErrors.sol | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/contracts/exchange-libs/contracts/src/LibExchangeRichErrors.sol b/contracts/exchange-libs/contracts/src/LibExchangeRichErrors.sol index 29490a2431..b1f6885821 100644 --- a/contracts/exchange-libs/contracts/src/LibExchangeRichErrors.sol +++ b/contracts/exchange-libs/contracts/src/LibExchangeRichErrors.sol @@ -131,6 +131,10 @@ library LibExchangeRichErrors { // bytes4(keccak256("TransactionExecutionError(bytes32,bytes)")) bytes4 internal constant TRANSACTION_EXECUTION_ERROR_SELECTOR = 0x20d11f61; + + // bytes4(keccak256("TransactionGasPriceError(bytes32,unt256,uint256)")) + bytes4 internal constant TRANSACTION_GAS_PRICE_ERROR_SELECTOR = + 0xa26dac09; // bytes4(keccak256("IncompleteFillError(uint8,uint256,uint256)")) bytes4 internal constant INCOMPLETE_FILL_ERROR_SELECTOR = @@ -293,6 +297,14 @@ library LibExchangeRichErrors { return BATCH_MATCH_ORDERS_ERROR_SELECTOR; } + function TransactionGasPriceErrorSelector() + internal + pure + returns (bytes4) + { + return TRANSACTION_GAS_PRICE_ERROR_SELECTOR; + } + function BatchMatchOrdersError( BatchMatchOrdersErrorCodes errorCode ) @@ -581,6 +593,23 @@ library LibExchangeRichErrors { ); } + function TransactionGasPriceError( + bytes32 transactionHash, + uint256 actualGasPrice, + uint256 requiredGasPrice + ) + internal + pure + returns (bytes memory) + { + return abi.encodeWithSelector( + TRANSACTION_GAS_PRICE_ERROR_SELECTOR, + transactionHash, + actualGasPrice, + requiredGasPrice + ); + } + function IncompleteFillError( IncompleteFillErrorCode errorCode, uint256 expectedAssetFillAmount, From 47da97137fb236057c7d099df3899586919b3d09 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Tue, 20 Aug 2019 15:07:35 -0700 Subject: [PATCH 12/34] Add _assertExecutableTransaction function and add gasPrice check --- .../contracts/src/MixinTransactions.sol | 101 ++++++++++++------ 1 file changed, 66 insertions(+), 35 deletions(-) diff --git a/contracts/exchange/contracts/src/MixinTransactions.sol b/contracts/exchange/contracts/src/MixinTransactions.sol index b14147eceb..2228b1517b 100644 --- a/contracts/exchange/contracts/src/MixinTransactions.sol +++ b/contracts/exchange/contracts/src/MixinTransactions.sol @@ -42,7 +42,7 @@ contract MixinTransactions is address public currentContextAddress; /// @dev Executes an Exchange method call in the context of signer. - /// @param transaction 0x transaction containing salt, signerAddress, and data. + /// @param transaction 0x transaction structure. /// @param signature Proof that transaction has been signed by signer. /// @return ABI encoded return data of the underlying Exchange function call. function executeTransaction( @@ -56,7 +56,7 @@ contract MixinTransactions is } /// @dev Executes a batch of Exchange method calls in the context of signer(s). - /// @param transactions Array of 0x transactions containing salt, signerAddress, and data. + /// @param transactions Array of 0x transaction structures. /// @param signatures Array of proofs that transactions have been signed by signer(s). /// @return Array containing ABI encoded return data for each of the underlying Exchange function calls. function batchExecuteTransactions( @@ -75,7 +75,7 @@ contract MixinTransactions is } /// @dev Executes an Exchange method call in the context of signer. - /// @param transaction 0x transaction containing salt, signerAddress, and data. + /// @param transaction 0x transaction structure. /// @param signature Proof that transaction has been signed by signer. /// @return ABI encoded return data of the underlying Exchange function call. function _executeTransaction( @@ -87,6 +87,49 @@ contract MixinTransactions is { bytes32 transactionHash = transaction.getTypedDataHash(EIP712_EXCHANGE_DOMAIN_HASH); + _assertExecutableTransaction( + transaction, + signature, + transactionHash + ); + + address signerAddress = transaction.signerAddress; + if (signerAddress != msg.sender) { + // Set the current transaction signer + currentContextAddress = signerAddress; + } + + // Execute transaction + transactionsExecuted[transactionHash] = true; + (bool didSucceed, bytes memory returnData) = address(this).delegatecall(transaction.data); + if (!didSucceed) { + LibRichErrors.rrevert(LibExchangeRichErrors.TransactionExecutionError( + transactionHash, + returnData + )); + } + + // Reset current transaction signer if it was previously updated + if (signerAddress != msg.sender) { + currentContextAddress = address(0); + } + + emit TransactionExecution(transactionHash); + + return returnData; + } + + /// @dev Validates context for executeTransaction. Succeeds or throws. + /// @param transaction 0x transaction structure. + /// @param signature Proof that transaction has been signed by signer. + /// @param transactionHash EIP712 typed data hash of 0x transaction. + function _assertExecutableTransaction( + LibZeroExTransaction.ZeroExTransaction memory transaction, + bytes memory signature, + bytes32 transactionHash + ) + internal + { // Check transaction is not expired // solhint-disable-next-line not-rely-on-time if (block.timestamp >= transaction.expirationTimeSeconds) { @@ -96,6 +139,16 @@ contract MixinTransactions is )); } + // Validate that transaction is executed with the correct gasPrice + uint256 requiredGasPrice = transaction.gasPrice; + if (tx.gasprice != requiredGasPrice) { + LibRichErrors.rrevert(LibExchangeRichErrors.TransactionGasPriceError( + transactionHash, + tx.gasprice, + requiredGasPrice + )); + } + // Prevent reentrancy if (currentContextAddress != address(0)) { LibRichErrors.rrevert(LibExchangeRichErrors.TransactionError( @@ -112,43 +165,21 @@ contract MixinTransactions is )); } + // Validate signature // Transaction always valid if signer is sender of transaction address signerAddress = transaction.signerAddress; - if (signerAddress != msg.sender) { - // Validate signature - if (!_isValidTransactionWithHashSignature( - transaction, - transactionHash, - signature)) { - LibRichErrors.rrevert(LibExchangeRichErrors.TransactionSignatureError( - transactionHash, - signerAddress, - signature - )); - } - - // Set the current transaction signer - currentContextAddress = signerAddress; - } - - // Execute transaction - transactionsExecuted[transactionHash] = true; - (bool didSucceed, bytes memory returnData) = address(this).delegatecall(transaction.data); - if (!didSucceed) { - LibRichErrors.rrevert(LibExchangeRichErrors.TransactionExecutionError( + if (signerAddress != msg.sender && !_isValidTransactionWithHashSignature( + transaction, transactionHash, - returnData + signature + ) + ) { + LibRichErrors.rrevert(LibExchangeRichErrors.TransactionSignatureError( + transactionHash, + signerAddress, + signature )); } - - // Reset current transaction signer if it was previously updated - if (signerAddress != msg.sender) { - currentContextAddress = address(0); - } - - emit TransactionExecution(transactionHash); - - return returnData; } /// @dev The current function will be called in the context of this address (either 0x transaction signer or `msg.sender`). From 27e2a761106c458aa7b45ad079b4da894cd6c67f Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Tue, 20 Aug 2019 15:09:19 -0700 Subject: [PATCH 13/34] Update remaining contracts to use new transaction schema --- contracts/exchange/contracts/examples/ExchangeWrapper.sol | 2 ++ contracts/exchange/contracts/examples/Whitelist.sol | 1 + contracts/exchange/contracts/src/MixinExchangeCore.sol | 7 +++++-- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/contracts/exchange/contracts/examples/ExchangeWrapper.sol b/contracts/exchange/contracts/examples/ExchangeWrapper.sol index 0149600f6b..6461022452 100644 --- a/contracts/exchange/contracts/examples/ExchangeWrapper.sol +++ b/contracts/exchange/contracts/examples/ExchangeWrapper.sol @@ -61,6 +61,7 @@ contract ExchangeWrapper { LibZeroExTransaction.ZeroExTransaction memory transaction = LibZeroExTransaction.ZeroExTransaction({ salt: salt, expirationTimeSeconds: transactionExpirationTimeSeconds, + gasPrice: tx.gasprice, data: data, signerAddress: makerAddress }); @@ -99,6 +100,7 @@ contract ExchangeWrapper { LibZeroExTransaction.ZeroExTransaction memory transaction = LibZeroExTransaction.ZeroExTransaction({ salt: salt, expirationTimeSeconds: transactionExpirationTimeSeconds, + gasPrice: tx.gasprice, data: data, signerAddress: takerAddress }); diff --git a/contracts/exchange/contracts/examples/Whitelist.sol b/contracts/exchange/contracts/examples/Whitelist.sol index 2c9d1d8810..5a9ccaae01 100644 --- a/contracts/exchange/contracts/examples/Whitelist.sol +++ b/contracts/exchange/contracts/examples/Whitelist.sol @@ -134,6 +134,7 @@ contract Whitelist is LibZeroExTransaction.ZeroExTransaction memory transaction = LibZeroExTransaction.ZeroExTransaction({ salt: salt, data: data, + gasPrice: tx.gasprice, expirationTimeSeconds: uint256(-1), signerAddress: takerAddress }); diff --git a/contracts/exchange/contracts/src/MixinExchangeCore.sol b/contracts/exchange/contracts/src/MixinExchangeCore.sol index 242f87e784..995fcc3e49 100644 --- a/contracts/exchange/contracts/src/MixinExchangeCore.sol +++ b/contracts/exchange/contracts/src/MixinExchangeCore.sol @@ -361,11 +361,14 @@ contract MixinExchangeCore is orderInfo.orderHash, makerAddress, signature - )) { + ) + ) { if (!_isValidOrderWithHashSignature( order, orderInfo.orderHash, - signature)) { + signature + ) + ) { LibRichErrors.rrevert(LibExchangeRichErrors.SignatureError( LibExchangeRichErrors.SignatureErrorCodes.BAD_SIGNATURE, orderInfo.orderHash, From 1dd216b566279313287f02b28b9f172d89ea1551 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Tue, 20 Aug 2019 15:22:40 -0700 Subject: [PATCH 14/34] Update ZeroExTransaction interface --- packages/types/src/index.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index bdd8554ae5..b89f1cbb9a 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -56,6 +56,7 @@ export enum MarketOperation { export interface ZeroExTransaction { salt: BigNumber; expirationTimeSeconds: BigNumber; + gasPrice: BigNumber; signerAddress: string; data: string; domain: EIP712DomainWithDefaultSchema; From e6b81a824da99016500eeca909e4d7af5144c263 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Tue, 20 Aug 2019 15:25:01 -0700 Subject: [PATCH 15/34] Update ZeroExTransaction schema --- packages/json-schemas/schemas/zero_ex_transaction_schema.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/json-schemas/schemas/zero_ex_transaction_schema.json b/packages/json-schemas/schemas/zero_ex_transaction_schema.json index 240e02ab7e..9d470b1244 100644 --- a/packages/json-schemas/schemas/zero_ex_transaction_schema.json +++ b/packages/json-schemas/schemas/zero_ex_transaction_schema.json @@ -5,8 +5,9 @@ "signerAddress": { "$ref": "/addressSchema" }, "salt": { "$ref": "/wholeNumberSchema" }, "expirationTimeSeconds": { "$ref": "/wholeNumberSchema" }, + "gasPrice": { "$ref": "/wholeNumberSchema" }, "domain": { "$ref": "/eip712DomainSchema" } }, - "required": ["data", "salt", "expirationTimeSeconds", "signerAddress", "domain"], + "required": ["data", "salt", "expirationTimeSeconds", "gasPrice", "signerAddress", "domain"], "type": "object" } From 59369cea2aefca921f7d1accc5dd6a3fc1d0affc Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Tue, 20 Aug 2019 16:15:49 -0700 Subject: [PATCH 16/34] Update transaction hashing and tests --- packages/order-utils/src/constants.ts | 1 + packages/order-utils/test/eip712_utils_test.ts | 1 + packages/order-utils/test/signature_utils_test.ts | 1 + packages/order-utils/test/transaction_hash_test.ts | 4 +++- 4 files changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/order-utils/src/constants.ts b/packages/order-utils/src/constants.ts index 0bab1402a9..ce0e4f66b9 100644 --- a/packages/order-utils/src/constants.ts +++ b/packages/order-utils/src/constants.ts @@ -134,6 +134,7 @@ export const constants = { parameters: [ { name: 'salt', type: 'uint256' }, { name: 'expirationTimeSeconds', type: 'uint256' }, + { name: 'gasPrice', type: 'uint256' }, { name: 'signerAddress', type: 'address' }, { name: 'data', type: 'bytes' }, ], diff --git a/packages/order-utils/test/eip712_utils_test.ts b/packages/order-utils/test/eip712_utils_test.ts index ec306c8973..4082b6edc4 100644 --- a/packages/order-utils/test/eip712_utils_test.ts +++ b/packages/order-utils/test/eip712_utils_test.ts @@ -59,6 +59,7 @@ describe('EIP712 Utils', () => { const typedData = eip712Utils.createZeroExTransactionTypedData({ salt: new BigNumber(0), expirationTimeSeconds: new BigNumber(0), + gasPrice: new BigNumber(0), data: constants.NULL_BYTES, signerAddress: constants.NULL_ADDRESS, domain: { diff --git a/packages/order-utils/test/signature_utils_test.ts b/packages/order-utils/test/signature_utils_test.ts index f769e257f6..09f3460b5a 100644 --- a/packages/order-utils/test/signature_utils_test.ts +++ b/packages/order-utils/test/signature_utils_test.ts @@ -55,6 +55,7 @@ describe('Signature utils', () => { signerAddress: makerAddress, data: '0x6927e990021d23b1eb7b8789f6a6feaf98fe104bb0cf8259421b79f9a34222b0', expirationTimeSeconds: new BigNumber(0), + gasPrice: new BigNumber(0), }; }); describe('#isValidSignatureAsync', () => { diff --git a/packages/order-utils/test/transaction_hash_test.ts b/packages/order-utils/test/transaction_hash_test.ts index d38961c01f..c85883b4d1 100644 --- a/packages/order-utils/test/transaction_hash_test.ts +++ b/packages/order-utils/test/transaction_hash_test.ts @@ -14,13 +14,14 @@ const expect = chai.expect; describe('0x transaction hashing', () => { describe('#getTransactionHashHex', () => { - const expectedTransactionHash = '0x9779e4ca195f8c9c6f137f495599e9a1944806310b64748479bfa6c6b1ae7eb4'; + const expectedTransactionHash = '0x420b19f08d5b09c012f381f4bf80a97740b8629f2bac7f42dd7f6aefbb24f3c0'; const fakeVerifyingContractAddress = '0x5e72914535f202659083db3a02c984188fa26e9f'; const fakeChainId = 1337; const transaction: ZeroExTransaction = { signerAddress: constants.NULL_ADDRESS, salt: new BigNumber(0), expirationTimeSeconds: new BigNumber(0), + gasPrice: new BigNumber(0), data: constants.NULL_BYTES, domain: { verifyingContractAddress: fakeVerifyingContractAddress, @@ -40,6 +41,7 @@ describe('0x transaction hashing', () => { ...transaction, salt: '0', expirationTimeSeconds: '0', + gasPrice: '0', } as any); expect(transactionHash).to.be.equal(expectedTransactionHash); }); From 23dd7113965b9881a974c4cb71f4619d2dcfe478 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Tue, 20 Aug 2019 16:16:06 -0700 Subject: [PATCH 17/34] Update ZeroExTransaction tests --- contracts/exchange-libs/test/lib_zero_ex_transaction.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/contracts/exchange-libs/test/lib_zero_ex_transaction.ts b/contracts/exchange-libs/test/lib_zero_ex_transaction.ts index f49100188c..bd398223b3 100644 --- a/contracts/exchange-libs/test/lib_zero_ex_transaction.ts +++ b/contracts/exchange-libs/test/lib_zero_ex_transaction.ts @@ -18,6 +18,7 @@ blockchainTests('LibZeroExTransaction', env => { const EMPTY_TRANSACTION: ZeroExTransaction = { salt: constants.ZERO_AMOUNT, expirationTimeSeconds: constants.ZERO_AMOUNT, + gasPrice: constants.ZERO_AMOUNT, signerAddress: constants.NULL_ADDRESS, data: constants.NULL_BYTES, domain: { @@ -66,6 +67,7 @@ blockchainTests('LibZeroExTransaction', env => { await testGetTypedDataHashAsync({ salt: randomUint256(), expirationTimeSeconds: randomUint256(), + gasPrice: randomUint256(), signerAddress: randomAddress(), data: randomAssetData(), domain: { @@ -121,6 +123,7 @@ blockchainTests('LibZeroExTransaction', env => { await testGetStructHashAsync({ salt: randomUint256(), expirationTimeSeconds: randomUint256(), + gasPrice: randomUint256(), signerAddress: randomAddress(), data: randomAssetData(), // The domain is not used in this test, so it's okay if it is left empty. From 2b1e0be4fc35b9984bcfb2b8872ea388df75b433 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Tue, 20 Aug 2019 16:48:13 -0700 Subject: [PATCH 18/34] Add default gasPrice to web3Wrapper instance and TransactionFactory --- contracts/test-utils/src/constants.ts | 1 + contracts/test-utils/src/transaction_factory.ts | 2 ++ contracts/test-utils/src/web3_wrapper.ts | 2 ++ 3 files changed, 5 insertions(+) diff --git a/contracts/test-utils/src/constants.ts b/contracts/test-utils/src/constants.ts index 82ddf44ed8..3c10572ef1 100644 --- a/contracts/test-utils/src/constants.ts +++ b/contracts/test-utils/src/constants.ts @@ -68,4 +68,5 @@ export const constants = { ONE_ETHER: new BigNumber(1e18), EIP712_DOMAIN_NAME: '0x Protocol', EIP712_DOMAIN_VERSION: '3.0.0', + DEFAULT_GAS_PRICE: 1, }; diff --git a/contracts/test-utils/src/transaction_factory.ts b/contracts/test-utils/src/transaction_factory.ts index 9f0b4e165b..ab387d38c6 100644 --- a/contracts/test-utils/src/transaction_factory.ts +++ b/contracts/test-utils/src/transaction_factory.ts @@ -4,6 +4,7 @@ import { BigNumber } from '@0x/utils'; import * as ethUtil from 'ethereumjs-util'; import { getLatestBlockTimestampAsync } from './block_timestamp'; +import { constants } from './constants'; import { signingUtils } from './signing_utils'; export class TransactionFactory { @@ -34,6 +35,7 @@ export class TransactionFactory { signerAddress, data: customTransactionParams.data, expirationTimeSeconds: new BigNumber(currentBlockTimestamp).plus(tenMinutesInSeconds), + gasPrice: new BigNumber(constants.DEFAULT_GAS_PRICE), domain: { verifyingContractAddress: this._exchangeAddress, chainId: this._chainId, diff --git a/contracts/test-utils/src/web3_wrapper.ts b/contracts/test-utils/src/web3_wrapper.ts index b002c70efb..68669abfd8 100644 --- a/contracts/test-utils/src/web3_wrapper.ts +++ b/contracts/test-utils/src/web3_wrapper.ts @@ -4,6 +4,7 @@ import { logUtils } from '@0x/utils'; import { Web3Wrapper } from '@0x/web3-wrapper'; import * as _ from 'lodash'; +import { constants } from './constants'; import { coverage } from './coverage'; import { profiler } from './profiler'; import { revertTrace } from './revert_trace'; @@ -31,6 +32,7 @@ switch (process.env.TEST_PROVIDER) { const ganacheTxDefaults = { from: devConstants.TESTRPC_FIRST_ADDRESS, gas: devConstants.GAS_LIMIT, + gasPrice: constants.DEFAULT_GAS_PRICE, }; const gethTxDefaults = { from: devConstants.TESTRPC_FIRST_ADDRESS, From f41a29ce5539bc59941bd7683cb98c068f65ff0b Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Tue, 20 Aug 2019 19:25:59 -0700 Subject: [PATCH 19/34] Fix weth tests --- contracts/erc20/test/weth9.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/erc20/test/weth9.ts b/contracts/erc20/test/weth9.ts index e410880a6e..602abd29f9 100644 --- a/contracts/erc20/test/weth9.ts +++ b/contracts/erc20/test/weth9.ts @@ -20,7 +20,7 @@ const blockchainLifecycle = new BlockchainLifecycle(web3Wrapper); describe('EtherToken', () => { let account: string; - const gasPrice = Web3Wrapper.toBaseUnitAmount(new BigNumber(20), 9); + const gasPrice = new BigNumber(constants.DEFAULT_GAS_PRICE); let etherToken: WETH9Contract; before(async () => { From f32732db1c27dce1ab98f0ecd6553adc221b94d7 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Tue, 20 Aug 2019 20:02:03 -0700 Subject: [PATCH 20/34] Add TransactionGasPriceError rich revert class --- packages/order-utils/src/exchange_revert_errors.ts | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/packages/order-utils/src/exchange_revert_errors.ts b/packages/order-utils/src/exchange_revert_errors.ts index 3d8c02a21c..cd61141c5e 100644 --- a/packages/order-utils/src/exchange_revert_errors.ts +++ b/packages/order-utils/src/exchange_revert_errors.ts @@ -213,6 +213,20 @@ export class TransactionExecutionError extends RevertError { } } +export class TransactionGasPriceError extends RevertError { + constructor(transactionHash?: string, actualGasPrice?: BigNumber, requiredGasPrice?: BigNumber) { + super( + 'TransactionGasPriceError', + 'TransactionGasPriceError(bytes32 transactionHash, uint256 actualGasPrice, uint256 requiredGasPrice)', + { + transactionHash, + actualGasPrice, + requiredGasPrice, + }, + ); + } +} + export class IncompleteFillError extends RevertError { constructor( error?: IncompleteFillErrorCode, From 9d38bf731fc09e3840fce95cef0ffef9bbe5508a Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Tue, 20 Aug 2019 20:02:19 -0700 Subject: [PATCH 21/34] Add transaction gasPrice tests --- contracts/exchange/test/transactions.ts | 42 +++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/contracts/exchange/test/transactions.ts b/contracts/exchange/test/transactions.ts index d8321aeecc..f5a0e8dd1f 100644 --- a/contracts/exchange/test/transactions.ts +++ b/contracts/exchange/test/transactions.ts @@ -177,6 +177,48 @@ blockchainTests.resets('Exchange transactions', env => { const tx = exchangeWrapper.executeTransactionAsync(transaction, senderAddress); return expect(tx).to.revertWith(expectedError); }); + it('should revert if the actual gasPrice is greater than expected', async () => { + const order = await orderFactory.newSignedOrderAsync(); + const orders = [order]; + const data = exchangeDataEncoder.encodeOrdersToExchangeData(ExchangeFunctionName.FillOrder, orders); + const transaction = await takerTransactionFactory.newSignedTransactionAsync({ + data, + }); + const transactionHashHex = transactionHashUtils.getTransactionHashHex(transaction); + const actualGasPrice = transaction.gasPrice.plus(1); + const expectedError = new ExchangeRevertErrors.TransactionGasPriceError( + transactionHashHex, + actualGasPrice, + transaction.gasPrice, + ); + const tx = exchangeInstance.executeTransaction.sendTransactionAsync( + transaction, + transaction.signature, + { gasPrice: actualGasPrice, from: senderAddress }, + ); + return expect(tx).to.revertWith(expectedError); + }); + it('should revert if the actual gasPrice is less than expected', async () => { + const order = await orderFactory.newSignedOrderAsync(); + const orders = [order]; + const data = exchangeDataEncoder.encodeOrdersToExchangeData(ExchangeFunctionName.FillOrder, orders); + const transaction = await takerTransactionFactory.newSignedTransactionAsync({ + data, + }); + const transactionHashHex = transactionHashUtils.getTransactionHashHex(transaction); + const actualGasPrice = transaction.gasPrice.minus(1); + const expectedError = new ExchangeRevertErrors.TransactionGasPriceError( + transactionHashHex, + actualGasPrice, + transaction.gasPrice, + ); + const tx = exchangeInstance.executeTransaction.sendTransactionAsync( + transaction, + transaction.signature, + { gasPrice: actualGasPrice, from: senderAddress }, + ); + return expect(tx).to.revertWith(expectedError); + }); }); describe('fill methods', () => { for (const fnName of [ From a114bbb30e1de45b0691ac44daa00bc7f5c162f0 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Tue, 20 Aug 2019 22:16:05 -0700 Subject: [PATCH 22/34] Reduce code duplication in unit tests --- .../exchange/test/transactions_unit_tests.ts | 328 +++++------------- 1 file changed, 96 insertions(+), 232 deletions(-) diff --git a/contracts/exchange/test/transactions_unit_tests.ts b/contracts/exchange/test/transactions_unit_tests.ts index 842b73cb2c..0d5ede2d3c 100644 --- a/contracts/exchange/test/transactions_unit_tests.ts +++ b/contracts/exchange/test/transactions_unit_tests.ts @@ -15,7 +15,7 @@ import * as _ from 'lodash'; import { artifacts, TestTransactionsContract, TestTransactionsTransactionExecutionEventArgs } from '../src'; -blockchainTests.resets.only('Transaction Unit Tests', ({ provider, web3Wrapper, txDefaults }) => { +blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDefaults }) => { let transactionsContract: TestTransactionsContract; let accounts: string[]; let domain: EIP712DomainWithDefaultSchema; @@ -26,6 +26,7 @@ blockchainTests.resets.only('Transaction Unit Tests', ({ provider, web3Wrapper, const EMPTY_ZERO_EX_TRANSACTION = { salt: constants.ZERO_AMOUNT, expirationTimeSeconds: constants.ZERO_AMOUNT, + gasPrice: constants.ZERO_AMOUNT, signerAddress: constants.NULL_ADDRESS, data: constants.NULL_BYTES, domain: { @@ -67,23 +68,53 @@ blockchainTests.resets.only('Transaction Unit Tests', ({ provider, web3Wrapper, ); } + interface GenerateZeroExTransactionParams { + salt?: BigNumber; + expirationTimeSeconds?: BigNumber; + gasPrice?: BigNumber; + signerAddress?: string; + data?: string; + domain?: EIP712DomainWithDefaultSchema; + shouldSucceed?: boolean; + callData?: string; + returnData?: string; + } + + async function generateZeroExTransactionAsync( + opts: GenerateZeroExTransactionParams = {}, + ): Promise { + const shouldSucceed = opts.shouldSucceed === undefined ? true : opts.shouldSucceed; + const callData = opts.callData === undefined ? constants.NULL_BYTES : opts.callData; + const returnData = opts.returnData === undefined ? constants.NULL_BYTES : opts.returnData; + const data = opts.data === undefined ? getExecutableCallData(shouldSucceed, callData, returnData) : opts.data; + const gasPrice = opts.gasPrice === undefined ? new BigNumber(constants.DEFAULT_GAS_PRICE) : opts.gasPrice; + const _domain = opts.domain === undefined ? domain : opts.domain; + const expirationTimeSeconds = + opts.expirationTimeSeconds === undefined + ? new BigNumber(await getLatestBlockTimestampAsync()).plus(10) + : opts.expirationTimeSeconds; + const transaction = { + ...EMPTY_ZERO_EX_TRANSACTION, + ...opts, + data, + expirationTimeSeconds, + domain: _domain, + gasPrice, + }; + return transaction; + } + describe('batchExecuteTransaction', () => { it('should revert if the only call to executeTransaction fails', async () => { // Create an expired transaction that will fail when used to call `batchExecuteTransactions()`. - const currentTimestamp = await getLatestBlockTimestampAsync(); - const transaction = { - ...EMPTY_ZERO_EX_TRANSACTION, - expirationTimeSeconds: new BigNumber(currentTimestamp).minus(10), // Set the expiration time to before the current timestamp - data: getExecutableCallData(false, constants.NULL_BYTES, constants.NULL_BYTES), - domain, - }; + const transaction = await generateZeroExTransactionAsync({ shouldSucceed: false }); const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); - // We expect a `TransactionError` to be returned because that is the error that will be triggered in the call to - // `executeTransaction`. - const expectedError = new ExchangeRevertErrors.TransactionError( - ExchangeRevertErrors.TransactionErrorCode.Expired, + // Create the StringRevertError that reflects the returndata that will be returned by the failed transaction. + const executableError = new StringRevertError('EXECUTABLE_FAILED'); + const expectedError = new ExchangeRevertErrors.TransactionExecutionError( transactionHash, + executableError.encode(), ); // Call the `batchExecuteTransactions()` function and ensure that it reverts with the expected revert error. @@ -96,21 +127,10 @@ blockchainTests.resets.only('Transaction Unit Tests', ({ provider, web3Wrapper, it('should revert if the second call to executeTransaction fails', async () => { // Create a transaction that will succeed when used to call `batchExecuteTransactions()`. - const currentTimestamp = await getLatestBlockTimestampAsync(); - const transaction1 = { - ...EMPTY_ZERO_EX_TRANSACTION, - data: getExecutableCallData(true, constants.NULL_BYTES, constants.NULL_BYTES), // This call should succeed - expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), - domain, - }; + const transaction1 = await generateZeroExTransactionAsync(); // Create a transaction that will fail when used to call `batchExecuteTransactions()` because the call to executable will fail. - const transaction2 = { - ...EMPTY_ZERO_EX_TRANSACTION, - data: getExecutableCallData(false, constants.NULL_BYTES, constants.NULL_BYTES), // This call should fail - expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), - domain, - }; + const transaction2 = await generateZeroExTransactionAsync({ shouldSucceed: false }); const transactionHash = transactionHashUtils.getTransactionHashHex(transaction2); // Create the StringRevertError that reflects the returndata that will be returned by the failed transaction. @@ -130,21 +150,10 @@ blockchainTests.resets.only('Transaction Unit Tests', ({ provider, web3Wrapper, it('should revert if the first call to executeTransaction fails', async () => { // Create a transaction that will fail when used to call `batchExecuteTransactions()` because the call to executable will fail. - const currentTimestamp = await getLatestBlockTimestampAsync(); - const transaction1 = { - ...EMPTY_ZERO_EX_TRANSACTION, - data: getExecutableCallData(false, constants.NULL_BYTES, constants.NULL_BYTES), // This call should fail - expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), - domain, - }; + const transaction1 = await generateZeroExTransactionAsync({ shouldSucceed: false }); // Create a transaction that will succeed when used to call `batchExecuteTransactions()`. - const transaction2 = { - ...EMPTY_ZERO_EX_TRANSACTION, - data: getExecutableCallData(true, constants.NULL_BYTES, constants.NULL_BYTES), - expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), - domain, - }; + const transaction2 = await generateZeroExTransactionAsync(); const transactionHash = transactionHashUtils.getTransactionHashHex(transaction1); // Create the StringRevertError that reflects the returndata that will be returned by the failed transaction. @@ -164,24 +173,11 @@ blockchainTests.resets.only('Transaction Unit Tests', ({ provider, web3Wrapper, it('should revert if the same transaction is executed twice in a batch', async () => { // Create a transaction that will succeed when used to call `batchExecuteTransactions()`. - const currentTimestamp = await getLatestBlockTimestampAsync(); - const transaction1 = { - ...EMPTY_ZERO_EX_TRANSACTION, - expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), - signerAddress: accounts[1], - data: getExecutableCallData(true, constants.NULL_BYTES, constants.NULL_BYTES), - domain, - }; + const transaction1 = await generateZeroExTransactionAsync({ signerAddress: accounts[1] }); // Duplicate the first transaction. This should cause the call to `batchExecuteTransactions()` to fail // because this transaction will have the same order hash as transaction1. - const transaction2 = { - ...EMPTY_ZERO_EX_TRANSACTION, - expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), - signerAddress: accounts[1], - data: getExecutableCallData(true, constants.NULL_BYTES, constants.NULL_BYTES), - domain, - }; + const transaction2 = transaction1; const transactionHash2 = transactionHashUtils.getTransactionHashHex(transaction2); // Call the `batchExecuteTransactions()` function and ensure that it reverts with the expected revert error. @@ -200,16 +196,11 @@ blockchainTests.resets.only('Transaction Unit Tests', ({ provider, web3Wrapper, }); it('should succeed if the only call to executeTransaction succeeds', async () => { - const currentTimestamp = await getLatestBlockTimestampAsync(); - // Create a transaction that will succeed when used to call `batchExecuteTransactions()`. - const transaction = { - ...EMPTY_ZERO_EX_TRANSACTION, - expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), + const transaction = await generateZeroExTransactionAsync({ signerAddress: accounts[1], - data: getExecutableCallData(true, constants.NULL_BYTES, '0xdeadbeef'), - domain, - }; + returnData: '0xdeadbeef', + }); const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); const validSignature = randomSignature(); @@ -258,23 +249,15 @@ blockchainTests.resets.only('Transaction Unit Tests', ({ provider, web3Wrapper, }); it('should succeed if the both calls to executeTransaction succeed', async () => { - const currentTimestamp = await getLatestBlockTimestampAsync(); - // Create two transactions that will succeed when used to call `batchExecuteTransactions()`. - const transaction1 = { - ...EMPTY_ZERO_EX_TRANSACTION, - expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), - signerAddress: accounts[0], // This is different than the account that will be used to send. - data: getExecutableCallData(true, constants.NULL_BYTES, '0xdeadbeef'), - domain, - }; - const transaction2 = { - ...EMPTY_ZERO_EX_TRANSACTION, - expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), - signerAddress: accounts[1], // Different than transaction1's signer address - data: getExecutableCallData(true, constants.NULL_BYTES, '0xbeefdead'), - domain, - }; + const transaction1 = await generateZeroExTransactionAsync({ + signerAddress: accounts[0], + returnData: '0xdeadbeef', + }); + const transaction2 = await generateZeroExTransactionAsync({ + signerAddress: accounts[1], + returnData: '0xbeefdead', + }); const transactionHash1 = transactionHashUtils.getTransactionHashHex(transaction1); const transactionHash2 = transactionHashUtils.getTransactionHashHex(transaction2); @@ -347,14 +330,11 @@ blockchainTests.resets.only('Transaction Unit Tests', ({ provider, web3Wrapper, signature, ); } - it('should revert if the current time is past the expiration time', async () => { const currentTimestamp = await getLatestBlockTimestampAsync(); - const transaction = { - ...EMPTY_ZERO_EX_TRANSACTION, + const transaction = await generateZeroExTransactionAsync({ expirationTimeSeconds: new BigNumber(currentTimestamp).minus(10), // Set the expiration time to before the current timestamp - domain, - }; + }); const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); const expectedError = new ExchangeRevertErrors.TransactionError( ExchangeRevertErrors.TransactionErrorCode.Expired, @@ -363,185 +343,100 @@ blockchainTests.resets.only('Transaction Unit Tests', ({ provider, web3Wrapper, const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction, randomSignature()); return expect(tx).to.revertWith(expectedError); }); - // FIXME - This should be unskipped when the contracts have been updated to fix this problem. it.skip('should revert if reentrancy occurs in the middle of an executeTransaction call and msg.sender == signer for both calls', async () => { const validSignature = randomSignature(); - - const currentTimestamp = await getLatestBlockTimestampAsync(); - const transaction1 = { - ...EMPTY_ZERO_EX_TRANSACTION, - expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), - data: getExecutableCallData(true, constants.NULL_BYTES, constants.NULL_BYTES), // This should never get called - signerAddress: accounts[0], - domain, - }; + const transaction1 = await generateZeroExTransactionAsync({ signerAddress: accounts[0] }); const transactionHash1 = transactionHashUtils.getTransactionHashHex(transaction1); - - const callData = getExecutableCallData( - true, - getExecuteTransactionCallData(transaction1, validSignature), - '0xdeadbeef', - ); - - const transaction2 = { - ...EMPTY_ZERO_EX_TRANSACTION, - expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), - data: callData, + const transaction2 = await generateZeroExTransactionAsync({ signerAddress: accounts[0], - domain, - }; + callData: getExecuteTransactionCallData(transaction1, validSignature), + returnData: '0xdeadbeef', + }); const transactionHash2 = transactionHashUtils.getTransactionHashHex(transaction2); const abiEncoder = AbiEncoder.createMethod('TransactionError', ['uint8', 'bytes32']); const errorData = abiEncoder.encode([ ExchangeRevertErrors.TransactionErrorCode.NoReentrancy, transactionHash1, ]); - const expectedError = new ExchangeRevertErrors.TransactionExecutionError(transactionHash2, errorData); const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction2, validSignature, { from: accounts[0], }); return expect(tx).to.revertWith(expectedError); }); - it('should revert if reentrancy occurs in the middle of an executeTransaction call and msg.sender != signer for both calls', async () => { const validSignature = randomSignature(); - - const currentTimestamp = await getLatestBlockTimestampAsync(); - const transaction1 = { - ...EMPTY_ZERO_EX_TRANSACTION, - expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), - data: getExecutableCallData(true, constants.NULL_BYTES, constants.NULL_BYTES), // This should never get called - signerAddress: accounts[0], - domain, - }; + const transaction1 = await generateZeroExTransactionAsync({ signerAddress: accounts[0] }); const transactionHash1 = transactionHashUtils.getTransactionHashHex(transaction1); - - const callData = getExecutableCallData( - true, - getExecuteTransactionCallData(transaction1, validSignature), - '0xdeadbeef', - ); - - const transaction2 = { - ...EMPTY_ZERO_EX_TRANSACTION, - expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), - data: callData, + const transaction2 = await generateZeroExTransactionAsync({ signerAddress: accounts[0], - domain, - }; + callData: getExecuteTransactionCallData(transaction1, validSignature), + returnData: '0xdeadbeef', + }); const transactionHash2 = transactionHashUtils.getTransactionHashHex(transaction2); const abiEncoder = AbiEncoder.createMethod('TransactionError', ['uint8', 'bytes32']); const errorData = abiEncoder.encode([ ExchangeRevertErrors.TransactionErrorCode.NoReentrancy, transactionHash1, ]); - const expectedError = new ExchangeRevertErrors.TransactionExecutionError(transactionHash2, errorData); const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction2, validSignature, { from: accounts[1], // Different then the signing addresses }); return expect(tx).to.revertWith(expectedError); }); - it('should revert if reentrancy occurs in the middle of an executeTransaction call and msg.sender != signer and then msg.sender == sender', async () => { const validSignature = randomSignature(); - - const currentTimestamp = await getLatestBlockTimestampAsync(); - const transaction1 = { - ...EMPTY_ZERO_EX_TRANSACTION, - expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), - data: getExecutableCallData(true, constants.NULL_BYTES, constants.NULL_BYTES), // This should never get called - signerAddress: accounts[1], - domain, - }; + const transaction1 = await generateZeroExTransactionAsync({ signerAddress: accounts[1] }); const transactionHash1 = transactionHashUtils.getTransactionHashHex(transaction1); - - const callData = getExecutableCallData( - true, - getExecuteTransactionCallData(transaction1, validSignature), - '0xdeadbeef', - ); - - const transaction2 = { - ...EMPTY_ZERO_EX_TRANSACTION, - expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), - data: callData, + const transaction2 = await generateZeroExTransactionAsync({ signerAddress: accounts[0], - domain, - }; + callData: getExecuteTransactionCallData(transaction1, validSignature), + returnData: '0xdeadbeef', + }); const transactionHash2 = transactionHashUtils.getTransactionHashHex(transaction2); const abiEncoder = AbiEncoder.createMethod('TransactionError', ['uint8', 'bytes32']); const errorData = abiEncoder.encode([ ExchangeRevertErrors.TransactionErrorCode.NoReentrancy, transactionHash1, ]); - const expectedError = new ExchangeRevertErrors.TransactionExecutionError(transactionHash2, errorData); const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction2, validSignature, { from: accounts[1], // Different then the signing addresses }); return expect(tx).to.revertWith(expectedError); }); - // FIXME - This should be unskipped when the contracts have been updated to fix this problem. it.skip('should revert if reentrancy occurs in the middle of an executeTransaction call and msg.sender == signer and then msg.sender != sender', async () => { const validSignature = randomSignature(); - - const currentTimestamp = await getLatestBlockTimestampAsync(); - const transaction1 = { - ...EMPTY_ZERO_EX_TRANSACTION, - expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), - data: getExecutableCallData(true, constants.NULL_BYTES, constants.NULL_BYTES), // This should never get called - signerAddress: accounts[0], - domain, - }; + const transaction1 = await generateZeroExTransactionAsync({ signerAddress: accounts[0] }); const transactionHash1 = transactionHashUtils.getTransactionHashHex(transaction1); - - const callData = getExecutableCallData( - true, - getExecuteTransactionCallData(transaction1, validSignature), - '0xdeadbeef', - ); - - const transaction2 = { - ...EMPTY_ZERO_EX_TRANSACTION, - expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), - data: callData, - signerAddress: accounts[1], - domain, - }; + const transaction2 = await generateZeroExTransactionAsync({ + signerAddress: accounts[0], + callData: getExecuteTransactionCallData(transaction1, validSignature), + returnData: '0xdeadbeef', + }); const transactionHash2 = transactionHashUtils.getTransactionHashHex(transaction2); const abiEncoder = AbiEncoder.createMethod('TransactionError', ['uint8', 'bytes32']); const errorData = abiEncoder.encode([ ExchangeRevertErrors.TransactionErrorCode.NoReentrancy, transactionHash1, ]); - const expectedError = new ExchangeRevertErrors.TransactionExecutionError(transactionHash2, errorData); const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction2, validSignature, { from: accounts[1], // Different then the signing addresses }); return expect(tx).to.revertWith(expectedError); }); - it('should revert if the transaction has been executed previously', async () => { const validSignature = randomSignature(); - const currentTimestamp = await getLatestBlockTimestampAsync(); - const transaction = { - ...EMPTY_ZERO_EX_TRANSACTION, - expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), - data: getExecutableCallData(true, constants.NULL_BYTES, constants.NULL_BYTES), - domain, - }; + const transaction = await generateZeroExTransactionAsync(); const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); - // Use the transaction in execute transaction. await expect( transactionsContract.executeTransaction.sendTransactionAsync(transaction, validSignature), ).to.be.fulfilled(''); - // Use the same transaction to make another call const expectedError = new ExchangeRevertErrors.TransactionError( ExchangeRevertErrors.TransactionErrorCode.AlreadyExecuted, @@ -550,15 +445,8 @@ blockchainTests.resets.only('Transaction Unit Tests', ({ provider, web3Wrapper, const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction, validSignature); return expect(tx).to.revertWith(expectedError); }); - it('should revert if the signer != msg.sender and the signature is not valid', async () => { - const currentTimestamp = await getLatestBlockTimestampAsync(); - const transaction = { - ...EMPTY_ZERO_EX_TRANSACTION, - expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), - signerAddress: accounts[1], // This is different than the account that will be used to send. - domain, - }; + const transaction = await generateZeroExTransactionAsync({ signerAddress: accounts[1] }); const signature = '0x0000'; // This is the invalid signature const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); const expectedError = new ExchangeRevertErrors.TransactionSignatureError( @@ -571,18 +459,12 @@ blockchainTests.resets.only('Transaction Unit Tests', ({ provider, web3Wrapper, }); return expect(tx).to.revertWith(expectedError); }); - it('should revert if the signer == msg.sender but the delegatecall fails', async () => { // This calldata is encoded to fail when it hits the executable function. - const callData = getExecutableCallData(false, constants.NULL_BYTES, constants.NULL_BYTES); - const currentTimestamp = await getLatestBlockTimestampAsync(); - const transaction = { - ...EMPTY_ZERO_EX_TRANSACTION, - expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), + const transaction = await generateZeroExTransactionAsync({ signerAddress: accounts[1], - data: callData, - domain, - }; + shouldSucceed: false, + }); const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); const executableError = new StringRevertError('EXECUTABLE_FAILED'); const expectedError = new ExchangeRevertErrors.TransactionExecutionError( @@ -594,18 +476,12 @@ blockchainTests.resets.only('Transaction Unit Tests', ({ provider, web3Wrapper, }); return expect(tx).to.revertWith(expectedError); }); - it('should revert if the signer != msg.sender and the signature is valid but the delegatecall fails', async () => { // This calldata is encoded to fail when it hits the executable function. - const callData = getExecutableCallData(false, constants.NULL_BYTES, constants.NULL_BYTES); - const currentTimestamp = await getLatestBlockTimestampAsync(); - const transaction = { - ...EMPTY_ZERO_EX_TRANSACTION, - expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), - signerAddress: accounts[1], // This is different than the account that will be used to send. - data: callData, - domain, - }; + const transaction = await generateZeroExTransactionAsync({ + signerAddress: accounts[1], + shouldSucceed: false, + }); const validSignature = randomSignature(); // Valid because length != 2 const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); const executableError = new StringRevertError('EXECUTABLE_FAILED'); @@ -618,30 +494,21 @@ blockchainTests.resets.only('Transaction Unit Tests', ({ provider, web3Wrapper, }); return expect(tx).to.revertWith(expectedError); }); - it('should succeed with the correct return hash and event emitted', async () => { // This calldata is encoded to succeed when it hits the executable function. - const callData = getExecutableCallData(true, constants.NULL_BYTES, '0xdeadbeef'); - const currentTimestamp = await getLatestBlockTimestampAsync(); const validSignature = randomSignature(); // Valid because length != 2 - const transaction = { - ...EMPTY_ZERO_EX_TRANSACTION, - expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), - signerAddress: accounts[1], // This is different than the account that will be used to send. - data: callData, - domain, - }; + const transaction = await generateZeroExTransactionAsync({ + signerAddress: accounts[1], + returnData: '0xdeadbeef', + }); const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); - // Verify that the returndata of the transaction is 0xDEADBEEF const result = await transactionsContract.executeTransaction.callAsync(transaction, validSignature, { from: accounts[0], }); - // Create an abiEncoder for bytes. This will be used to decode the result and encode what // is expected. const abiEncoder = AbiEncoder.create('bytes'); - // Ensure that the result contains the abi-encoded bytes "0xdeadbeef" const encodedDeadbeef = abiEncoder.encode('0xdeadbeef'); expect( @@ -650,18 +517,15 @@ blockchainTests.resets.only('Transaction Unit Tests', ({ provider, web3Wrapper, encodedDeadbeef.slice(2, encodedDeadbeef.length), ), ).to.be.true(); - // Verify that the logs returned from the call are correct. const receipt = await logDecoder.getTxWithDecodedLogsAsync( await transactionsContract.executeTransaction.sendTransactionAsync(transaction, validSignature, { from: accounts[0], }), ); - // Ensure that the correct number of events were logged. const logs = receipt.logs as Array>; expect(logs.length).to.be.eq(2); - // Ensure that the correct events were logged. expect(logs[0].event).to.be.eq('ExecutableCalled'); expect(logs[0].args.data).to.be.eq(constants.NULL_BYTES); From eb6637afd526ef246d1272a1d6c12d4c5f429c20 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Wed, 21 Aug 2019 08:18:36 -0700 Subject: [PATCH 23/34] Add public version of _assertExecutableTransaction --- .../exchange/contracts/src/MixinTransactions.sol | 1 + .../exchange/contracts/test/TestTransactions.sol | 14 ++++++++++++++ 2 files changed, 15 insertions(+) diff --git a/contracts/exchange/contracts/src/MixinTransactions.sol b/contracts/exchange/contracts/src/MixinTransactions.sol index 2228b1517b..28b00948b7 100644 --- a/contracts/exchange/contracts/src/MixinTransactions.sol +++ b/contracts/exchange/contracts/src/MixinTransactions.sol @@ -129,6 +129,7 @@ contract MixinTransactions is bytes32 transactionHash ) internal + view { // Check transaction is not expired // solhint-disable-next-line not-rely-on-time diff --git a/contracts/exchange/contracts/test/TestTransactions.sol b/contracts/exchange/contracts/test/TestTransactions.sol index af0bb93e73..c0637ddfbb 100644 --- a/contracts/exchange/contracts/test/TestTransactions.sol +++ b/contracts/exchange/contracts/test/TestTransactions.sol @@ -53,6 +53,20 @@ contract TestTransactions is return _getCurrentContextAddress(); } + function assertExecutableTransaction( + LibZeroExTransaction.ZeroExTransaction memory transaction, + bytes memory signature + ) + public + view + { + return _assertExecutableTransaction( + transaction, + signature, + transaction.getTypedDataHash(EIP712_EXCHANGE_DOMAIN_HASH) + ); + } + // This function will execute arbitrary calldata via a delegatecall. This is highly unsafe to use in production, and this // is only meant to be used during testing. function executable( From ca35eed95547bea024764ea5cd6e7a6118ecd33b Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Wed, 21 Aug 2019 08:49:43 -0700 Subject: [PATCH 24/34] Add _assertExecutableTransaction unit tests --- .../exchange/test/transactions_unit_tests.ts | 100 ++++++++++++++++++ 1 file changed, 100 insertions(+) diff --git a/contracts/exchange/test/transactions_unit_tests.ts b/contracts/exchange/test/transactions_unit_tests.ts index 0d5ede2d3c..e0f0cff130 100644 --- a/contracts/exchange/test/transactions_unit_tests.ts +++ b/contracts/exchange/test/transactions_unit_tests.ts @@ -343,6 +343,20 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction, randomSignature()); return expect(tx).to.revertWith(expectedError); }); + it('should revert if the transaction is submitted with a gasPrice that does not equal the required gasPrice', async () => { + const transaction = await generateZeroExTransactionAsync(); + const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); + const actualGasPrice = transaction.gasPrice.plus(1); + const expectedError = new ExchangeRevertErrors.TransactionGasPriceError( + transactionHash, + actualGasPrice, + transaction.gasPrice, + ); + const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction, randomSignature(), { + gasPrice: actualGasPrice, + }); + return expect(tx).to.revertWith(expectedError); + }); // FIXME - This should be unskipped when the contracts have been updated to fix this problem. it.skip('should revert if reentrancy occurs in the middle of an executeTransaction call and msg.sender == signer for both calls', async () => { const validSignature = randomSignature(); @@ -535,6 +549,92 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef }); }); + blockchainTests.resets('assertExecutableTransaction', () => { + it('should revert if the transaction is expired', async () => { + const currentTimestamp = await getLatestBlockTimestampAsync(); + const transaction = await generateZeroExTransactionAsync({ + expirationTimeSeconds: new BigNumber(currentTimestamp).minus(10), + }); + const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); + const expectedError = new ExchangeRevertErrors.TransactionError( + ExchangeRevertErrors.TransactionErrorCode.Expired, + transactionHash, + ); + expect( + transactionsContract.assertExecutableTransaction.callAsync(transaction, randomSignature()), + ).to.revertWith(expectedError); + }); + it('should revert if the gasPrice is less than required', async () => { + const transaction = await generateZeroExTransactionAsync({}); + const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); + const actualGasPrice = transaction.gasPrice.minus(1); + const expectedError = new ExchangeRevertErrors.TransactionGasPriceError( + transactionHash, + actualGasPrice, + transaction.gasPrice, + ); + expect( + transactionsContract.assertExecutableTransaction.callAsync(transaction, randomSignature(), { + gasPrice: actualGasPrice, + }), + ).to.revertWith(expectedError); + }); + it('should revert if the gasPrice is greater than required', async () => { + const transaction = await generateZeroExTransactionAsync({}); + const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); + const actualGasPrice = transaction.gasPrice.plus(1); + const expectedError = new ExchangeRevertErrors.TransactionGasPriceError( + transactionHash, + actualGasPrice, + transaction.gasPrice, + ); + expect( + transactionsContract.assertExecutableTransaction.callAsync(transaction, randomSignature(), { + gasPrice: actualGasPrice, + }), + ).to.revertWith(expectedError); + }); + it('should revert if currentContextAddress is non-zero', async () => { + await transactionsContract.setCurrentContextAddress.awaitTransactionSuccessAsync(accounts[0]); + const transaction = await generateZeroExTransactionAsync({}); + const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); + const expectedError = new ExchangeRevertErrors.TransactionError( + ExchangeRevertErrors.TransactionErrorCode.NoReentrancy, + transactionHash, + ); + expect( + transactionsContract.assertExecutableTransaction.callAsync(transaction, randomSignature()), + ).to.revertWith(expectedError); + }); + it('should revert if the transaction has already been executed', async () => { + const transaction = await generateZeroExTransactionAsync({}); + const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); + await transactionsContract.setTransactionHash.awaitTransactionSuccessAsync(transactionHash); + const expectedError = new ExchangeRevertErrors.TransactionError( + ExchangeRevertErrors.TransactionErrorCode.AlreadyExecuted, + transactionHash, + ); + expect( + transactionsContract.assertExecutableTransaction.callAsync(transaction, randomSignature()), + ).to.revertWith(expectedError); + }); + it('should revert if signer != msg.sender and the signature is invalid', async () => { + const transaction = await generateZeroExTransactionAsync({ signerAddress: accounts[0] }); + const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); + const invalidSignature = '0x0000'; + const expectedError = new ExchangeRevertErrors.TransactionSignatureError( + transactionHash, + accounts[0], + invalidSignature, + ); + expect( + transactionsContract.assertExecutableTransaction.callAsync(transaction, invalidSignature, { + from: accounts[1], + }), + ).to.revertWith(expectedError); + }); + }); + describe('getCurrentContext', () => { it('should return the sender address when there is not a saved context address', async () => { const currentContextAddress = await transactionsContract.getCurrentContextAddress.callAsync({ From 7b96fa8d7637f5cf1210ab1e9a4f2d6b8cf13949 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Wed, 21 Aug 2019 10:05:50 -0700 Subject: [PATCH 25/34] Add more unit tests --- .../exchange/test/transactions_unit_tests.ts | 25 ++++++++++++++++--- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/contracts/exchange/test/transactions_unit_tests.ts b/contracts/exchange/test/transactions_unit_tests.ts index e0f0cff130..6e198c52b6 100644 --- a/contracts/exchange/test/transactions_unit_tests.ts +++ b/contracts/exchange/test/transactions_unit_tests.ts @@ -565,7 +565,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef ).to.revertWith(expectedError); }); it('should revert if the gasPrice is less than required', async () => { - const transaction = await generateZeroExTransactionAsync({}); + const transaction = await generateZeroExTransactionAsync(); const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); const actualGasPrice = transaction.gasPrice.minus(1); const expectedError = new ExchangeRevertErrors.TransactionGasPriceError( @@ -580,7 +580,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef ).to.revertWith(expectedError); }); it('should revert if the gasPrice is greater than required', async () => { - const transaction = await generateZeroExTransactionAsync({}); + const transaction = await generateZeroExTransactionAsync(); const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); const actualGasPrice = transaction.gasPrice.plus(1); const expectedError = new ExchangeRevertErrors.TransactionGasPriceError( @@ -596,7 +596,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef }); it('should revert if currentContextAddress is non-zero', async () => { await transactionsContract.setCurrentContextAddress.awaitTransactionSuccessAsync(accounts[0]); - const transaction = await generateZeroExTransactionAsync({}); + const transaction = await generateZeroExTransactionAsync(); const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); const expectedError = new ExchangeRevertErrors.TransactionError( ExchangeRevertErrors.TransactionErrorCode.NoReentrancy, @@ -607,7 +607,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef ).to.revertWith(expectedError); }); it('should revert if the transaction has already been executed', async () => { - const transaction = await generateZeroExTransactionAsync({}); + const transaction = await generateZeroExTransactionAsync(); const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); await transactionsContract.setTransactionHash.awaitTransactionSuccessAsync(transactionHash); const expectedError = new ExchangeRevertErrors.TransactionError( @@ -633,6 +633,23 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef }), ).to.revertWith(expectedError); }); + it('should be successful if signer == msg.sender and the signature is invalid', async () => { + const transaction = await generateZeroExTransactionAsync({ signerAddress: accounts[0] }); + const invalidSignature = '0x0000'; + expect( + transactionsContract.assertExecutableTransaction.callAsync(transaction, invalidSignature, { + from: accounts[0], + }), + ).to.be.fulfilled(''); + }); + it('should be successful if not expired, the gasPrice is correct, the tx has not been executed, currentContextAddress has not been set, signer != msg.sender, and the signature is valid', async () => { + const transaction = await generateZeroExTransactionAsync({ signerAddress: accounts[0] }); + expect( + transactionsContract.assertExecutableTransaction.callAsync(transaction, randomSignature(), { + from: accounts[1], + }), + ).to.be.fulfilled(''); + }); }); describe('getCurrentContext', () => { From 44753bb168adef30c04e59398e21b73004022f2c Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Thu, 22 Aug 2019 15:46:28 -0700 Subject: [PATCH 26/34] Add TransactionInvalidContextError and remove NO_REENTRANCY errorCode --- .../contracts/src/LibExchangeRichErrors.sol | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/contracts/exchange-libs/contracts/src/LibExchangeRichErrors.sol b/contracts/exchange-libs/contracts/src/LibExchangeRichErrors.sol index b1f6885821..04ce429e21 100644 --- a/contracts/exchange-libs/contracts/src/LibExchangeRichErrors.sol +++ b/contracts/exchange-libs/contracts/src/LibExchangeRichErrors.sol @@ -53,7 +53,6 @@ library LibExchangeRichErrors { } enum TransactionErrorCodes { - NO_REENTRANCY, ALREADY_EXECUTED, EXPIRED } @@ -136,6 +135,10 @@ library LibExchangeRichErrors { bytes4 internal constant TRANSACTION_GAS_PRICE_ERROR_SELECTOR = 0xa26dac09; + // bytes4(keccak256("TransactionInvalidContextError(bytes32,address)")) + bytes4 internal constant TRANSACTION_INVALID_CONTEXT_ERROR_SELECTOR = + 0xdec4aedf; + // bytes4(keccak256("IncompleteFillError(uint8,uint256,uint256)")) bytes4 internal constant INCOMPLETE_FILL_ERROR_SELECTOR = 0x18e4b141; @@ -610,6 +613,21 @@ library LibExchangeRichErrors { ); } + function TransactionInvalidContextError( + bytes32 transactionHash, + address currentContextAddress + ) + internal + pure + returns (bytes memory) + { + return abi.encodeWithSelector( + TRANSACTION_INVALID_CONTEXT_ERROR_SELECTOR, + transactionHash, + currentContextAddress + ); + } + function IncompleteFillError( IncompleteFillErrorCode errorCode, uint256 expectedAssetFillAmount, From edb923b8bb77e4f98dfbb1590df7bec02fbac99e Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Thu, 22 Aug 2019 15:47:25 -0700 Subject: [PATCH 27/34] Use TransactionInvalidContextError in _assertExecutableTransaction --- .../exchange/contracts/src/MixinTransactions.sol | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/contracts/exchange/contracts/src/MixinTransactions.sol b/contracts/exchange/contracts/src/MixinTransactions.sol index 28b00948b7..86788d19d0 100644 --- a/contracts/exchange/contracts/src/MixinTransactions.sol +++ b/contracts/exchange/contracts/src/MixinTransactions.sol @@ -150,11 +150,12 @@ contract MixinTransactions is )); } - // Prevent reentrancy - if (currentContextAddress != address(0)) { - LibRichErrors.rrevert(LibExchangeRichErrors.TransactionError( - LibExchangeRichErrors.TransactionErrorCodes.NO_REENTRANCY, - transactionHash + // Prevent `executeTransaction` from being called when context is already set + address currentContextAddress_ = currentContextAddress; + if (currentContextAddress_ != address(0)) { + LibRichErrors.rrevert(LibExchangeRichErrors.TransactionInvalidContextError( + transactionHash, + currentContextAddress_ )); } From 37cc9487415d0e71bff307f206c89a1ae3687b78 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Thu, 22 Aug 2019 15:48:44 -0700 Subject: [PATCH 28/34] Add TransactionInvalidContextError class --- packages/order-utils/src/exchange_revert_errors.ts | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/packages/order-utils/src/exchange_revert_errors.ts b/packages/order-utils/src/exchange_revert_errors.ts index cd61141c5e..dacb4a7446 100644 --- a/packages/order-utils/src/exchange_revert_errors.ts +++ b/packages/order-utils/src/exchange_revert_errors.ts @@ -33,7 +33,6 @@ export enum AssetProxyDispatchErrorCode { } export enum TransactionErrorCode { - NoReentrancy, AlreadyExecuted, Expired, } @@ -227,6 +226,19 @@ export class TransactionGasPriceError extends RevertError { } } +export class TransactionInvalidContextError extends RevertError { + constructor(transactionHash?: string, currentContextAddress?: string) { + super( + 'TransactionInvalidContextError', + 'TransactionInvalidContextError(bytes32 transactionHash, address currentContextAddress)', + { + transactionHash, + currentContextAddress, + }, + ); + } +} + export class IncompleteFillError extends RevertError { constructor( error?: IncompleteFillErrorCode, From 890bfd18fa83fc2f33f931fdad80b578e5b032d9 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Thu, 22 Aug 2019 15:51:59 -0700 Subject: [PATCH 29/34] Update tests to use new TransactionInvalidContextError --- contracts/exchange/src/artifacts.ts | 2 +- contracts/exchange/test/transactions.ts | 6 +- .../exchange/test/transactions_unit_tests.ts | 63 +++---------------- 3 files changed, 11 insertions(+), 60 deletions(-) diff --git a/contracts/exchange/src/artifacts.ts b/contracts/exchange/src/artifacts.ts index a5551815fd..72d4f1c186 100644 --- a/contracts/exchange/src/artifacts.ts +++ b/contracts/exchange/src/artifacts.ts @@ -14,11 +14,11 @@ import * as IExchange from '../generated-artifacts/IExchange.json'; import * as IExchangeCore from '../generated-artifacts/IExchangeCore.json'; import * as IMatchOrders from '../generated-artifacts/IMatchOrders.json'; import * as ISignatureValidator from '../generated-artifacts/ISignatureValidator.json'; +import * as IsolatedExchange from '../generated-artifacts/IsolatedExchange.json'; import * as ITransactions from '../generated-artifacts/ITransactions.json'; import * as ITransferSimulator from '../generated-artifacts/ITransferSimulator.json'; import * as IWallet from '../generated-artifacts/IWallet.json'; import * as IWrapperFunctions from '../generated-artifacts/IWrapperFunctions.json'; -import * as IsolatedExchange from '../generated-artifacts/IsolatedExchange.json'; import * as LibExchangeRichErrorDecoder from '../generated-artifacts/LibExchangeRichErrorDecoder.json'; import * as MixinAssetProxyDispatcher from '../generated-artifacts/MixinAssetProxyDispatcher.json'; import * as MixinExchangeCore from '../generated-artifacts/MixinExchangeCore.json'; diff --git a/contracts/exchange/test/transactions.ts b/contracts/exchange/test/transactions.ts index f5a0e8dd1f..140e2868de 100644 --- a/contracts/exchange/test/transactions.ts +++ b/contracts/exchange/test/transactions.ts @@ -43,7 +43,7 @@ import { const artifacts = { ...erc20Artifacts, ...localArtifacts }; // tslint:disable:no-unnecessary-type-assertion -blockchainTests.resets('Exchange transactions', env => { +blockchainTests.resets.only('Exchange transactions', env => { let chainId: number; let senderAddress: string; let owner: string; @@ -349,9 +349,9 @@ blockchainTests.resets('Exchange transactions', env => { const recursiveTransactionHashHex = transactionHashUtils.getTransactionHashHex( recursiveTransaction, ); - const noReentrancyError = new ExchangeRevertErrors.TransactionError( - ExchangeRevertErrors.TransactionErrorCode.NoReentrancy, + const noReentrancyError = new ExchangeRevertErrors.TransactionInvalidContextError( transactionHashHex, + transaction.signerAddress, ).encode(); const expectedError = new ExchangeRevertErrors.TransactionExecutionError( recursiveTransactionHashHex, diff --git a/contracts/exchange/test/transactions_unit_tests.ts b/contracts/exchange/test/transactions_unit_tests.ts index 6e198c52b6..fab0ee0851 100644 --- a/contracts/exchange/test/transactions_unit_tests.ts +++ b/contracts/exchange/test/transactions_unit_tests.ts @@ -357,28 +357,6 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef }); return expect(tx).to.revertWith(expectedError); }); - // FIXME - This should be unskipped when the contracts have been updated to fix this problem. - it.skip('should revert if reentrancy occurs in the middle of an executeTransaction call and msg.sender == signer for both calls', async () => { - const validSignature = randomSignature(); - const transaction1 = await generateZeroExTransactionAsync({ signerAddress: accounts[0] }); - const transactionHash1 = transactionHashUtils.getTransactionHashHex(transaction1); - const transaction2 = await generateZeroExTransactionAsync({ - signerAddress: accounts[0], - callData: getExecuteTransactionCallData(transaction1, validSignature), - returnData: '0xdeadbeef', - }); - const transactionHash2 = transactionHashUtils.getTransactionHashHex(transaction2); - const abiEncoder = AbiEncoder.createMethod('TransactionError', ['uint8', 'bytes32']); - const errorData = abiEncoder.encode([ - ExchangeRevertErrors.TransactionErrorCode.NoReentrancy, - transactionHash1, - ]); - const expectedError = new ExchangeRevertErrors.TransactionExecutionError(transactionHash2, errorData); - const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction2, validSignature, { - from: accounts[0], - }); - return expect(tx).to.revertWith(expectedError); - }); it('should revert if reentrancy occurs in the middle of an executeTransaction call and msg.sender != signer for both calls', async () => { const validSignature = randomSignature(); const transaction1 = await generateZeroExTransactionAsync({ signerAddress: accounts[0] }); @@ -389,11 +367,10 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef returnData: '0xdeadbeef', }); const transactionHash2 = transactionHashUtils.getTransactionHashHex(transaction2); - const abiEncoder = AbiEncoder.createMethod('TransactionError', ['uint8', 'bytes32']); - const errorData = abiEncoder.encode([ - ExchangeRevertErrors.TransactionErrorCode.NoReentrancy, + const errorData = new ExchangeRevertErrors.TransactionInvalidContextError( transactionHash1, - ]); + accounts[0], + ).encode(); const expectedError = new ExchangeRevertErrors.TransactionExecutionError(transactionHash2, errorData); const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction2, validSignature, { from: accounts[1], // Different then the signing addresses @@ -410,33 +387,10 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef returnData: '0xdeadbeef', }); const transactionHash2 = transactionHashUtils.getTransactionHashHex(transaction2); - const abiEncoder = AbiEncoder.createMethod('TransactionError', ['uint8', 'bytes32']); - const errorData = abiEncoder.encode([ - ExchangeRevertErrors.TransactionErrorCode.NoReentrancy, + const errorData = new ExchangeRevertErrors.TransactionInvalidContextError( transactionHash1, - ]); - const expectedError = new ExchangeRevertErrors.TransactionExecutionError(transactionHash2, errorData); - const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction2, validSignature, { - from: accounts[1], // Different then the signing addresses - }); - return expect(tx).to.revertWith(expectedError); - }); - // FIXME - This should be unskipped when the contracts have been updated to fix this problem. - it.skip('should revert if reentrancy occurs in the middle of an executeTransaction call and msg.sender == signer and then msg.sender != sender', async () => { - const validSignature = randomSignature(); - const transaction1 = await generateZeroExTransactionAsync({ signerAddress: accounts[0] }); - const transactionHash1 = transactionHashUtils.getTransactionHashHex(transaction1); - const transaction2 = await generateZeroExTransactionAsync({ - signerAddress: accounts[0], - callData: getExecuteTransactionCallData(transaction1, validSignature), - returnData: '0xdeadbeef', - }); - const transactionHash2 = transactionHashUtils.getTransactionHashHex(transaction2); - const abiEncoder = AbiEncoder.createMethod('TransactionError', ['uint8', 'bytes32']); - const errorData = abiEncoder.encode([ - ExchangeRevertErrors.TransactionErrorCode.NoReentrancy, - transactionHash1, - ]); + accounts[0], + ).encode(); const expectedError = new ExchangeRevertErrors.TransactionExecutionError(transactionHash2, errorData); const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction2, validSignature, { from: accounts[1], // Different then the signing addresses @@ -598,10 +552,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef await transactionsContract.setCurrentContextAddress.awaitTransactionSuccessAsync(accounts[0]); const transaction = await generateZeroExTransactionAsync(); const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); - const expectedError = new ExchangeRevertErrors.TransactionError( - ExchangeRevertErrors.TransactionErrorCode.NoReentrancy, - transactionHash, - ); + const expectedError = new ExchangeRevertErrors.TransactionInvalidContextError(transactionHash, accounts[0]); expect( transactionsContract.assertExecutableTransaction.callAsync(transaction, randomSignature()), ).to.revertWith(expectedError); From 5e51233b4910839e94c63ab9f0d58597d5d3e9c9 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Thu, 22 Aug 2019 17:11:50 -0700 Subject: [PATCH 30/34] Address PR feedback --- .../contracts/src/LibExchangeRichErrors.sol | 2 +- .../contracts/test/TestTransactions.sol | 14 +- contracts/exchange/test/transactions.ts | 2 +- .../exchange/test/transactions_unit_tests.ts | 208 ++++++------------ .../order-utils/src/exchange_revert_errors.ts | 2 +- 5 files changed, 81 insertions(+), 147 deletions(-) diff --git a/contracts/exchange-libs/contracts/src/LibExchangeRichErrors.sol b/contracts/exchange-libs/contracts/src/LibExchangeRichErrors.sol index 04ce429e21..d2caadfd56 100644 --- a/contracts/exchange-libs/contracts/src/LibExchangeRichErrors.sol +++ b/contracts/exchange-libs/contracts/src/LibExchangeRichErrors.sol @@ -131,7 +131,7 @@ library LibExchangeRichErrors { bytes4 internal constant TRANSACTION_EXECUTION_ERROR_SELECTOR = 0x20d11f61; - // bytes4(keccak256("TransactionGasPriceError(bytes32,unt256,uint256)")) + // bytes4(keccak256("TransactionGasPriceError(bytes32,uint256,uint256)")) bytes4 internal constant TRANSACTION_GAS_PRICE_ERROR_SELECTOR = 0xa26dac09; diff --git a/contracts/exchange/contracts/test/TestTransactions.sol b/contracts/exchange/contracts/test/TestTransactions.sol index c0637ddfbb..aa5ffb265d 100644 --- a/contracts/exchange/contracts/test/TestTransactions.sol +++ b/contracts/exchange/contracts/test/TestTransactions.sol @@ -26,7 +26,11 @@ import "../src/Exchange.sol"; contract TestTransactions is Exchange { - event ExecutableCalled(bytes data, bytes returnData); + event ExecutableCalled( + bytes data, + bytes returnData, + address contextAddress + ); constructor () public @@ -39,7 +43,7 @@ contract TestTransactions is currentContextAddress = context; } - function setTransactionHash(bytes32 hash) + function setTransactionExecuted(bytes32 hash) external { transactionsExecuted[hash] = true; @@ -77,7 +81,11 @@ contract TestTransactions is public returns (bytes memory) { - emit ExecutableCalled(data, returnData); + emit ExecutableCalled( + data, + returnData, + currentContextAddress + ); require(shouldSucceed, "EXECUTABLE_FAILED"); if (data.length != 0) { (bool didSucceed, bytes memory callResultData) = address(this).delegatecall(data); // This is a delegatecall to preserve the `msg.sender` field diff --git a/contracts/exchange/test/transactions.ts b/contracts/exchange/test/transactions.ts index 140e2868de..839b3bacb1 100644 --- a/contracts/exchange/test/transactions.ts +++ b/contracts/exchange/test/transactions.ts @@ -43,7 +43,7 @@ import { const artifacts = { ...erc20Artifacts, ...localArtifacts }; // tslint:disable:no-unnecessary-type-assertion -blockchainTests.resets.only('Exchange transactions', env => { +blockchainTests.resets('Exchange transactions', env => { let chainId: number; let senderAddress: string; let owner: string; diff --git a/contracts/exchange/test/transactions_unit_tests.ts b/contracts/exchange/test/transactions_unit_tests.ts index fab0ee0851..394829b0c5 100644 --- a/contracts/exchange/test/transactions_unit_tests.ts +++ b/contracts/exchange/test/transactions_unit_tests.ts @@ -1,15 +1,7 @@ -import { - blockchainTests, - constants, - describe, - expect, - getLatestBlockTimestampAsync, - hexRandom, - LogDecoder, -} from '@0x/contracts-test-utils'; +import { blockchainTests, constants, describe, expect, hexRandom, TransactionHelper } from '@0x/contracts-test-utils'; import { ExchangeRevertErrors, transactionHashUtils } from '@0x/order-utils'; import { EIP712DomainWithDefaultSchema, ZeroExTransaction } from '@0x/types'; -import { AbiEncoder, BigNumber, StringRevertError } from '@0x/utils'; +import { BigNumber, StringRevertError } from '@0x/utils'; import { LogWithDecodedArgs } from 'ethereum-types'; import * as _ from 'lodash'; @@ -19,7 +11,6 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef let transactionsContract: TestTransactionsContract; let accounts: string[]; let domain: EIP712DomainWithDefaultSchema; - let logDecoder: LogDecoder; const randomSignature = () => hexRandom(66); @@ -35,13 +26,15 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef }, }; + const DEADBEEF_RETURN_DATA = '0xdeadbeef'; + const INVALID_SIGNATURE = '0x0000'; + + const transactionHelper = new TransactionHelper(web3Wrapper, artifacts); + before(async () => { // A list of available addresses to use during testing. accounts = await web3Wrapper.getAvailableAddressesAsync(); - // Insantiate a LogDecoder instance - logDecoder = new LogDecoder(web3Wrapper, artifacts); - // Deploy the transaction test contract. transactionsContract = await TestTransactionsContract.deployFrom0xArtifactAsync( artifacts.TestTransactions, @@ -90,9 +83,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef const gasPrice = opts.gasPrice === undefined ? new BigNumber(constants.DEFAULT_GAS_PRICE) : opts.gasPrice; const _domain = opts.domain === undefined ? domain : opts.domain; const expirationTimeSeconds = - opts.expirationTimeSeconds === undefined - ? new BigNumber(await getLatestBlockTimestampAsync()).plus(10) - : opts.expirationTimeSeconds; + opts.expirationTimeSeconds === undefined ? constants.MAX_UINT256 : opts.expirationTimeSeconds; const transaction = { ...EMPTY_ZERO_EX_TRANSACTION, ...opts, @@ -199,42 +190,21 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef // Create a transaction that will succeed when used to call `batchExecuteTransactions()`. const transaction = await generateZeroExTransactionAsync({ signerAddress: accounts[1], - returnData: '0xdeadbeef', + returnData: DEADBEEF_RETURN_DATA, }); const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); const validSignature = randomSignature(); - // Verify that the returndata of the transaction is 0xDEADBEEF - const result = await transactionsContract.batchExecuteTransactions.callAsync( + const [result, receipt] = await transactionHelper.getResultAndReceiptAsync( + transactionsContract.batchExecuteTransactions, [transaction], - [randomSignature()], - { - from: accounts[0], - }, + [validSignature], + { from: accounts[0] }, ); expect(result.length).to.be.eq(1); - - // Create an abiEncoder for bytes. This will be used to decode the result and encode what - // is expected. - const abiEncoder = AbiEncoder.create('bytes'); - - // Ensure that the result contains the abi-encoded bytes "0xdeadbeef" - const encodedDeadbeef = abiEncoder.encode('0xdeadbeef'); - expect( - result[0] === - '0x0000000000000000000000000000000000000000000000000000000000000020'.concat( - encodedDeadbeef.slice(2, encodedDeadbeef.length), - ), - ).to.be.true(); - - // Verify that the logs returned from the call are correct. - const receipt = await logDecoder.getTxWithDecodedLogsAsync( - await transactionsContract.batchExecuteTransactions.sendTransactionAsync( - [transaction], - [validSignature], - ), - ); + const returnData = transactionsContract.executeTransaction.getABIDecodedReturnData(result[0]); + expect(returnData).to.equal(DEADBEEF_RETURN_DATA); // Ensure that the correct number of events were logged. const logs = receipt.logs as Array>; @@ -243,7 +213,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef // Ensure that the correct events were logged. expect(logs[0].event).to.be.eq('ExecutableCalled'); expect(logs[0].args.data).to.be.eq(constants.NULL_BYTES); - expect(logs[0].args.returnData).to.be.eq('0xdeadbeef'); + expect(logs[0].args.returnData).to.be.eq(DEADBEEF_RETURN_DATA); expect(logs[1].event).to.be.eq('TransactionExecution'); expect(logs[1].args.transactionHash).to.eq(transactionHash); }); @@ -252,58 +222,28 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef // Create two transactions that will succeed when used to call `batchExecuteTransactions()`. const transaction1 = await generateZeroExTransactionAsync({ signerAddress: accounts[0], - returnData: '0xdeadbeef', + returnData: DEADBEEF_RETURN_DATA, }); + const returnData2 = '0xbeefdead'; const transaction2 = await generateZeroExTransactionAsync({ signerAddress: accounts[1], - returnData: '0xbeefdead', + returnData: returnData2, }); const transactionHash1 = transactionHashUtils.getTransactionHashHex(transaction1); const transactionHash2 = transactionHashUtils.getTransactionHashHex(transaction2); - // Verify that the returndata of the transaction is 0xDEADBEEF - const result = await transactionsContract.batchExecuteTransactions.callAsync( + const [result, receipt] = await transactionHelper.getResultAndReceiptAsync( + transactionsContract.batchExecuteTransactions, [transaction1, transaction2], [randomSignature(), randomSignature()], - { - from: accounts[0], - }, + { from: accounts[0] }, ); - // Create an abiEncoder for bytes. This will be used to decode the result and encode what - // is expected. - const abiEncoder = AbiEncoder.create('bytes'); - - // Ensure that the result contains the abi-encoded bytes "0xdeadbeef" - const encodedDeadbeef = abiEncoder.encode('0xdeadbeef'); - expect(result.length).to.be.eq(2); - expect( - result[0] === - '0x0000000000000000000000000000000000000000000000000000000000000020'.concat( - encodedDeadbeef.slice(2, encodedDeadbeef.length), - ), - ).to.be.true(); - - // Ensure that the result contains the abi-encoded bytes "0xdeadbeef" - const encodedBeefdead = abiEncoder.encode('0xbeefdead'); expect(result.length).to.be.eq(2); - expect( - result[1] === - '0x0000000000000000000000000000000000000000000000000000000000000020'.concat( - encodedBeefdead.slice(2, encodedBeefdead.length), - ), - ).to.be.true(); - - // Verify that the logs returned from the call are correct. - const receipt = await logDecoder.getTxWithDecodedLogsAsync( - await transactionsContract.batchExecuteTransactions.sendTransactionAsync( - [transaction1, transaction2], - [randomSignature(), randomSignature()], - { - from: accounts[0], - }, - ), + expect(transactionsContract.executeTransaction.getABIDecodedReturnData(result[0])).to.equal( + DEADBEEF_RETURN_DATA, ); + expect(transactionsContract.executeTransaction.getABIDecodedReturnData(result[1])).to.equal(returnData2); // Verify that the correct number of events were logged. const logs = receipt.logs as Array>; @@ -312,7 +252,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef // Ensure that the correct events were logged. expect(logs[0].event).to.be.eq('ExecutableCalled'); expect(logs[0].args.data).to.be.eq(constants.NULL_BYTES); - expect(logs[0].args.returnData).to.be.eq('0xdeadbeef'); + expect(logs[0].args.returnData).to.be.eq(DEADBEEF_RETURN_DATA); expect(logs[1].event).to.be.eq('TransactionExecution'); expect(logs[1].args.transactionHash).to.eq(transactionHash1); expect(logs[2].event).to.be.eq('ExecutableCalled'); @@ -331,9 +271,8 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef ); } it('should revert if the current time is past the expiration time', async () => { - const currentTimestamp = await getLatestBlockTimestampAsync(); const transaction = await generateZeroExTransactionAsync({ - expirationTimeSeconds: new BigNumber(currentTimestamp).minus(10), // Set the expiration time to before the current timestamp + expirationTimeSeconds: constants.ZERO_AMOUNT, }); const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); const expectedError = new ExchangeRevertErrors.TransactionError( @@ -359,40 +298,40 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef }); it('should revert if reentrancy occurs in the middle of an executeTransaction call and msg.sender != signer for both calls', async () => { const validSignature = randomSignature(); - const transaction1 = await generateZeroExTransactionAsync({ signerAddress: accounts[0] }); - const transactionHash1 = transactionHashUtils.getTransactionHashHex(transaction1); - const transaction2 = await generateZeroExTransactionAsync({ + const innerTransaction = await generateZeroExTransactionAsync({ signerAddress: accounts[0] }); + const innerTransactionHash = transactionHashUtils.getTransactionHashHex(innerTransaction); + const outerTransaction = await generateZeroExTransactionAsync({ signerAddress: accounts[0], - callData: getExecuteTransactionCallData(transaction1, validSignature), - returnData: '0xdeadbeef', + callData: getExecuteTransactionCallData(innerTransaction, validSignature), + returnData: DEADBEEF_RETURN_DATA, }); - const transactionHash2 = transactionHashUtils.getTransactionHashHex(transaction2); + const outerTransactionHash = transactionHashUtils.getTransactionHashHex(outerTransaction); const errorData = new ExchangeRevertErrors.TransactionInvalidContextError( - transactionHash1, + innerTransactionHash, accounts[0], ).encode(); - const expectedError = new ExchangeRevertErrors.TransactionExecutionError(transactionHash2, errorData); - const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction2, validSignature, { + const expectedError = new ExchangeRevertErrors.TransactionExecutionError(outerTransactionHash, errorData); + const tx = transactionsContract.executeTransaction.sendTransactionAsync(outerTransaction, validSignature, { from: accounts[1], // Different then the signing addresses }); return expect(tx).to.revertWith(expectedError); }); it('should revert if reentrancy occurs in the middle of an executeTransaction call and msg.sender != signer and then msg.sender == sender', async () => { const validSignature = randomSignature(); - const transaction1 = await generateZeroExTransactionAsync({ signerAddress: accounts[1] }); - const transactionHash1 = transactionHashUtils.getTransactionHashHex(transaction1); - const transaction2 = await generateZeroExTransactionAsync({ + const innerTransaction = await generateZeroExTransactionAsync({ signerAddress: accounts[1] }); + const innerTransactionHash = transactionHashUtils.getTransactionHashHex(innerTransaction); + const outerTransaction = await generateZeroExTransactionAsync({ signerAddress: accounts[0], - callData: getExecuteTransactionCallData(transaction1, validSignature), - returnData: '0xdeadbeef', + callData: getExecuteTransactionCallData(innerTransaction, validSignature), + returnData: DEADBEEF_RETURN_DATA, }); - const transactionHash2 = transactionHashUtils.getTransactionHashHex(transaction2); + const outerTransactionHash = transactionHashUtils.getTransactionHashHex(outerTransaction); const errorData = new ExchangeRevertErrors.TransactionInvalidContextError( - transactionHash1, + innerTransactionHash, accounts[0], ).encode(); - const expectedError = new ExchangeRevertErrors.TransactionExecutionError(transactionHash2, errorData); - const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction2, validSignature, { + const expectedError = new ExchangeRevertErrors.TransactionExecutionError(outerTransactionHash, errorData); + const tx = transactionsContract.executeTransaction.sendTransactionAsync(outerTransaction, validSignature, { from: accounts[1], // Different then the signing addresses }); return expect(tx).to.revertWith(expectedError); @@ -403,7 +342,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); // Use the transaction in execute transaction. await expect( - transactionsContract.executeTransaction.sendTransactionAsync(transaction, validSignature), + transactionsContract.executeTransaction.awaitTransactionSuccessAsync(transaction, validSignature), ).to.be.fulfilled(''); // Use the same transaction to make another call const expectedError = new ExchangeRevertErrors.TransactionError( @@ -415,14 +354,13 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef }); it('should revert if the signer != msg.sender and the signature is not valid', async () => { const transaction = await generateZeroExTransactionAsync({ signerAddress: accounts[1] }); - const signature = '0x0000'; // This is the invalid signature const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); const expectedError = new ExchangeRevertErrors.TransactionSignatureError( transactionHash, accounts[1], - signature, + INVALID_SIGNATURE, ); - const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction, signature, { + const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction, INVALID_SIGNATURE, { from: accounts[0], }); return expect(tx).to.revertWith(expectedError); @@ -467,37 +405,28 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef const validSignature = randomSignature(); // Valid because length != 2 const transaction = await generateZeroExTransactionAsync({ signerAddress: accounts[1], - returnData: '0xdeadbeef', + returnData: DEADBEEF_RETURN_DATA, }); const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); - // Verify that the returndata of the transaction is 0xDEADBEEF - const result = await transactionsContract.executeTransaction.callAsync(transaction, validSignature, { - from: accounts[0], - }); - // Create an abiEncoder for bytes. This will be used to decode the result and encode what - // is expected. - const abiEncoder = AbiEncoder.create('bytes'); - // Ensure that the result contains the abi-encoded bytes "0xdeadbeef" - const encodedDeadbeef = abiEncoder.encode('0xdeadbeef'); - expect( - result === - '0x0000000000000000000000000000000000000000000000000000000000000020'.concat( - encodedDeadbeef.slice(2, encodedDeadbeef.length), - ), - ).to.be.true(); - // Verify that the logs returned from the call are correct. - const receipt = await logDecoder.getTxWithDecodedLogsAsync( - await transactionsContract.executeTransaction.sendTransactionAsync(transaction, validSignature, { - from: accounts[0], - }), + + const [result, receipt] = await transactionHelper.getResultAndReceiptAsync( + transactionsContract.executeTransaction, + transaction, + validSignature, + { from: accounts[0] }, + ); + + expect(transactionsContract.executeTransaction.getABIDecodedReturnData(result)).to.equal( + DEADBEEF_RETURN_DATA, ); + // Ensure that the correct number of events were logged. const logs = receipt.logs as Array>; expect(logs.length).to.be.eq(2); // Ensure that the correct events were logged. expect(logs[0].event).to.be.eq('ExecutableCalled'); expect(logs[0].args.data).to.be.eq(constants.NULL_BYTES); - expect(logs[0].args.returnData).to.be.eq('0xdeadbeef'); + expect(logs[0].args.returnData).to.be.eq(DEADBEEF_RETURN_DATA); expect(logs[1].event).to.be.eq('TransactionExecution'); expect(logs[1].args.transactionHash).to.eq(transactionHash); }); @@ -505,9 +434,8 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef blockchainTests.resets('assertExecutableTransaction', () => { it('should revert if the transaction is expired', async () => { - const currentTimestamp = await getLatestBlockTimestampAsync(); const transaction = await generateZeroExTransactionAsync({ - expirationTimeSeconds: new BigNumber(currentTimestamp).minus(10), + expirationTimeSeconds: constants.ZERO_AMOUNT, }); const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); const expectedError = new ExchangeRevertErrors.TransactionError( @@ -560,7 +488,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef it('should revert if the transaction has already been executed', async () => { const transaction = await generateZeroExTransactionAsync(); const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); - await transactionsContract.setTransactionHash.awaitTransactionSuccessAsync(transactionHash); + await transactionsContract.setTransactionExecuted.awaitTransactionSuccessAsync(transactionHash); const expectedError = new ExchangeRevertErrors.TransactionError( ExchangeRevertErrors.TransactionErrorCode.AlreadyExecuted, transactionHash, @@ -572,23 +500,21 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef it('should revert if signer != msg.sender and the signature is invalid', async () => { const transaction = await generateZeroExTransactionAsync({ signerAddress: accounts[0] }); const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); - const invalidSignature = '0x0000'; const expectedError = new ExchangeRevertErrors.TransactionSignatureError( transactionHash, accounts[0], - invalidSignature, + INVALID_SIGNATURE, ); expect( - transactionsContract.assertExecutableTransaction.callAsync(transaction, invalidSignature, { + transactionsContract.assertExecutableTransaction.callAsync(transaction, INVALID_SIGNATURE, { from: accounts[1], }), ).to.revertWith(expectedError); }); it('should be successful if signer == msg.sender and the signature is invalid', async () => { const transaction = await generateZeroExTransactionAsync({ signerAddress: accounts[0] }); - const invalidSignature = '0x0000'; expect( - transactionsContract.assertExecutableTransaction.callAsync(transaction, invalidSignature, { + transactionsContract.assertExecutableTransaction.callAsync(transaction, INVALID_SIGNATURE, { from: accounts[0], }), ).to.be.fulfilled(''); @@ -613,7 +539,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef it('should return the sender address when there is a saved context address', async () => { // Set the current context address to the taker address - await transactionsContract.setCurrentContextAddress.sendTransactionAsync(accounts[1]); + await transactionsContract.setCurrentContextAddress.awaitTransactionSuccessAsync(accounts[1]); // Ensure that the queried current context address is the same as the address that was set. const currentContextAddress = await transactionsContract.getCurrentContextAddress.callAsync({ diff --git a/packages/order-utils/src/exchange_revert_errors.ts b/packages/order-utils/src/exchange_revert_errors.ts index dacb4a7446..8351007acb 100644 --- a/packages/order-utils/src/exchange_revert_errors.ts +++ b/packages/order-utils/src/exchange_revert_errors.ts @@ -135,7 +135,7 @@ export class FillError extends RevertError { } export class OrderEpochError extends RevertError { - constructor(maker?: string, sender?: string, currentEpoch?: BigNumber | number | string) { + constructor(maker?: string, sender?: string, currentEpoch?: BigNumber) { super('OrderEpochError', 'OrderEpochError(address maker, address sender, uint256 currentEpoch)', { maker, sender, From 76c0708cf241aa5e7de91fc62fd9a858abaa8618 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Fri, 23 Aug 2019 08:51:53 -0700 Subject: [PATCH 31/34] Fix config.yml --- .circleci/config.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index fb98ccdffd..6e1a4d05d8 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -110,7 +110,7 @@ jobs: - restore_cache: keys: - repo-{{ .Environment.CIRCLE_SHA1 }} - - run: yarn wsrun --stages test:circleci @0x/contracts-multisig @0x/contracts-utils @0x/contracts-exchange-libs @0x/contracts-erc20 @0x/contracts-erc721 @0x/contracts-erc1155 @0x/contracts-asset-proxy @0x/contracts-exchange-forwarder @0x/contracts-dev-utils @0x/contracts-staking + - run: yarn wsrun test:circleci @0x/contracts-multisig @0x/contracts-utils @0x/contracts-exchange-libs @0x/contracts-erc20 @0x/contracts-erc721 @0x/contracts-erc1155 @0x/contracts-asset-proxy @0x/contracts-exchange-forwarder @0x/contracts-dev-utils @0x/contracts-staking # TODO(dorothy-zbornak): Re-enable after updating this package for 3.0. # - run: yarn wsrun test:circleci @0x/contracts-extensions # TODO(abandeali): Re-enable after this package is complete. From 8f8c16bd0e48b32ae4fac708b7e83be2623bf5fa Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Fri, 23 Aug 2019 09:25:09 -0700 Subject: [PATCH 32/34] Add more recursion tests --- .../exchange/test/transactions_unit_tests.ts | 133 +++++++++++++++++- 1 file changed, 129 insertions(+), 4 deletions(-) diff --git a/contracts/exchange/test/transactions_unit_tests.ts b/contracts/exchange/test/transactions_unit_tests.ts index 394829b0c5..cf8bb91d13 100644 --- a/contracts/exchange/test/transactions_unit_tests.ts +++ b/contracts/exchange/test/transactions_unit_tests.ts @@ -213,6 +213,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef // Ensure that the correct events were logged. expect(logs[0].event).to.be.eq('ExecutableCalled'); expect(logs[0].args.data).to.be.eq(constants.NULL_BYTES); + expect(logs[0].args.contextAddress).to.be.eq(accounts[1]); expect(logs[0].args.returnData).to.be.eq(DEADBEEF_RETURN_DATA); expect(logs[1].event).to.be.eq('TransactionExecution'); expect(logs[1].args.transactionHash).to.eq(transactionHash); @@ -253,14 +254,78 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef expect(logs[0].event).to.be.eq('ExecutableCalled'); expect(logs[0].args.data).to.be.eq(constants.NULL_BYTES); expect(logs[0].args.returnData).to.be.eq(DEADBEEF_RETURN_DATA); + expect(logs[0].args.contextAddress).to.be.eq(constants.NULL_ADDRESS); expect(logs[1].event).to.be.eq('TransactionExecution'); expect(logs[1].args.transactionHash).to.eq(transactionHash1); expect(logs[2].event).to.be.eq('ExecutableCalled'); expect(logs[2].args.data).to.be.eq(constants.NULL_BYTES); expect(logs[2].args.returnData).to.be.eq('0xbeefdead'); + expect(logs[2].args.contextAddress).to.be.eq(accounts[1]); expect(logs[3].event).to.be.eq('TransactionExecution'); expect(logs[3].args.transactionHash).to.eq(transactionHash2); }); + it('should not allow recursion if currentContextAddress is already set', async () => { + const innerTransaction1 = await generateZeroExTransactionAsync({ signerAddress: accounts[0] }); + const innerTransaction2 = await generateZeroExTransactionAsync({ signerAddress: accounts[1] }); + const innerBatchExecuteTransaction = await generateZeroExTransactionAsync({ + signerAddress: accounts[2], + callData: transactionsContract.batchExecuteTransactions.getABIEncodedTransactionData( + [innerTransaction1, innerTransaction2], + [randomSignature(), randomSignature()], + ), + }); + const outerExecuteTransaction = await generateZeroExTransactionAsync({ + signerAddress: accounts[1], + callData: transactionsContract.executeTransaction.getABIEncodedTransactionData( + innerBatchExecuteTransaction, + randomSignature(), + ), + }); + const innerBatchExecuteTransactionHash = transactionHashUtils.getTransactionHashHex( + innerBatchExecuteTransaction, + ); + const innerExpectedError = new ExchangeRevertErrors.TransactionInvalidContextError( + innerBatchExecuteTransactionHash, + accounts[1], + ); + const outerExecuteTransactionHash = transactionHashUtils.getTransactionHashHex(outerExecuteTransaction); + const outerExpectedError = new ExchangeRevertErrors.TransactionExecutionError( + outerExecuteTransactionHash, + innerExpectedError.encode(), + ); + const tx = transactionsContract.batchExecuteTransactions.sendTransactionAsync( + [outerExecuteTransaction], + [randomSignature()], + { from: accounts[2] }, + ); + return expect(tx).to.revertWith(outerExpectedError); + }); + it('should allow recursion as long as currentContextAddress is not set', async () => { + const innerTransaction1 = await generateZeroExTransactionAsync({ signerAddress: accounts[0] }); + const innerTransaction2 = await generateZeroExTransactionAsync({ signerAddress: accounts[1] }); + // From this point on, all transactions and calls will have the same sender, which does not change currentContextAddress when called + const innerBatchExecuteTransaction = await generateZeroExTransactionAsync({ + signerAddress: accounts[2], + callData: transactionsContract.batchExecuteTransactions.getABIEncodedTransactionData( + [innerTransaction1, innerTransaction2], + [randomSignature(), randomSignature()], + ), + }); + const outerExecuteTransaction = await generateZeroExTransactionAsync({ + signerAddress: accounts[2], + callData: transactionsContract.executeTransaction.getABIEncodedTransactionData( + innerBatchExecuteTransaction, + randomSignature(), + ), + }); + return expect( + transactionsContract.batchExecuteTransactions.awaitTransactionSuccessAsync( + [outerExecuteTransaction], + [randomSignature()], + { from: accounts[2] }, + ), + ).to.be.fulfilled(''); + }); }); describe('executeTransaction', () => { @@ -316,7 +381,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef }); return expect(tx).to.revertWith(expectedError); }); - it('should revert if reentrancy occurs in the middle of an executeTransaction call and msg.sender != signer and then msg.sender == sender', async () => { + it('should revert if reentrancy occurs in the middle of an executeTransaction call and msg.sender != signer and then msg.sender == signer', async () => { const validSignature = randomSignature(); const innerTransaction = await generateZeroExTransactionAsync({ signerAddress: accounts[1] }); const innerTransactionHash = transactionHashUtils.getTransactionHashHex(innerTransaction); @@ -336,6 +401,34 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef }); return expect(tx).to.revertWith(expectedError); }); + it('should allow reentrancy in the middle of an executeTransaction call if msg.sender == signer for both calls', async () => { + const validSignature = randomSignature(); + const innerTransaction = await generateZeroExTransactionAsync({ signerAddress: accounts[0] }); + const outerTransaction = await generateZeroExTransactionAsync({ + signerAddress: accounts[0], + callData: getExecuteTransactionCallData(innerTransaction, validSignature), + returnData: DEADBEEF_RETURN_DATA, + }); + return expect( + transactionsContract.executeTransaction.sendTransactionAsync(outerTransaction, validSignature, { + from: accounts[0], + }), + ).to.be.fulfilled(''); + }); + it('should allow reentrancy in the middle of an executeTransaction call if msg.sender == signer and then msg.sender != signer', async () => { + const validSignature = randomSignature(); + const innerTransaction = await generateZeroExTransactionAsync({ signerAddress: accounts[1] }); + const outerTransaction = await generateZeroExTransactionAsync({ + signerAddress: accounts[0], + callData: getExecuteTransactionCallData(innerTransaction, validSignature), + returnData: DEADBEEF_RETURN_DATA, + }); + return expect( + transactionsContract.executeTransaction.sendTransactionAsync(outerTransaction, validSignature, { + from: accounts[0], + }), + ).to.be.fulfilled(''); + }); it('should revert if the transaction has been executed previously', async () => { const validSignature = randomSignature(); const transaction = await generateZeroExTransactionAsync(); @@ -400,7 +493,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef }); return expect(tx).to.revertWith(expectedError); }); - it('should succeed with the correct return hash and event emitted', async () => { + it('should succeed with the correct return hash and event emitted when msg.sender != signer', async () => { // This calldata is encoded to succeed when it hits the executable function. const validSignature = randomSignature(); // Valid because length != 2 const transaction = await generateZeroExTransactionAsync({ @@ -427,6 +520,38 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef expect(logs[0].event).to.be.eq('ExecutableCalled'); expect(logs[0].args.data).to.be.eq(constants.NULL_BYTES); expect(logs[0].args.returnData).to.be.eq(DEADBEEF_RETURN_DATA); + expect(logs[0].args.contextAddress).to.be.eq(accounts[1]); + expect(logs[1].event).to.be.eq('TransactionExecution'); + expect(logs[1].args.transactionHash).to.eq(transactionHash); + }); + it('should succeed with the correct return hash and event emitted when msg.sender == signer', async () => { + // This calldata is encoded to succeed when it hits the executable function. + const validSignature = randomSignature(); // Valid because length != 2 + const transaction = await generateZeroExTransactionAsync({ + signerAddress: accounts[0], + returnData: DEADBEEF_RETURN_DATA, + }); + const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); + + const [result, receipt] = await transactionHelper.getResultAndReceiptAsync( + transactionsContract.executeTransaction, + transaction, + validSignature, + { from: accounts[0] }, + ); + + expect(transactionsContract.executeTransaction.getABIDecodedReturnData(result)).to.equal( + DEADBEEF_RETURN_DATA, + ); + + // Ensure that the correct number of events were logged. + const logs = receipt.logs as Array>; + expect(logs.length).to.be.eq(2); + // Ensure that the correct events were logged. + expect(logs[0].event).to.be.eq('ExecutableCalled'); + expect(logs[0].args.data).to.be.eq(constants.NULL_BYTES); + expect(logs[0].args.returnData).to.be.eq(DEADBEEF_RETURN_DATA); + expect(logs[0].args.contextAddress).to.be.eq(constants.NULL_ADDRESS); expect(logs[1].event).to.be.eq('TransactionExecution'); expect(logs[1].args.transactionHash).to.eq(transactionHash); }); @@ -513,7 +638,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef }); it('should be successful if signer == msg.sender and the signature is invalid', async () => { const transaction = await generateZeroExTransactionAsync({ signerAddress: accounts[0] }); - expect( + return expect( transactionsContract.assertExecutableTransaction.callAsync(transaction, INVALID_SIGNATURE, { from: accounts[0], }), @@ -521,7 +646,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef }); it('should be successful if not expired, the gasPrice is correct, the tx has not been executed, currentContextAddress has not been set, signer != msg.sender, and the signature is valid', async () => { const transaction = await generateZeroExTransactionAsync({ signerAddress: accounts[0] }); - expect( + return expect( transactionsContract.assertExecutableTransaction.callAsync(transaction, randomSignature(), { from: accounts[1], }), From 830d6f726e395692032cc6b541f3382de0484d7a Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Fri, 23 Aug 2019 09:33:05 -0700 Subject: [PATCH 33/34] Use default gasPrice in Forwarder tests --- contracts/exchange-forwarder/test/forwarder.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/contracts/exchange-forwarder/test/forwarder.ts b/contracts/exchange-forwarder/test/forwarder.ts index 2463123530..d008704f2b 100644 --- a/contracts/exchange-forwarder/test/forwarder.ts +++ b/contracts/exchange-forwarder/test/forwarder.ts @@ -45,7 +45,7 @@ blockchainTests.only(ContractName.Forwarder, env => { let tx: TransactionReceiptWithDecodedLogs; let erc721MakerAssetIds: BigNumber[]; - let gasPrice: BigNumber; + const gasPrice = new BigNumber(constants.DEFAULT_GAS_PRICE); before(async () => { await env.blockchainLifecycle.startAsync(); @@ -61,10 +61,6 @@ blockchainTests.only(ContractName.Forwarder, env => { forwarderFeeRecipientAddress, ] = accounts); - const txHash = await env.web3Wrapper.sendTransactionAsync({ from: accounts[0], to: accounts[0], value: 0 }); - const transaction = await env.web3Wrapper.getTransactionByHashAsync(txHash); - gasPrice = new BigNumber(transaction.gasPrice); - const erc721Wrapper = new ERC721Wrapper(env.provider, usedAddresses, owner); erc20Wrapper = new ERC20Wrapper(env.provider, usedAddresses, owner); From 798fb183a59063c88f25dd2a7bf058722916d177 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Fri, 23 Aug 2019 15:14:04 -0700 Subject: [PATCH 34/34] Address remaining PR feedback --- .../exchange-forwarder/test/forwarder.ts | 2 +- .../exchange/test/transactions_unit_tests.ts | 92 +++++++++++++------ 2 files changed, 66 insertions(+), 28 deletions(-) diff --git a/contracts/exchange-forwarder/test/forwarder.ts b/contracts/exchange-forwarder/test/forwarder.ts index d008704f2b..5261b309b4 100644 --- a/contracts/exchange-forwarder/test/forwarder.ts +++ b/contracts/exchange-forwarder/test/forwarder.ts @@ -20,7 +20,7 @@ import { artifacts, ForwarderContract, ForwarderTestFactory, ForwarderWrapper } const DECIMALS_DEFAULT = 18; -blockchainTests.only(ContractName.Forwarder, env => { +blockchainTests(ContractName.Forwarder, env => { let chainId: number; let makerAddress: string; let owner: string; diff --git a/contracts/exchange/test/transactions_unit_tests.ts b/contracts/exchange/test/transactions_unit_tests.ts index cf8bb91d13..bcaf3ca0d5 100644 --- a/contracts/exchange/test/transactions_unit_tests.ts +++ b/contracts/exchange/test/transactions_unit_tests.ts @@ -109,7 +109,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef ); // Call the `batchExecuteTransactions()` function and ensure that it reverts with the expected revert error. - const tx = transactionsContract.batchExecuteTransactions.sendTransactionAsync( + const tx = transactionsContract.batchExecuteTransactions.awaitTransactionSuccessAsync( [transaction], [randomSignature()], ); @@ -132,7 +132,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef ); // Call the `batchExecuteTransactions()` function and ensure that it reverts with the expected revert error. - const tx = transactionsContract.batchExecuteTransactions.sendTransactionAsync( + const tx = transactionsContract.batchExecuteTransactions.awaitTransactionSuccessAsync( [transaction1, transaction2], [randomSignature(), randomSignature()], ); @@ -155,7 +155,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef ); // Call the `batchExecuteTransactions()` function and ensure that it reverts with the expected revert error. - const tx = transactionsContract.batchExecuteTransactions.sendTransactionAsync( + const tx = transactionsContract.batchExecuteTransactions.awaitTransactionSuccessAsync( [transaction1, transaction2], [randomSignature(), randomSignature()], ); @@ -176,7 +176,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef ExchangeRevertErrors.TransactionErrorCode.AlreadyExecuted, transactionHash2, ); - const tx = transactionsContract.batchExecuteTransactions.sendTransactionAsync( + const tx = transactionsContract.batchExecuteTransactions.awaitTransactionSuccessAsync( [transaction1, transaction2], [randomSignature(), randomSignature()], { @@ -293,7 +293,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef outerExecuteTransactionHash, innerExpectedError.encode(), ); - const tx = transactionsContract.batchExecuteTransactions.sendTransactionAsync( + const tx = transactionsContract.batchExecuteTransactions.awaitTransactionSuccessAsync( [outerExecuteTransaction], [randomSignature()], { from: accounts[2] }, @@ -344,7 +344,10 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef ExchangeRevertErrors.TransactionErrorCode.Expired, transactionHash, ); - const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction, randomSignature()); + const tx = transactionsContract.executeTransaction.awaitTransactionSuccessAsync( + transaction, + randomSignature(), + ); return expect(tx).to.revertWith(expectedError); }); it('should revert if the transaction is submitted with a gasPrice that does not equal the required gasPrice', async () => { @@ -356,9 +359,13 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef actualGasPrice, transaction.gasPrice, ); - const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction, randomSignature(), { - gasPrice: actualGasPrice, - }); + const tx = transactionsContract.executeTransaction.awaitTransactionSuccessAsync( + transaction, + randomSignature(), + { + gasPrice: actualGasPrice, + }, + ); return expect(tx).to.revertWith(expectedError); }); it('should revert if reentrancy occurs in the middle of an executeTransaction call and msg.sender != signer for both calls', async () => { @@ -376,9 +383,13 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef accounts[0], ).encode(); const expectedError = new ExchangeRevertErrors.TransactionExecutionError(outerTransactionHash, errorData); - const tx = transactionsContract.executeTransaction.sendTransactionAsync(outerTransaction, validSignature, { - from: accounts[1], // Different then the signing addresses - }); + const tx = transactionsContract.executeTransaction.awaitTransactionSuccessAsync( + outerTransaction, + validSignature, + { + from: accounts[1], // Different then the signing addresses + }, + ); return expect(tx).to.revertWith(expectedError); }); it('should revert if reentrancy occurs in the middle of an executeTransaction call and msg.sender != signer and then msg.sender == signer', async () => { @@ -396,9 +407,13 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef accounts[0], ).encode(); const expectedError = new ExchangeRevertErrors.TransactionExecutionError(outerTransactionHash, errorData); - const tx = transactionsContract.executeTransaction.sendTransactionAsync(outerTransaction, validSignature, { - from: accounts[1], // Different then the signing addresses - }); + const tx = transactionsContract.executeTransaction.awaitTransactionSuccessAsync( + outerTransaction, + validSignature, + { + from: accounts[1], // Different then the signing addresses + }, + ); return expect(tx).to.revertWith(expectedError); }); it('should allow reentrancy in the middle of an executeTransaction call if msg.sender == signer for both calls', async () => { @@ -410,7 +425,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef returnData: DEADBEEF_RETURN_DATA, }); return expect( - transactionsContract.executeTransaction.sendTransactionAsync(outerTransaction, validSignature, { + transactionsContract.executeTransaction.awaitTransactionSuccessAsync(outerTransaction, validSignature, { from: accounts[0], }), ).to.be.fulfilled(''); @@ -424,7 +439,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef returnData: DEADBEEF_RETURN_DATA, }); return expect( - transactionsContract.executeTransaction.sendTransactionAsync(outerTransaction, validSignature, { + transactionsContract.executeTransaction.awaitTransactionSuccessAsync(outerTransaction, validSignature, { from: accounts[0], }), ).to.be.fulfilled(''); @@ -442,7 +457,10 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef ExchangeRevertErrors.TransactionErrorCode.AlreadyExecuted, transactionHash, ); - const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction, validSignature); + const tx = transactionsContract.executeTransaction.awaitTransactionSuccessAsync( + transaction, + validSignature, + ); return expect(tx).to.revertWith(expectedError); }); it('should revert if the signer != msg.sender and the signature is not valid', async () => { @@ -453,9 +471,13 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef accounts[1], INVALID_SIGNATURE, ); - const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction, INVALID_SIGNATURE, { - from: accounts[0], - }); + const tx = transactionsContract.executeTransaction.awaitTransactionSuccessAsync( + transaction, + INVALID_SIGNATURE, + { + from: accounts[0], + }, + ); return expect(tx).to.revertWith(expectedError); }); it('should revert if the signer == msg.sender but the delegatecall fails', async () => { @@ -470,9 +492,13 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef transactionHash, executableError.encode(), ); - const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction, randomSignature(), { - from: accounts[1], - }); + const tx = transactionsContract.executeTransaction.awaitTransactionSuccessAsync( + transaction, + randomSignature(), + { + from: accounts[1], + }, + ); return expect(tx).to.revertWith(expectedError); }); it('should revert if the signer != msg.sender and the signature is valid but the delegatecall fails', async () => { @@ -488,9 +514,13 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef transactionHash, executableError.encode(), ); - const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction, validSignature, { - from: accounts[0], - }); + const tx = transactionsContract.executeTransaction.awaitTransactionSuccessAsync( + transaction, + validSignature, + { + from: accounts[0], + }, + ); return expect(tx).to.revertWith(expectedError); }); it('should succeed with the correct return hash and event emitted when msg.sender != signer', async () => { @@ -644,6 +674,14 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef }), ).to.be.fulfilled(''); }); + it('should be successful if signer == msg.sender and the signature is valid', async () => { + const transaction = await generateZeroExTransactionAsync({ signerAddress: accounts[0] }); + return expect( + transactionsContract.assertExecutableTransaction.callAsync(transaction, randomSignature(), { + from: accounts[0], + }), + ).to.be.fulfilled(''); + }); it('should be successful if not expired, the gasPrice is correct, the tx has not been executed, currentContextAddress has not been set, signer != msg.sender, and the signature is valid', async () => { const transaction = await generateZeroExTransactionAsync({ signerAddress: accounts[0] }); return expect(