diff --git a/contracts/exchange/contracts/src/MixinSignatureValidator.sol b/contracts/exchange/contracts/src/MixinSignatureValidator.sol index f80b1ffeb1..f55f6eea82 100644 --- a/contracts/exchange/contracts/src/MixinSignatureValidator.sol +++ b/contracts/exchange/contracts/src/MixinSignatureValidator.sol @@ -239,7 +239,6 @@ contract MixinSignatureValidator is order, orderHash ), - orderHash, signerAddress, signature ); @@ -295,7 +294,6 @@ contract MixinSignatureValidator is transaction, transactionHash ), - transactionHash, signerAddress, signature ); @@ -473,54 +471,54 @@ contract MixinSignatureValidator is view returns (bool) { - (bool isValid, bytes memory returnData) = _staticCallLegacyWallet( - walletAddress, + // Backup length of signature + uint256 signatureLength = signature.length; + // Temporarily remove signatureType byte from end of signature + signature.writeLength(signatureLength - 1); + // Encode the call data. + bytes memory callData = abi.encodeWithSelector( + IWallet(address(0)).isValidSignature.selector, hash, signature ); - if (!isValid) { - // Static call to verifier failed. - LibRichErrors.rrevert(LibExchangeRichErrors.SignatureWalletError( - hash, - walletAddress, - signature, - returnData - )); + // Restore the original signature length + signature.writeLength(signatureLength); + // Static call the verification function. + (bool didSucceed, bytes memory returnData) = walletAddress.staticcall(callData); + // Return the validity of the signature if the call was successful + if (didSucceed && returnData.length == 32) { + return returnData.readBytes4(0) == LEGACY_WALLET_MAGIC_VALUE; } + // Revert if the call was unsuccessful + LibRichErrors.rrevert(LibExchangeRichErrors.SignatureWalletError( + hash, + walletAddress, + signature, + returnData + )); } /// @dev Verifies arbitrary data and a signature via an EIP1271 Wallet /// contract, where the wallet address is also the signer address. /// @param data Arbitrary signed data. - /// @param hash The hash associated with the data. /// @param walletAddress Contract that will verify the data and signature. /// @param signature Proof that the data has been signed by signer. /// @return isValid True if the signature is validated by the Wallet. function _validateBytesWithWallet( bytes memory data, - bytes32 hash, address walletAddress, bytes memory signature ) private view - returns (bool) + returns (bool isValid) { - (bool isValid, bytes memory returnData) = _staticCallEIP1271WalletWithReducedSignatureLength( + isValid = _staticCallEIP1271WalletWithReducedSignatureLength( walletAddress, data, signature, 1 // The last byte of the signature (signatureType) is removed before making the staticcall ); - if (!isValid) { - // Static call to verifier failed. - LibRichErrors.rrevert(LibExchangeRichErrors.SignatureWalletError( - hash, - walletAddress, - signature, - returnData - )); - } return isValid; } @@ -539,7 +537,7 @@ contract MixinSignatureValidator is ) private view - returns (bool) + returns (bool isValid) { uint256 signatureLength = signature.length; if (signatureLength < 21) { @@ -560,80 +558,36 @@ contract MixinSignatureValidator is validatorAddress )); } - (bool isValid, bytes memory returnData) = _staticCallEIP1271WalletWithReducedSignatureLength( + isValid = _staticCallEIP1271WalletWithReducedSignatureLength( validatorAddress, data, signature, 21 // The last 21 bytes of the signature (validatorAddress + signatureType) are removed before making the staticcall ); - if (!isValid) { - // Static call to verifier failed. - LibRichErrors.rrevert(LibExchangeRichErrors.SignatureValidatorError( - hash, - signerAddress, - validatorAddress, - signature, - returnData - )); - } return isValid; } - /// @dev Performs a staticcall to `Wallet.isValidSignature` and validates the output. - /// @param walletAddress Address of Wallet contract. - /// @param hash Any 32 byte hash. - /// @param signature Proof that the hash has been signed by signer. The last byte will be temporarily - /// popped off of the signature before calling `Wallet.isValidSignature`. - /// @return The validity of the signature and the raw returnData of the call. - function _staticCallLegacyWallet( - address walletAddress, - bytes32 hash, - bytes memory signature - ) - private - view - returns (bool isValid, bytes memory) - { - // Backup length of signature - uint256 signatureLength = signature.length; - // Temporarily remove signatureType byte from end of signature - signature.writeLength(signatureLength - 1); - // Encode the call data. - bytes memory callData = abi.encodeWithSelector( - IWallet(address(0)).isValidSignature.selector, - hash, - signature - ); - // Restore the original signature length - signature.writeLength(signatureLength); - // Static call the verification function. - (bool didSucceed, bytes memory returnData) = walletAddress.staticcall(callData); - // Ensure that the call was successful and that the correct value was returned - isValid = didSucceed && returnData.length <= 32 && returnData.readBytes4(0) == LEGACY_WALLET_MAGIC_VALUE; - return (isValid, returnData); - } - /// @dev Performs a staticcall to and EIP11271 compiant `isValidSignature` function and validates the output. - /// @param staticCallTargetAddress Address of EIP1271Wallet or Validator contract. + /// @param verifyingContractAddress Address of EIP1271Wallet or Validator contract. /// @param data Arbitrary signed data. /// @param signature Proof that the hash has been signed by signer. Bytes will be temporarily be popped /// off of the signature before calling `isValidSignature`. - /// @param ignoredSignatureByteLen The amount of bytes that will be temporarily popped off the the signature. + /// @param ignoredSignatureBytesLen The amount of bytes that will be temporarily popped off the the signature. /// @return The validity of the signature and the raw returnData of the call. function _staticCallEIP1271WalletWithReducedSignatureLength( - address staticCallTargetAddress, + address verifyingContractAddress, bytes memory data, bytes memory signature, - uint256 ignoredSignatureByteLen + uint256 ignoredSignatureBytesLen ) private view - returns (bool isValid, bytes memory) + returns (bool isValid) { // Backup length of the signature uint256 signatureLength = signature.length; // Temporarily remove bytes from signature end - signature.writeLength(signatureLength - ignoredSignatureByteLen); + signature.writeLength(signatureLength - ignoredSignatureBytesLen); bytes memory callData = abi.encodeWithSelector( IEIP1271Wallet(address(0)).isValidSignature.selector, data, @@ -642,9 +596,17 @@ contract MixinSignatureValidator is // Restore original signature length signature.writeLength(signatureLength); // Static call the verification function - (bool didSucceed, bytes memory returnData) = staticCallTargetAddress.staticcall(callData); - // Ensure that the call was successful and that the correct value was returned - isValid = didSucceed && returnData.length <= 32 && returnData.readBytes4(0) == EIP1271_MAGIC_VALUE; - return (isValid, returnData); + (bool didSucceed, bytes memory returnData) = verifyingContractAddress.staticcall(callData); + // Return the validity of the signature if the call was successful + if (didSucceed && returnData.length == 32) { + return returnData.readBytes4(0) == EIP1271_MAGIC_VALUE; + } + // Revert if the call was unsuccessful + LibRichErrors.rrevert(LibExchangeRichErrors.EIP1271SignatureError( + verifyingContractAddress, + data, + signature, + returnData + )); } } diff --git a/contracts/exchange/test/signature_validator.ts b/contracts/exchange/test/signature_validator.ts index 3f62ccec4a..9ebd9ae248 100644 --- a/contracts/exchange/test/signature_validator.ts +++ b/contracts/exchange/test/signature_validator.ts @@ -18,12 +18,13 @@ import { transactionHashUtils, } from '@0x/order-utils'; import { SignatureType, SignedOrder, SignedZeroExTransaction } from '@0x/types'; -import { BigNumber, LibBytesRevertErrors, StringRevertError } from '@0x/utils'; +import { BigNumber, StringRevertError } from '@0x/utils'; import { LogWithDecodedArgs } from 'ethereum-types'; import ethUtil = require('ethereumjs-util'); import { artifacts, + IEIP1271DataContract, TestSignatureValidatorContract, TestSignatureValidatorSignatureValidatorApprovalEventArgs, TestValidatorWalletContract, @@ -41,6 +42,7 @@ blockchainTests.resets('MixinSignatureValidator', env => { let signerPrivateKey: Buffer; let notSignerAddress: string; + const eip1271Data = new IEIP1271DataContract(constants.NULL_ADDRESS, env.provider, env.txDefaults); before(async () => { chainId = await env.getChainIdAsync(); const accounts = await env.getAccountAddressesAsync(); @@ -241,10 +243,11 @@ blockchainTests.resets('MixinSignatureValidator', env => { it('should revert when signer is an EOA and SignatureType=Wallet', async () => { const hashHex = getCurrentHashHex(); const signatureHex = hexConcat(SignatureType.Wallet); - const expectedError = new LibBytesRevertErrors.InvalidByteOperationError( - LibBytesRevertErrors.InvalidByteOperationErrorCodes.LengthGreaterThanOrEqualsFourRequired, - new BigNumber(0), - new BigNumber(4), + const expectedError = new ExchangeRevertErrors.SignatureWalletError( + hashHex, + signerAddress, + signatureHex, + constants.NULL_BYTES, ); const tx = validateAsync(hashHex, signerAddress, signatureHex); return expect(tx).to.revertWith(expectedError); @@ -263,12 +266,13 @@ blockchainTests.resets('MixinSignatureValidator', env => { }); it('should revert when validator returns nothing and SignatureType=Wallet', async () => { - const hashHex = getCurrentHashHex(); + const hashHex = getCurrentHashHex(validatorWallet.address); const signatureHex = hexConcat(SignatureType.Wallet); - const expectedError = new LibBytesRevertErrors.InvalidByteOperationError( - LibBytesRevertErrors.InvalidByteOperationErrorCodes.LengthGreaterThanOrEqualsFourRequired, - new BigNumber(0), - new BigNumber(4), + const expectedError = new ExchangeRevertErrors.SignatureWalletError( + hashHex, + validatorWallet.address, + signatureHex, + constants.NULL_BYTES, ); const tx = validateAsync( hashHex, @@ -525,10 +529,13 @@ blockchainTests.resets('MixinSignatureValidator', env => { it('should revert when validator returns nothing and SignatureType=Validator', async () => { const signatureDataHex = generateRandomSignature(); const signatureHex = hexConcat(signatureDataHex, validatorWallet.address, SignatureType.Validator); - const expectedError = new LibBytesRevertErrors.InvalidByteOperationError( - LibBytesRevertErrors.InvalidByteOperationErrorCodes.LengthGreaterThanOrEqualsFourRequired, - new BigNumber(0), - new BigNumber(4), + const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); + const data = eip1271Data.OrderWithHash.getABIEncodedTransactionData(signedOrder, orderHashHex); + const expectedError = new ExchangeRevertErrors.EIP1271SignatureError( + validatorWallet.address, + data, + signatureHex, + constants.NULL_BYTES, ); const tx = validateAsync(signedOrder, signatureHex, ValidatorWalletAction.ReturnNothing, signatureDataHex); return expect(tx).to.revertWith(expectedError); @@ -540,10 +547,10 @@ blockchainTests.resets('MixinSignatureValidator', env => { const signatureDataHex = generateRandomSignature(); const signatureHex = hexConcat(signatureDataHex, validatorWallet.address, SignatureType.Validator); const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); - const expectedError = new ExchangeRevertErrors.SignatureValidatorError( - orderHashHex, - signedOrder.makerAddress, + const data = eip1271Data.OrderWithHash.getABIEncodedTransactionData(signedOrder, orderHashHex); + const expectedError = new ExchangeRevertErrors.EIP1271SignatureError( validatorWallet.address, + data, signatureHex, constants.NULL_BYTES, ); @@ -557,10 +564,10 @@ blockchainTests.resets('MixinSignatureValidator', env => { const signatureDataHex = generateRandomSignature(); const signatureHex = hexConcat(signatureDataHex, validatorWallet.address, SignatureType.Validator); const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); - const expectedError = new ExchangeRevertErrors.SignatureValidatorError( - orderHashHex, - signedOrder.makerAddress, + const data = eip1271Data.OrderWithHash.getABIEncodedTransactionData(signedOrder, orderHashHex); + const expectedError = new ExchangeRevertErrors.EIP1271SignatureError( validatorWallet.address, + data, signatureHex, new StringRevertError(validatorWalletRevertReason).encode(), ); @@ -638,10 +645,13 @@ blockchainTests.resets('MixinSignatureValidator', env => { signedOrder.makerAddress = validatorWallet.address; const signatureDataHex = generateRandomSignature(); const signatureHex = hexConcat(signatureDataHex, SignatureType.EIP1271Wallet); - const expectedError = new LibBytesRevertErrors.InvalidByteOperationError( - LibBytesRevertErrors.InvalidByteOperationErrorCodes.LengthGreaterThanOrEqualsFourRequired, - new BigNumber(0), - new BigNumber(4), + const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); + const data = eip1271Data.OrderWithHash.getABIEncodedTransactionData(signedOrder, orderHashHex); + const expectedError = new ExchangeRevertErrors.EIP1271SignatureError( + validatorWallet.address, + data, + signatureHex, + constants.NULL_BYTES, ); const tx = validateAsync(signedOrder, signatureHex, ValidatorWalletAction.ReturnNothing, signatureDataHex); return expect(tx).to.revertWith(expectedError); @@ -653,9 +663,10 @@ blockchainTests.resets('MixinSignatureValidator', env => { // just does a hash comparison. const signatureHex = hexConcat(generateRandomSignature(), SignatureType.EIP1271Wallet); const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); - const expectedError = new ExchangeRevertErrors.SignatureWalletError( - orderHashHex, + const data = eip1271Data.OrderWithHash.getABIEncodedTransactionData(signedOrder, orderHashHex); + const expectedError = new ExchangeRevertErrors.EIP1271SignatureError( validatorWallet.address, + data, signatureHex, constants.NULL_BYTES, ); @@ -667,9 +678,10 @@ blockchainTests.resets('MixinSignatureValidator', env => { signedOrder.makerAddress = validatorWallet.address; const signatureHex = hexConcat(SignatureType.EIP1271Wallet); const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); - const expectedError = new ExchangeRevertErrors.SignatureWalletError( - orderHashHex, + const data = eip1271Data.OrderWithHash.getABIEncodedTransactionData(signedOrder, orderHashHex); + const expectedError = new ExchangeRevertErrors.EIP1271SignatureError( validatorWallet.address, + data, signatureHex, new StringRevertError(validatorWalletRevertReason).encode(), ); @@ -679,10 +691,14 @@ blockchainTests.resets('MixinSignatureValidator', env => { it('should revert when signer is an EOA and SignatureType=EIP1271Wallet', async () => { const signatureHex = hexConcat(SignatureType.EIP1271Wallet); - const expectedError = new LibBytesRevertErrors.InvalidByteOperationError( - LibBytesRevertErrors.InvalidByteOperationErrorCodes.LengthGreaterThanOrEqualsFourRequired, - new BigNumber(0), - new BigNumber(4), + signedOrder.makerAddress = notSignerAddress; + const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); + const data = eip1271Data.OrderWithHash.getABIEncodedTransactionData(signedOrder, orderHashHex); + const expectedError = new ExchangeRevertErrors.EIP1271SignatureError( + notSignerAddress, + data, + signatureHex, + constants.NULL_BYTES, ); const tx = signatureValidator.isValidOrderSignature.callAsync(signedOrder, signatureHex); return expect(tx).to.revertWith(expectedError); @@ -690,10 +706,13 @@ blockchainTests.resets('MixinSignatureValidator', env => { it('should revert when signer is an EOA and SignatureType=Validator', async () => { const signatureHex = hexConcat(notSignerAddress, SignatureType.Validator); - const expectedError = new LibBytesRevertErrors.InvalidByteOperationError( - LibBytesRevertErrors.InvalidByteOperationErrorCodes.LengthGreaterThanOrEqualsFourRequired, - new BigNumber(0), - new BigNumber(4), + const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); + const data = eip1271Data.OrderWithHash.getABIEncodedTransactionData(signedOrder, orderHashHex); + const expectedError = new ExchangeRevertErrors.EIP1271SignatureError( + notSignerAddress, + data, + signatureHex, + constants.NULL_BYTES, ); // Register an EOA as a validator. await signatureValidator.setSignatureValidatorApproval.awaitTransactionSuccessAsync( @@ -821,10 +840,16 @@ blockchainTests.resets('MixinSignatureValidator', env => { it('should revert when validator returns nothing and SignatureType=Validator', async () => { const signatureDataHex = generateRandomSignature(); const signatureHex = hexConcat(signatureDataHex, validatorWallet.address, SignatureType.Validator); - const expectedError = new LibBytesRevertErrors.InvalidByteOperationError( - LibBytesRevertErrors.InvalidByteOperationErrorCodes.LengthGreaterThanOrEqualsFourRequired, - new BigNumber(0), - new BigNumber(4), + const transactionHashHex = transactionHashUtils.getTransactionHashHex(signedTransaction); + const data = eip1271Data.ZeroExTransactionWithHash.getABIEncodedTransactionData( + signedTransaction, + transactionHashHex, + ); + const expectedError = new ExchangeRevertErrors.EIP1271SignatureError( + validatorWallet.address, + data, + signatureHex, + constants.NULL_BYTES, ); const tx = validateAsync( signedTransaction, @@ -841,10 +866,13 @@ blockchainTests.resets('MixinSignatureValidator', env => { const signatureDataHex = generateRandomSignature(); const signatureHex = hexConcat(signatureDataHex, validatorWallet.address, SignatureType.Validator); const transactionHashHex = transactionHashUtils.getTransactionHashHex(signedTransaction); - const expectedError = new ExchangeRevertErrors.SignatureValidatorError( + const data = eip1271Data.ZeroExTransactionWithHash.getABIEncodedTransactionData( + signedTransaction, transactionHashHex, - signedTransaction.signerAddress, + ); + const expectedError = new ExchangeRevertErrors.EIP1271SignatureError( validatorWallet.address, + data, signatureHex, constants.NULL_BYTES, ); @@ -858,10 +886,13 @@ blockchainTests.resets('MixinSignatureValidator', env => { const signatureDataHex = generateRandomSignature(); const signatureHex = hexConcat(signatureDataHex, validatorWallet.address, SignatureType.Validator); const transactionHashHex = transactionHashUtils.getTransactionHashHex(signedTransaction); - const expectedError = new ExchangeRevertErrors.SignatureValidatorError( + const data = eip1271Data.ZeroExTransactionWithHash.getABIEncodedTransactionData( + signedTransaction, transactionHashHex, - signedTransaction.signerAddress, + ); + const expectedError = new ExchangeRevertErrors.EIP1271SignatureError( validatorWallet.address, + data, signatureHex, new StringRevertError(validatorWalletRevertReason).encode(), ); @@ -939,10 +970,16 @@ blockchainTests.resets('MixinSignatureValidator', env => { signedTransaction.signerAddress = validatorWallet.address; const signatureDataHex = generateRandomSignature(); const signatureHex = hexConcat(signatureDataHex, SignatureType.EIP1271Wallet); - const expectedError = new LibBytesRevertErrors.InvalidByteOperationError( - LibBytesRevertErrors.InvalidByteOperationErrorCodes.LengthGreaterThanOrEqualsFourRequired, - new BigNumber(0), - new BigNumber(4), + const transactionHashHex = transactionHashUtils.getTransactionHashHex(signedTransaction); + const data = eip1271Data.ZeroExTransactionWithHash.getABIEncodedTransactionData( + signedTransaction, + transactionHashHex, + ); + const expectedError = new ExchangeRevertErrors.EIP1271SignatureError( + validatorWallet.address, + data, + signatureHex, + constants.NULL_BYTES, ); const tx = validateAsync( signedTransaction, @@ -959,9 +996,13 @@ blockchainTests.resets('MixinSignatureValidator', env => { // just does a hash comparison. const signatureHex = hexConcat(generateRandomSignature(), SignatureType.EIP1271Wallet); const transactionHashHex = transactionHashUtils.getTransactionHashHex(signedTransaction); - const expectedError = new ExchangeRevertErrors.SignatureWalletError( + const data = eip1271Data.ZeroExTransactionWithHash.getABIEncodedTransactionData( + signedTransaction, transactionHashHex, + ); + const expectedError = new ExchangeRevertErrors.EIP1271SignatureError( validatorWallet.address, + data, signatureHex, constants.NULL_BYTES, ); @@ -975,9 +1016,13 @@ blockchainTests.resets('MixinSignatureValidator', env => { // just does a hash comparison. const signatureHex = hexConcat(generateRandomSignature(), SignatureType.EIP1271Wallet); const transactionHashHex = transactionHashUtils.getTransactionHashHex(signedTransaction); - const expectedError = new ExchangeRevertErrors.SignatureWalletError( + const data = eip1271Data.ZeroExTransactionWithHash.getABIEncodedTransactionData( + signedTransaction, transactionHashHex, + ); + const expectedError = new ExchangeRevertErrors.EIP1271SignatureError( validatorWallet.address, + data, signatureHex, new StringRevertError(validatorWalletRevertReason).encode(), ); @@ -987,10 +1032,16 @@ blockchainTests.resets('MixinSignatureValidator', env => { it('should revert when signer is an EOA and SignatureType=EIP1271Wallet', async () => { const signatureHex = hexConcat(SignatureType.EIP1271Wallet); - const expectedError = new LibBytesRevertErrors.InvalidByteOperationError( - LibBytesRevertErrors.InvalidByteOperationErrorCodes.LengthGreaterThanOrEqualsFourRequired, - new BigNumber(0), - new BigNumber(4), + const transactionHashHex = transactionHashUtils.getTransactionHashHex(signedTransaction); + const data = eip1271Data.ZeroExTransactionWithHash.getABIEncodedTransactionData( + signedTransaction, + transactionHashHex, + ); + const expectedError = new ExchangeRevertErrors.EIP1271SignatureError( + signedTransaction.signerAddress, + data, + signatureHex, + constants.NULL_BYTES, ); const tx = signatureValidator.isValidTransactionSignature.callAsync(signedTransaction, signatureHex); return expect(tx).to.revertWith(expectedError); @@ -998,10 +1049,16 @@ blockchainTests.resets('MixinSignatureValidator', env => { it('should revert when signer is an EOA and SignatureType=Validator', async () => { const signatureHex = hexConcat(notSignerAddress, SignatureType.Validator); - const expectedError = new LibBytesRevertErrors.InvalidByteOperationError( - LibBytesRevertErrors.InvalidByteOperationErrorCodes.LengthGreaterThanOrEqualsFourRequired, - new BigNumber(0), - new BigNumber(4), + const transactionHashHex = transactionHashUtils.getTransactionHashHex(signedTransaction); + const data = eip1271Data.ZeroExTransactionWithHash.getABIEncodedTransactionData( + signedTransaction, + transactionHashHex, + ); + const expectedError = new ExchangeRevertErrors.EIP1271SignatureError( + notSignerAddress, + data, + signatureHex, + constants.NULL_BYTES, ); // Register an EOA as a validator. await signatureValidator.setSignatureValidatorApproval.awaitTransactionSuccessAsync(