[BOA-05M] Incorrect Validation Logic

[Haypierre]

BOA-05M: Incorrect Validation Logic

Type	Severity	Location
Logical Fault		BaseOpenfortAccount.sol:L163-L169

Description:

The current BaseOpenfortAccount::_validateSignature validation logic will consider all calls with invalid ECDSA payloads that produce a signer address of 0 to be correct.

Impact:

All invalid signatures will be considered correct by the current BaseOpenfortAccount::_validateSignature mechanism.

Example:

function _validateSignature(bytes32 _hash, bytes memory _signature, uint256 _to, bytes calldata _data)
    internal
    returns (bytes4 magic)
{
    magic = EIP1271_SUCCESS_RETURN_VALUE;
    address signerAddress = _recoverECDSAsignature(_hash, _signature);

    // Note, that we should abstain from using the require here in order to allow for fee estimation to work
    if (signerAddress != owner() && signerAddress != address(0)) {
        // if not owner, try session key validation
        if (!isValidSessionKey(signerAddress, _to, _data)) {
            magic = "";
        }
    }
}

Recommendation:

We advise the code to assign an EIP1271_SUCCESS_RETURN_VALUE to the magic variable solely when verification has been successful (i.e. if (signerAddress == owner() || isValidSessionKey(signerAddress, _to, _data)) magic = EIP1271_SUCCESS_RETURN_VALUE;).

[Haypierre]

Remark:

Description and Impact are incompatible: only invalid signatures with a hash that recovers to address(0) will be considered valid, NOT All invalid signatures.

The ecrecover precompile will yield the zero address on failure which can be trivially crafted:

• https://docs.soliditylang.org/en/v0.8.9/units-and-global-variables.html?highlight=ecrecover#mathematical-and-cryptographic-functions

This is the reason that the standard OpenZeppelin library checks for this scenario here:

• https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/ECDSA.sol#L148-L151

This has been addressed in #11