Merge pull request #1015 from 0xProject/feature/contracts/removeCallerSigType

Remove SignatureType.Caller

9 files changed, 67 insertions, 187 deletions

diff --git a/packages/contracts/src/2.0.0/examples/Whitelist/Whitelist.sol b/packages/contracts/src/2.0.0/examples/Whitelist/Whitelist.sol
index 60cac26ea..e4e25038c 100644
--- a/packages/contracts/src/2.0.0/examples/Whitelist/Whitelist.sol
+++ b/packages/contracts/src/2.0.0/examples/Whitelist/Whitelist.sol
@@ -37,7 +37,7 @@ contract Whitelist is
     bytes internal TX_ORIGIN_SIGNATURE;
     // solhint-enable var-name-mixedcase
 
-    byte constant internal VALIDATOR_SIGNATURE_BYTE = "\x06";
+    byte constant internal VALIDATOR_SIGNATURE_BYTE = "\x05";
 
     constructor (address _exchange)
         public
diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinSignatureValidator.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinSignatureValidator.sol
index 017da742e..f30adcdb8 100644
--- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinSignatureValidator.sol
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinSignatureValidator.sol
@@ -48,14 +48,16 @@ contract MixinSignatureValidator is
     )
         external
     {
-        require(
-            isValidSignature(
-                hash,
-                signerAddress,
-                signature
-            ),
-            "INVALID_SIGNATURE"
-        );
+        if (signerAddress != msg.sender) {
+            require(
+                isValidSignature(
+                    hash,
+                    signerAddress,
+                    signature
+                ),
+                "INVALID_SIGNATURE"
+            );
+        }
         preSigned[hash][signerAddress] = true;
     }
 
@@ -172,22 +174,6 @@ contract MixinSignatureValidator is
             isValid = signerAddress == recovered;
             return isValid;
 
-        // Implicitly signed by caller.
-        // The signer has initiated the call. In the case of non-contract
-        // accounts it means the transaction itself was signed.
-        // Example: let's say for a particular operation three signatures
-        // A, B and C are required. To submit the transaction, A and B can
-        // give a signature to C, who can then submit the transaction using
-        // `Caller` for his own signature. Or A and C can sign and B can
-        // submit using `Caller`. Having `Caller` allows this flexibility.
-        } else if (signatureType == SignatureType.Caller) {
-            require(
-                signature.length == 0,
-                "LENGTH_0_REQUIRED"
-            );
-            isValid = signerAddress == msg.sender;
-            return isValid;
-
         // Signature verified by wallet contract.
         // If used with an order, the maker of the order is the wallet contract.
         } else if (signatureType == SignatureType.Wallet) {
@@ -225,34 +211,6 @@ contract MixinSignatureValidator is
         } else if (signatureType == SignatureType.PreSigned) {
             isValid = preSigned[hash][signerAddress];
             return isValid;
-
-        // Signature from Trezor hardware wallet.
-        // It differs from web3.eth_sign in the encoding of message length
-        // (Bitcoin varint encoding vs ascii-decimal, the latter is not
-        // self-terminating which leads to ambiguities).
-        // See also:
-        // https://en.bitcoin.it/wiki/Protocol_documentation#Variable_length_integer
-        // https://github.com/trezor/trezor-mcu/blob/master/firmware/ethereum.c#L602
-        // https://github.com/trezor/trezor-mcu/blob/master/firmware/crypto.c#L36
-        } else if (signatureType == SignatureType.Trezor) {
-            require(
-                signature.length == 65,
-                "LENGTH_65_REQUIRED"
-            );
-            v = uint8(signature[0]);
-            r = signature.readBytes32(1);
-            s = signature.readBytes32(33);
-            recovered = ecrecover(
-                keccak256(abi.encodePacked(
-                    "\x19Ethereum Signed Message:\n\x20",
-                    hash
-                )),
-                v,
-                r,
-                s
-            );
-            isValid = signerAddress == recovered;
-            return isValid;
         }
 
         // Anything else is illegal (We do not return false because
diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MSignatureValidator.sol b/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MSignatureValidator.sol
index 75fe9ec46..1fe88b908 100644
--- a/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MSignatureValidator.sol
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MSignatureValidator.sol
@@ -36,12 +36,10 @@ contract MSignatureValidator is
         Invalid,         // 0x01
         EIP712,          // 0x02
         EthSign,         // 0x03
-        Caller,          // 0x04
-        Wallet,          // 0x05
-        Validator,       // 0x06
-        PreSigned,       // 0x07
-        Trezor,          // 0x08
-        NSignatureTypes  // 0x09, number of signature types. Always leave at end.
+        Wallet,          // 0x04
+        Validator,       // 0x05
+        PreSigned,       // 0x06
+        NSignatureTypes  // 0x07, number of signature types. Always leave at end.
     }
 
     /// @dev Verifies signature using logic defined by Wallet contract.
diff --git a/packages/contracts/test/exchange/signature_validator.ts b/packages/contracts/test/exchange/signature_validator.ts
index 5b64d853c..da2febfd8 100644
--- a/packages/contracts/test/exchange/signature_validator.ts
+++ b/packages/contracts/test/exchange/signature_validator.ts
@@ -281,32 +281,6 @@ describe('MixinSignatureValidator', () => {
             expect(isValidSignature).to.be.false();
         });
 
-        it('should return true when SignatureType=Caller and signer is caller', async () => {
-            const signature = ethUtil.toBuffer(`0x${SignatureType.Caller}`);
-            const signatureHex = ethUtil.bufferToHex(signature);
-            const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder);
-            const isValidSignature = await signatureValidator.publicIsValidSignature.callAsync(
-                orderHashHex,
-                signerAddress,
-                signatureHex,
-                { from: signerAddress },
-            );
-            expect(isValidSignature).to.be.true();
-        });
-
-        it('should return false when SignatureType=Caller and signer is not caller', async () => {
-            const signature = ethUtil.toBuffer(`0x${SignatureType.Caller}`);
-            const signatureHex = ethUtil.bufferToHex(signature);
-            const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder);
-            const isValidSignature = await signatureValidator.publicIsValidSignature.callAsync(
-                orderHashHex,
-                signerAddress,
-                signatureHex,
-                { from: notSignerAddress },
-            );
-            expect(isValidSignature).to.be.false();
-        });
-
         it('should return true when SignatureType=Wallet and signature is valid', async () => {
             // Create EIP712 signature
             const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder);
@@ -440,53 +414,6 @@ describe('MixinSignatureValidator', () => {
             expect(isValidSignature).to.be.false();
         });
 
-        it('should return true when SignatureType=Trezor and signature is valid', async () => {
-            // Create Trezor signature
-            const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder);
-            const orderHashWithTrezorPrefixHex = signatureUtils.addSignedMessagePrefix(orderHashHex, SignerType.Trezor);
-            const orderHashWithTrezorPrefixBuffer = ethUtil.toBuffer(orderHashWithTrezorPrefixHex);
-            const ecSignature = ethUtil.ecsign(orderHashWithTrezorPrefixBuffer, signerPrivateKey);
-            // Create 0x signature from Trezor signature
-            const signature = Buffer.concat([
-                ethUtil.toBuffer(ecSignature.v),
-                ecSignature.r,
-                ecSignature.s,
-                ethUtil.toBuffer(`0x${SignatureType.Trezor}`),
-            ]);
-            const signatureHex = ethUtil.bufferToHex(signature);
-            // Validate signature
-            const isValidSignature = await signatureValidator.publicIsValidSignature.callAsync(
-                orderHashHex,
-                signerAddress,
-                signatureHex,
-            );
-            expect(isValidSignature).to.be.true();
-        });
-
-        it('should return false when SignatureType=Trezor and signature is invalid', async () => {
-            // Create Trezor signature
-            const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder);
-            const orderHashWithTrezorPrefixHex = signatureUtils.addSignedMessagePrefix(orderHashHex, SignerType.Trezor);
-            const orderHashWithTrezorPrefixBuffer = ethUtil.toBuffer(orderHashWithTrezorPrefixHex);
-            const ecSignature = ethUtil.ecsign(orderHashWithTrezorPrefixBuffer, signerPrivateKey);
-            // Create 0x signature from Trezor signature
-            const signature = Buffer.concat([
-                ethUtil.toBuffer(ecSignature.v),
-                ecSignature.r,
-                ecSignature.s,
-                ethUtil.toBuffer(`0x${SignatureType.Trezor}`),
-            ]);
-            const signatureHex = ethUtil.bufferToHex(signature);
-            // Validate signature.
-            // This will fail because `signerAddress` signed the message, but we're passing in `notSignerAddress`
-            const isValidSignature = await signatureValidator.publicIsValidSignature.callAsync(
-                orderHashHex,
-                notSignerAddress,
-                signatureHex,
-            );
-            expect(isValidSignature).to.be.false();
-        });
-
         it('should return true when SignatureType=Presigned and signer has presigned hash', async () => {
             // Presign hash
             const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder);
@@ -520,6 +447,42 @@ describe('MixinSignatureValidator', () => {
             );
             expect(isValidSignature).to.be.false();
         });
