[BOA-03M] Improper Incorporation of Session Keys in EIP-1271

[Haypierre]

BOA-03M: Improper Incorporation of Session Keys in EIP-1271

Type	Severity	Location
Logical Fault		BaseOpenfortAccount.sol:L262

Description:

The structure of session keys in the BaseOpenfortAccount is inherently incompatible with the EIP-1271 standard as the current implementation poses security risks.

Specifically, it evaluates the session key's limit as being non-zero yet does not decrement it and it ignores whitelist configurations entirely.

Impact:

Session keys will improperly pass EIP-1271 signature validation even though they should potentially not be accepted.

Example:

function _validateSessionKeyCall(SessionKeyStruct storage _sessionKey, address _to) internal returns (bool) {
    // Check if reenter, do not allow
    if (_to == address(this)) return false;
    // Limit of transactions per sessionKey reached
    if (_sessionKey.limit == 0) return false;
    // Deduct one use of the limit for the given session key
    unchecked {
        _sessionKey.limit = _sessionKey.limit - 1;
    }
    // If there is no whitelist or there is, but the target is whitelisted, return true
    if (!_sessionKey.whitelisting || _sessionKey.whitelist[_to]) {
        return true;
    }
    // All other cases, deny
    return false;
}

/*
 * @notice See EIP-1271
 * Owner and session keys need to sign using EIP712.
 */
function isValidSignature(bytes32 _hash, bytes memory _signature) public view override returns (bytes4) {
    bytes32 structHash = keccak256(abi.encode(OF_MSG_TYPEHASH, _hash));
    bytes32 digest = _hashTypedDataV4(structHash);
    address signer = digest.recover(_signature);
    if (owner() == signer) return EIP1271_SUCCESS_RETURN_VALUE;

    SessionKeyStruct storage sessionKey = sessionKeys[signer];
    // If the signer is a session key that is still valid
    if (
        sessionKey.validUntil == 0 || sessionKey.validAfter > block.timestamp
            || sessionKey.validUntil < block.timestamp || (!sessionKey.masterSessionKey && sessionKey.limit < 1)
    ) {
        return 0xffffffff;
    }
    // Not owner or session key revoked
    else if (sessionKey.registrarAddress != owner()) {
        return 0xffffffff;
    } else {
        return EIP1271_SUCCESS_RETURN_VALUE;
    }
}

Recommendation:

We advise session keys to not be supported unless a masterSessionKey has been identified as the current integration has been improperly performed.

[Haypierre]

ACK.
The EIP-1271 isValidSignature method is ISO with openfort-contracts.

We don't decrement the session key limit nor check for whitelist because the method is only used for message signature validation (i.e. singing an EIP-712 to authenticate to Opensea )

The signer is recovered after adding message prefix to the hash so it can't be an execution.