From d46e3f677805c8e3e4b4873fb9b0cec2988a4d63 Mon Sep 17 00:00:00 2001 From: Greg Hysen Date: Tue, 19 Jun 2018 17:20:53 -0700 Subject: Twenty new tests for MixinSignatureValidator. Fixed handling of unsupported types. Fixed trezor prefix. --- .../protocol/Exchange/MixinSignatureValidator.sol | 11 +++- .../Exchange/mixins/MSignatureValidator.sol | 19 ++++--- .../current/test/TestValidator/TestValidator.sol | 52 +++++++++++++++++ .../current/test/TestWallet/TestWallet.sol | 65 ++++++++++++++++++++++ packages/contracts/src/utils/artifacts.ts | 4 ++ packages/contracts/src/utils/types.ts | 1 + 6 files changed, 141 insertions(+), 11 deletions(-) create mode 100644 packages/contracts/src/contracts/current/test/TestValidator/TestValidator.sol create mode 100644 packages/contracts/src/contracts/current/test/TestWallet/TestWallet.sol (limited to 'packages/contracts/src') 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', } -- cgit v1.2.3 From 2c7358d64ffb0e9fcbc462bbe3cd89edd67cec81 Mon Sep 17 00:00:00 2001 From: Greg Hysen Date: Tue, 19 Jun 2018 17:27:09 -0700 Subject: Minor style tweaks --- .../current/protocol/Exchange/mixins/MSignatureValidator.sol | 2 +- .../contracts/current/test/TestValidator/TestValidator.sol | 2 +- .../src/contracts/current/test/TestWallet/TestWallet.sol | 12 ++++++------ 3 files changed, 8 insertions(+), 8 deletions(-) (limited to 'packages/contracts/src') 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 0cab95193..9c6fbe22b 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MSignatureValidator.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MSignatureValidator.sol @@ -34,6 +34,6 @@ contract MSignatureValidator is Validator, // 0x06 PreSigned, // 0x07 Trezor, // 0x08 - NSignatureTypes // 0x09, always leave at end. + NSignatureTypes // 0x09, number of signature types. 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 index ba661f89d..c0daf8f38 100644 --- a/packages/contracts/src/contracts/current/test/TestValidator/TestValidator.sol +++ b/packages/contracts/src/contracts/current/test/TestValidator/TestValidator.sol @@ -33,7 +33,7 @@ contract TestValidator is validSigner = _validSigner; } - /// @dev Verifies that a signature is valid. + /// @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. diff --git a/packages/contracts/src/contracts/current/test/TestWallet/TestWallet.sol b/packages/contracts/src/contracts/current/test/TestWallet/TestWallet.sol index 5baba1d98..51556971d 100644 --- a/packages/contracts/src/contracts/current/test/TestWallet/TestWallet.sol +++ b/packages/contracts/src/contracts/current/test/TestWallet/TestWallet.sol @@ -40,24 +40,24 @@ contract TestWallet is /// @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. + /// @param eip712Signature Proof of signing. /// @return Validity of order signature. function isValidSignature( bytes32 hash, - bytes eip721Signature + bytes eip712Signature ) external view returns (bool isValid) { require( - eip721Signature.length == 65, + eip712Signature.length == 65, LENGTH_65_REQUIRED ); - uint8 v = uint8(eip721Signature[0]); - bytes32 r = readBytes32(eip721Signature, 1); - bytes32 s = readBytes32(eip721Signature, 33); + uint8 v = uint8(eip712Signature[0]); + bytes32 r = readBytes32(eip712Signature, 1); + bytes32 s = readBytes32(eip712Signature, 33); address recoveredAddress = ecrecover(hash, v, r, s); isValid = walletOwner == recoveredAddress; return isValid; -- cgit v1.2.3 From 8d003dbc30735d72b860e739fd7dc47fad557df9 Mon Sep 17 00:00:00 2001 From: Greg Hysen Date: Tue, 19 Jun 2018 18:26:42 -0700 Subject: Fixed two mislabelled revert reasons + Signature Validator revert tests working on Geth --- .../contracts/current/protocol/Exchange/libs/LibExchangeErrors.sol | 4 ++-- packages/contracts/src/utils/constants.ts | 5 +++++ 2 files changed, 7 insertions(+), 2 deletions(-) (limited to 'packages/contracts/src') diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibExchangeErrors.sol b/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibExchangeErrors.sol index adf27bec3..aab428e74 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibExchangeErrors.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibExchangeErrors.sol @@ -56,6 +56,6 @@ contract LibExchangeErrors { /// Length validation errors /// string constant LENGTH_GREATER_THAN_0_REQUIRED = "LENGTH_GREATER_THAN_0_REQUIRED"; // Byte array must have a length greater than 0. - string constant LENGTH_0_REQUIRED = "LENGTH_1_REQUIRED"; // Byte array must have a length of 1. - string constant LENGTH_65_REQUIRED = "LENGTH_66_REQUIRED"; // Byte array must have a length of 66. + string constant LENGTH_0_REQUIRED = "LENGTH_0_REQUIRED"; // Byte array must have a length of 0. + string constant LENGTH_65_REQUIRED = "LENGTH_65_REQUIRED"; // Byte array must have a length of 65. } diff --git a/packages/contracts/src/utils/constants.ts b/packages/contracts/src/utils/constants.ts index ec3c8fd36..f21b8c7a0 100644 --- a/packages/contracts/src/utils/constants.ts +++ b/packages/contracts/src/utils/constants.ts @@ -27,6 +27,11 @@ export const constants = { LIB_BYTES_GREATER_OR_EQUAL_TO_SOURCE_BYTES_LENGTH_REQUIRED: 'GREATER_OR_EQUAL_TO_SOURCE_BYTES_LENGTH_REQUIRED', ERC20_INSUFFICIENT_BALANCE: 'Insufficient balance to complete transfer.', ERC20_INSUFFICIENT_ALLOWANCE: 'Insufficient allowance to complete transfer.', + EXCHANGE_LENGTH_GREATER_THAN_0_REQUIRED: 'LENGTH_GREATER_THAN_0_REQUIRED', + EXCHANGE_SIGNATURE_UNSUPPORTED: 'SIGNATURE_UNSUPPORTED', + EXCHANGE_SIGNATURE_ILLEGAL: 'SIGNATURE_ILLEGAL', + EXCHANGE_LENGTH_0_REQUIRED: 'LENGTH_0_REQUIRED', + EXCHANGE_LENGTH_65_REQUIRED: 'LENGTH_65_REQUIRED', TESTRPC_NETWORK_ID: 50, // Note(albrow): In practice V8 and most other engines limit the minimum // interval for setInterval to 10ms. We still set it to 0 here in order to -- cgit v1.2.3 From 7814a391d8698f0b865d5f2675b9cc068eeab986 Mon Sep 17 00:00:00 2001 From: Greg Hysen Date: Tue, 19 Jun 2018 18:55:19 -0700 Subject: Few more minor #nit wording changes --- .../src/contracts/current/test/TestValidator/TestValidator.sol | 6 +++--- .../contracts/src/contracts/current/test/TestWallet/TestWallet.sol | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) (limited to 'packages/contracts/src') diff --git a/packages/contracts/src/contracts/current/test/TestValidator/TestValidator.sol b/packages/contracts/src/contracts/current/test/TestValidator/TestValidator.sol index c0daf8f38..7eba61073 100644 --- a/packages/contracts/src/contracts/current/test/TestValidator/TestValidator.sol +++ b/packages/contracts/src/contracts/current/test/TestValidator/TestValidator.sol @@ -28,16 +28,16 @@ contract TestValidator is address validSigner; /// @dev constructs a new `TestValidator` with a single valid signer. - /// @param _validSigner The sole signer for this wallet. + /// @param _validSigner The sole, valid signer. constructor (address _validSigner) public { validSigner = _validSigner; } - /// @dev Verifies that a signature is valid. + /// @dev Verifies that a signature is valid. `signer` must match `validSigner`. /// @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. + /// @return Validity of signature. function isValidSignature( bytes32 hash, address signer, diff --git a/packages/contracts/src/contracts/current/test/TestWallet/TestWallet.sol b/packages/contracts/src/contracts/current/test/TestWallet/TestWallet.sol index 51556971d..e7ddf1d9f 100644 --- a/packages/contracts/src/contracts/current/test/TestWallet/TestWallet.sol +++ b/packages/contracts/src/contracts/current/test/TestWallet/TestWallet.sol @@ -38,10 +38,10 @@ contract TestWallet is } /// @dev Validates an EIP712 signature. - /// The signer must match the signer of this wallet. + /// The signer must match the owner of this wallet. /// @param hash Message hash that is signed. /// @param eip712Signature Proof of signing. - /// @return Validity of order signature. + /// @return Validity of signature. function isValidSignature( bytes32 hash, bytes eip712Signature -- cgit v1.2.3 From 12e16d532bd1844b561a5c1d32fc1c9b1fae2efc Mon Sep 17 00:00:00 2001 From: Greg Hysen Date: Wed, 20 Jun 2018 13:50:11 -0700 Subject: Renamed constants in test wallet/validator --- .../contracts/current/test/TestValidator/TestValidator.sol | 12 ++++++------ .../src/contracts/current/test/TestWallet/TestWallet.sol | 10 +++++----- 2 files changed, 11 insertions(+), 11 deletions(-) (limited to 'packages/contracts/src') diff --git a/packages/contracts/src/contracts/current/test/TestValidator/TestValidator.sol b/packages/contracts/src/contracts/current/test/TestValidator/TestValidator.sol index 7eba61073..13953b482 100644 --- a/packages/contracts/src/contracts/current/test/TestValidator/TestValidator.sol +++ b/packages/contracts/src/contracts/current/test/TestValidator/TestValidator.sol @@ -25,15 +25,15 @@ contract TestValidator is { // The single valid signer for this wallet. - address validSigner; + address VALID_SIGNER; /// @dev constructs a new `TestValidator` with a single valid signer. - /// @param _validSigner The sole, valid signer. - constructor (address _validSigner) public { - validSigner = _validSigner; + /// @param validSigner The sole, valid signer. + constructor (address validSigner) public { + VALID_SIGNER = validSigner; } - /// @dev Verifies that a signature is valid. `signer` must match `validSigner`. + /// @dev Verifies that a signature is valid. `signer` must match `VALID_SIGNER`. /// @param hash Message hash that is signed. /// @param signer Address that should have signed the given hash. /// @param signature Proof of signing. @@ -47,6 +47,6 @@ contract TestValidator is view returns (bool isValid) { - return (signer == validSigner); + return (signer == VALID_SIGNER); } } diff --git a/packages/contracts/src/contracts/current/test/TestWallet/TestWallet.sol b/packages/contracts/src/contracts/current/test/TestWallet/TestWallet.sol index e7ddf1d9f..aca74b409 100644 --- a/packages/contracts/src/contracts/current/test/TestWallet/TestWallet.sol +++ b/packages/contracts/src/contracts/current/test/TestWallet/TestWallet.sol @@ -29,12 +29,12 @@ contract TestWallet is string constant LENGTH_65_REQUIRED = "LENGTH_65_REQUIRED"; // The owner of this wallet. - address walletOwner; + address WALLET_OWNER; /// @dev constructs a new `TestWallet` with a single owner. - /// @param _walletOwner The owner of this wallet. - constructor (address _walletOwner) public { - walletOwner = _walletOwner; + /// @param walletOwner The owner of this wallet. + constructor (address walletOwner) public { + WALLET_OWNER = walletOwner; } /// @dev Validates an EIP712 signature. @@ -59,7 +59,7 @@ contract TestWallet is bytes32 r = readBytes32(eip712Signature, 1); bytes32 s = readBytes32(eip712Signature, 33); address recoveredAddress = ecrecover(hash, v, r, s); - isValid = walletOwner == recoveredAddress; + isValid = WALLET_OWNER == recoveredAddress; return isValid; } } -- cgit v1.2.3