No zero value return check in ecrecover in signature verification in HolographFactory contract

[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

Impact

The _verifySigner() function in HolographFactory contract makes a ecrecover() call without checking for address(0) result. Due to user controlled signer input, it is possible for anyone to call the deployHolographableContract() function with address(0) as signer, and crafting signature input such that ecrecover() would return address(0) , then this will return true in _verifySigner() allowing the user to deploy contracts.

Proof of Concept

 return (ecrecover(keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", hash)), v, r, s) == signer ||
      ecrecover(hash, v, r, s) == signer);

The code snippet above is missing a check to ensure the ecrecover() result != address(0).

With above bug, Alice can;

1. craft the signature input in HolographFactory.deployHolographableContract() to ensure ecrecover would return address(0).
2. input address(0) as signer
3. With signature and signer controlled by Alice, Alice calls deployHolographableContract and passes the require check in line 220 and is able to deploy contracts from the Factory contract.

Tools Used

Manual review

Recommended Mitigation Steps

Add a check to ensure that ecrecover returned result is not equals to address(0)

[JustDravee]

I actually submitted this as QA, with way less details and no description of real impact though : #221
Nice report!

[gzeoneth]

true but lack exploit, doesn't seems the attacker can do anything with address(0) deployed a HolographableContract

[alexanderattar]

Low severity since it sets owner/admin to zero address, and would render the contract unusable

[gzeoneth]

Consider with #303