diff --git a/contracts/exchange/CHANGELOG.json b/contracts/exchange/CHANGELOG.json index c16c0b77d0..ca7469e6b5 100644 --- a/contracts/exchange/CHANGELOG.json +++ b/contracts/exchange/CHANGELOG.json @@ -21,6 +21,14 @@ { "note": "Upgrade all string reverts to rich reverts", "pr": 1761 + }, + { + "note": "Add support for `SignatureType.OrderValidator` for orders", + "pr": 1774 + }, + { + "note": "Add support for `SignatureType.WalletOrderValidator` for orders", + "pr": 1774 } ] }, diff --git a/contracts/exchange/compiler.json b/contracts/exchange/compiler.json index 7bc44f3c43..485ba87b0f 100644 --- a/contracts/exchange/compiler.json +++ b/contracts/exchange/compiler.json @@ -33,6 +33,7 @@ "src/interfaces/IExchange.sol", "src/interfaces/IExchangeCore.sol", "src/interfaces/IMatchOrders.sol", + "src/interfaces/IOrderValidator.sol", "src/interfaces/ISignatureValidator.sol", "src/interfaces/ITransactions.sol", "src/interfaces/IValidator.sol", diff --git a/contracts/exchange/contracts/examples/Validator.sol b/contracts/exchange/contracts/examples/Validator.sol index 86c39cd232..25203d0516 100644 --- a/contracts/exchange/contracts/examples/Validator.sol +++ b/contracts/exchange/contracts/examples/Validator.sol @@ -17,15 +17,19 @@ */ pragma solidity ^0.5.5; +pragma experimental ABIEncoderV2; +import "@0x/contracts-exchange-libs/contracts/src/LibOrder.sol"; import "../src/interfaces/IValidator.sol"; +import "../src/interfaces/IOrderValidator.sol"; -contract Validator is - IValidator +contract Validator is + IValidator, + IOrderValidator { - // The single valid signer for this wallet. + // The single valid signer for this validator. // solhint-disable-next-line var-name-mixedcase address internal VALID_SIGNER; @@ -35,12 +39,12 @@ contract Validator is VALID_SIGNER = validSigner; } + // solhint-disable no-unused-vars /// @dev Verifies that a signature is valid. `signer` must match `VALID_SIGNER`. /// @param hash Message hash that is signed. /// @param signerAddress Address that should have signed the given hash. /// @param signature Proof of signing. /// @return Validity of signature. - // solhint-disable no-unused-vars function isValidSignature( bytes32 hash, address signerAddress, @@ -53,4 +57,23 @@ contract Validator is return (signerAddress == VALID_SIGNER); } // solhint-enable no-unused-vars + + // solhint-disable no-unused-vars + /// @dev Verifies that an order and signature is valid. `signer` must match `VALID_SIGNER`. + /// @param order The order. + /// @param orderHash The order hash. + /// @param signature Proof of signing. + /// @return Validity of signature. + function isValidOrderSignature( + LibOrder.Order calldata order, + bytes32 orderHash, + bytes calldata signature + ) + external + view + returns (bool isValid) + { + return (order.makerAddress == VALID_SIGNER); + } + // solhint-enable no-unused-vars } diff --git a/contracts/exchange/contracts/examples/Wallet.sol b/contracts/exchange/contracts/examples/Wallet.sol index 57df916ff0..acac4a57aa 100644 --- a/contracts/exchange/contracts/examples/Wallet.sol +++ b/contracts/exchange/contracts/examples/Wallet.sol @@ -17,12 +17,14 @@ */ pragma solidity ^0.5.5; +pragma experimental ABIEncoderV2; import "../src/interfaces/IWallet.sol"; import "@0x/contracts-utils/contracts/src/LibBytes.sol"; +import "@0x/contracts-exchange-libs/contracts/src/LibOrder.sol"; -contract Wallet is +contract Wallet is IWallet { using LibBytes for bytes; @@ -40,26 +42,59 @@ contract Wallet is /// @dev Validates an EIP712 signature. /// The signer must match the owner of this wallet. /// @param hash Message hash that is signed. - /// @param eip712Signature Proof of signing. + /// @param signature Proof of signing. /// @return Validity of signature. function isValidSignature( bytes32 hash, - bytes calldata eip712Signature + bytes calldata signature ) external view returns (bool isValid) { require( - eip712Signature.length == 65, + signature.length == 65, "LENGTH_65_REQUIRED" ); - uint8 v = uint8(eip712Signature[0]); - bytes32 r = eip712Signature.readBytes32(1); - bytes32 s = eip712Signature.readBytes32(33); + return validateEIP712Signature(hash, signature); + } + + /// @dev Validates an order AND EIP712 signature. + /// The signer must match the owner of this wallet. + /// @param order The order. + /// @param orderHash The order hash. + /// @param signature Proof of signing. + /// @return Validity of order and signature. + function isValidOrderSignature( + LibOrder.Order calldata order, + bytes32 orderHash, + bytes calldata signature + ) + external + view + returns (bool isValid) + { + // Ensure order hash is correct. + require( + order.makerAddress == WALLET_OWNER, + "INVALID_ORDER_MAKER" + ); + return validateEIP712Signature(orderHash, signature); + } + + function validateEIP712Signature( + bytes32 hash, + bytes memory signature + ) + private + view + returns (bool isValid) + { + uint8 v = uint8(signature[0]); + bytes32 r = signature.readBytes32(1); + bytes32 s = signature.readBytes32(33); address recoveredAddress = ecrecover(hash, v, r, s); - isValid = WALLET_OWNER == recoveredAddress; - return isValid; + return WALLET_OWNER == recoveredAddress; } } diff --git a/contracts/exchange/contracts/src/MixinExchangeCore.sol b/contracts/exchange/contracts/src/MixinExchangeCore.sol index 9a1117dae5..4a526c5e09 100644 --- a/contracts/exchange/contracts/src/MixinExchangeCore.sol +++ b/contracts/exchange/contracts/src/MixinExchangeCore.sol @@ -353,7 +353,8 @@ contract MixinExchangeCore is // Validate Maker signature (check only if first time seen) if (orderInfo.orderTakerAssetFilledAmount == 0) { address makerAddress = order.makerAddress; - if (!isValidSignature( + if (!isValidOrderWithHashSignature( + order, orderInfo.orderHash, makerAddress, signature)) { diff --git a/contracts/exchange/contracts/src/MixinExchangeRichErrors.sol b/contracts/exchange/contracts/src/MixinExchangeRichErrors.sol index a0693c8979..9dfff47582 100644 --- a/contracts/exchange/contracts/src/MixinExchangeRichErrors.sol +++ b/contracts/exchange/contracts/src/MixinExchangeRichErrors.sol @@ -85,6 +85,44 @@ contract MixinExchangeRichErrors is ); } + function SignatureOrderValidatorError( + bytes32 orderHash, + address signer, + bytes memory signature, + bytes memory errorData + ) + internal + pure + returns (bytes memory) + { + return abi.encodeWithSelector( + SIGNATURE_ORDER_VALIDATOR_ERROR_SELECTOR, + orderHash, + signer, + signature, + errorData + ); + } + + function SignatureWalletOrderValidatorError( + bytes32 orderHash, + address wallet, + bytes memory signature, + bytes memory errorData + ) + internal + pure + returns (bytes memory) + { + return abi.encodeWithSelector( + SIGNATURE_WALLET_ORDER_VALIDATOR_ERROR_SELECTOR, + orderHash, + wallet, + signature, + errorData + ); + } + function OrderStatusError( LibOrder.OrderStatus orderStatus, bytes32 orderHash diff --git a/contracts/exchange/contracts/src/MixinSignatureValidator.sol b/contracts/exchange/contracts/src/MixinSignatureValidator.sol index a34ad2ed08..c0c45a332a 100644 --- a/contracts/exchange/contracts/src/MixinSignatureValidator.sol +++ b/contracts/exchange/contracts/src/MixinSignatureValidator.sol @@ -21,15 +21,18 @@ pragma experimental ABIEncoderV2; import "@0x/contracts-utils/contracts/src/LibBytes.sol"; import "@0x/contracts-utils/contracts/src/ReentrancyGuard.sol"; +import "@0x/contracts-exchange-libs/contracts/src/LibOrder.sol"; import "./mixins/MSignatureValidator.sol"; import "./mixins/MTransactions.sol"; import "./mixins/MExchangeRichErrors.sol"; import "./interfaces/IWallet.sol"; import "./interfaces/IValidator.sol"; +import "./interfaces/IOrderValidator.sol"; contract MixinSignatureValidator is ReentrancyGuard, + LibOrder, MSignatureValidator, MTransactions, MExchangeRichErrors @@ -42,6 +45,9 @@ contract MixinSignatureValidator is // Mapping of signer => validator => approved mapping (address => mapping (address => bool)) public allowedValidators; + // Mapping of signer => order validator => approved + mapping (address => mapping (address => bool)) public allowedOrderValidators; + /// @dev Approves a hash on-chain using any valid signature type. /// After presigning a hash, the preSign signature type will become valid for that hash and signer. /// @param signerAddress Address that should have signed the given hash. @@ -54,7 +60,7 @@ contract MixinSignatureValidator is external { if (signerAddress != msg.sender) { - if (!isValidSignature( + if (!isValidHashSignature( hash, signerAddress, signature)) { @@ -69,7 +75,8 @@ contract MixinSignatureValidator is preSigned[hash][signerAddress] = true; } - /// @dev Approves/unnapproves a Validator contract to verify signatures on signer's behalf. + /// @dev Approves/unnapproves a Validator contract to verify signatures on signer's behalf + /// using the `Validator` signature type. /// @param validatorAddress Address of Validator contract. /// @param approval Approval or disapproval of Validator contract. function setSignatureValidatorApproval( @@ -88,12 +95,55 @@ contract MixinSignatureValidator is ); } + /// @dev Approves/unnapproves an OrderValidator contract to verify signatures on signer's behalf + /// using the `OrderValidator` signature type. + /// @param validatorAddress Address of Validator contract. + /// @param approval Approval or disapproval of Validator contract. + function setOrderValidatorApproval( + address validatorAddress, + bool approval + ) + external + nonReentrant + { + address signerAddress = getCurrentContextAddress(); + allowedOrderValidators[signerAddress][validatorAddress] = approval; + emit SignatureValidatorApproval( + signerAddress, + validatorAddress, + approval + ); + } + + /// @dev Verifies that a signature for an order is valid. + /// @param order The order. + /// @param signerAddress Address that should have signed the given order. + /// @param signature Proof that the order has been signed by signer. + /// @return True if the signature is valid for the given order and signer. + function isValidOrderSignature( + Order memory order, + address signerAddress, + bytes memory signature + ) + public + view + returns (bool isValid) + { + bytes32 orderHash = getOrderHash(order); + return isValidOrderWithHashSignature( + order, + orderHash, + signerAddress, + signature + ); + } + /// @dev Verifies that a hash has been signed by the given signer. - /// @param hash Any 32 byte hash. + /// @param hash Any 32-byte hash. /// @param signerAddress Address that should have signed the given hash. /// @param signature Proof that the hash has been signed by signer. - /// @return True if the address recovered from the provided signature matches the input signer address. - function isValidSignature( + /// @return True if the signature is valid for the given hash and signer. + function isValidHashSignature( bytes32 hash, address signerAddress, bytes memory signature @@ -101,6 +151,90 @@ contract MixinSignatureValidator is public view returns (bool isValid) + { + SignatureType signatureType = readValidSignatureType( + hash, + signerAddress, + signature + ); + // Only hash-compatible signature types can be handled by this + // function. + if (signatureType == SignatureType.OrderValidator || + signatureType == SignatureType.WalletOrderValidator) { + rrevert(SignatureError( + SignatureErrorCodes.INAPPROPRIATE_SIGNATURE_TYPE, + hash, + signerAddress, + signature + )); + } + return validateHashSignatureTypes( + signatureType, + hash, + signerAddress, + signature + ); + } + + /// @dev Verifies that an order, with provided order hash, has been signed + /// by the given signer. + /// @param order The order. + /// @param orderHash The hash of the order. + /// @param signerAddress Address that should have signed the.Signat given hash. + /// @param signature Proof that the hash has been signed by signer. + /// @return True if the signature is valid for the given hash and signer. + function isValidOrderWithHashSignature( + Order memory order, + bytes32 orderHash, + address signerAddress, + bytes memory signature + ) + internal + view + returns (bool isValid) + { + SignatureType signatureType = readValidSignatureType( + orderHash, + signerAddress, + signature + ); + if (signatureType == SignatureType.OrderValidator) { + // The entire order is verified by validator contract. + isValid = validateOrderWithValidator( + order, + orderHash, + signerAddress, + signature + ); + return isValid; + } else if (signatureType == SignatureType.WalletOrderValidator) { + // The entire order is verified by a wallet contract. + isValid = validateOrderWithWallet( + order, + orderHash, + signerAddress, + signature + ); + return isValid; + } + // Otherwise, it's one of the hash-compatible signature types. + return validateHashSignatureTypes( + signatureType, + orderHash, + signerAddress, + signature + ); + } + + /// Reads the `SignatureType` from the end of a signature and validates it. + function readValidSignatureType( + bytes32 hash, + address signerAddress, + bytes memory signature + ) + private + pure + returns (SignatureType signatureType) { if (signature.length == 0) { rrevert(SignatureError( @@ -124,126 +258,21 @@ contract MixinSignatureValidator is )); } - SignatureType signatureType = SignatureType(signatureTypeRaw); - - // Variables are not scoped in Solidity. - uint8 v; - bytes32 r; - bytes32 s; - address recovered; - // Always illegal signature. // This is always an implicit option since a signer can create a // signature array with invalid type or length. We may as well make // it an explicit option. This aids testing and analysis. It is // also the initialization value for the enum type. - if (signatureType == SignatureType.Illegal) { + if (SignatureType(signatureTypeRaw) == SignatureType.Illegal) { rrevert(SignatureError( SignatureErrorCodes.ILLEGAL, hash, signerAddress, signature )); - - // Always invalid signature. - // Like Illegal, this is always implicitly available and therefore - // offered explicitly. It can be implicitly created by providing - // a correctly formatted but incorrect signature. - } else if (signatureType == SignatureType.Invalid) { - if (signature.length != 1) { - rrevert(SignatureError( - SignatureErrorCodes.INVALID_LENGTH, - hash, - signerAddress, - signature - )); - } - isValid = false; - return isValid; - - // Signature using EIP712 - } else if (signatureType == SignatureType.EIP712) { - if (signature.length != 66) { - rrevert(SignatureError( - SignatureErrorCodes.INVALID_LENGTH, - hash, - signerAddress, - signature - )); - } - v = uint8(signature[0]); - r = signature.readBytes32(1); - s = signature.readBytes32(33); - recovered = ecrecover( - hash, - v, - r, - s - ); - isValid = signerAddress == recovered; - return isValid; - - // Signed using web3.eth_sign - } else if (signatureType == SignatureType.EthSign) { - if (signature.length != 66) { - rrevert(SignatureError( - SignatureErrorCodes.INVALID_LENGTH, - hash, - signerAddress, - signature - )); - } - v = uint8(signature[0]); - r = signature.readBytes32(1); - s = signature.readBytes32(33); - recovered = ecrecover( - keccak256(abi.encodePacked( - "\x19Ethereum Signed Message:\n32", - hash - )), - v, - r, - s - ); - isValid = signerAddress == recovered; - return isValid; - - // Signature verified by wallet contract. - // If used with an order, the maker of the order is the wallet contract. - } else if (signatureType == SignatureType.Wallet) { - isValid = isValidWalletSignature( - hash, - signerAddress, - signature - ); - return isValid; - - // Signature verified by validator contract. - } else if (signatureType == SignatureType.Validator) { - isValid = isValidValidatorSignature( - hash, - signerAddress, - signature - ); - return isValid; - - // Signer signed hash previously using the preSign function. - } else if (signatureType == SignatureType.PreSigned) { - isValid = preSigned[hash][signerAddress]; - return isValid; } - // Anything else is illegal (We do not return false because - // the signature may actually be valid, just not in a format - // that we currently support. In this case returning false - // may lead the caller to incorrectly believe that the - // signature was invalid.) - rrevert(SignatureError( - SignatureErrorCodes.UNSUPPORTED, - hash, - signerAddress, - signature - )); + return SignatureType(signatureTypeRaw); } /// @dev Verifies signature using logic defined by Wallet contract. @@ -251,8 +280,8 @@ contract MixinSignatureValidator is /// @param walletAddress Address that should have signed the given hash /// and defines its own signature verification method. /// @param signature Proof that the hash has been signed by signer. - /// @return True if signature is valid for given wallet.. - function isValidWalletSignature( + /// @return True if the signature is validated by the Walidator. + function validateHashWithWallet( bytes32 hash, address walletAddress, bytes memory signature @@ -292,11 +321,12 @@ contract MixinSignatureValidator is } /// @dev Verifies signature using logic defined by Validator contract. + /// If used with an order, the maker of the order can still be an EOA. /// @param hash Any 32 byte hash. /// @param signerAddress Address that should have signed the given hash. /// @param signature Proof that the hash has been signed by signer. - /// @return True if the address recovered from the provided signature matches the input signer address. - function isValidValidatorSignature( + /// @return True if the signature is validated by the Validator. + function validateHashWithValidator( bytes32 hash, address signerAddress, bytes memory signature @@ -348,4 +378,214 @@ contract MixinSignatureValidator is returnData )); } + + /// @dev Verifies order AND signature via a Wallet contract. + /// @param order The order. + /// @param orderHash The order hash. + /// @param walletAddress Address that should have signed the given hash + /// and defines its own order/signature verification method. + /// @param signature Proof that the order has been signed by signer. + /// @return True if order and signature are validated by the Wallet. + function validateOrderWithWallet( + Order memory order, + bytes32 orderHash, + address walletAddress, + bytes memory signature + ) + private + view + returns (bool isValid) + { + uint256 signatureLength = signature.length; + // Shave the signature type off the signature. + assembly { + mstore(signature, sub(signatureLength, 1)) + } + // Encode the call data. + bytes memory callData = abi.encodeWithSelector( + IWallet(walletAddress).isValidOrderSignature.selector, + order, + orderHash, + signature + ); + // Restore the full signature. + assembly { + mstore(signature, signatureLength) + } + // Static call the verification function. + (bool didSucceed, bytes memory returnData) = walletAddress.staticcall(callData); + // Return data should be a single bool. + if (didSucceed && returnData.length == 32) { + return returnData.readUint256(0) == 1; + } + // Static call to verifier failed. + rrevert(SignatureWalletOrderValidatorError( + orderHash, + walletAddress, + signature, + returnData + )); + } + + /// @dev Verifies order AND signature via Validator contract. + /// If used with an order, the maker of the order can still be an EOA. + /// @param order The order. + /// @param orderHash The order hash. + /// @param signerAddress Address that should have signed the given hash. + /// @param signature Proof that the hash has been signed by signer. + /// @return True if order and signature are validated by the Validator. + function validateOrderWithValidator( + Order memory order, + bytes32 orderHash, + address signerAddress, + bytes memory signature + ) + private + view + returns (bool isValid) + { + // A signature using this type should be encoded as: + // | Offset | Length | Contents | + // | 0x00 | x | Signature to validate | + // | 0x00 + x | 20 | Address of validator contract | + // | 0x14 + x | 1 | Signature type is always "\x07" | + + uint256 signatureLength = signature.length; + // Read the validator address from the signature. + address validatorAddress = signature.readAddress(signatureLength - 21); + // Ensure signer has approved validator. + if (!allowedOrderValidators[signerAddress][validatorAddress]) { + return false; + } + // Shave the validator address and signature type from the signature. + assembly { + mstore(signature, sub(signatureLength, 21)) + } + // Encode the call data. + bytes memory callData = abi.encodeWithSelector( + IOrderValidator(validatorAddress).isValidOrderSignature.selector, + order, + orderHash, + signature + ); + // Restore the full signature. + assembly { + mstore(signature, signatureLength) + } + // Static call the verification function. + (bool didSucceed, bytes memory returnData) = validatorAddress.staticcall(callData); + // Return data should be a single bool. + if (didSucceed && returnData.length == 32) { + return returnData.readUint256(0) == 1; + } + // Static call to verifier failed. + rrevert(SignatureOrderValidatorError( + orderHash, + signerAddress, + signature, + returnData + )); + } + + /// Validates a hash-compatible signature type + /// (anything but `OrderValidator` and `WalletOrderValidator`). + function validateHashSignatureTypes( + SignatureType signatureType, + bytes32 hash, + address signerAddress, + bytes memory signature + ) + private + view + returns (bool isValid) + { + // Always invalid signature. + // Like Illegal, this is always implicitly available and therefore + // offered explicitly. It can be implicitly created by providing + // a correctly formatted but incorrect signature. + if (signatureType == SignatureType.Invalid) { + if (signature.length != 1) { + rrevert(SignatureError( + SignatureErrorCodes.INVALID_LENGTH, + hash, + signerAddress, + signature + )); + } + isValid = false; + return isValid; + + // Signature using EIP712 + } else if (signatureType == SignatureType.EIP712) { + if (signature.length != 66) { + rrevert(SignatureError( + SignatureErrorCodes.INVALID_LENGTH, + hash, + signerAddress, + signature + )); + } + uint8 v = uint8(signature[0]); + bytes32 r = signature.readBytes32(1); + bytes32 s = signature.readBytes32(33); + address recovered = ecrecover( + hash, + v, + r, + s + ); + isValid = signerAddress == recovered; + return isValid; + + // Signed using web3.eth_sign + } else if (signatureType == SignatureType.EthSign) { + if (signature.length != 66) { + rrevert(SignatureError( + SignatureErrorCodes.INVALID_LENGTH, + hash, + signerAddress, + signature + )); + } + uint8 v = uint8(signature[0]); + bytes32 r = signature.readBytes32(1); + bytes32 s = signature.readBytes32(33); + address recovered = ecrecover( + keccak256(abi.encodePacked( + "\x19Ethereum Signed Message:\n32", + hash + )), + v, + r, + s + ); + isValid = signerAddress == recovered; + return isValid; + + // Signature verified by wallet contract. + // If used with an order, the maker of the order is the wallet contract. + } else if (signatureType == SignatureType.Wallet) { + isValid = validateHashWithWallet( + hash, + signerAddress, + signature + ); + return isValid; + + // Signature verified by validator contract. + // If used with an order, the maker of the order can still be an EOA. + } else if (signatureType == SignatureType.Validator) { + isValid = validateHashWithValidator( + hash, + signerAddress, + signature + ); + return isValid; + + } + // Otherwise, signatureType == SignatureType.PreSigned + assert(signatureType == SignatureType.PreSigned); + // Signer signed hash previously using the preSign function. + return preSigned[hash][signerAddress]; + } } diff --git a/contracts/exchange/contracts/src/MixinTransactions.sol b/contracts/exchange/contracts/src/MixinTransactions.sol index 460cde5563..8e635f2556 100644 --- a/contracts/exchange/contracts/src/MixinTransactions.sol +++ b/contracts/exchange/contracts/src/MixinTransactions.sol @@ -70,7 +70,7 @@ contract MixinTransactions is address signerAddress = transaction.signerAddress; if (signerAddress != msg.sender) { // Validate signature - if (!isValidSignature( + if (!isValidHashSignature( transactionHash, signerAddress, signature)) { diff --git a/contracts/exchange/contracts/src/interfaces/IOrderValidator.sol b/contracts/exchange/contracts/src/interfaces/IOrderValidator.sol new file mode 100644 index 0000000000..0e366513b2 --- /dev/null +++ b/contracts/exchange/contracts/src/interfaces/IOrderValidator.sol @@ -0,0 +1,40 @@ +/* + + 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.5.5; +pragma experimental ABIEncoderV2; + +import "@0x/contracts-exchange-libs/contracts/src/LibOrder.sol"; + + +contract IOrderValidator { + + /// @dev Verifies that an order AND a signature is valid. + /// @param order The order. + /// @param orderHash The order hash. + /// @param signature Proof of signing. + /// @return Validity of order and signature. + function isValidOrderSignature( + LibOrder.Order calldata order, + bytes32 orderHash, + bytes calldata signature + ) + external + view + returns (bool isValid); +} diff --git a/contracts/exchange/contracts/src/interfaces/ISignatureValidator.sol b/contracts/exchange/contracts/src/interfaces/ISignatureValidator.sol index 70f78dfb89..2685279338 100644 --- a/contracts/exchange/contracts/src/interfaces/ISignatureValidator.sol +++ b/contracts/exchange/contracts/src/interfaces/ISignatureValidator.sol @@ -17,6 +17,9 @@ */ pragma solidity ^0.5.5; +pragma experimental ABIEncoderV2; + +import "@0x/contracts-exchange-libs/contracts/src/LibOrder.sol"; contract ISignatureValidator { @@ -31,7 +34,7 @@ contract ISignatureValidator { bytes calldata signature ) external; - + /// @dev Approves/unnapproves a Validator contract to verify signatures on signer's behalf. /// @param validatorAddress Address of Validator contract. /// @param approval Approval or disapproval of Validator contract. @@ -41,12 +44,12 @@ contract ISignatureValidator { ) external; - /// @dev Verifies that a signature is valid. + /// @dev Verifies that a signature for a hash is valid. /// @param hash Message hash that is signed. /// @param signerAddress Address of signer. /// @param signature Proof of signing. /// @return Validity of order signature. - function isValidSignature( + function isValidHashSignature( bytes32 hash, address signerAddress, bytes memory signature @@ -54,4 +57,18 @@ contract ISignatureValidator { public view returns (bool isValid); + + /// @dev Verifies that a signature for an order is valid. + /// @param order The order. + /// @param signerAddress Address of signer. + /// @param signature Proof of signing. + /// @return Validity of order signature. + function isValidOrderSignature( + LibOrder.Order memory order, + address signerAddress, + bytes memory signature + ) + public + view + returns (bool isValid); } diff --git a/contracts/exchange/contracts/src/interfaces/IValidator.sol b/contracts/exchange/contracts/src/interfaces/IValidator.sol index e76d23880c..f2dc33d64b 100644 --- a/contracts/exchange/contracts/src/interfaces/IValidator.sol +++ b/contracts/exchange/contracts/src/interfaces/IValidator.sol @@ -17,6 +17,9 @@ */ pragma solidity ^0.5.5; +pragma experimental ABIEncoderV2; + +import "@0x/contracts-exchange-libs/contracts/src/LibOrder.sol"; contract IValidator { diff --git a/contracts/exchange/contracts/src/interfaces/IWallet.sol b/contracts/exchange/contracts/src/interfaces/IWallet.sol index f56b3115f2..d71088106a 100644 --- a/contracts/exchange/contracts/src/interfaces/IWallet.sol +++ b/contracts/exchange/contracts/src/interfaces/IWallet.sol @@ -17,6 +17,9 @@ */ pragma solidity ^0.5.5; +pragma experimental ABIEncoderV2; + +import "@0x/contracts-exchange-libs/contracts/src/LibOrder.sol"; contract IWallet { @@ -32,4 +35,18 @@ contract IWallet { external view returns (bool isValid); + + /// @dev Verifies that an order AND a signature is valid. + /// @param order The order. + /// @param orderHash The order hash. + /// @param signature Proof of signing. + /// @return Validity of order and signature. + function isValidOrderSignature( + LibOrder.Order calldata order, + bytes32 orderHash, + bytes calldata signature + ) + external + view + returns (bool isValid); } diff --git a/contracts/exchange/contracts/src/mixins/MExchangeRichErrors.sol b/contracts/exchange/contracts/src/mixins/MExchangeRichErrors.sol index 6b28cb6162..f55ce025da 100644 --- a/contracts/exchange/contracts/src/mixins/MExchangeRichErrors.sol +++ b/contracts/exchange/contracts/src/mixins/MExchangeRichErrors.sol @@ -36,7 +36,8 @@ contract MExchangeRichErrors is BAD_SIGNATURE, INVALID_LENGTH, UNSUPPORTED, - ILLEGAL + ILLEGAL, + INAPPROPRIATE_SIGNATURE_TYPE } enum AssetProxyDispatchErrorCodes { @@ -89,6 +90,32 @@ contract MExchangeRichErrors is pure returns (bytes memory); + bytes4 internal constant SIGNATURE_ORDER_VALIDATOR_ERROR_SELECTOR = + bytes4(keccak256("SignatureOrderValidatorError(bytes32,address,bytes,bytes)")); + + function SignatureOrderValidatorError( + bytes32 orderHash, + address signer, + bytes memory signature, + bytes memory errorData + ) + internal + pure + returns (bytes memory); + + bytes4 internal constant SIGNATURE_WALLET_ORDER_VALIDATOR_ERROR_SELECTOR = + bytes4(keccak256("SignatureWalletOrderValidatorError(bytes32,address,bytes,bytes)")); + + function SignatureWalletOrderValidatorError( + bytes32 orderHash, + address wallet, + bytes memory signature, + bytes memory errorData + ) + internal + pure + returns (bytes memory); + bytes4 internal constant ORDER_STATUS_ERROR_SELECTOR = bytes4(keccak256("OrderStatusError(uint8,bytes32)")); diff --git a/contracts/exchange/contracts/src/mixins/MSignatureValidator.sol b/contracts/exchange/contracts/src/mixins/MSignatureValidator.sol index b637b18d6e..624c226197 100644 --- a/contracts/exchange/contracts/src/mixins/MSignatureValidator.sol +++ b/contracts/exchange/contracts/src/mixins/MSignatureValidator.sol @@ -19,6 +19,7 @@ pragma solidity ^0.5.5; pragma experimental ABIEncoderV2; +import "@0x/contracts-exchange-libs/contracts/src/LibOrder.sol"; import "../interfaces/ISignatureValidator.sol"; @@ -33,13 +34,32 @@ contract MSignatureValidator is // Allowed signature types. enum SignatureType { - Illegal, // 0x00, default value - Invalid, // 0x01 - EIP712, // 0x02 - EthSign, // 0x03 - Wallet, // 0x04 - Validator, // 0x05 - PreSigned, // 0x06 - NSignatureTypes // 0x07, number of signature types. Always leave at end. + Illegal, // 0x00, default value + Invalid, // 0x01 + EIP712, // 0x02 + EthSign, // 0x03 + Wallet, // 0x04 + Validator, // 0x05 + PreSigned, // 0x06 + OrderValidator, // 0x07 + WalletOrderValidator, // 0x08 + NSignatureTypes // 0x09, number of signature types. Always leave at end. } + + /// @dev Verifies that an order, with provided order hash, has been signed + /// by the given signer. + /// @param order The order. + /// @param orderHash The hash of the order. + /// @param signerAddress Address that should have signed the.Signat given hash. + /// @param signature Proof that the hash has been signed by signer. + /// @return True if the signature is valid for the given hash and signer. + function isValidOrderWithHashSignature( + LibOrder.Order memory order, + bytes32 orderHash, + address signerAddress, + bytes memory signature + ) + internal + view + returns (bool isValid); } diff --git a/contracts/exchange/contracts/test/TestRevertReceiver.sol b/contracts/exchange/contracts/test/TestRevertReceiver.sol index 9a01048c5f..2e2f5b9cc8 100644 --- a/contracts/exchange/contracts/test/TestRevertReceiver.sol +++ b/contracts/exchange/contracts/test/TestRevertReceiver.sol @@ -17,7 +17,9 @@ */ pragma solidity ^0.5.5; +pragma experimental ABIEncoderV2; +import "@0x/contracts-exchange-libs/contracts/src/LibOrder.sol"; import "@0x/contracts-erc20/contracts/src/interfaces/IERC20Token.sol"; @@ -55,4 +57,20 @@ contract TestRevertReceiver { { revert(REVERT_REASON); } + + /// @dev Reverts with `REVERT_REASON`. Intended to be used with `OrderValidator` signature type. + /// @param order The order. + /// @param hash Message hash that is signed. + /// @param signature Proof of signing. + /// @return Validity of order signature. + function isValidOrderSignature( + LibOrder.Order calldata order, + bytes32 hash, + bytes calldata signature + ) + external + returns (bool isValid) + { + revert(REVERT_REASON); + } } diff --git a/contracts/exchange/contracts/test/TestSignatureValidator.sol b/contracts/exchange/contracts/test/TestSignatureValidator.sol index 4f68928ac4..5959708924 100644 --- a/contracts/exchange/contracts/test/TestSignatureValidator.sol +++ b/contracts/exchange/contracts/test/TestSignatureValidator.sol @@ -37,21 +37,4 @@ contract TestSignatureValidator is public LibEIP712ExchangeDomain(chainId, address(0)) {} - - function publicIsValidSignature( - bytes32 hash, - address signer, - bytes memory signature - ) - public - view - returns (bool isValid) - { - isValid = isValidSignature( - hash, - signer, - signature - ); - return isValid; - } } diff --git a/contracts/exchange/contracts/test/TestStaticCallReceiver.sol b/contracts/exchange/contracts/test/TestStaticCallReceiver.sol index 4397051891..38ea50f9d2 100644 --- a/contracts/exchange/contracts/test/TestStaticCallReceiver.sol +++ b/contracts/exchange/contracts/test/TestStaticCallReceiver.sol @@ -17,7 +17,9 @@ */ pragma solidity ^0.5.5; +pragma experimental ABIEncoderV2; +import "@0x/contracts-exchange-libs/contracts/src/LibOrder.sol"; import "@0x/contracts-erc20/contracts/src/interfaces/IERC20Token.sol"; @@ -58,6 +60,23 @@ contract TestStaticCallReceiver { return true; } + /// @dev Updates state and returns true. Intended to be used with `OrderValidator` signature type. + /// @param order The order. + /// @param orderHash The order hash. + /// @param signature Proof of signing. + /// @return Validity of order signature. + function isValidOrderSignature( + LibOrder.Order calldata order, + bytes32 orderHash, + bytes calldata signature + ) + external + returns (bool isValid) + { + updateState(); + return true; + } + /// @dev Approves an ERC20 token to spend tokens from this address. /// @param token Address of ERC20 token. /// @param spender Address that will spend tokens. diff --git a/contracts/exchange/package.json b/contracts/exchange/package.json index 20ff29fe00..79507d5d93 100644 --- a/contracts/exchange/package.json +++ b/contracts/exchange/package.json @@ -34,7 +34,7 @@ "lint-contracts": "solhint -c ../.solhint.json contracts/**/**/**/**/*.sol" }, "config": { - "abis": "./generated-artifacts/@(Exchange|ExchangeWrapper|IAssetProxyDispatcher|IExchange|IExchangeCore|IMatchOrders|ISignatureValidator|ITransactions|IValidator|IWallet|IWrapperFunctions|ReentrantERC20Token|TestAssetProxyDispatcher|TestExchangeInternals|TestRevertReceiver|TestSignatureValidator|TestStaticCallReceiver|Validator|Wallet|Whitelist).json", + "abis": "./generated-artifacts/@(Exchange|ExchangeWrapper|IAssetProxyDispatcher|IExchange|IExchangeCore|IMatchOrders|IOrderValidator|ISignatureValidator|ITransactions|IValidator|IWallet|IWrapperFunctions|ReentrantERC20Token|TestAssetProxyDispatcher|TestExchangeInternals|TestRevertReceiver|TestSignatureValidator|TestStaticCallReceiver|Validator|Wallet|Whitelist).json", "abis:comment": "This list is auto-generated by contracts-gen. Don't edit manually." }, "repository": { diff --git a/contracts/exchange/src/artifacts.ts b/contracts/exchange/src/artifacts.ts index 19c1a66f43..a6bdb3e103 100644 --- a/contracts/exchange/src/artifacts.ts +++ b/contracts/exchange/src/artifacts.ts @@ -11,6 +11,7 @@ import * as IAssetProxyDispatcher from '../generated-artifacts/IAssetProxyDispat import * as IExchange from '../generated-artifacts/IExchange.json'; import * as IExchangeCore from '../generated-artifacts/IExchangeCore.json'; import * as IMatchOrders from '../generated-artifacts/IMatchOrders.json'; +import * as IOrderValidator from '../generated-artifacts/IOrderValidator.json'; import * as ISignatureValidator from '../generated-artifacts/ISignatureValidator.json'; import * as ITransactions from '../generated-artifacts/ITransactions.json'; import * as IValidator from '../generated-artifacts/IValidator.json'; @@ -38,12 +39,13 @@ export const artifacts = { ISignatureValidator: ISignatureValidator as ContractArtifact, ITransactions: ITransactions as ContractArtifact, IValidator: IValidator as ContractArtifact, + IOrderValidator: IOrderValidator as ContractArtifact, IWallet: IWallet as ContractArtifact, IWrapperFunctions: IWrapperFunctions as ContractArtifact, ReentrantERC20Token: ReentrantERC20Token as ContractArtifact, TestAssetProxyDispatcher: TestAssetProxyDispatcher as ContractArtifact, TestExchangeInternals: TestExchangeInternals as ContractArtifact, + TestRevertReceiver: TestRevertReceiver as ContractArtifact, TestSignatureValidator: TestSignatureValidator as ContractArtifact, TestStaticCallReceiver: TestStaticCallReceiver as ContractArtifact, - TestRevertReceiver: TestRevertReceiver as ContractArtifact, }; diff --git a/contracts/exchange/src/wrappers.ts b/contracts/exchange/src/wrappers.ts index 3a355d08c3..0c97d94931 100644 --- a/contracts/exchange/src/wrappers.ts +++ b/contracts/exchange/src/wrappers.ts @@ -9,6 +9,7 @@ export * from '../generated-wrappers/i_asset_proxy_dispatcher'; export * from '../generated-wrappers/i_exchange'; export * from '../generated-wrappers/i_exchange_core'; export * from '../generated-wrappers/i_match_orders'; +export * from '../generated-wrappers/i_order_validator'; export * from '../generated-wrappers/i_signature_validator'; export * from '../generated-wrappers/i_transactions'; export * from '../generated-wrappers/i_validator'; diff --git a/contracts/exchange/test/signature_validator.ts b/contracts/exchange/test/signature_validator.ts index 3c9d0677fb..9c6583da1a 100644 --- a/contracts/exchange/test/signature_validator.ts +++ b/contracts/exchange/test/signature_validator.ts @@ -42,6 +42,7 @@ describe('MixinSignatureValidator', () => { let maliciousValidator: TestStaticCallReceiverContract; let revertingWallet: TestRevertReceiverContract; let revertingValidator: TestRevertReceiverContract; + let externalRevertReason: string; let signerAddress: string; let signerPrivateKey: Buffer; let notSignerAddress: string; @@ -88,33 +89,23 @@ describe('MixinSignatureValidator', () => { provider, txDefaults, ); + externalRevertReason = await revertingWallet.REVERT_REASON.callAsync(); + signatureValidatorLogDecoder = new LogDecoder(web3Wrapper, artifacts); - await web3Wrapper.awaitTransactionSuccessAsync( - await signatureValidator.setSignatureValidatorApproval.sendTransactionAsync(testValidator.address, true, { - from: signerAddress, - }), - constants.AWAIT_TRANSACTION_MINED_MS, - ); - await web3Wrapper.awaitTransactionSuccessAsync( - await signatureValidator.setSignatureValidatorApproval.sendTransactionAsync( - maliciousValidator.address, - true, - { - from: signerAddress, - }, - ), - constants.AWAIT_TRANSACTION_MINED_MS, - ); - await web3Wrapper.awaitTransactionSuccessAsync( - await signatureValidator.setSignatureValidatorApproval.sendTransactionAsync( - revertingValidator.address, - true, - { - from: signerAddress, - }, - ), - constants.AWAIT_TRANSACTION_MINED_MS, - ); + const approveValidator = async (validatorAddress: string) => { + type SendApproveTx = (address: string, approved: boolean, txData: { from: string }) => Promise; + const sendTx = async (fn: { sendTransactionAsync: SendApproveTx }) => { + return web3Wrapper.awaitTransactionSuccessAsync( + await fn.sendTransactionAsync(validatorAddress, true, { from: signerAddress }), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + }; + await sendTx(signatureValidator.setSignatureValidatorApproval); + await sendTx(signatureValidator.setOrderValidatorApproval); + }; + await approveValidator(testValidator.address); + await approveValidator(maliciousValidator.address); + await approveValidator(revertingValidator.address); const defaultOrderParams = { ...constants.STATIC_ORDER_PARAMS, @@ -139,13 +130,9 @@ describe('MixinSignatureValidator', () => { await blockchainLifecycle.revertAsync(); }); - describe('isValidSignature', () => { - const REVERT_REASON = 'you shall not pass'; - - beforeEach(async () => { - signedOrder = await orderFactory.newSignedOrderAsync(); - }); + type ValidateCallAsync = (order: SignedOrder, signerAddress: string, ignatureHex: string) => Promise; + const createHashSignatureTests = (validateCallAsync: ValidateCallAsync) => { it('should revert when signature is empty', async () => { const emptySignature = constants.NULL_BYTES; const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); @@ -155,11 +142,7 @@ describe('MixinSignatureValidator', () => { signedOrder.makerAddress, emptySignature, ); - const tx = signatureValidator.publicIsValidSignature.callAsync( - orderHashHex, - signedOrder.makerAddress, - emptySignature, - ); + const tx = validateCallAsync(signedOrder, signedOrder.makerAddress, emptySignature); return expect(tx).to.revertWith(expectedError); }); @@ -173,11 +156,7 @@ describe('MixinSignatureValidator', () => { signedOrder.makerAddress, unsupportedSignatureHex, ); - const tx = signatureValidator.publicIsValidSignature.callAsync( - orderHashHex, - signedOrder.makerAddress, - unsupportedSignatureHex, - ); + const tx = validateCallAsync(signedOrder, signedOrder.makerAddress, unsupportedSignatureHex); return expect(tx).to.revertWith(expectedError); }); @@ -190,22 +169,13 @@ describe('MixinSignatureValidator', () => { signedOrder.makerAddress, illegalSignatureHex, ); - const tx = signatureValidator.publicIsValidSignature.callAsync( - orderHashHex, - signedOrder.makerAddress, - illegalSignatureHex, - ); + const tx = validateCallAsync(signedOrder, signedOrder.makerAddress, illegalSignatureHex); return expect(tx).to.revertWith(expectedError); }); it('should return false when SignatureType=Invalid and signature has a length of zero', async () => { const signatureHex = `0x${Buffer.from([SignatureType.Invalid]).toString('hex')}`; - const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); - const isValidSignature = await signatureValidator.publicIsValidSignature.callAsync( - orderHashHex, - signedOrder.makerAddress, - signatureHex, - ); + const isValidSignature = await validateCallAsync(signedOrder, signedOrder.makerAddress, signatureHex); expect(isValidSignature).to.be.false(); }); @@ -221,11 +191,7 @@ describe('MixinSignatureValidator', () => { signedOrder.makerAddress, signatureHex, ); - const tx = signatureValidator.publicIsValidSignature.callAsync( - orderHashHex, - signedOrder.makerAddress, - signatureHex, - ); + const tx = validateCallAsync(signedOrder, signedOrder.makerAddress, signatureHex); return expect(tx).to.revertWith(expectedError); }); @@ -243,11 +209,7 @@ describe('MixinSignatureValidator', () => { ]); const signatureHex = ethUtil.bufferToHex(signature); // Validate signature - const isValidSignature = await signatureValidator.publicIsValidSignature.callAsync( - orderHashHex, - signerAddress, - signatureHex, - ); + const isValidSignature = await validateCallAsync(signedOrder, signerAddress, signatureHex); expect(isValidSignature).to.be.true(); }); @@ -266,11 +228,7 @@ describe('MixinSignatureValidator', () => { const signatureHex = ethUtil.bufferToHex(signature); // Validate signature. // This will fail because `signerAddress` signed the message, but we're passing in `notSignerAddress` - const isValidSignature = await signatureValidator.publicIsValidSignature.callAsync( - orderHashHex, - notSignerAddress, - signatureHex, - ); + const isValidSignature = await validateCallAsync(signedOrder, notSignerAddress, signatureHex); expect(isValidSignature).to.be.false(); }); @@ -289,11 +247,7 @@ describe('MixinSignatureValidator', () => { ]); const signatureHex = ethUtil.bufferToHex(signature); // Validate signature - const isValidSignature = await signatureValidator.publicIsValidSignature.callAsync( - orderHashHex, - signerAddress, - signatureHex, - ); + const isValidSignature = await validateCallAsync(signedOrder, signerAddress, signatureHex); expect(isValidSignature).to.be.true(); }); @@ -313,11 +267,7 @@ describe('MixinSignatureValidator', () => { const signatureHex = ethUtil.bufferToHex(signature); // Validate signature. // This will fail because `signerAddress` signed the message, but we're passing in `notSignerAddress` - const isValidSignature = await signatureValidator.publicIsValidSignature.callAsync( - orderHashHex, - notSignerAddress, - signatureHex, - ); + const isValidSignature = await validateCallAsync(signedOrder, notSignerAddress, signatureHex); expect(isValidSignature).to.be.false(); }); @@ -335,11 +285,7 @@ describe('MixinSignatureValidator', () => { ]); const signatureHex = ethUtil.bufferToHex(signature); // Validate signature - const isValidSignature = await signatureValidator.publicIsValidSignature.callAsync( - orderHashHex, - testWallet.address, - signatureHex, - ); + const isValidSignature = await validateCallAsync(signedOrder, testWallet.address, signatureHex); expect(isValidSignature).to.be.true(); }); @@ -358,11 +304,7 @@ describe('MixinSignatureValidator', () => { ]); const signatureHex = ethUtil.bufferToHex(signature); // Validate signature - const isValidSignature = await signatureValidator.publicIsValidSignature.callAsync( - orderHashHex, - testWallet.address, - signatureHex, - ); + const isValidSignature = await validateCallAsync(signedOrder, testWallet.address, signatureHex); expect(isValidSignature).to.be.false(); }); @@ -385,11 +327,7 @@ describe('MixinSignatureValidator', () => { signatureHex, constants.NULL_BYTES, ); - const tx = signatureValidator.publicIsValidSignature.callAsync( - orderHashHex, - maliciousWallet.address, - signatureHex, - ); + const tx = validateCallAsync(signedOrder, maliciousWallet.address, signatureHex); return expect(tx).to.revertWith(expectedError); }); @@ -410,13 +348,9 @@ describe('MixinSignatureValidator', () => { orderHashHex, revertingWallet.address, signatureHex, - new StringRevertError(REVERT_REASON).encode(), - ); - const tx = signatureValidator.publicIsValidSignature.callAsync( - orderHashHex, - revertingWallet.address, - signatureHex, + new StringRevertError(externalRevertReason).encode(), ); + const tx = validateCallAsync(signedOrder, revertingWallet.address, signatureHex); return expect(tx).to.revertWith(expectedError); }); @@ -425,12 +359,7 @@ describe('MixinSignatureValidator', () => { const signatureType = ethUtil.toBuffer(`0x${SignatureType.Validator}`); const signature = Buffer.concat([validatorAddress, signatureType]); const signatureHex = ethUtil.bufferToHex(signature); - const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); - const isValidSignature = await signatureValidator.publicIsValidSignature.callAsync( - orderHashHex, - signerAddress, - signatureHex, - ); + const isValidSignature = await validateCallAsync(signedOrder, signerAddress, signatureHex); expect(isValidSignature).to.be.true(); }); @@ -439,14 +368,9 @@ describe('MixinSignatureValidator', () => { const signatureType = ethUtil.toBuffer(`0x${SignatureType.Validator}`); const signature = Buffer.concat([validatorAddress, signatureType]); const signatureHex = ethUtil.bufferToHex(signature); - const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); - // This will return false because we signed the message with `signerAddress`, but - // are validating against `notSignerAddress` - const isValidSignature = await signatureValidator.publicIsValidSignature.callAsync( - orderHashHex, - notSignerAddress, - signatureHex, - ); + // This will return false because the validator requires `signerAddress` + // to be the signer. + const isValidSignature = await validateCallAsync(signedOrder, notSignerAddress, signatureHex); expect(isValidSignature).to.be.false(); }); @@ -462,7 +386,7 @@ describe('MixinSignatureValidator', () => { signatureHex, constants.NULL_BYTES, ); - const tx = signatureValidator.publicIsValidSignature.callAsync(orderHashHex, signerAddress, signatureHex); + const tx = validateCallAsync(signedOrder, signerAddress, signatureHex); return expect(tx).to.revertWith(expectedError); }); @@ -476,9 +400,9 @@ describe('MixinSignatureValidator', () => { orderHashHex, signedOrder.makerAddress, signatureHex, - new StringRevertError(REVERT_REASON).encode(), + new StringRevertError(externalRevertReason).encode(), ); - const tx = signatureValidator.publicIsValidSignature.callAsync(orderHashHex, signerAddress, signatureHex); + const tx = validateCallAsync(signedOrder, signerAddress, signatureHex); return expect(tx).to.revertWith(expectedError); }); @@ -497,12 +421,7 @@ describe('MixinSignatureValidator', () => { const signatureType = ethUtil.toBuffer(`0x${SignatureType.Validator}`); const signature = Buffer.concat([validatorAddress, signatureType]); const signatureHex = ethUtil.bufferToHex(signature); - const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); - const isValidSignature = await signatureValidator.publicIsValidSignature.callAsync( - orderHashHex, - signerAddress, - signatureHex, - ); + const isValidSignature = await validateCallAsync(signedOrder, signerAddress, signatureHex); expect(isValidSignature).to.be.false(); }); @@ -520,24 +439,39 @@ describe('MixinSignatureValidator', () => { // Validate presigned signature const signature = ethUtil.toBuffer(`0x${SignatureType.PreSigned}`); const signatureHex = ethUtil.bufferToHex(signature); - const isValidSignature = await signatureValidator.publicIsValidSignature.callAsync( - orderHashHex, - signedOrder.makerAddress, - signatureHex, - ); + const isValidSignature = await validateCallAsync(signedOrder, signedOrder.makerAddress, signatureHex); expect(isValidSignature).to.be.true(); }); it('should return false when SignatureType=Presigned and signer has not presigned hash', async () => { const signature = ethUtil.toBuffer(`0x${SignatureType.PreSigned}`); const signatureHex = ethUtil.bufferToHex(signature); + const isValidSignature = await validateCallAsync(signedOrder, signedOrder.makerAddress, signatureHex); + expect(isValidSignature).to.be.false(); + }); + }; + + describe('isValidHashSignature', () => { + const validateCallAsync = async (order: SignedOrder, signer: string, signatureHex: string) => { + const orderHashHex = orderHashUtils.getOrderHashHex(order); + return signatureValidator.isValidHashSignature.callAsync(orderHashHex, signer, signatureHex); + }; + + beforeEach(async () => { + signedOrder = await orderFactory.newSignedOrderAsync(); + }); + + it('should revert when SignatureType=OrderValidator', async () => { + const inappropriateSignatureHex = `0x${Buffer.from([SignatureType.OrderValidator]).toString('hex')}`; const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); - const isValidSignature = await signatureValidator.publicIsValidSignature.callAsync( + const expectedError = new ExchangeRevertErrors.SignatureError( + ExchangeRevertErrors.SignatureErrorCode.InappropriateSignatureType, orderHashHex, signedOrder.makerAddress, - signatureHex, + inappropriateSignatureHex, ); - expect(isValidSignature).to.be.false(); + const tx = validateCallAsync(signedOrder, signerAddress, inappropriateSignatureHex); + return expect(tx).to.revertWith(expectedError); }); it('should return true when message was signed by a Trezor One (firmware version 1.6.2)', async () => { @@ -550,7 +484,7 @@ describe('MixinSignatureValidator', () => { const trezorSignatureType = ethUtil.toBuffer(`0x${SignatureType.EthSign}`); const signature = Buffer.concat([v, r, s, trezorSignatureType]); const signatureHex = ethUtil.bufferToHex(signature); - const isValidSignature = await signatureValidator.publicIsValidSignature.callAsync( + const isValidSignature = await signatureValidator.isValidHashSignature.callAsync( messageHash, signer, signatureHex, @@ -568,13 +502,179 @@ describe('MixinSignatureValidator', () => { const trezorSignatureType = ethUtil.toBuffer(`0x${SignatureType.EthSign}`); const signature = Buffer.concat([v, r, s, trezorSignatureType]); const signatureHex = ethUtil.bufferToHex(signature); - const isValidSignature = await signatureValidator.publicIsValidSignature.callAsync( + const isValidSignature = await signatureValidator.isValidHashSignature.callAsync( messageHash, signer, signatureHex, ); expect(isValidSignature).to.be.true(); }); + + createHashSignatureTests(validateCallAsync); + }); + + describe('isValidOrderSignature', () => { + const validateCallAsync = async (order: SignedOrder, signer: string, signatureHex: string) => { + return signatureValidator.isValidOrderSignature.callAsync(order, signer, signatureHex); + }; + + beforeEach(async () => { + signedOrder = await orderFactory.newSignedOrderAsync(); + }); + + it('should return true when SignatureType=OrderValidator, signature is valid and validator is approved', async () => { + const validatorAddress = ethUtil.toBuffer(`${testValidator.address}`); + const signatureType = ethUtil.toBuffer(`0x${SignatureType.OrderValidator}`); + const signature = Buffer.concat([validatorAddress, signatureType]); + const signatureHex = ethUtil.bufferToHex(signature); + const isValidSignature = await validateCallAsync(signedOrder, signerAddress, signatureHex); + expect(isValidSignature).to.be.true(); + }); + + it('should return false when SignatureType=OrderValidator, signature is invalid and validator is approved', async () => { + const validatorAddress = ethUtil.toBuffer(`${testValidator.address}`); + const signatureType = ethUtil.toBuffer(`0x${SignatureType.OrderValidator}`); + const signature = Buffer.concat([validatorAddress, signatureType]); + const signatureHex = ethUtil.bufferToHex(signature); + // This will return false because the validator requires `signerAddress` + // to be the signer. + const isValidSignature = await validateCallAsync(signedOrder, notSignerAddress, signatureHex); + expect(isValidSignature).to.be.false(); + }); + + it('should revert when `isValidOrderSignature` attempts to update state and SignatureType=OrderValidator', async () => { + const validatorAddress = ethUtil.toBuffer(`${maliciousValidator.address}`); + const signatureType = ethUtil.toBuffer(`0x${SignatureType.OrderValidator}`); + const signature = Buffer.concat([validatorAddress, signatureType]); + const signatureHex = ethUtil.bufferToHex(signature); + const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); + const expectedError = new ExchangeRevertErrors.SignatureOrderValidatorError( + orderHashHex, + signedOrder.makerAddress, + signatureHex, + constants.NULL_BYTES, + ); + const tx = validateCallAsync(signedOrder, signerAddress, signatureHex); + return expect(tx).to.revertWith(expectedError); + }); + + it('should revert when `isValidOrderSignature` reverts and SignatureType=OrderValidator', async () => { + const validatorAddress = ethUtil.toBuffer(`${revertingValidator.address}`); + const signatureType = ethUtil.toBuffer(`0x${SignatureType.OrderValidator}`); + const signature = Buffer.concat([validatorAddress, signatureType]); + const signatureHex = ethUtil.bufferToHex(signature); + const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); + const expectedError = new ExchangeRevertErrors.SignatureOrderValidatorError( + orderHashHex, + signedOrder.makerAddress, + signatureHex, + new StringRevertError(externalRevertReason).encode(), + ); + const tx = validateCallAsync(signedOrder, signerAddress, signatureHex); + return expect(tx).to.revertWith(expectedError); + }); + + it('should return false when SignatureType=OrderValidator, signature is valid and validator is not approved', async () => { + // Set approval of signature validator to false + await web3Wrapper.awaitTransactionSuccessAsync( + await signatureValidator.setOrderValidatorApproval.sendTransactionAsync(testValidator.address, false, { + from: signerAddress, + }), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + // Validate signature + const validatorAddress = ethUtil.toBuffer(`${testValidator.address}`); + const signatureType = ethUtil.toBuffer(`0x${SignatureType.OrderValidator}`); + const signature = Buffer.concat([validatorAddress, signatureType]); + const signatureHex = ethUtil.bufferToHex(signature); + const isValidSignature = await validateCallAsync(signedOrder, signerAddress, signatureHex); + expect(isValidSignature).to.be.false(); + }); + + it('should return true when SignatureType=WalletOrderValidator and signature is valid', async () => { + // Create EIP712 signature + const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); + const orderHashBuffer = ethUtil.toBuffer(orderHashHex); + const ecSignature = ethUtil.ecsign(orderHashBuffer, signerPrivateKey); + // Create 0x signature from EIP712 signature + const signature = Buffer.concat([ + ethUtil.toBuffer(ecSignature.v), + ecSignature.r, + ecSignature.s, + ethUtil.toBuffer(`0x${SignatureType.WalletOrderValidator}`), + ]); + const signatureHex = ethUtil.bufferToHex(signature); + // Validate signature + const isValidSignature = await validateCallAsync(signedOrder, testWallet.address, signatureHex); + expect(isValidSignature).to.be.true(); + }); + + it('should return false when SignatureType=WalletOrderValidator and signature is invalid', async () => { + // Create EIP712 signature using a private key that does not belong to the wallet owner. + const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); + const orderHashBuffer = ethUtil.toBuffer(orderHashHex); + const notWalletOwnerPrivateKey = notSignerPrivateKey; + const ecSignature = ethUtil.ecsign(orderHashBuffer, notWalletOwnerPrivateKey); + // Create 0x signature from EIP712 signature + const signature = Buffer.concat([ + ethUtil.toBuffer(ecSignature.v), + ecSignature.r, + ecSignature.s, + ethUtil.toBuffer(`0x${SignatureType.WalletOrderValidator}`), + ]); + const signatureHex = ethUtil.bufferToHex(signature); + // Validate signature + const isValidSignature = await validateCallAsync(signedOrder, testWallet.address, signatureHex); + expect(isValidSignature).to.be.false(); + }); + + it('should revert when `isValidSignature` attempts to update state and SignatureType=WalletOrderValidator', async () => { + // Create EIP712 signature + const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); + const orderHashBuffer = ethUtil.toBuffer(orderHashHex); + const ecSignature = ethUtil.ecsign(orderHashBuffer, signerPrivateKey); + // Create 0x signature from EIP712 signature + const signature = Buffer.concat([ + ethUtil.toBuffer(ecSignature.v), + ecSignature.r, + ecSignature.s, + ethUtil.toBuffer(`0x${SignatureType.WalletOrderValidator}`), + ]); + const signatureHex = ethUtil.bufferToHex(signature); + const expectedError = new ExchangeRevertErrors.SignatureWalletOrderValidatorError( + orderHashHex, + maliciousWallet.address, + signatureHex, + constants.NULL_BYTES, + ); + const tx = validateCallAsync(signedOrder, maliciousWallet.address, signatureHex); + return expect(tx).to.revertWith(expectedError); + }); + + it('should revert when `isValidSignature` reverts and SignatureType=WalletOrderValidator', async () => { + // Create EIP712 signature + const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); + const orderHashBuffer = ethUtil.toBuffer(orderHashHex); + const ecSignature = ethUtil.ecsign(orderHashBuffer, signerPrivateKey); + // Create 0x signature from EIP712 signature + const signature = Buffer.concat([ + ethUtil.toBuffer(ecSignature.v), + ecSignature.r, + ecSignature.s, + ethUtil.toBuffer(`0x${SignatureType.WalletOrderValidator}`), + ]); + const signatureHex = ethUtil.bufferToHex(signature); + const expectedError = new ExchangeRevertErrors.SignatureWalletOrderValidatorError( + orderHashHex, + revertingWallet.address, + signatureHex, + new StringRevertError(externalRevertReason).encode(), + ); + const tx = validateCallAsync(signedOrder, revertingWallet.address, signatureHex); + return expect(tx).to.revertWith(expectedError); + }); + + createHashSignatureTests(validateCallAsync); }); describe('setSignatureValidatorApproval', () => { diff --git a/contracts/exchange/tsconfig.json b/contracts/exchange/tsconfig.json index 46bfc600bb..736b507cfb 100644 --- a/contracts/exchange/tsconfig.json +++ b/contracts/exchange/tsconfig.json @@ -9,6 +9,7 @@ "generated-artifacts/IExchange.json", "generated-artifacts/IExchangeCore.json", "generated-artifacts/IMatchOrders.json", + "generated-artifacts/IOrderValidator.json", "generated-artifacts/ISignatureValidator.json", "generated-artifacts/ITransactions.json", "generated-artifacts/IValidator.json", diff --git a/packages/order-utils/CHANGELOG.json b/packages/order-utils/CHANGELOG.json index 834da3f590..db712f0423 100644 --- a/packages/order-utils/CHANGELOG.json +++ b/packages/order-utils/CHANGELOG.json @@ -17,6 +17,14 @@ { "note": "Add Exchange `RevertError` types to `ExchangeRevertErrors`", "pr": 1761 + }, + { + "note": "Add `SignatureOrderValidatorError` type to `ExchangeRevertErrors`", + "pr": 1774 + }, + { + "note": "Add `SignatureWalletOrderValidatorError` type to `ExchangeRevertErrors`", + "pr": 1774 } ] }, diff --git a/packages/order-utils/src/exchange_revert_errors.ts b/packages/order-utils/src/exchange_revert_errors.ts index a9e82fefb2..eda39d2021 100644 --- a/packages/order-utils/src/exchange_revert_errors.ts +++ b/packages/order-utils/src/exchange_revert_errors.ts @@ -16,8 +16,7 @@ export enum SignatureErrorCode { InvalidLength, Unsupported, Illegal, - WalletError, - ValidatorError, + InappropriateSignatureType, } export enum AssetProxyDispatchErrorCode { @@ -53,8 +52,19 @@ export class SignatureValidatorError extends RevertError { } export class SignatureWalletError extends RevertError { + constructor(hash?: string, wallet?: string, signature?: string, errorData?: string) { + super('SignatureWalletError(bytes32 hash, address wallet, bytes signature, bytes errorData)', { + hash, + wallet, + signature, + errorData, + }); + } +} + +export class SignatureOrderValidatorError extends RevertError { constructor(hash?: string, signer?: string, signature?: string, errorData?: string) { - super('SignatureWalletError(bytes32 hash, address signer, bytes signature, bytes errorData)', { + super('SignatureOrderValidatorError(bytes32 hash, address signer, bytes signature, bytes errorData)', { hash, signer, signature, @@ -63,6 +73,17 @@ export class SignatureWalletError extends RevertError { } } +export class SignatureWalletOrderValidatorError extends RevertError { + constructor(hash?: string, wallet?: string, signature?: string, errorData?: string) { + super('SignatureWalletOrderValidatorError(bytes32 hash, address wallet, bytes signature, bytes errorData)', { + hash, + wallet, + signature, + errorData, + }); + } +} + export class OrderStatusError extends RevertError { constructor(status?: OrderStatus, orderHash?: string) { super('OrderStatusError(uint8 status, bytes32 orderHash)', { status, orderHash }); @@ -168,6 +189,8 @@ const types = [ SignatureError, SignatureWalletError, SignatureValidatorError, + SignatureOrderValidatorError, + SignatureWalletOrderValidatorError, InvalidSenderError, InvalidTakerError, InvalidMakerError, diff --git a/packages/types/CHANGELOG.json b/packages/types/CHANGELOG.json index 326800ab45..bc0bb3a8c9 100644 --- a/packages/types/CHANGELOG.json +++ b/packages/types/CHANGELOG.json @@ -40,6 +40,14 @@ { "note": "Add `chainId` field to `EIP712DomainWithDefaultSchema`", "pr": 1742 + }, + { + "note": "Add `OrderStatus` type", + "pr": 1761 + }, + { + "note": "Add `SignatureType.OrderValidator` and `SignatureType.WalletOrderValidator`", + "pr": 1774 } ] }, diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 804ea86124..ebd4071a8d 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -161,6 +161,8 @@ export enum SignatureType { Wallet, Validator, PreSigned, + OrderValidator, + WalletOrderValidator, NSignatureTypes, }