Upgrade signature validation#

This is the code in OrderValidator.sol that validates signatures:

bytes32 hash = LibOrder.hash(order);
if (_hashTypedDataV4(hash).recover(signature) != order.maker) {
    if (order.maker.isContract()) {
        require(
            ERC1271(order.maker).isValidSignature(_hashTypedDataV4(hash), signature) == MAGICVALUE,
            "contract order signature verification error"
        );
    } else {
        revert("order signature verification error");
    }
}

recover reverts internally so isValidSignature can't be called#

A problem with this code is that the recover() function reverts internally if it determines a signature to be invalid.

It is possible for ERC1271(order.maker).isValidSignature to validate a signature differently. What may be valid for ERC1271(order.maker).isValidSignature may not be valid for recover.

But ERC1271(order.maker).isValidSignature will never get a chance to validate signatures if recover reverts.

recover does not support short signatures#

The recover function comes from LibSignature.sol which is coped from ECDSAUpgradeable.sol#release-v4.3, which is an older version that does not support short signatures.

Current versions of recover in ECDSAUpgradeable.sol from OpenZeppelin support short signatures.

Recommendation#

Delete LibSignature.sol and use the isValidSignatureNow function from SignatureCheckerUpgradeable.sol instead.

The isValidSignatureNow function combines the recover and isValidSignature function calls into a single function call in a way that solves Item 1 and Item 2 mentioned above.

The code could look like this:

At the top of the OrderValidate.sol file import SignatureCheckerUpgradeable:

import "@openzeppelin/contracts-upgradeable/utils/cryptography/SignatureCheckerUpgradeable.sol";

bytes32 hash = LibOrder.hash(order);
require(SignatureCheckerUpgradeable.isValidSignatureNow(order.maker, _hashTypedDataV4(hash), signature), "order signature verification error");