diff --git a/contracts/exchange/contracts/src/MixinExchangeCore.sol b/contracts/exchange/contracts/src/MixinExchangeCore.sol index efa8cd6d20..540fc87c0b 100644 --- a/contracts/exchange/contracts/src/MixinExchangeCore.sol +++ b/contracts/exchange/contracts/src/MixinExchangeCore.sol @@ -354,9 +354,15 @@ contract MixinExchangeCore is } } - // Validate Maker signature (check only if first time seen) - if (orderInfo.orderTakerAssetFilledAmount == 0) { - address makerAddress = order.makerAddress; + // Validate either on the first fill or if the signature type requires + // regular validation. + address makerAddress = order.makerAddress; + if (orderInfo.orderTakerAssetFilledAmount == 0 || + doesSignatureRequireRegularValidation( + orderInfo.orderHash, + makerAddress, + signature + )) { if (!_isValidOrderWithHashSignature( order, orderInfo.orderHash, diff --git a/contracts/exchange/contracts/src/MixinSignatureValidator.sol b/contracts/exchange/contracts/src/MixinSignatureValidator.sol index d5899673d7..a2b7c9dd17 100644 --- a/contracts/exchange/contracts/src/MixinSignatureValidator.sol +++ b/contracts/exchange/contracts/src/MixinSignatureValidator.sol @@ -31,13 +31,13 @@ import "./MixinTransactions.sol"; import "./MixinExchangeRichErrors.sol"; -contract MixinSignatureValidator is - MixinExchangeRichErrors, - ReentrancyGuard, - LibOrder, +contract MixinSignatureValidator is + MixinExchangeRichErrors, + ReentrancyGuard, + LibOrder, ISignatureValidator, MixinTransactions -{ +{ using LibBytes for bytes; // Mapping of hash => signer => signed @@ -161,13 +161,41 @@ contract MixinSignatureValidator is ); } + /// @dev Checks if a signature is of a type that should be verified for + /// every subsequent fill. + /// @param orderHash The hash of the order. + /// @param signerAddress The address of the signer. + /// @param signature The signature for `orderHash`. + /// @return needsRegularValidation True if the signature should be validated + /// for every operation. + function doesSignatureRequireRegularValidation( + bytes32 orderHash, + address signerAddress, + bytes memory signature + ) + public + pure + returns (bool needsRegularValidation) + { + SignatureType signatureType = _readValidSignatureType( + orderHash, + signerAddress, + signature + ); + // Only signature types that take a full order should be validated + // regularly. + return + signatureType == SignatureType.OrderValidator || + signatureType == SignatureType.WalletOrderValidator; + } + /// @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. + /// @return isValid True if the signature is valid for the given hash and signer. function _isValidOrderWithHashSignature( Order memory order, bytes32 orderHash, diff --git a/contracts/exchange/contracts/src/interfaces/ISignatureValidator.sol b/contracts/exchange/contracts/src/interfaces/ISignatureValidator.sol index aa0d04d6cd..ca51182337 100644 --- a/contracts/exchange/contracts/src/interfaces/ISignatureValidator.sol +++ b/contracts/exchange/contracts/src/interfaces/ISignatureValidator.sol @@ -96,4 +96,20 @@ contract ISignatureValidator { public view returns (bool isValid); + + /// @dev Checks if a signature is of a type that should be verified for + /// every subsequent fill. + /// @param orderHash The hash of the order. + /// @param signerAddress The address of the signer. + /// @param signature The signature for `orderHash`. + /// @return needsRegularValidation True if the signature should be validated + /// for every operation. + function doesSignatureRequireRegularValidation( + bytes32 orderHash, + address signerAddress, + bytes memory signature + ) + public + pure + returns (bool needsRegularValidation); } diff --git a/contracts/exchange/test/signature_validator.ts b/contracts/exchange/test/signature_validator.ts index 80eeca0b8c..fd8f5d9103 100644 --- a/contracts/exchange/test/signature_validator.ts +++ b/contracts/exchange/test/signature_validator.ts @@ -13,7 +13,7 @@ import { assetDataUtils, ExchangeRevertErrors, orderHashUtils, signatureUtils } import { SignatureType, SignedOrder } from '@0x/types'; import { BigNumber, providerUtils, StringRevertError } from '@0x/utils'; import * as chai from 'chai'; -import { LogWithDecodedArgs } from 'ethereum-types'; +import { LogWithDecodedArgs, TransactionReceiptWithDecodedLogs } from 'ethereum-types'; import ethUtil = require('ethereumjs-util'); import { @@ -31,7 +31,7 @@ const expect = chai.expect; const blockchainLifecycle = new BlockchainLifecycle(web3Wrapper); // tslint:disable:no-unnecessary-type-assertion -describe('MixinSignatureValidator', () => { +describe.only('MixinSignatureValidator', () => { let chainId: number; let signedOrder: SignedOrder; let orderFactory: OrderFactory; @@ -93,12 +93,9 @@ describe('MixinSignatureValidator', () => { signatureValidatorLogDecoder = new LogDecoder(web3Wrapper, artifacts); 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, - ); + type SendApproveTx = (address: string, approved: boolean, txData: { from: string }) => Promise; + const sendTx = async (fn: { awaitTransactionSuccessAsync: SendApproveTx }) => { + return fn.awaitTransactionSuccessAsync(validatorAddress, true, { from: signerAddress }) as Promise; }; await sendTx(signatureValidator.setSignatureValidatorApproval); await sendTx(signatureValidator.setOrderValidatorApproval); @@ -410,13 +407,10 @@ describe('MixinSignatureValidator', () => { 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( - await signatureValidator.setSignatureValidatorApproval.sendTransactionAsync( - testValidator.address, - false, - { from: signerAddress }, - ), - constants.AWAIT_TRANSACTION_MINED_MS, + await signatureValidator.setSignatureValidatorApproval.awaitTransactionSuccessAsync( + testValidator.address, + false, + { from: signerAddress }, ); // Validate signature const validatorAddress = ethUtil.toBuffer(`${testValidator.address}`); @@ -430,10 +424,7 @@ describe('MixinSignatureValidator', () => { it('should return true when SignatureType=Presigned and signer has presigned hash', async () => { // Presign hash const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); - await web3Wrapper.awaitTransactionSuccessAsync( - await signatureValidator.preSign.sendTransactionAsync(orderHashHex, { from: signedOrder.makerAddress }), - constants.AWAIT_TRANSACTION_MINED_MS, - ); + await signatureValidator.preSign.awaitTransactionSuccessAsync(orderHashHex, { from: signedOrder.makerAddress }); // Validate presigned signature const signature = ethUtil.toBuffer(`0x${SignatureType.PreSigned}`); const signatureHex = ethUtil.bufferToHex(signature); @@ -574,11 +565,10 @@ describe('MixinSignatureValidator', () => { 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, + await signatureValidator.setOrderValidatorApproval.awaitTransactionSuccessAsync( + testValidator.address, + false, + { from: signerAddress }, ); // Validate signature const validatorAddress = ethUtil.toBuffer(`${testValidator.address}`); diff --git a/packages/base-contract/src/index.ts b/packages/base-contract/src/index.ts index a730d8a6e0..59808f9d7d 100644 --- a/packages/base-contract/src/index.ts +++ b/packages/base-contract/src/index.ts @@ -38,19 +38,27 @@ export interface AbiEncoderByFunctionSignature { * `awaitTransactionSuccessAsync()`. * Maybe there's a better place for this. */ -export class PromiseWithTransactionHash implements PromiseLike { +export class PromiseWithTransactionHash implements Promise { public readonly txHashPromise: Promise; private readonly _promise: Promise; constructor(txHashPromise: Promise, promise: Promise) { this.txHashPromise = txHashPromise; this._promise = promise; } + // tslint:disable-next-line:async-suffix public then( - onFulfilled?: (v: T) => TResult | PromiseLike, - onRejected?: (reason: any) => PromiseLike, - ): PromiseLike { + onFulfilled?: (v: T) => TResult | Promise, + onRejected?: (reason: any) => Promise, + ): Promise { return this._promise.then(onFulfilled, onRejected); } + // tslint:disable-next-line:async-suffix + public catch(onRejected?: (reason: any) => Promise): Promise { + return this._promise.catch(onRejected); + } + get [Symbol.toStringTag](): 'Promise' { + return this._promise[Symbol.toStringTag]; + } } export class BaseContract {