From 822e319efea9c862702ddf589eb2344ff02e5bc5 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Mon, 21 May 2018 15:59:58 -0700 Subject: Use last byte of signature as signature type --- .../protocol/Exchange/MixinSignatureValidator.sol | 58 +++++++++------------- .../current/test/TestLibBytes/TestLibBytes.sol | 31 +++++++----- .../contracts/current/test/Whitelist/Whitelist.sol | 2 +- .../contracts/current/utils/LibBytes/LibBytes.sol | 54 +++++++++++--------- packages/contracts/src/utils/signing_utils.ts | 4 +- packages/contracts/test/exchange/core.ts | 5 +- .../contracts/test/exchange/signature_validator.ts | 8 ++- packages/contracts/test/libraries/lib_bytes.ts | 47 ++++++++---------- 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( -- cgit v1.2.3