diff --git a/contracts/exchange/CHANGELOG.json b/contracts/exchange/CHANGELOG.json index 2582ec0ebb..1e7b6d8db0 100644 --- a/contracts/exchange/CHANGELOG.json +++ b/contracts/exchange/CHANGELOG.json @@ -17,6 +17,10 @@ { "note": "Refactor example contracts that use `executeTransaction`", "pr": 1753 + }, + { + "note": "Add support for `SignatureType.OrderValidator` for orders", + "pr": 0 } ] }, diff --git a/contracts/exchange/contracts/examples/Validator.sol b/contracts/exchange/contracts/examples/Validator.sol index 86c39cd232..d92552139c 100644 --- a/contracts/exchange/contracts/examples/Validator.sol +++ b/contracts/exchange/contracts/examples/Validator.sol @@ -17,15 +17,17 @@ */ pragma solidity ^0.5.5; +pragma experimental ABIEncoderV2; +import "@0x/contracts-exchange-libs/contracts/src/LibOrder.sol"; import "../src/interfaces/IValidator.sol"; -contract Validator is +contract Validator is IValidator { - // 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 +37,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 +55,23 @@ contract Validator is return (signerAddress == VALID_SIGNER); } // solhint-enable no-unused-vars + + // solhint-disable no-unused-vars + /// @dev Verifies that a signature is valid. `signer` must match `VALID_SIGNER`. + /// @param order The order. + /// @param signerAddress Address that should have signed the given hash. + /// @param signature Proof of signing. + /// @return Validity of signature. + function isValidOrder( + LibOrder.Order calldata order, + address signerAddress, + bytes calldata signature + ) + external + view + returns (bool isValid) + { + return (signerAddress == VALID_SIGNER); + } + // solhint-enable no-unused-vars } diff --git a/contracts/exchange/contracts/src/MixinExchangeCore.sol b/contracts/exchange/contracts/src/MixinExchangeCore.sol index b95d547839..fcf3eb1f37 100644 --- a/contracts/exchange/contracts/src/MixinExchangeCore.sol +++ b/contracts/exchange/contracts/src/MixinExchangeCore.sol @@ -64,12 +64,12 @@ contract MixinExchangeCore is address senderAddress = makerAddress == msg.sender ? address(0) : msg.sender; // orderEpoch is initialized to 0, so to cancelUpTo we need salt + 1 - uint256 newOrderEpoch = targetOrderEpoch + 1; + uint256 newOrderEpoch = targetOrderEpoch + 1; uint256 oldOrderEpoch = orderEpoch[makerAddress][senderAddress]; // Ensure orderEpoch is monotonically increasing require( - newOrderEpoch > oldOrderEpoch, + newOrderEpoch > oldOrderEpoch, "INVALID_NEW_ORDER_EPOCH" ); @@ -193,7 +193,7 @@ contract MixinExchangeCore is // Fetch taker address address takerAddress = getCurrentContextAddress(); - + // Assert that the order is fillable by taker assertFillableOrder( order, @@ -201,7 +201,7 @@ contract MixinExchangeCore is takerAddress, signature ); - + // Get amount of takerAsset to fill uint256 remainingTakerAssetAmount = safeSub(order.takerAssetAmount, orderInfo.orderTakerAssetFilledAmount); uint256 takerAssetFilledAmount = min256(takerAssetFillAmount, remainingTakerAssetAmount); @@ -226,7 +226,7 @@ contract MixinExchangeCore is orderInfo.orderTakerAssetFilledAmount, fillResults ); - + // Settle order settleOrder( order, @@ -309,7 +309,7 @@ contract MixinExchangeCore is order.takerAssetData ); } - + /// @dev Validates context for fillOrder. Succeeds or throws. /// @param order to be filled. /// @param orderInfo OrderStatus, orderHash, and amount already filled of order. @@ -329,7 +329,7 @@ contract MixinExchangeCore is orderInfo.orderStatus == uint8(OrderStatus.FILLABLE), "ORDER_UNFILLABLE" ); - + // Validate sender is allowed to fill this order if (order.senderAddress != address(0)) { require( @@ -337,7 +337,7 @@ contract MixinExchangeCore is "INVALID_SENDER" ); } - + // Validate taker is allowed to fill this order if (order.takerAddress != address(0)) { require( @@ -345,11 +345,12 @@ contract MixinExchangeCore is "INVALID_TAKER" ); } - + // Validate Maker signature (check only if first time seen) if (orderInfo.orderTakerAssetFilledAmount == 0) { require( - isValidSignature( + isValidOrderWithHashSignature( + order, orderInfo.orderHash, order.makerAddress, signature @@ -358,7 +359,7 @@ contract MixinExchangeCore is ); } } - + /// @dev Validates context for fillOrder. Succeeds or throws. /// @param order to be filled. /// @param orderInfo OrderStatus, orderHash, and amount already filled of order. @@ -381,7 +382,7 @@ contract MixinExchangeCore is takerAssetFillAmount != 0, "INVALID_TAKER_AMOUNT" ); - + // Make sure taker does not pay more than desired amount // NOTE: This assertion should never fail, it is here // as an extra defence against potential bugs. @@ -389,7 +390,7 @@ contract MixinExchangeCore is takerAssetFilledAmount <= takerAssetFillAmount, "TAKER_OVERPAY" ); - + // Make sure order is not overfilled // NOTE: This assertion should never fail, it is here // as an extra defence against potential bugs. @@ -397,7 +398,7 @@ contract MixinExchangeCore is safeAdd(orderInfo.orderTakerAssetFilledAmount, takerAssetFilledAmount) <= order.takerAssetAmount, "ORDER_OVERFILL" ); - + // Make sure order is filled at acceptable price. // The order has an implied price from the makers perspective: // order price = order.makerAssetAmount / order.takerAssetAmount @@ -417,7 +418,7 @@ contract MixinExchangeCore is // as an extra defence against potential bugs. require( safeMul(makerAssetFilledAmount, order.takerAssetAmount) - <= + <= safeMul(order.makerAssetAmount, takerAssetFilledAmount), "INVALID_FILL_PRICE" ); diff --git a/contracts/exchange/contracts/src/MixinSignatureValidator.sol b/contracts/exchange/contracts/src/MixinSignatureValidator.sol index 66bbdd43fb..b614d42515 100644 --- a/contracts/exchange/contracts/src/MixinSignatureValidator.sol +++ b/contracts/exchange/contracts/src/MixinSignatureValidator.sol @@ -21,6 +21,7 @@ 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 "./interfaces/IWallet.sol"; @@ -29,11 +30,12 @@ import "./interfaces/IValidator.sol"; contract MixinSignatureValidator is ReentrancyGuard, + LibOrder, MSignatureValidator, MTransactions { using LibBytes for bytes; - + // Mapping of hash => signer => signed mapping (bytes32 => mapping (address => bool)) public preSigned; @@ -53,7 +55,7 @@ contract MixinSignatureValidator is { if (signerAddress != msg.sender) { require( - isValidSignature( + isValidHashSignature( hash, signerAddress, signature @@ -83,12 +85,35 @@ contract MixinSignatureValidator is ); } + /// @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 signerAddress Address that should have signed the given hash. + /// @param hash Any 32-byte hash. + /// @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 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 @@ -97,132 +122,70 @@ contract MixinSignatureValidator is view returns (bool isValid) { + SignatureType signatureType = popValidSignatureType(signature); + // Only hash-compatible signature types can be handled by this + // function. require( - signature.length > 0, - "LENGTH_GREATER_THAN_0_REQUIRED" + signatureType != SignatureType.OrderValidator, + "INAPPROPRIATE_SIGNATURE_TYPE" ); - - // Pop last byte off of signature byte array. - uint8 signatureTypeRaw = uint8(signature.popLastByte()); - - // Ensure signature is supported - require( - signatureTypeRaw < uint8(SignatureType.NSignatureTypes), - "SIGNATURE_UNSUPPORTED" + return validateHashSignatureTypes( + signatureType, + hash, + signerAddress, + signature ); + } - 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) { - revert("SIGNATURE_ILLEGAL"); - - // 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) { - require( - signature.length == 0, - "LENGTH_0_REQUIRED" - ); - isValid = false; - return isValid; - - // Signature using EIP712 - } else if (signatureType == SignatureType.EIP712) { - require( - signature.length == 65, - "LENGTH_65_REQUIRED" - ); - 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) { - require( - signature.length == 65, - "LENGTH_65_REQUIRED" - ); - 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; + /// @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 = popValidSignatureType(signature); + if (signatureType == SignatureType.OrderValidator) { + // The entire order is verified by validator contract. - // 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; + // 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" | - // 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 | x | Signature to validate | - // | 0x00 + x | 20 | Address of validator contract | - // | 0x14 + x | 1 | Signature type is always "\x06" | - } else if (signatureType == SignatureType.Validator) { // Pop last 20 bytes off of signature byte array. address validatorAddress = signature.popLast20Bytes(); - + // Ensure signer has approved validator. if (!allowedValidators[signerAddress][validatorAddress]) { return false; } - isValid = isValidValidatorSignature( + isValid = isValidOrderValidatorSignature( validatorAddress, - hash, + order, 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.) - revert("SIGNATURE_UNSUPPORTED"); + // Otherwise, it's one of the hash-compatible signature types. + return validateHashSignatureTypes( + signatureType, + orderHash, + signerAddress, + signature + ); } /// @dev Verifies signature using logic defined by Wallet contract. @@ -322,4 +285,190 @@ contract MixinSignatureValidator is } return isValid; } + + /// @dev Verifies order AND signature via Validator contract. + /// @param validatorAddress Address of validator contract. + /// @param order The order. + /// @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 isValidOrderValidatorSignature( + address validatorAddress, + Order memory order, + address signerAddress, + bytes memory signature + ) + internal + view + returns (bool isValid) + { + bytes memory callData = abi.encodeWithSelector( + IValidator(signerAddress).isValidOrder.selector, + order, + signerAddress, + signature + ); + assembly { + let cdStart := add(callData, 32) + let success := staticcall( + gas, // forward all gas + validatorAddress, // address of Validator contract + cdStart, // pointer to start of input + mload(callData), // length of input + cdStart, // write output over input + 32 // output size is 32 bytes + ) + + switch success + case 0 { + // Revert with `Error("VALIDATOR_ERROR")` + mstore(0, 0x08c379a000000000000000000000000000000000000000000000000000000000) + mstore(32, 0x0000002000000000000000000000000000000000000000000000000000000000) + mstore(64, 0x0000000f56414c494441544f525f4552524f5200000000000000000000000000) + mstore(96, 0) + revert(0, 100) + } + case 1 { + // Signature is valid if call did not revert and returned true + isValid := mload(cdStart) + } + } + return isValid; + } + + /// Pops the `SignatureType` from the end of a signature and validates it. + function popValidSignatureType( + bytes memory signature + ) + private + view + returns (SignatureType signatureType) + { + require( + signature.length > 0, + "LENGTH_GREATER_THAN_0_REQUIRED" + ); + + // Pop last byte off of signature byte array. + uint8 signatureTypeRaw = uint8(signature.popLastByte()); + + // Ensure signature is supported + require( + signatureTypeRaw < uint8(SignatureType.NSignatureTypes), + "SIGNATURE_UNSUPPORTED" + ); + + return SignatureType(signatureTypeRaw); + } + + /// Validates a hash-compatible signature type + /// (anything but `SignatureType.OrderValidator`). + function validateHashSignatureTypes( + SignatureType signatureType, + bytes32 hash, + address signerAddress, + bytes memory signature + ) + private + view + returns (bool isValid) + { + // 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) { + revert("SIGNATURE_ILLEGAL"); + + // 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) { + require( + signature.length == 0, + "LENGTH_0_REQUIRED" + ); + isValid = false; + return isValid; + + // Signature using EIP712 + } else if (signatureType == SignatureType.EIP712) { + require( + signature.length == 65, + "LENGTH_65_REQUIRED" + ); + 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) { + require( + signature.length == 65, + "LENGTH_65_REQUIRED" + ); + 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 = isValidWalletSignature( + 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. + // 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 "\x06" | + } else if (signatureType == SignatureType.Validator) { + // Pop last 20 bytes off of signature byte array. + address validatorAddress = signature.popLast20Bytes(); + + // Ensure signer has approved validator. + if (!allowedValidators[signerAddress][validatorAddress]) { + return false; + } + isValid = isValidValidatorSignature( + validatorAddress, + 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 9995170332..6f426d9d80 100644 --- a/contracts/exchange/contracts/src/MixinTransactions.sol +++ b/contracts/exchange/contracts/src/MixinTransactions.sol @@ -65,7 +65,7 @@ contract MixinTransactions is if (signerAddress != msg.sender) { // Validate signature require( - isValidSignature( + isValidHashSignature( transactionHash, signerAddress, signature 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..df45cfc2dd 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 { @@ -34,4 +37,30 @@ contract IValidator { external view returns (bool isValid); + + /// @param order The order. + /// @param signerAddress Address that should have signed the given order. + /// @param signature Proof of signing. + /// @return Validity of order signature. + function isValidOrderSignature( + LibOrder.Order calldata order, + address signerAddress, + bytes calldata signature + ) + external + view + returns (bool isValid); + + /// @param order The order. + /// @param signerAddress Address that should have signed the given order. + /// @param signature Proof of signing. + /// @return Validity of order. + function isValidOrder( + LibOrder.Order calldata order, + address signerAddress, + bytes calldata signature + ) + external + view + returns (bool isValid); } diff --git a/contracts/exchange/contracts/src/mixins/MSignatureValidator.sol b/contracts/exchange/contracts/src/mixins/MSignatureValidator.sol index 705c795ca7..de6036101d 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"; @@ -40,15 +41,33 @@ contract MSignatureValidator is Wallet, // 0x04 Validator, // 0x05 PreSigned, // 0x06 - NSignatureTypes // 0x07, number of signature types. Always leave at end. + OrderValidator, // 0x07 + NSignatureTypes // 0x08, 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); + /// @dev Verifies signature using logic defined by Wallet contract. /// @param hash Any 32 byte hash. /// @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 the address recovered from the provided signature matches the input signer address. + /// @return True if the validator approves the signature. function isValidWalletSignature( bytes32 hash, address walletAddress, @@ -63,7 +82,7 @@ contract MSignatureValidator is /// @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. + /// @return True if the validator approves the signature. function isValidValidatorSignature( address validatorAddress, bytes32 hash, @@ -73,4 +92,20 @@ contract MSignatureValidator is internal view returns (bool isValid); + + /// @dev Verifies order AND signature using logic defined by Validator contract. + /// @param validatorAddress Address of validator contract. + /// @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 validator approves the order signature. + function isValidOrderValidatorSignature( + address validatorAddress, + LibOrder.Order memory order, + address signerAddress, + bytes memory signature + ) + internal + view + returns (bool isValid); } diff --git a/contracts/exchange/contracts/test/TestSignatureValidator.sol b/contracts/exchange/contracts/test/TestSignatureValidator.sol index 108974bdaa..0ab6b7169c 100644 --- a/contracts/exchange/contracts/test/TestSignatureValidator.sol +++ b/contracts/exchange/contracts/test/TestSignatureValidator.sol @@ -35,21 +35,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..0cd43bf9fe 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"; @@ -43,6 +45,23 @@ contract TestStaticCallReceiver { return true; } + /// @dev Updates state and returns true. Intended to be used with `OrderValidator` signature type. + /// @param order The order. + /// @param signerAddress Address that should have signed the given hash. + /// @param signature Proof of signing. + /// @return Validity of order signature. + function isValidOrder( + LibOrder.Order calldata order, + address signerAddress, + bytes calldata signature + ) + external + returns (bool isValid) + { + updateState(); + return true; + } + /// @dev Updates state and returns true. Intended to be used with `Wallet` signature type. /// @param hash Message hash that is signed. /// @param signature Proof of signing. diff --git a/contracts/exchange/src/artifacts.ts b/contracts/exchange/src/artifacts.ts index 5928b8a43e..86e75f08bb 100644 --- a/contracts/exchange/src/artifacts.ts +++ b/contracts/exchange/src/artifacts.ts @@ -39,9 +39,9 @@ export const artifacts = { IValidator: IValidator as ContractArtifact, IWallet: IWallet as ContractArtifact, IWrapperFunctions: IWrapperFunctions as ContractArtifact, + ReentrantERC20Token: ReentrantERC20Token as ContractArtifact, TestAssetProxyDispatcher: TestAssetProxyDispatcher as ContractArtifact, TestExchangeInternals: TestExchangeInternals as ContractArtifact, TestSignatureValidator: TestSignatureValidator as ContractArtifact, TestStaticCallReceiver: TestStaticCallReceiver as ContractArtifact, - ReentrantERC20Token: ReentrantERC20Token as ContractArtifact, }; diff --git a/contracts/exchange/test/signature_validator.ts b/contracts/exchange/test/signature_validator.ts index ef820d3e6c..4181084e7e 100644 --- a/contracts/exchange/test/signature_validator.ts +++ b/contracts/exchange/test/signature_validator.ts @@ -122,20 +122,13 @@ describe('MixinSignatureValidator', () => { await blockchainLifecycle.revertAsync(); }); - describe('isValidSignature', () => { - 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 = '0x'; - const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); return expectContractCallFailedAsync( - signatureValidator.publicIsValidSignature.callAsync( - orderHashHex, - signedOrder.makerAddress, - emptySignature, - ), + validateCallAsync(signedOrder, signedOrder.makerAddress, emptySignature), RevertReason.LengthGreaterThan0Required, ); }); @@ -143,38 +136,23 @@ describe('MixinSignatureValidator', () => { it('should revert when signature type is unsupported', async () => { const unsupportedSignatureType = SignatureType.NSignatureTypes; const unsupportedSignatureHex = `0x${Buffer.from([unsupportedSignatureType]).toString('hex')}`; - const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); return expectContractCallFailedAsync( - signatureValidator.publicIsValidSignature.callAsync( - orderHashHex, - signedOrder.makerAddress, - unsupportedSignatureHex, - ), + validateCallAsync(signedOrder, signedOrder.makerAddress, unsupportedSignatureHex), RevertReason.SignatureUnsupported, ); }); it('should revert when SignatureType=Illegal', async () => { const unsupportedSignatureHex = `0x${Buffer.from([SignatureType.Illegal]).toString('hex')}`; - const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); return expectContractCallFailedAsync( - signatureValidator.publicIsValidSignature.callAsync( - orderHashHex, - signedOrder.makerAddress, - unsupportedSignatureHex, - ), + validateCallAsync(signedOrder, signedOrder.makerAddress, unsupportedSignatureHex), RevertReason.SignatureIllegal, ); }); 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(); }); @@ -183,13 +161,8 @@ describe('MixinSignatureValidator', () => { const signatureType = ethUtil.toBuffer(`0x${SignatureType.Invalid}`); const signatureBuffer = Buffer.concat([fillerData, signatureType]); const signatureHex = ethUtil.bufferToHex(signatureBuffer); - const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); return expectContractCallFailedAsync( - signatureValidator.publicIsValidSignature.callAsync( - orderHashHex, - signedOrder.makerAddress, - signatureHex, - ), + validateCallAsync(signedOrder, signedOrder.makerAddress, signatureHex), RevertReason.Length0Required, ); }); @@ -208,11 +181,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(); }); @@ -231,11 +200,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(); }); @@ -254,11 +219,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(); }); @@ -278,11 +239,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(); }); @@ -300,11 +257,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(); }); @@ -323,11 +276,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(); }); @@ -345,11 +294,7 @@ describe('MixinSignatureValidator', () => { ]); const signatureHex = ethUtil.bufferToHex(signature); await expectContractCallFailedAsync( - signatureValidator.publicIsValidSignature.callAsync( - orderHashHex, - maliciousWallet.address, - signatureHex, - ), + validateCallAsync(signedOrder, maliciousWallet.address, signatureHex), RevertReason.WalletError, ); }); @@ -359,12 +304,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(); }); @@ -373,14 +313,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, - ); + const isValidSignature = await validateCallAsync(signedOrder, notSignerAddress, signatureHex); expect(isValidSignature).to.be.false(); }); @@ -389,12 +324,12 @@ 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); await expectContractCallFailedAsync( - signatureValidator.publicIsValidSignature.callAsync(orderHashHex, signerAddress, signatureHex), + validateCallAsync(signedOrder, signerAddress, signatureHex), RevertReason.ValidatorError, ); }); + it('should return false when SignatureType=Validator, signature is valid and validator is not approved', async () => { // Set approval of signature validator to false await web3Wrapper.awaitTransactionSuccessAsync( @@ -410,12 +345,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(); }); @@ -433,25 +363,35 @@ 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 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(); }); + }; + + 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')}`; + return expectContractCallFailedAsync( + validateCallAsync(signedOrder, signerAddress, inappropriateSignatureHex), + RevertReason.InappropriateSignatureType, + ); + }); it('should return true when message was signed by a Trezor One (firmware version 1.6.2)', async () => { // messageHash translates to 0x2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b @@ -463,7 +403,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, @@ -481,13 +421,77 @@ 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 we signed the message with `signerAddress`, but + // are validating against `notSignerAddress` + const isValidSignature = await validateCallAsync(signedOrder, notSignerAddress, signatureHex); + expect(isValidSignature).to.be.false(); + }); + + it('should revert when `isValidSignature` 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); + await expectContractCallFailedAsync( + validateCallAsync(signedOrder, signerAddress, signatureHex), + RevertReason.ValidatorError, + ); + }); + + 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.setSignatureValidatorApproval.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(); + }); + + createHashSignatureTests(validateCallAsync); }); describe('setSignatureValidatorApproval', () => { diff --git a/packages/types/CHANGELOG.json b/packages/types/CHANGELOG.json index a392bfba16..c0e96cd0e4 100644 --- a/packages/types/CHANGELOG.json +++ b/packages/types/CHANGELOG.json @@ -13,6 +13,10 @@ { "note": "Add `chainId` field to `EIP712DomainWithDefaultSchema`", "pr": 1742 + }, + { + "note": "Add `SignatureType.OrderValidator`, `RevertReason.InappropriateSignatureType`", + "pr": 0 } ] }, diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 4e52682e1d..7b009bad38 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -156,6 +156,7 @@ export enum SignatureType { Wallet, Validator, PreSigned, + OrderValidator, NSignatureTypes, } @@ -229,6 +230,7 @@ export enum RevertReason { SignatureIllegal = 'SIGNATURE_ILLEGAL', SignatureInvalid = 'SIGNATURE_INVALID', SignatureUnsupported = 'SIGNATURE_UNSUPPORTED', + InappropriateSignatureType = 'INAPPROPRIATE_SIGNATURE_TYPE', TakerOverpay = 'TAKER_OVERPAY', OrderOverfill = 'ORDER_OVERFILL', InvalidFillPrice = 'INVALID_FILL_PRICE',