From a0223835b837b9a62f1a9cfbaf19924f3a8b2d0e Mon Sep 17 00:00:00 2001 From: Lawrence Forman Date: Thu, 11 Apr 2019 18:13:55 -0400 Subject: [PATCH] In `@0x/order-utils`: Add `signerAddress` and `signature` to `ExchangeRevertErrors.SignatureError`. In `@0x/contracts-exchange`: Add `signerAddress` and `signature` to `SignatureError` reverts. --- .../contracts/src/MixinExchangeCore.sol | 10 +- .../contracts/src/MixinExchangeRichErrors.sol | 8 +- .../contracts/src/MixinSignatureValidator.sol | 154 +++++++++++------- .../src/mixins/MExchangeRichErrors.sol | 6 +- .../src/mixins/MSignatureValidator.sol | 31 ---- contracts/exchange/test/core.ts | 6 + contracts/exchange/test/match_orders.ts | 4 + .../exchange/test/signature_validator.ts | 16 +- contracts/exchange/test/wrapper.ts | 4 + .../order-utils/src/exchange_revert_errors.ts | 9 +- 10 files changed, 142 insertions(+), 106 deletions(-) diff --git a/contracts/exchange/contracts/src/MixinExchangeCore.sol b/contracts/exchange/contracts/src/MixinExchangeCore.sol index 5d8b12b462..9a1117dae5 100644 --- a/contracts/exchange/contracts/src/MixinExchangeCore.sol +++ b/contracts/exchange/contracts/src/MixinExchangeCore.sol @@ -352,14 +352,16 @@ contract MixinExchangeCore is // Validate Maker signature (check only if first time seen) if (orderInfo.orderTakerAssetFilledAmount == 0) { + address makerAddress = order.makerAddress; if (!isValidSignature( orderInfo.orderHash, - order.makerAddress, - signature - )) { + makerAddress, + signature)) { rrevert(SignatureError( SignatureErrorCodes.BAD_SIGNATURE, - orderInfo.orderHash + orderInfo.orderHash, + makerAddress, + signature )); } } diff --git a/contracts/exchange/contracts/src/MixinExchangeRichErrors.sol b/contracts/exchange/contracts/src/MixinExchangeRichErrors.sol index bfbbec81da..23c77a8985 100644 --- a/contracts/exchange/contracts/src/MixinExchangeRichErrors.sol +++ b/contracts/exchange/contracts/src/MixinExchangeRichErrors.sol @@ -30,7 +30,9 @@ contract MixinExchangeRichErrors is // solhint-disable func-name-mixedcase function SignatureError( SignatureErrorCodes error, - bytes32 orderHash + bytes32 orderHash, + address signer, + bytes memory signature ) internal pure @@ -39,7 +41,9 @@ contract MixinExchangeRichErrors is return abi.encodeWithSelector( SIGNATURE_ERROR_SELECTOR, error, - orderHash + orderHash, + signer, + signature ); } diff --git a/contracts/exchange/contracts/src/MixinSignatureValidator.sol b/contracts/exchange/contracts/src/MixinSignatureValidator.sol index 326fdedb9c..01b97a608d 100644 --- a/contracts/exchange/contracts/src/MixinSignatureValidator.sol +++ b/contracts/exchange/contracts/src/MixinSignatureValidator.sol @@ -57,11 +57,12 @@ contract MixinSignatureValidator is if (!isValidSignature( hash, signerAddress, - signature - )) { + signature)) { rrevert(SignatureError( SignatureErrorCodes.BAD_SIGNATURE, - hash + hash, + signerAddress, + signature )); } } @@ -102,15 +103,25 @@ contract MixinSignatureValidator is returns (bool isValid) { if (signature.length == 0) { - rrevert(SignatureError(SignatureErrorCodes.INVALID_LENGTH, hash)); + rrevert(SignatureError( + SignatureErrorCodes.INVALID_LENGTH, + hash, + signerAddress, + signature + )); } // Pop last byte off of signature byte array. - uint8 signatureTypeRaw = uint8(signature.popLastByte()); + uint8 signatureTypeRaw = uint8(signature[signature.length-1]); // Ensure signature is supported if (signatureTypeRaw >= uint8(SignatureType.NSignatureTypes)) { - rrevert(SignatureError(SignatureErrorCodes.UNSUPPORTED, hash)); + rrevert(SignatureError( + SignatureErrorCodes.UNSUPPORTED, + hash, + signerAddress, + signature + )); } SignatureType signatureType = SignatureType(signatureTypeRaw); @@ -129,7 +140,9 @@ contract MixinSignatureValidator is if (signatureType == SignatureType.Illegal) { rrevert(SignatureError( SignatureErrorCodes.ILLEGAL, - hash + hash, + signerAddress, + signature )); // Always invalid signature. @@ -137,10 +150,12 @@ contract MixinSignatureValidator is // offered explicitly. It can be implicitly created by providing // a correctly formatted but incorrect signature. } else if (signatureType == SignatureType.Invalid) { - if (signature.length != 0) { + if (signature.length != 1) { rrevert(SignatureError( SignatureErrorCodes.INVALID_LENGTH, - hash + hash, + signerAddress, + signature )); } isValid = false; @@ -148,10 +163,12 @@ contract MixinSignatureValidator is // Signature using EIP712 } else if (signatureType == SignatureType.EIP712) { - if (signature.length != 65) { + if (signature.length != 66) { rrevert(SignatureError( SignatureErrorCodes.INVALID_LENGTH, - hash + hash, + signerAddress, + signature )); } v = uint8(signature[0]); @@ -168,10 +185,12 @@ contract MixinSignatureValidator is // Signed using web3.eth_sign } else if (signatureType == SignatureType.EthSign) { - if (signature.length != 65) { + if (signature.length != 66) { rrevert(SignatureError( SignatureErrorCodes.INVALID_LENGTH, - hash + hash, + signerAddress, + signature )); } v = uint8(signature[0]); @@ -207,15 +226,7 @@ contract MixinSignatureValidator is // | 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 @@ -233,7 +244,12 @@ contract MixinSignatureValidator is // 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)); + rrevert(SignatureError( + SignatureErrorCodes.UNSUPPORTED, + hash, + signerAddress, + signature + )); } /// @dev Verifies signature using logic defined by Wallet contract. @@ -247,78 +263,90 @@ contract MixinSignatureValidator is address walletAddress, bytes memory signature ) - internal + 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).isValidSignature.selector, hash, signature ); - bool didSucceed; - assembly { - let cdStart := add(callData, 32) - didSucceed := staticcall( - gas, // forward all gas - walletAddress, // address of Wallet contract - cdStart, // pointer to start of input - mload(callData), // length of input - cdStart, // write output over input - 32 // output size is 32 bytes - ) + // 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) != 0; - if eq(didSucceed, 1) { - // Signature is valid if call did not revert and returned true - isValid := mload(cdStart) - } + // Static call to verifier failed. + // Restore the full signature. + assembly { + mstore(signature, signatureLength) } - if (didSucceed) - return isValid; - rrevert(SignatureError(SignatureErrorCodes.WALLET_ERROR, hash)); + rrevert(SignatureError( + SignatureErrorCodes.WALLET_ERROR, + hash, + walletAddress, + signature + )); } /// @dev Verifies signature using logic defined by Validator contract. - /// @param validatorAddress Address of validator contract. /// @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( - address validatorAddress, bytes32 hash, address signerAddress, bytes memory signature ) - internal + private view returns (bool isValid) { + uint256 signatureLength = signature.length; + // Read the validator address from the signature. + address validatorAddress = signature.readAddress(signatureLength - 21); + // Ensure signer has approved validator. + if (!allowedValidators[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( - IValidator(signerAddress).isValidSignature.selector, + IValidator(validatorAddress).isValidSignature.selector, hash, signerAddress, signature ); - bool didSucceed; - assembly { - let cdStart := add(callData, 32) - didSucceed := 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 - ) + // 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) != 0; - if eq(didSucceed, 1) { - // Signature is valid if call did not revert and returned true - isValid := mload(cdStart) - } + // Static call to verifier failed. + // Restore the full signature. + assembly { + mstore(signature, signatureLength) } - if (didSucceed) - return isValid; - rrevert(SignatureError(SignatureErrorCodes.VALIDATOR_ERROR, hash)); + rrevert(SignatureError( + SignatureErrorCodes.VALIDATOR_ERROR, + hash, + signerAddress, + signature + )); } } diff --git a/contracts/exchange/contracts/src/mixins/MExchangeRichErrors.sol b/contracts/exchange/contracts/src/mixins/MExchangeRichErrors.sol index 2a8c86a453..61cd2c32a0 100644 --- a/contracts/exchange/contracts/src/mixins/MExchangeRichErrors.sol +++ b/contracts/exchange/contracts/src/mixins/MExchangeRichErrors.sol @@ -54,11 +54,13 @@ contract MExchangeRichErrors is // solhint-disable func-name-mixedcase bytes4 internal constant SIGNATURE_ERROR_SELECTOR = - bytes4(keccak256("SignatureError(uint8,bytes32)")); + bytes4(keccak256("SignatureError(uint8,bytes32,address,bytes)")); function SignatureError( SignatureErrorCodes error, - bytes32 orderHash + bytes32 orderHash, + address signer, + bytes memory signature ) internal pure diff --git a/contracts/exchange/contracts/src/mixins/MSignatureValidator.sol b/contracts/exchange/contracts/src/mixins/MSignatureValidator.sol index 705c795ca7..b637b18d6e 100644 --- a/contracts/exchange/contracts/src/mixins/MSignatureValidator.sol +++ b/contracts/exchange/contracts/src/mixins/MSignatureValidator.sol @@ -42,35 +42,4 @@ contract MSignatureValidator is PreSigned, // 0x06 NSignatureTypes // 0x07, number of signature types. Always leave at end. } - - /// @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. - function isValidWalletSignature( - bytes32 hash, - address walletAddress, - bytes memory signature - ) - internal - view - returns (bool isValid); - - /// @dev Verifies signature using logic defined by Validator contract. - /// @param validatorAddress Address of validator contract. - /// @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( - address validatorAddress, - bytes32 hash, - address signerAddress, - bytes memory signature - ) - internal - view - returns (bool isValid); } diff --git a/contracts/exchange/test/core.ts b/contracts/exchange/test/core.ts index 00ef38ca7a..1716592dac 100644 --- a/contracts/exchange/test/core.ts +++ b/contracts/exchange/test/core.ts @@ -265,6 +265,8 @@ describe('Exchange core', () => { const expectedError = new ExchangeRevertErrors.SignatureError( ExchangeRevertErrors.SignatureErrorCode.BadSignature, orderHashHex, + signedOrder.makerAddress, + invalidSigHex, ); const tx = exchangeWrapper.fillOrderAsync(signedOrder, takerAddress); return expect(tx).to.revertWith(expectedError); @@ -299,6 +301,8 @@ describe('Exchange core', () => { const expectedError = new ExchangeRevertErrors.SignatureError( ExchangeRevertErrors.SignatureErrorCode.WalletError, orderHashHex, + signedOrder.makerAddress, + signedOrder.signature, ); const tx = exchangeWrapper.fillOrderAsync(signedOrder, takerAddress); return expect(tx).to.revertWith(expectedError); @@ -316,6 +320,8 @@ describe('Exchange core', () => { const expectedError = new ExchangeRevertErrors.SignatureError( ExchangeRevertErrors.SignatureErrorCode.ValidatorError, orderHashHex, + signedOrder.makerAddress, + signedOrder.signature, ); const tx = exchangeWrapper.fillOrderAsync(signedOrder, takerAddress); return expect(tx).to.revertWith(expectedError); diff --git a/contracts/exchange/test/match_orders.ts b/contracts/exchange/test/match_orders.ts index a3f36f5e7f..75859dc646 100644 --- a/contracts/exchange/test/match_orders.ts +++ b/contracts/exchange/test/match_orders.ts @@ -1191,6 +1191,8 @@ describe('matchOrders', () => { const expectedError = new ExchangeRevertErrors.SignatureError( ExchangeRevertErrors.SignatureErrorCode.BadSignature, orderHashHex, + signedOrderRight.makerAddress, + signedOrderRight.signature, ); // Match orders const tx = exchangeWrapper.matchOrdersAsync(signedOrderLeft, signedOrderRight, takerAddress); @@ -1216,6 +1218,8 @@ describe('matchOrders', () => { const expectedError = new ExchangeRevertErrors.SignatureError( ExchangeRevertErrors.SignatureErrorCode.BadSignature, orderHashHex, + signedOrderRight.makerAddress, + signedOrderRight.signature, ); // Match orders const tx = exchangeWrapper.matchOrdersAsync(signedOrderLeft, signedOrderRight, takerAddress); diff --git a/contracts/exchange/test/signature_validator.ts b/contracts/exchange/test/signature_validator.ts index 7a6ea84b55..47f1543726 100644 --- a/contracts/exchange/test/signature_validator.ts +++ b/contracts/exchange/test/signature_validator.ts @@ -132,6 +132,8 @@ describe('MixinSignatureValidator', () => { const expectedError = new ExchangeRevertErrors.SignatureError( ExchangeRevertErrors.SignatureErrorCode.InvalidLength, orderHashHex, + signedOrder.makerAddress, + emptySignature, ); const tx = signatureValidator.publicIsValidSignature.callAsync( orderHashHex, @@ -148,6 +150,8 @@ describe('MixinSignatureValidator', () => { const expectedError = new ExchangeRevertErrors.SignatureError( ExchangeRevertErrors.SignatureErrorCode.Unsupported, orderHashHex, + signedOrder.makerAddress, + unsupportedSignatureHex, ); const tx = signatureValidator.publicIsValidSignature.callAsync( orderHashHex, @@ -158,16 +162,18 @@ describe('MixinSignatureValidator', () => { }); it('should revert when SignatureType=Illegal', async () => { - const unsupportedSignatureHex = `0x${Buffer.from([SignatureType.Illegal]).toString('hex')}`; + const illegalSignatureHex = `0x${Buffer.from([SignatureType.Illegal]).toString('hex')}`; const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); const expectedError = new ExchangeRevertErrors.SignatureError( ExchangeRevertErrors.SignatureErrorCode.Illegal, orderHashHex, + signedOrder.makerAddress, + illegalSignatureHex, ); const tx = signatureValidator.publicIsValidSignature.callAsync( orderHashHex, signedOrder.makerAddress, - unsupportedSignatureHex, + illegalSignatureHex, ); return expect(tx).to.revertWith(expectedError); }); @@ -192,6 +198,8 @@ describe('MixinSignatureValidator', () => { const expectedError = new ExchangeRevertErrors.SignatureError( ExchangeRevertErrors.SignatureErrorCode.InvalidLength, orderHashHex, + signedOrder.makerAddress, + signatureHex, ); const tx = signatureValidator.publicIsValidSignature.callAsync( orderHashHex, @@ -354,6 +362,8 @@ describe('MixinSignatureValidator', () => { const expectedError = new ExchangeRevertErrors.SignatureError( ExchangeRevertErrors.SignatureErrorCode.WalletError, orderHashHex, + maliciousWallet.address, + signatureHex, ); const tx = signatureValidator.publicIsValidSignature.callAsync( orderHashHex, @@ -402,6 +412,8 @@ describe('MixinSignatureValidator', () => { const expectedError = new ExchangeRevertErrors.SignatureError( ExchangeRevertErrors.SignatureErrorCode.ValidatorError, orderHashHex, + signedOrder.makerAddress, + signatureHex, ); const tx = signatureValidator.publicIsValidSignature.callAsync(orderHashHex, signerAddress, signatureHex); return expect(tx).to.revertWith(expectedError); diff --git a/contracts/exchange/test/wrapper.ts b/contracts/exchange/test/wrapper.ts index 6ba20fcaba..5fa7b0460d 100644 --- a/contracts/exchange/test/wrapper.ts +++ b/contracts/exchange/test/wrapper.ts @@ -835,6 +835,8 @@ describe('Exchange wrappers', () => { const expectedError = new ExchangeRevertErrors.SignatureError( ExchangeRevertErrors.SignatureErrorCode.BadSignature, orderHashHex, + signedOrders[1].makerAddress, + signedOrders[1].signature, ); const tx = exchangeWrapper.marketSellOrdersAsync(signedOrders, takerAddress, { takerAssetFillAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(1000), 18), @@ -1098,6 +1100,8 @@ describe('Exchange wrappers', () => { const expectedError = new ExchangeRevertErrors.SignatureError( ExchangeRevertErrors.SignatureErrorCode.BadSignature, orderHashHex, + signedOrders[1].makerAddress, + signedOrders[1].signature, ); const tx = exchangeWrapper.marketBuyOrdersAsync(signedOrders, takerAddress, { makerAssetFillAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(1000), 18), diff --git a/packages/order-utils/src/exchange_revert_errors.ts b/packages/order-utils/src/exchange_revert_errors.ts index 9792513030..373e29b40c 100644 --- a/packages/order-utils/src/exchange_revert_errors.ts +++ b/packages/order-utils/src/exchange_revert_errors.ts @@ -32,8 +32,13 @@ export enum TransactionErrorCode { } export class SignatureError extends RevertError { - constructor(error?: SignatureErrorCode, orderHash?: string) { - super('SignatureError(uint8 error,bytes32 orderHash)', { error, orderHash }); + constructor(error?: SignatureErrorCode, orderHash?: string, signer?: string, signature?: string) { + super('SignatureError(uint8 error, bytes32 orderHash, address signer, bytes signature)', { + error, + orderHash, + signer, + signature, + }); } }