+
+        it('should return true when message was signed by a Trezor One (firmware version 1.6.2)', async () => {
+            // messageHash translates to 0x2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b
+            const messageHash = ethUtil.bufferToHex(ethUtil.toBuffer('++++++++++++++++++++++++++++++++'));
+            const signer = '0xc28b145f10f0bcf0fc000e778615f8fd73490bad';
+            const v = ethUtil.toBuffer('0x1c');
+            const r = ethUtil.toBuffer('0x7b888b596ccf87f0bacab0dcb483124973f7420f169b4824d7a12534ac1e9832');
+            const s = ethUtil.toBuffer('0x0c8e14f7edc01459e13965f1da56e0c23ed11e2cca932571eee1292178f90424');
+            const trezorSignatureType = ethUtil.toBuffer(`0x${SignatureType.EthSign}`);
+            const signature = Buffer.concat([v, r, s, trezorSignatureType]);
+            const signatureHex = ethUtil.bufferToHex(signature);
+            const isValidSignature = await signatureValidator.publicIsValidSignature.callAsync(
+                messageHash,
+                signer,
+                signatureHex,
+            );
+            expect(isValidSignature).to.be.true();
+        });
+
+        it('should return true when message was signed by a Trezor Model T (firmware version 2.0.7)', async () => {
+            // messageHash translates to 0x2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b
+            const messageHash = ethUtil.bufferToHex(ethUtil.toBuffer('++++++++++++++++++++++++++++++++'));
+            const signer = '0x98ce6d9345e8ffa7d99ee0822272fae9d2c0e895';
+            const v = ethUtil.toBuffer('0x1c');
+            const r = ethUtil.toBuffer('0x423b71062c327f0ec4fe199b8da0f34185e59b4c1cb4cc23df86cac4a601fb3f');
+            const s = ethUtil.toBuffer('0x53810d6591b5348b7ee08ee812c874b0fdfb942c9849d59512c90e295221091f');
+            const trezorSignatureType = ethUtil.toBuffer(`0x${SignatureType.EthSign}`);
+            const signature = Buffer.concat([v, r, s, trezorSignatureType]);
+            const signatureHex = ethUtil.bufferToHex(signature);
+            const isValidSignature = await signatureValidator.publicIsValidSignature.callAsync(
+                messageHash,
+                signer,
+                signatureHex,
+            );
+            expect(isValidSignature).to.be.true();
+        });
     });
 
     describe('setSignatureValidatorApproval', () => {
diff --git a/packages/order-utils/CHANGELOG.json b/packages/order-utils/CHANGELOG.json
index 81782b501..d18bf51ed 100644
--- a/packages/order-utils/CHANGELOG.json
+++ b/packages/order-utils/CHANGELOG.json
@@ -1,5 +1,14 @@
 [
     {
+        "version": "1.0.1-rc.5",
+        "changes": [
+            {
+                "note": "Remove Caller and Trezor SignatureTypes",
+                "pr": 1015
+            }
+        ]
+    },
+    {
         "version": "1.0.1-rc.4",
         "changes": [
             {
diff --git a/packages/order-utils/src/signature_utils.ts b/packages/order-utils/src/signature_utils.ts
index 40bbcef98..c0c9e71a7 100644
--- a/packages/order-utils/src/signature_utils.ts
+++ b/packages/order-utils/src/signature_utils.ts
@@ -53,11 +53,6 @@ export const signatureUtils = {
                 return signatureUtils.isValidECSignature(prefixedMessageHex, ecSignature, signerAddress);
             }
 
-            case SignatureType.Caller:
-                // HACK: We currently do not "validate" the caller signature type.
-                // It can only be validated during Exchange contract execution.
-                throw new Error('Caller signature type cannot be validated off-chain');
-
             case SignatureType.Wallet: {
                 const isValid = await signatureUtils.isValidWalletSignatureAsync(
                     provider,
@@ -82,12 +77,6 @@ export const signatureUtils = {
                 return signatureUtils.isValidPresignedSignatureAsync(provider, data, signerAddress);
             }
 
-            case SignatureType.Trezor: {
-                const prefixedMessageHex = signatureUtils.addSignedMessagePrefix(data, SignerType.Trezor);
-                const ecSignature = signatureUtils.parseECSignature(signature);
-                return signatureUtils.isValidECSignature(prefixedMessageHex, ecSignature, signerAddress);
-            }
-
             default:
                 throw new Error(`Unhandled SignatureType: ${signatureTypeIndexIfExists}`);
         }
@@ -293,10 +282,6 @@ export const signatureUtils = {
                 signatureType = SignatureType.EthSign;
                 break;
             }
-            case SignerType.Trezor: {
-                signatureType = SignatureType.Trezor;
-                break;
-            }
             default:
                 throw new Error(`Unrecognized SignerType: ${signerType}`);
         }
@@ -306,7 +291,7 @@ export const signatureUtils = {
     /**
      * Combines the signature proof and the Signature Type.
      * @param signature The hex encoded signature proof
-     * @param signatureType The signature type, i.e EthSign, Trezor, Wallet etc.
+     * @param signatureType The signature type, i.e EthSign, Wallet etc.
      * @return Hex encoded string of signature proof with Signature Type
      */
     convertToSignatureWithType(signature: string, signatureType: SignatureType): string {
@@ -333,12 +318,6 @@ export const signatureUtils = {
                 const prefixedMsgHex = ethUtil.bufferToHex(prefixedMsgBuff);
                 return prefixedMsgHex;
             }
-            case SignerType.Trezor: {
-                const msgBuff = ethUtil.toBuffer(message);
-                const prefixedMsgBuff = hashTrezorPersonalMessage(msgBuff);
-                const prefixedMsgHex = ethUtil.bufferToHex(prefixedMsgBuff);
-                return prefixedMsgHex;
-            }
             default:
                 throw new Error(`Unrecognized SignerType: ${signerType}`);
         }
@@ -350,7 +329,7 @@ export const signatureUtils = {
      */
     parseECSignature(signature: string): ECSignature {
         assert.isHexString('signature', signature);
-        const ecSignatureTypes = [SignatureType.EthSign, SignatureType.EIP712, SignatureType.Trezor];
+        const ecSignatureTypes = [SignatureType.EthSign, SignatureType.EIP712];
         assert.isOneOfExpectedSignatureTypes(signature, ecSignatureTypes);
 
         // tslint:disable-next-line:custom-no-magic-numbers
@@ -361,11 +340,6 @@ export const signatureUtils = {
     },
 };
 
-function hashTrezorPersonalMessage(message: Buffer): Buffer {
-    const prefix = ethUtil.toBuffer('\x19Ethereum Signed Message:\n' + String.fromCharCode(message.byteLength));
-    return ethUtil.sha3(Buffer.concat([prefix, message]));
-}
-
 function parseValidatorSignature(signature: string): ValidatorSignature {
     assert.isOneOfExpectedSignatureTypes(signature, [SignatureType.Validator]);
     // tslint:disable:custom-no-magic-numbers
diff --git a/packages/order-utils/test/signature_utils_test.ts b/packages/order-utils/test/signature_utils_test.ts
index 4ce99a1c7..2ca1109a1 100644
--- a/packages/order-utils/test/signature_utils_test.ts
+++ b/packages/order-utils/test/signature_utils_test.ts
@@ -76,20 +76,6 @@ describe('Signature utils', () => {
             );
             expect(isValidSignatureLocal).to.be.true();
         });
-
-        it('should return true for a valid Trezor signature', async () => {
-            dataHex = '0xd0d994e31c88f33fd8a572552a70ed339de579e5ba49ee1d17cc978bbe1cdd21';
-            address = '0x6ecbe1db9ef729cbe972c83fb886247691fb6beb';
-            const trezorSignature =
-                '0x1ce4760660e6495b5ae6723087bea073b3a99ce98ea81fdf00c240279c010e63d05b87bc34c4d67d4776e8d5aeb023a67484f4eaf0fd353b40893e5101e845cd9908';
-            const isValidSignatureLocal = await signatureUtils.isValidSignatureAsync(
-                provider,
-                dataHex,
-                trezorSignature,
-                address,
-            );
-            expect(isValidSignatureLocal).to.be.true();
-        });
     });
     describe('#isValidECSignature', () => {
         const signature = {
@@ -270,15 +256,6 @@ describe('Signature utils', () => {
             r: '0xaca7da997ad177f040240cdccf6905b71ab16b74434388c3a72f34fd25d64393',
             s: '0x46b2bac274ff29b48b3ea6e2d04c1336eaceafda3c53ab483fc3ff12fac3ebf2',
         };
-        it('should concatenate v,r,s and append the Trezor signature type', async () => {
-            const expectedSignatureWithSignatureType =
-                '0x1baca7da997ad177f040240cdccf6905b71ab16b74434388c3a72f34fd25d6439346b2bac274ff29b48b3ea6e2d04c1336eaceafda3c53ab483fc3ff12fac3ebf208';
-            const signatureWithSignatureType = signatureUtils.convertECSignatureToSignatureHex(
-                ecSignature,
-                SignerType.Trezor,
-            );
-            expect(signatureWithSignatureType).to.equal(expectedSignatureWithSignatureType);
-        });
         it('should concatenate v,r,s and append the EthSign signature type when SignerType is Default', async () => {
             const expectedSignatureWithSignatureType =
                 '0x1baca7da997ad177f040240cdccf6905b71ab16b74434388c3a72f34fd25d6439346b2bac274ff29b48b3ea6e2d04c1336eaceafda3c53ab483fc3ff12fac3ebf203';
diff --git a/packages/types/CHANGELOG.json b/packages/types/CHANGELOG.json
index 980a12ad3..1210168d4 100644
--- a/packages/types/CHANGELOG.json
+++ b/packages/types/CHANGELOG.json
@@ -5,6 +5,10 @@
             {
                 "note": "Add WalletError and ValidatorError revert reasons",
                 "pr": 1012
+            },
+            {
+                "note": "Remove Caller and Trezor SignatureTypes",
+                "pr": 1015
             }
         ]
     },
diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts
index 2831d816d..4375fc631 100644
--- a/packages/types/src/index.ts
+++ b/packages/types/src/index.ts
@@ -133,23 +133,20 @@ export enum SignatureType {
     Invalid,
     EIP712,
     EthSign,
-    Caller,
     Wallet,
     Validator,
     PreSigned,
-    Trezor,
     NSignatureTypes,
 }
 
 /**
- * The type of the Signer implementation. Some signer implementations use different message prefixes (e.g Trezor) or implement different
+ * The type of the Signer implementation. Some signer implementations use different message prefixes or implement different
  * eth_sign behaviour (e.g Metamask). Default assumes a spec compliant `eth_sign`.
  */
 export enum SignerType {
     Default = 'DEFAULT',
     Ledger = 'LEDGER',
     Metamask = 'METAMASK',
-    Trezor = 'TREZOR',
 }
 
 export enum AssetProxyId {