_verifySigner check can be bypassed

[code423n4]

Lines of code

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

Vulnerability details

Description

The current implementation of the _verifySigner function is insufficient and can be bypassed.

When _verifySigner function is called, ecrecover will return address(0) if the signature is invalid.
As a result, the requirement of _verifySigner call in HolographFactory.deployHolographableContract() can bypassed by sending an invalid signature and assigning signer = 0x0

There is no immediate issue currently that I've noticed as this will result in the hash being different and resulting in a different holographerAddress, an attacker wouldn't be able to take over a holographer deployed address.

However, future implementation that would utilize the _verifySigner check can potentially be vulnerable if not implemented correctly.

Proof of Concept

function _verifySigner(
    bytes32 r,
    bytes32 s,
    uint8 v,
    bytes32 hash,
    address signer
  ) private pure returns (bool) {
    if (v < 27) {
      v += 27;
    }
    /**
     * @dev signature is checked against EIP-191 first, then directly, to support legacy wallets
     */
    return (ecrecover(keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", hash)), v, r, s) == signer ||
      ecrecover(hash, v, r, s) == signer);
}

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

require(_verifySigner(signature.r, signature.s, signature.v, hash, signer), "HOLOGRAPH: invalid signature");

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

Recommended Mitigation Steps

I advise an additional check to be imposed within _verifySigner that ensures the signer or the results of ecrecover is not the zero-address which will alleviate this check. For more details, consult the EIP721 implementation by OpenZeppelin ( https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v3.4.0/contracts/cryptography/ECDSA.sol#L53-L71).

[gzeoneth]

Duplicate of #357

[gzeoneth]

Consider with #68