Direct usage of ecrecover allows signature malleability

[code423n4]

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographFactory.sol#L320-L335

Vulnerability details

Impact

The _verifySigner function of Holograph calls the Solidity ecrecover function directly to verify the given signatures.
However, the ecrecover EVM opcode allows malleable (non-unique) signatures and thus is susceptible to replay attacks.

If the expected address is also the zero address (address(0)), the signature verification will pass although the signature is invalid.

Ensuring the signatures are not malleable is considered a best practice (and so is checking signer != address(0), where address(0) means an invalid signature).

https://swcregistry.io/docs/SWC-117
https://swcregistry.io/docs/SWC-121

Proof of Concept

contracts/HolographFactory.sol:
  319     */
  320:   function _verifySigner(
  321:     bytes32 r,
  322:     bytes32 s,
  323:     uint8 v,
  324:     bytes32 hash,
  325:     address signer
  326:   ) private pure returns (bool) {
  327:     if (v < 27) {
  328:       v += 27;
  329:     }
  330:     /**
  331:      * @dev signature is checked against EIP-191 first, then directly, to support legacy wallets
  332:      */
  333:     return (ecrecover(keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", hash)), v, r, s) == signer ||
  334:       ecrecover(hash, v, r, s) == signer);
  335:   }

Tools Used

Manual Code Review

Recommended Mitigation Steps

Use the recover function from OpenZeppelin's ECDSA library for signature verification.

[gzeoneth]

./library is out-of-scope and no direct exploit

[alexanderattar]

We will add a check for zero address

[gzeoneth]

We will add a check for zero address

this is regarding malleability btw

[ACC01ADE]

This is in-scope since the referenced code is not in library but a direct function in HolographFactory. Agreeing that this is not regarding zero address but signature malleability (specifically s-values in the upper range).

[gzeoneth]

Consider with #397