diff options
Diffstat (limited to 'packages')
5 files changed, 50 insertions, 31 deletions
diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol index 97c1f1541..80120bc74 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol @@ -18,7 +18,6 @@ pragma solidity ^0.4.21; pragma experimental ABIEncoderV2; -pragma experimental "v0.5.0"; import "./mixins/MExchangeCore.sol"; import "./mixins/MSettlement.sol"; @@ -117,13 +116,13 @@ contract MixinExchangeCore is require(isValidSignature(orderHash, order.makerAddress, signature)); } - // Validate sender + // Validate sender is allowed to fill this order if (order.senderAddress != address(0)) { require(order.senderAddress == msg.sender); } - // Validate transaction signed by taker - address takerAddress = getSignerAddress(); + // Validate taker is allowed to fill this order + address takerAddress = getCurrentContextAddress(); if (order.takerAddress != address(0)) { require(order.takerAddress == takerAddress); } @@ -177,13 +176,13 @@ contract MixinExchangeCore is require(order.makerAssetAmount > 0); require(order.takerAssetAmount > 0); - // Validate sender + // Validate sender is allowed to cancel this order if (order.senderAddress != address(0)) { require(order.senderAddress == msg.sender); } // Validate transaction signed by maker - address makerAddress = getSignerAddress(); + address makerAddress = getCurrentContextAddress(); require(order.makerAddress == makerAddress); if (block.timestamp >= order.expirationTimeSeconds) { diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol index e4fc4767b..9edb1694f 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol @@ -27,10 +27,11 @@ contract MixinTransactions is { // Mapping of transaction hash => executed + // This prevents transactions from being executed more than once. mapping (bytes32 => bool) public transactions; // Address of current transaction signer - address currentSigner; + address public currentContextAddress; /// @dev Executes an exchange method call in the context of signer. /// @param salt Arbitrary number to ensure uniqueness of transaction hash. @@ -45,7 +46,7 @@ contract MixinTransactions is external { // Prevent reentrancy - require(currentSigner == address(0)); + require(currentContextAddress == address(0)); // Calculate transaction hash bytes32 transactionHash = keccak256( @@ -63,7 +64,7 @@ contract MixinTransactions is require(isValidSignature(transactionHash, signer, signature)); // Set the current transaction signer - currentSigner = signer; + currentContextAddress = signer; } // Execute transaction @@ -71,15 +72,21 @@ contract MixinTransactions is require(address(this).delegatecall(data)); // Reset current transaction signer - currentSigner = address(0); + // TODO: Check if gas is paid when currentContextAddress is already 0. + currentContextAddress = address(0); } - function getSignerAddress() + /// @dev The current function will be called in the context of this address (either 0x transaction signer or `msg.sender`). + /// If calling a fill function, this address will represent the taker. + /// If calling a cancel function, this address will represent the maker. + /// @return Signer of 0x transaction if entry point is `executeTransaction`. + /// `msg.sender` if entry point is any other function. + function getCurrentContextAddress() internal view returns (address) { - address signerAddress = currentSigner == address(0) ? msg.sender : currentSigner; - return signerAddress; + address contextAddress = currentContextAddress == address(0) ? msg.sender : currentContextAddress; + return contextAddress; } } diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MTransactions.sol b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MTransactions.sol index 31209de02..10bfcb035 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MTransactions.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MTransactions.sol @@ -34,7 +34,12 @@ contract MTransactions is MSignatureValidator { bytes signature) external; - function getSignerAddress() + /// @dev The current function will be called in the context of this address (either 0x transaction signer or `msg.sender`). + /// If calling a fill function, this address will represent the taker. + /// If calling a cancel function, this address will represent the maker. + /// @return Signer of 0x transaction if entry point is `executeTransaction`. + /// `msg.sender` if entry point is any other function. + function getCurrentContextAddress() internal view returns (address); diff --git a/packages/contracts/src/utils/crypto.ts b/packages/contracts/src/utils/crypto.ts index 85a37122a..222cb7cca 100644 --- a/packages/contracts/src/utils/crypto.ts +++ b/packages/contracts/src/utils/crypto.ts @@ -29,8 +29,6 @@ export const crypto = { args[i] = new BN(arg.toString(10), 10); } else if (ethUtil.isValidAddress(arg)) { argTypes.push('address'); - } else if (arg instanceof Buffer) { - argTypes.push('bytes'); } else if (_.isString(arg)) { argTypes.push('string'); } else if (_.isBuffer(arg)) { diff --git a/packages/contracts/test/exchange/transactions.ts b/packages/contracts/test/exchange/transactions.ts index 62bba0ab8..2bc078153 100644 --- a/packages/contracts/test/exchange/transactions.ts +++ b/packages/contracts/test/exchange/transactions.ts @@ -49,6 +49,7 @@ describe('Exchange transactions', () => { let erc20Balances: ERC20BalancesByOwner; let signedOrder: SignedOrder; + let signedTx: SignedTransaction; let order: OrderStruct; let orderFactory: OrderFactory; let makerTransactionFactory: TransactionFactory; @@ -111,33 +112,28 @@ describe('Exchange transactions', () => { describe('executeTransaction', () => { describe('fillOrder', () => { + let takerAssetFillAmount: BigNumber; beforeEach(async () => { erc20Balances = await erc20Wrapper.getBalancesAsync(); signedOrder = orderFactory.newSignedOrder(); order = orderUtils.getOrderStruct(signedOrder); - }); - it('should throw if not called by specified sender', async () => { - const takerAssetFillAmount = signedOrder.takerAssetAmount.div(2); + takerAssetFillAmount = signedOrder.takerAssetAmount.div(2); const data = exchange.fillOrder.getABIEncodedTransactionData( order, takerAssetFillAmount, signedOrder.signature, ); - const signedTx = takerTransactionFactory.newSignedTransaction(data); + signedTx = takerTransactionFactory.newSignedTransaction(data); + }); + + it('should throw if not called by specified sender', async () => { return expect(exchangeWrapper.executeTransactionAsync(signedTx, takerAddress)).to.be.rejectedWith( constants.REVERT, ); }); it('should transfer the correct amounts when signed by taker and called by sender', async () => { - const takerAssetFillAmount = signedOrder.takerAssetAmount.div(2); - const data = exchange.fillOrder.getABIEncodedTransactionData( - order, - takerAssetFillAmount, - signedOrder.signature, - ); - const signedTx = takerTransactionFactory.newSignedTransaction(data); await exchangeWrapper.executeTransactionAsync(signedTx, senderAddress); const newBalances = await erc20Wrapper.getBalancesAsync(); const makerAssetFillAmount = takerAssetFillAmount @@ -171,20 +167,34 @@ describe('Exchange transactions', () => { erc20Balances[feeRecipientAddress][zrxToken.address].add(makerFeePaid.add(takerFeePaid)), ); }); + + it('should throw if the a 0x transaction with the same transactionHash has already been executed', async () => { + await exchangeWrapper.executeTransactionAsync(signedTx, senderAddress); + return expect(exchangeWrapper.executeTransactionAsync(signedTx, senderAddress)).to.be.rejectedWith( + constants.REVERT, + ); + }); + + it('should reset the currentContextAddress', async () => { + await exchangeWrapper.executeTransactionAsync(signedTx, senderAddress); + const currentContextAddress = await exchange.currentContextAddress.callAsync(); + expect(currentContextAddress).to.equal(ZeroEx.NULL_ADDRESS); + }); }); describe('cancelOrder', () => { - it('should throw if not called by specified sender', async () => { + beforeEach(async () => { const data = exchange.cancelOrder.getABIEncodedTransactionData(order); - const signedTx = makerTransactionFactory.newSignedTransaction(data); + signedTx = makerTransactionFactory.newSignedTransaction(data); + }); + + it('should throw if not called by specified sender', async () => { return expect(exchangeWrapper.executeTransactionAsync(signedTx, makerAddress)).to.be.rejectedWith( constants.REVERT, ); }); it('should cancel the order when signed by maker and called by sender', async () => { - const data = exchange.cancelOrder.getABIEncodedTransactionData(order); - const signedTx = makerTransactionFactory.newSignedTransaction(data); await exchangeWrapper.executeTransactionAsync(signedTx, senderAddress); const res = await exchangeWrapper.fillOrderAsync(signedOrder, takerAddress); const newBalances = await erc20Wrapper.getBalancesAsync(); |