Use last byte of signature as signature type

8 files changed, 103 insertions, 106 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 {
diff --git a/packages/contracts/test/exchange/core.ts b/packages/contracts/test/exchange/core.ts
index be8d14cb0..bc476a5ee 100644
--- a/packages/contracts/test/exchange/core.ts
+++ b/packages/contracts/test/exchange/core.ts
@@ -460,10 +460,11 @@ describe('Exchange core', () => {
                 makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),
             });
 
+            const v = ethUtil.toBuffer(signedOrder.signature.slice(0, 4));
             const invalidR = ethUtil.sha3('invalidR');
             const invalidS = ethUtil.sha3('invalidS');
-            const signatureTypeAndV = signedOrder.signature.slice(0, 6);
-            const invalidSigBuff = Buffer.concat([ethUtil.toBuffer(signatureTypeAndV), invalidR, invalidS]);
+            const signatureType = ethUtil.toBuffer(`0x${signedOrder.signature.slice(-2)}`);
+            const invalidSigBuff = Buffer.concat([v, invalidR, invalidS, signatureType]);
             const invalidSigHex = `0x${invalidSigBuff.toString('hex')}`;
             signedOrder.signature = invalidSigHex;
             return expect(exchangeWrapper.fillOrderAsync(signedOrder, takerAddress)).to.be.rejectedWith(
diff --git a/packages/contracts/test/exchange/signature_validator.ts b/packages/contracts/test/exchange/signature_validator.ts
index 376fff438..ca679e344 100644
--- a/packages/contracts/test/exchange/signature_validator.ts
+++ b/packages/contracts/test/exchange/signature_validator.ts
@@ -75,13 +75,11 @@ describe('MixinSignatureValidator', () => {
         });
 
         it('should return false with an invalid signature', async () => {
+            const v = ethUtil.toBuffer(signedOrder.signature.slice(0, 4));
             const invalidR = ethUtil.sha3('invalidR');
             const invalidS = ethUtil.sha3('invalidS');
-            const invalidSigBuff = Buffer.concat([
-                ethUtil.toBuffer(signedOrder.signature.slice(0, 6)),
-                invalidR,
-                invalidS,
-            ]);
+            const signatureType = ethUtil.toBuffer(`0x${signedOrder.signature.slice(-2)}`);
+            const invalidSigBuff = Buffer.concat([v, invalidR, invalidS, signatureType]);
             const invalidSigHex = `0x${invalidSigBuff.toString('hex')}`;
             signedOrder.signature = invalidSigHex;
             const orderHashHex = orderUtils.getOrderHashHex(signedOrder);
diff --git a/packages/contracts/test/libraries/lib_bytes.ts b/packages/contracts/test/libraries/lib_bytes.ts
index c85cee5e3..4eedad25d 100644
--- a/packages/contracts/test/libraries/lib_bytes.ts
+++ b/packages/contracts/test/libraries/lib_bytes.ts
@@ -22,6 +22,7 @@ describe('LibBytes', () => {
     let owner: string;
     let libBytes: TestLibBytesContract;
     const byteArrayShorterThan32Bytes = '0x012345';
+    const byteArrayShorterThan20Bytes = byteArrayShorterThan32Bytes;
     const byteArrayLongerThan32Bytes =
         '0x0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef';
     const byteArrayLongerThan32BytesFirstBytesSwapped =
@@ -60,39 +61,35 @@ describe('LibBytes', () => {
         await blockchainLifecycle.revertAsync();
     });
 
-    describe('deepCopyBytes', () => {
-        const byteArrayLongerThan32BytesLen = (byteArrayLongerThan32Bytes.length - 2) / 2;
-        it('should return a byte array of length 0 if len is 0', async () => {
-            const index = new BigNumber(0);
-            const len = new BigNumber(0);
-            const copy = await libBytes.publicDeepCopyBytes.callAsync(byteArrayLongerThan32Bytes, index, len);
-            expect(copy).to.equal(constants.NULL_BYTES);
+    describe('popByte', () => {
+        it('should revert if length is 0', async () => {
+            return expect(libBytes.publicPopByte.callAsync(constants.NULL_BYTES)).to.be.rejectedWith(constants.REVERT);
         });
 
-        it('should throw if start index + length to copy is greater than length of byte array', async () => {
-            const index = new BigNumber(0);
-            const len = new BigNumber(byteArrayLongerThan32BytesLen + 1);
-            return expect(
-                libBytes.publicDeepCopyBytes.callAsync(byteArrayLongerThan32Bytes, index, len),
-            ).to.be.rejectedWith(constants.REVERT);
+        it('should pop the last byte from the input and return it', async () => {
+            const [newBytes, poppedByte] = await libBytes.publicPopByte.callAsync(byteArrayLongerThan32Bytes);
+            const expectedNewBytes = byteArrayLongerThan32Bytes.slice(0, -2);
+            const expectedPoppedByte = `0x${byteArrayLongerThan32Bytes.slice(-2)}`;
+            expect(newBytes).to.equal(expectedNewBytes);
+            expect(poppedByte).to.equal(expectedPoppedByte);
         });
+    });
 
-        it('should copy the entire byte array if index = 0 and len = b.length', async () => {
-            const index = new BigNumber(0);
-            const len = new BigNumber(byteArrayLongerThan32BytesLen);
-            const copy = await libBytes.publicDeepCopyBytes.callAsync(byteArrayLongerThan32Bytes, index, len);
-            expect(copy).to.equal(byteArrayLongerThan32Bytes);
+    describe('popAddress', () => {
+        it('should revert if length is less than 20', async () => {
+            return expect(libBytes.publicPopAddress.callAsync(byteArrayShorterThan20Bytes)).to.be.rejectedWith(
+                constants.REVERT,
+            );
         });
 
-        it('should copy part of the byte array if area to copy is less than b.length', async () => {
-            const index = new BigNumber(10);
-            const len = new BigNumber(4);
-            const copy = await libBytes.publicDeepCopyBytes.callAsync(byteArrayLongerThan32Bytes, index, len);
-            const expectedCopy = `0x${byteArrayLongerThan32Bytes.slice(22, 30)}`;
-            expect(copy).to.equal(expectedCopy);
+        it('should pop the last 20 bytes from the input and return it', async () => {
+            const [newBytes, poppedAddress] = await libBytes.publicPopAddress.callAsync(byteArrayLongerThan32Bytes);
+            const expectedNewBytes = byteArrayLongerThan32Bytes.slice(0, -40);
+            const expectedPoppedAddress = `0x${byteArrayLongerThan32Bytes.slice(-40)}`;
+            expect(newBytes).to.equal(expectedNewBytes);
+            expect(poppedAddress).to.equal(expectedPoppedAddress);
         });
     });
-
     describe('areBytesEqual', () => {
         it('should return true if byte arrays are equal (both arrays < 32 bytes)', async () => {
             const areBytesEqual = await libBytes.publicAreBytesEqual.callAsync(