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], }),