Use last byte of signature as signature type

5 files changed, 75 insertions, 74 deletions

diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol
index 423f306cb..6c018683e 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol
@@ -81,7 +81,9 @@ contract MixinSignatureValidator is
             signature.length >= 1,
             INVALID_SIGNATURE_LENGTH
         );
-        SignatureType signatureType = SignatureType(uint8(signature[0]));
+
+        // Pop last byte off of 
+        SignatureType signatureType = SignatureType(uint8(popByte(signature)));
 
         // Variables are not scoped in Solidity.
         uint8 v;
@@ -104,7 +106,7 @@ contract MixinSignatureValidator is
         // a correctly formatted but incorrect signature.
         } else if (signatureType == SignatureType.Invalid) {
             require(
-                signature.length == 1,
+                signature.length == 0,
                 INVALID_SIGNATURE_LENGTH
             );
             isValid = false;
@@ -113,12 +115,12 @@ contract MixinSignatureValidator is
         // Signature using EIP712
         } else if (signatureType == SignatureType.EIP712) {
             require(
-                signature.length == 66,
+                signature.length == 65,
                 INVALID_SIGNATURE_LENGTH
             );
-            v = uint8(signature[1]);
-            r = readBytes32(signature, 2);
-            s = readBytes32(signature, 34);
+            v = uint8(signature[0]);
+            r = readBytes32(signature, 1);
+            s = readBytes32(signature, 33);
             recovered = ecrecover(hash, v, r, s);
             isValid = signer == recovered;
             return isValid;
@@ -126,12 +128,12 @@ contract MixinSignatureValidator is
         // Signed using web3.eth_sign
         } else if (signatureType == SignatureType.Ecrecover) {
             require(
-                signature.length == 66,
+                signature.length == 65,
                 INVALID_SIGNATURE_LENGTH
             );
-            v = uint8(signature[1]);
-            r = readBytes32(signature, 2);
-            s = readBytes32(signature, 34);
+            v = uint8(signature[0]);
+            r = readBytes32(signature, 1);
+            s = readBytes32(signature, 33);
             recovered = ecrecover(
                 keccak256("\x19Ethereum Signed Message:\n32", hash),
                 v,
@@ -151,7 +153,7 @@ contract MixinSignatureValidator is
         // submit using `Caller`. Having `Caller` allows this flexibility.
         } else if (signatureType == SignatureType.Caller) {
             require(
-                signature.length == 1,
+                signature.length == 0,
                 INVALID_SIGNATURE_LENGTH
             );
             isValid = signer == msg.sender;
@@ -160,37 +162,25 @@ contract MixinSignatureValidator is
         // Signature verified by signer contract.
         // If used with an order, the maker of the order is the signer contract.
         } else if (signatureType == SignatureType.Signer) {
-            // Pass in signature without signature type.
-            bytes memory signatureWithoutType = deepCopyBytes(
-                signature,
-                1,
-                signature.length - 1
-            );
-            isValid = ISigner(signer).isValidSignature(hash, signatureWithoutType);
+            isValid = ISigner(signer).isValidSignature(hash, signature);
             return isValid;
 
         // Signature verified by validator contract.
         // If used with an order, the maker of the order can still be an EOA.
         // A signature using this type should be encoded as:
-        // | Offset | Length | Contents                        |
-        // | 0x00   | 1      | Signature type is always "\x06" |
-        // | 0x01   | 20     | Address of validator contract   |
-        // | 0x15   | **     | Signature to validate           |
+        // | Offset   | Length | Contents                        |
+        // | 0x00     | x      | Signature to validate           |
+        // | 0x00 + x | 20     | Address of validator contract   |
+        // | 0x14 + x | 1      | Signature type is always "\x06" |
         } else if (signatureType == SignatureType.Validator) {
-            address validator = readAddress(signature, 1);
+            address validator = popAddress(signature);
             if (!allowedValidators[signer][validator]) {
                 return false;
             }
-            // Pass in signature without type or validator address.
-            bytes memory signatureWithoutTypeOrAddress = deepCopyBytes(
-                signature,
-                21,
-                signature.length - 21
-            );
             isValid = IValidator(validator).isValidSignature(
                 hash,
                 signer,
-                signatureWithoutTypeOrAddress
+                signature
             );
             return isValid;
 
@@ -209,12 +199,12 @@ contract MixinSignatureValidator is
         // https://github.com/trezor/trezor-mcu/blob/master/firmware/crypto.c#L36
         } else if (signatureType == SignatureType.Trezor) {
             require(
-                signature.length == 66,
+                signature.length == 65,
                 INVALID_SIGNATURE_LENGTH
             );
-            v = uint8(signature[1]);
-            r = readBytes32(signature, 2);
-            s = readBytes32(signature, 34);
+            v = uint8(signature[0]);
+            r = readBytes32(signature, 1);
+            s = readBytes32(signature, 33);
             recovered = ecrecover(
                 keccak256("\x19Ethereum Signed Message:\n\x41", hash),
                 v,
diff --git a/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol b/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol
index b3ed1f620..cd5206eb8 100644
--- a/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol
+++ b/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol
@@ -25,21 +25,26 @@ contract TestLibBytes is
     LibBytes
 {
 
-    /// @dev Performs a deep copy of a section of a byte array.
-    /// @param b Byte array that will be sliced.
-    /// @param startIndex Index of first byte to copy.
-    /// @param endIndex Index of last byte to copy + 1.
-    /// @return A deep copy of b from startIndex to endIndex.
-    function publicDeepCopyBytes(
-        bytes memory b,
-        uint256 startIndex,
-        uint256 endIndex)
+    /// @dev Pops the last byte off of a byte array by modifying its length.
+    /// @param b Byte array that will be modified.
+    /// @return The byte that was popped off.
+    function publicPopByte(bytes memory b)
         public
-        pure
-        returns (bytes memory copy)
+        returns (bytes memory, byte result)
+    {
+        result = popByte(b);
+        return (b, result);
+    }
+
+    /// @dev Pops the last 20 bytes off of a byte array by modifying its length.
+    /// @param b Byte array that will be modified.
+    /// @return The 20 byte address that was popped off.
+    function publicPopAddress(bytes memory b)
+        public
+        returns (bytes memory, address result)
     {
-        copy = deepCopyBytes(b, startIndex, endIndex);
-        return copy;
+        result = popAddress(b);
+        return (b, result);
     }
 
     /// @dev Tests equality of two byte arrays.
diff --git a/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol b/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol
index 3bd3179e5..11576fd89 100644
--- a/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol
+++ b/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol
@@ -75,7 +75,7 @@ contract Whitelist is
         // This contract must be the entry point for the transaction.
         require(takerAddress == tx.origin);
 
-        // Check if maker is on the whitelist
+        // Check if maker is on the whitelist.
         require(
             isWhitelisted[order.makerAddress],
             ADDRESS_NOT_WHITELISTED
diff --git a/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol b/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol
index 5c4fdbb23..f0a622fe4 100644
--- a/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol
+++ b/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol
@@ -27,39 +27,45 @@ contract LibBytes {
     string constant GTE_32_LENGTH_REQUIRED = "Length must be greater than or equal to 32.";
     string constant INDEX_OUT_OF_BOUNDS = "Specified array index is out of bounds.";
 
-    /// @dev Performs a deep copy of a section of a byte array.
-    /// @param b Byte array that will be copied.
-    /// @param index Index of first byte to copy.
-    /// @param len Number of bytes to copy starting from index.
-    /// @return A deep copy `len` bytes of b starting from index.
-    function deepCopyBytes(
-        bytes memory b,
-        uint256 index,
-        uint256 len)
+    /// @dev Pops the last byte off of a byte array by modifying its length.
+    /// @param b Byte array that will be modified.
+    /// @return The byte that was popped off.
+    function popByte(bytes memory b)
         internal
-        pure
-        returns (bytes memory)
+        returns (byte result)
     {
         require(
-            b.length >= index + len,
-            INDEX_OUT_OF_BOUNDS
+            b.length > 0,
+            GT_ZERO_LENGTH_REQUIRED
         );
 
-        bytes memory copy = new bytes(len);
+        result = b[b.length - 1];
+        assembly {
+            let newLen := sub(mload(b), 1)
+            mstore(b, newLen)
+        }
+        result;
+    }
 
-        // Arrays are prefixed by a 256 bit length parameter
-        index += 32;
+    /// @dev Pops the last 20 bytes off of a byte array by modifying its length.
+    /// @param b Byte array that will be modified.
+    /// @return The 20 byte address that was popped off.
+    function popAddress(bytes memory b)
+        internal
+        returns (address result)
+    {
+        require(
+            b.length >= 20,
+            GTE_20_LENGTH_REQUIRED
+        );
+
+        result = readAddress(b, b.length - 20);
 
         assembly {
-            // Start storing onto copy after length field
-            let storeStartIndex := add(copy, 32)
-            // Start loading from b at index
-            let startLoadIndex := add(b, index)
-            for {let i := 0} lt(i, len) {i := add(i, 32)} {
-                mstore(add(storeStartIndex, i), mload(add(startLoadIndex, i)))
-            }
+            let newLen := sub(mload(b), 20)
+            mstore(b, newLen)
         }
-        return copy;
+        return result;
     }
 
     /// @dev Tests equality of two byte arrays.
diff --git a/packages/contracts/src/utils/signing_utils.ts b/packages/contracts/src/utils/signing_utils.ts
index 61ab1f138..4c36c8310 100644
--- a/packages/contracts/src/utils/signing_utils.ts
+++ b/packages/contracts/src/utils/signing_utils.ts
@@ -8,19 +8,19 @@ export const signingUtils = {
             const prefixedMessage = ethUtil.hashPersonalMessage(message);
             const ecSignature = ethUtil.ecsign(prefixedMessage, privateKey);
             const signature = Buffer.concat([
-                ethUtil.toBuffer(signatureType),
                 ethUtil.toBuffer(ecSignature.v),
                 ecSignature.r,
                 ecSignature.s,
+                ethUtil.toBuffer(signatureType),
             ]);
             return signature;
         } else if (signatureType === SignatureType.EIP712) {
             const ecSignature = ethUtil.ecsign(message, privateKey);
             const signature = Buffer.concat([
-                ethUtil.toBuffer(signatureType),
                 ethUtil.toBuffer(ecSignature.v),
                 ecSignature.r,
                 ecSignature.s,
+                ethUtil.toBuffer(signatureType),
             ]);
             return signature;
         } else {