Twenty new tests for MixinSignatureValidator. Fixed handling of unsupported types. Fixed trezor prefix.

6 files changed, 141 insertions, 11 deletions

diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol
index 8ad15aaff..4a2beff57 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol
@@ -33,7 +33,7 @@ contract MixinSignatureValidator is
 {
     // Personal message headers
     string constant ETH_PERSONAL_MESSAGE = "\x19Ethereum Signed Message:\n32";
-    string constant TREZOR_PERSONAL_MESSAGE = "\x19Ethereum Signed Message:\n\x41";
+    string constant TREZOR_PERSONAL_MESSAGE = "\x19Ethereum Signed Message:\n\x20";
 
     // Mapping of hash => signer => signed
     mapping (bytes32 => mapping (address => bool)) public preSigned;
@@ -92,8 +92,15 @@ contract MixinSignatureValidator is
             LENGTH_GREATER_THAN_0_REQUIRED
         );
 
+        // Ensure signature is supported
+        uint8 signatureTypeRaw = uint8(popLastByte(signature));
+        require(
+            signatureTypeRaw < uint8(SignatureType.NSignatureTypes),
+            SIGNATURE_UNSUPPORTED
+        );
+
         // Pop last byte off of signature byte array.
-        SignatureType signatureType = SignatureType(uint8(popLastByte(signature)));
+        SignatureType signatureType = SignatureType(signatureTypeRaw);
 
         // Variables are not scoped in Solidity.
         uint8 v;
diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MSignatureValidator.sol b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MSignatureValidator.sol
index 5e286e43a..0cab95193 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MSignatureValidator.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MSignatureValidator.sol
@@ -25,14 +25,15 @@ contract MSignatureValidator is
 {
     // Allowed signature types.
     enum SignatureType {
-        Illegal,    // 0x00, default value
-        Invalid,    // 0x01
-        EIP712,     // 0x02
-        EthSign,    // 0x03
-        Caller,     // 0x04
-        Wallet,     // 0x05
-        Validator,  // 0x06
-        PreSigned,  // 0x07
-        Trezor      // 0x08
+        Illegal,        // 0x00, default value
+        Invalid,        // 0x01
+        EIP712,         // 0x02
+        EthSign,        // 0x03
+        Caller,         // 0x04
+        Wallet,         // 0x05
+        Validator,      // 0x06
+        PreSigned,      // 0x07
+        Trezor,         // 0x08
+        NSignatureTypes // 0x09, always leave at end.
     }
 }
