Always validate callback signatures & add support for EIP-1271 signatures

[dorothy-zbornak]

Description

This PR addresses ZEIP #33.

Contract Signature Types Always Checked

We now always check any signature type that relies on a contract. Prior to this, we only checked the signature callback on the first fill. Now this occurs on every subsequent fill. The signature types affected are:

• Wallet
• Validator
• EIP1271Wallet (new: see below)

Consolidated Signature Types

• A new signature type, EIP1271Wallet, has been added for wallets that implement the EIP-1271 standard, which take an arbitrary bytes data field along with a signature.
• WalletOrderValidator has been removed since EIP1271Wallet is a much more flexible alternative now.
• There is now only a single Validator signature type for signatures that specify an external validator contract.
• All these signature types also work with ZeroExTransactions, in addition to Orders!

Interface for EIP1271Wallet and Validator contracts:

Both of these signature types follow the EIP-1271 pattern and require a contract that exposes the following callback:

function isValidSignature(
    // ABI-encoded data associated with the signature.
    bytes calldata data,
    // arbitrary signature bytes passed in top-level call.
    bytes calldata signature
)
    external
    view
    // Should return 0x20c13b0b if the signature is valid.
    returns (bytes4 magicValue);

EIP-1271 data Encoding

There are two ways the data parameter for EIP1271Wallet and Validator signatures can be encoded, depending on what type of data is signed:

• For Orders: abi.encode(Order order, bytes32 orderHash)
• For ZeroExTransactions: abi.encode(ZeroExTransaction transaction, bytes32 transactionHash)

Bonus Material

• I consolidated all the various wallet/validator test contracts into a single configurable one and rewrote most of the tests in signature_validator.ts to match.
• I killed a handful of signature tests in in core.ts because I felt like they were already pretty well covered in signature_validator.ts.
• A validatorAddress field has been added to SignatureValidatorError.
• Promises returned by awaitTransactionSuccessAsync() are actually compatible with the base Promise type now (not just PromiseLike).
• assertValidFill() and calculateMatchedFillResults() functions on the Exchange have been made public.

Testing instructions

yarn test:contracts and cross your fingers.

Types of changes

• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• Prefix PR title with [WIP] if necessary.
• Add tests to cover changes as needed.
• Update documentation as needed.
• Add new entries to the relevant CHANGELOG.jsons.

[coveralls]

Coverage decreased (-0.01%) to 78.144% when pulling d85e9c7 on feature/3.0/exchange/always-validate-order-signatures into f04fce7 on 3.0.

[abandeali1]

My main point of feedback is that we have a lot of signature types right now. This makes it pretty difficult to decide which to use as a user and which to support as a relayer. It also adds overhead when duplicating this verification logic in other contracts (like the Coordinator). I'm wondering how to best consolidate these, since it seems like there is a good amount of overlap between a lot of them.

[abandeali1]

I'm wondering if it still makes sense to pass the orderHash in here. Would you blindly trust that the hash corresponds to the data when verifying the signature? I feel like if you only want the hash, you'd probably just use the regular Wallet type. If you want both the hash and the data, you probably won't trust the hash without verifying.

[abandeali1]

I also feel like the Validator type isn't very useful with just a hash, generally speaking.

[dorothy-zbornak]

My arguments for including the hash would be that the hash is a very useful, canonical ID, which is not cheap to compute. For example, a validator could use it to immediately look up the filled state of the order.

Also, wouldn't just gating your callback such that msg.sender == exchange be sufficient to trust that the hash provided is authentic?

[abandeali1]

I suppose it can't hurt. Curious to see how much it gets used.

[abandeali1]

Same as above.

[jalextowle]

Very nice job!

[hysz]

Nice work! Solid test coverage as well. +1

[hysz]

This is out of scope for this PR, but I think it could be good to abstract a lot of this logic into a generic library like libSignatureValidator that can be used across our packages.