diff --git a/contracts/exchange/contracts/src/MixinExchangeRichErrors.sol b/contracts/exchange/contracts/src/MixinExchangeRichErrors.sol index 23c77a8985..48a9c4d419 100644 --- a/contracts/exchange/contracts/src/MixinExchangeRichErrors.sol +++ b/contracts/exchange/contracts/src/MixinExchangeRichErrors.sol @@ -216,6 +216,23 @@ contract MixinExchangeRichErrors is ); } + function TransactionSignatureError( + bytes32 transactionHash, + address signer, + bytes memory signature + ) + internal + pure + returns (bytes memory) + { + return abi.encodeWithSelector( + TRANSACTION_SIGNATURE_ERROR_SELECTOR, + transactionHash, + signer, + signature + ); + } + function TransactionExecutionError( bytes32 transactionHash, bytes memory errorData diff --git a/contracts/exchange/contracts/src/MixinSignatureValidator.sol b/contracts/exchange/contracts/src/MixinSignatureValidator.sol index 01b97a608d..fbc48823e1 100644 --- a/contracts/exchange/contracts/src/MixinSignatureValidator.sol +++ b/contracts/exchange/contracts/src/MixinSignatureValidator.sol @@ -272,24 +272,23 @@ contract MixinSignatureValidator is assembly { mstore(signature, sub(signatureLength, 1)) } - // Encode the call data. bytes memory callData = abi.encodeWithSelector( IWallet(walletAddress).isValidSignature.selector, hash, signature ); + // Restore the full signature. + assembly { + mstore(signature, signatureLength) + } + // Static call the verification function. (bool didSucceed, bytes memory returnData) = walletAddress.staticcall(callData); // Return data should be a single bool. if (didSucceed && returnData.length == 32) return returnData.readUint256(0) != 0; - // Static call to verifier failed. - // Restore the full signature. - assembly { - mstore(signature, signatureLength) - } rrevert(SignatureError( SignatureErrorCodes.WALLET_ERROR, hash, @@ -323,7 +322,6 @@ contract MixinSignatureValidator is assembly { mstore(signature, sub(signatureLength, 21)) } - // Encode the call data. bytes memory callData = abi.encodeWithSelector( IValidator(validatorAddress).isValidSignature.selector, @@ -331,17 +329,16 @@ contract MixinSignatureValidator is signerAddress, signature ); + // Restore the full signature. + assembly { + mstore(signature, signatureLength) + } // Static call the verification function. (bool didSucceed, bytes memory returnData) = validatorAddress.staticcall(callData); // Return data should be a single bool. if (didSucceed && returnData.length == 32) return returnData.readUint256(0) != 0; - // Static call to verifier failed. - // Restore the full signature. - assembly { - mstore(signature, signatureLength) - } rrevert(SignatureError( SignatureErrorCodes.VALIDATOR_ERROR, hash, diff --git a/contracts/exchange/contracts/src/MixinTransactions.sol b/contracts/exchange/contracts/src/MixinTransactions.sol index dab9ba5506..460cde5563 100644 --- a/contracts/exchange/contracts/src/MixinTransactions.sol +++ b/contracts/exchange/contracts/src/MixinTransactions.sol @@ -74,9 +74,10 @@ contract MixinTransactions is transactionHash, signerAddress, signature)) { - rrevert(TransactionError( - TransactionErrorCodes.BAD_SIGNATURE, - transactionHash + rrevert(TransactionSignatureError( + transactionHash, + signerAddress, + signature )); } diff --git a/contracts/exchange/contracts/src/mixins/MExchangeRichErrors.sol b/contracts/exchange/contracts/src/mixins/MExchangeRichErrors.sol index 61cd2c32a0..81f2a6fbae 100644 --- a/contracts/exchange/contracts/src/mixins/MExchangeRichErrors.sol +++ b/contracts/exchange/contracts/src/mixins/MExchangeRichErrors.sol @@ -48,8 +48,7 @@ contract MExchangeRichErrors is enum TransactionErrorCodes { NO_REENTRANCY, - ALREADY_EXECUTED, - BAD_SIGNATURE + ALREADY_EXECUTED } // solhint-disable func-name-mixedcase @@ -189,6 +188,18 @@ contract MExchangeRichErrors is pure returns (bytes memory); + bytes4 internal constant TRANSACTION_SIGNATURE_ERROR_SELECTOR = + bytes4(keccak256("TransactionSignatureError(bytes32,address,bytes)")); + + function TransactionSignatureError( + bytes32 transactionHash, + address signer, + bytes memory signature + ) + internal + pure + returns (bytes memory); + bytes4 internal constant TRANSACTION_EXECUTION_ERROR_SELECTOR = bytes4(keccak256("TransactionExecutionError(bytes32,bytes)")); diff --git a/contracts/exchange/test/transactions.ts b/contracts/exchange/test/transactions.ts index aea1b010d8..f8f25d5ba0 100644 --- a/contracts/exchange/test/transactions.ts +++ b/contracts/exchange/test/transactions.ts @@ -29,6 +29,7 @@ import { } from '@0x/types'; import { BigNumber, providerUtils } from '@0x/utils'; import * as chai from 'chai'; +import * as ethUtil from 'ethereumjs-util'; import * as _ from 'lodash'; import { artifacts, ExchangeContract, ExchangeWrapper, ExchangeWrapperContract, WhitelistContract } from '../src/'; @@ -136,7 +137,7 @@ describe('Exchange transactions', () => { makerTransactionFactory = new TransactionFactory(makerPrivateKey, exchange.address, chainId); takerTransactionFactory = new TransactionFactory(takerPrivateKey, exchange.address, chainId); }); - describe('executeTransaction', () => { + describe.only('executeTransaction', () => { describe('fillOrder', () => { let takerAssetFillAmount: BigNumber; beforeEach(async () => { @@ -153,6 +154,24 @@ describe('Exchange transactions', () => { signedTx = takerTransactionFactory.newSignedTransaction(data); }); + it('should throw if signature is invalid', async () => { + const v = ethUtil.toBuffer(signedTx.signature.slice(0, 4)); + const invalidR = ethUtil.sha3('invalidR'); + const invalidS = ethUtil.sha3('invalidS'); + const signatureType = ethUtil.toBuffer(`0x${signedTx.signature.slice(-2)}`); + const invalidSigBuff = Buffer.concat([v, invalidR, invalidS, signatureType]); + const invalidSigHex = `0x${invalidSigBuff.toString('hex')}`; + signedTx.signature = invalidSigHex; + const transactionHashHex = transactionHashUtils.getTransactionHashHex(signedTx); + const expectedError = new ExchangeRevertErrors.TransactionSignatureError( + transactionHashHex, + signedTx.signerAddress, + signedTx.signature, + ); + const tx = exchangeWrapper.executeTransactionAsync(signedTx, senderAddress); + return expect(tx).to.revertWith(expectedError); + }); + it('should throw if not called by specified sender', async () => { const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); const transactionHashHex = transactionHashUtils.getTransactionHashHex(signedTx); diff --git a/packages/order-utils/src/exchange_revert_errors.ts b/packages/order-utils/src/exchange_revert_errors.ts index 373e29b40c..d9f6777bc4 100644 --- a/packages/order-utils/src/exchange_revert_errors.ts +++ b/packages/order-utils/src/exchange_revert_errors.ts @@ -28,14 +28,13 @@ export enum AssetProxyDispatchErrorCode { export enum TransactionErrorCode { NoReentrancy, AlreadyExecuted, - BadSignature, } export class SignatureError extends RevertError { - constructor(error?: SignatureErrorCode, orderHash?: string, signer?: string, signature?: string) { - super('SignatureError(uint8 error, bytes32 orderHash, address signer, bytes signature)', { + constructor(error?: SignatureErrorCode, hash?: string, signer?: string, signature?: string) { + super('SignatureError(uint8 error, bytes32 hash, address signer, bytes signature)', { error, - orderHash, + hash, signer, signature, }); @@ -120,6 +119,16 @@ export class TransactionError extends RevertError { } } +export class TransactionSignatureError extends RevertError { + constructor(transactionHash?: string, signer?: string, signature?: string) { + super('TransactionSignatureError(bytes32 transactionHash, address signer, bytes signature)', { + transactionHash, + signer, + signature, + }); + } +} + export class TransactionExecutionError extends RevertError { constructor(transactionHash?: string, errorData?: string) { super('TransactionExecutionError(bytes32 transactionHash, bytes errorData)', { transactionHash, errorData }); @@ -145,6 +154,7 @@ const types = [ AssetProxyTransferError, NegativeSpreadError, TransactionError, + TransactionSignatureError, TransactionExecutionError, IncompleteFillError, ];