In @0x/order-utils: Add TransactionSignatureError to ExchangeRevertErrors.

In `@0x/contracts-exchange`: Add `TransactionSignatureError`, supplanting `TransactionErrorCodes.BAD_SIGNATURE`, and associated test.

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

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,

contracts/exchange/contracts/src/MixinTransactions.sol

@@ -74,9 +74,10 @@ contract MixinTransactions is
                     transactionHash,
                     signerAddress,
                     signature)) {
-                rrevert(TransactionError(
+                rrevert(TransactionSignatureError(
-                    TransactionErrorCodes.BAD_SIGNATURE,
+                    transactionHash,
-                    transactionHash
+                    signerAddress,
+                    signature
                 ));
             }
 

contracts/exchange/contracts/src/mixins/MExchangeRichErrors.sol

@@ -48,8 +48,7 @@ contract MExchangeRichErrors is
 
     enum TransactionErrorCodes {
         NO_REENTRANCY,
-        ALREADY_EXECUTED,
+        ALREADY_EXECUTED
-        BAD_SIGNATURE
     }
 
     // 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)"));
 

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);

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) {
+    constructor(error?: SignatureErrorCode, hash?: string, signer?: string, signature?: string) {
-        super('SignatureError(uint8 error, bytes32 orderHash, address signer, bytes signature)', {
+        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,
 ];