_assertValidSignature is not compatible with EIP-1271

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/lib/SignatureVerification.sol#L78
https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/lib/SignatureVerification.soll#L91
https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/lib/SignatureVerification.sol#L94

Vulnerability details

Impact

TLDR: Seaport signature validation is not 100% compatible with Gnosis Safe.

The way this function is structured can prevent valid EIP-1271 signatures from validating.

EIP-1271 doesn't mandate any kind of structure, but leaves it completely open for the future. It is ought to be forward-compatible with any kind of signature scheme, but is also utilised by contract wallets (such as Gnosis, Argent, etc.)

There are two different cases of errors here:

1. Those which fail on if (v != 27 && v != 28) in SignatureVerification.sol#L78
2. Those which fail on ecrecover(...) in SignatureVerification.sol#L91

Imagine a 64 or 65 byte long signature scheme which fails on one of these conditions, but would be accepted by the validating contract.

Case 1

The first case is curious, because it assumes that no EIP-1271 compatible scheme uses 65-bytes with special values for v. This assumptions is wrong, because Gnosis Safe has three special cases (v=0, v=1, v>30) potentially affected by this.

In the v=1 case the signature format is 65-bytes (same order as here), where r={address}, s={dirty}, v=1 and it will look up an internal map for hashes against the address. The same problem applies to the v>30 case. Likely it doesn't to the v=0 case.

The Safe code cannot be reached because of this revert here.

P.S. It is not well known Safe's support this, because it is hidden in a helper since 1.3.0, but confirmed by the authors.

Case 2

The second case is more about forward compatibility.

ecrecover has certain restrictions, and the cases it returns 0 are:

• r and s must not exceed 0xfffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364141
• v must be 27 or 28

If we craft a 65-byte scheme, the following hex string represents the maximum number of bits which can be set, before triggering a failure:

• fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364141fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd03641411b
• fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364141fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd03641411c

A future signature scheme could break this assumption, and the fallback-to-1271 branch cannot be entered due to the if (recoveredSigner == address(0)) check at SignatureVerification.sol#L94.

Proof of Concept

Context: SignatureVerification.sol#L34

Tools Used

Manual review

Recommended Mitigation Steps

• Reorder this function by first doing an extcodesize check and performing EIP-1271 validation, otherwise falling back to ecrecover.
• Always perform the EIP-1271 validation in case of an ecrecover failure/mismatch.

[0age]

This edge case was already documented in the code heading into the competition, but we did decide to implement a fix for it in part due to the rationale presented in this finding. It also was not a security issue, but is rather a compatibility issue (and a really unusual edge case at that) so we definitely disagree with the severity.

[HardlyDifficult]

EIP-1271 is pretty open ended in terms of what may be considered acceptable, supporting signature schemes other than just ECDSA. The current implementation assumes that the signature is a valid ECDSA signature so when the length happens to match but a different scheme was used, it's possible that verification will fail before the call to _assertValidEIP1271Signature.

Lowering to a Low severity since the impact is presumably rare. It does have the potential to prevent the contract from interacting with seaport but doesn't lead to other negative consequences.

Grouping with the warden's QA report #203