diff --git a/packages/contracts/src/contracts/current/test/TestValidator/TestValidator.sol b/packages/contracts/src/contracts/current/test/TestValidator/TestValidator.sol
new file mode 100644
index 000000000..ba661f89d
--- /dev/null
+++ b/packages/contracts/src/contracts/current/test/TestValidator/TestValidator.sol
@@ -0,0 +1,52 @@
+/*
+
+  Copyright 2018 ZeroEx Intl.
+
+  Licensed under the Apache License, Version 2.0 (the "License");
+  you may not use this file except in compliance with the License.
+  You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS,
+  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  See the License for the specific language governing permissions and
+  limitations under the License.
+
+*/
+
+pragma solidity ^0.4.24;
+
+import "../../protocol/Exchange/interfaces/IValidator.sol";
+
+contract TestValidator is 
+    IValidator
+{
+
+    // The single valid signer for this wallet.
+    address validSigner;
+
+    /// @dev constructs a new `TestValidator` with a single valid signer.
+    /// @param _validSigner The sole signer for this wallet.
+    constructor (address _validSigner) public {
+        validSigner = _validSigner;
+    }
+
+     /// @dev Verifies that a signature is valid.
+    /// @param hash Message hash that is signed.
+    /// @param signer Address that should have signed the given hash.
+    /// @param signature Proof of signing.
+    /// @return Validity of order signature.
+    function isValidSignature(
+        bytes32 hash,
+        address signer,
+        bytes signature
+    )
+        external
+        view
+        returns (bool isValid)
+    {
+        return (signer == validSigner);
+    }
+}
diff --git a/packages/contracts/src/contracts/current/test/TestWallet/TestWallet.sol b/packages/contracts/src/contracts/current/test/TestWallet/TestWallet.sol
new file mode 100644
index 000000000..5baba1d98
--- /dev/null
+++ b/packages/contracts/src/contracts/current/test/TestWallet/TestWallet.sol
@@ -0,0 +1,65 @@
+/*
+
+  Copyright 2018 ZeroEx Intl.
+
+  Licensed under the Apache License, Version 2.0 (the "License");
+  you may not use this file except in compliance with the License.
+  You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS,
+  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  See the License for the specific language governing permissions and
+  limitations under the License.
+
+*/
+
+pragma solidity ^0.4.24;
+
+import "../../protocol/Exchange/interfaces/IWallet.sol";
+import "../../utils/LibBytes/LibBytes.sol";
+
+contract TestWallet is 
+    IWallet,
+    LibBytes
+{
+
+    string constant LENGTH_65_REQUIRED = "LENGTH_65_REQUIRED";
+
+    // The owner of this wallet.
+    address walletOwner;
+
+    /// @dev constructs a new `TestWallet` with a single owner.
+    /// @param _walletOwner The owner of this wallet.
+    constructor (address _walletOwner) public {
+        walletOwner = _walletOwner;
+    }
+
+    /// @dev Validates an EIP712 signature.
+    ///      The signer must match the signer of this wallet.
+    /// @param hash Message hash that is signed.
+    /// @param eip721Signature Proof of signing.
+    /// @return Validity of order signature.
+    function isValidSignature(
+        bytes32 hash,
+        bytes eip721Signature
+    )
+        external
+        view
+        returns (bool isValid)
+    {
+        require(
+            eip721Signature.length == 65,
+            LENGTH_65_REQUIRED
+        );
+
+        uint8 v = uint8(eip721Signature[0]);
+        bytes32 r = readBytes32(eip721Signature, 1);
+        bytes32 s = readBytes32(eip721Signature, 33);
+        address recoveredAddress = ecrecover(hash, v, r, s);
+        isValid = walletOwner == recoveredAddress;
+        return isValid;
+    }
+}
diff --git a/packages/contracts/src/utils/artifacts.ts b/packages/contracts/src/utils/artifacts.ts
index 7ba467708..4375d87c6 100644
--- a/packages/contracts/src/utils/artifacts.ts
+++ b/packages/contracts/src/utils/artifacts.ts
@@ -17,6 +17,8 @@ import * as TestLibBytes from '../artifacts/TestLibBytes.json';
 import * as TestLibMem from '../artifacts/TestLibMem.json';
 import * as TestLibs from '../artifacts/TestLibs.json';
 import * as TestSignatureValidator from '../artifacts/TestSignatureValidator.json';
+import * as TestValidator from '../artifacts/TestValidator.json';
+import * as TestWallet from '../artifacts/TestWallet.json';
 import * as TokenRegistry from '../artifacts/TokenRegistry.json';
 import * as EtherToken from '../artifacts/WETH9.json';
 import * as Whitelist from '../artifacts/Whitelist.json';
@@ -41,6 +43,8 @@ export const artifacts = {
     TestLibMem: (TestLibMem as any) as ContractArtifact,
     TestLibs: (TestLibs as any) as ContractArtifact,
     TestSignatureValidator: (TestSignatureValidator as any) as ContractArtifact,
+    TestValidator: (TestValidator as any) as ContractArtifact,
+    TestWallet: (TestWallet as any) as ContractArtifact,
     TokenRegistry: (TokenRegistry as any) as ContractArtifact,
     Whitelist: (Whitelist as any) as ContractArtifact,
     ZRX: (ZRX as any) as ContractArtifact,
diff --git a/packages/contracts/src/utils/types.ts b/packages/contracts/src/utils/types.ts
index bb8c12088..a6d0c2a88 100644
--- a/packages/contracts/src/utils/types.ts
+++ b/packages/contracts/src/utils/types.ts
@@ -100,6 +100,7 @@ export enum ContractName {
     DummyERC721Receiver = 'DummyERC721Receiver',
     DummyERC721Token = 'DummyERC721Token',
     TestLibBytes = 'TestLibBytes',
+    TestWallet = 'TestWallet',
     Authorizable = 'Authorizable',
     Whitelist = 'Whitelist',
 }