[BOA-04M] Improper Revocation of Session Key

[Haypierre]

BOA-04M: Improper Revocation of Session Key

Type	Severity	Location
Logical Fault		BaseOpenfortAccount.sol:L384-L393

Description:

The BaseOpenfortAccount::revokeSessionKey function will improperly revoke a session key as any whitelisted entries will remain within it. As such, if it is ever reconfigured previously whitelisted addresses will still have access to it.

Impact:

A revoked session key that is re-configured will retain previously whitelisted entries which is incorrect.

Example:

/**
 * Register a session key to the account
 * @param _key session key to register
 * @param _validAfter - this session key is valid only after this timestamp.
 * @param _validUntil - this session key is valid only up to this timestamp.
 * @param _limit - limit of uses remaining.
 * @param _whitelist - this session key can only interact with the addresses in the _whitelist.
 */
function registerSessionKey(
    address _key,
    uint48 _validAfter,
    uint48 _validUntil,
    uint48 _limit,
    address[] calldata _whitelist
) external virtual {
    _requireFromOwner();
    require(_validUntil > block.timestamp, "Cannot register an expired session key");
    require(_validAfter < _validUntil, "_validAfter must be lower than _validUntil");
    require(sessionKeys[_key].validUntil == 0, "SessionKey already registered");
    require(_whitelist.length < 11, "Whitelist too big");
    uint256 i;
    for (i; i < _whitelist.length;) {
        sessionKeys[_key].whitelist[_whitelist[i]] = true;
        unchecked {
            ++i;
        }
    }
    if (i != 0) {
        // If there is some whitelisting, it is not a masterSessionKey
        sessionKeys[_key].whitelisting = true;
        sessionKeys[_key].masterSessionKey = false;
    } else {
        // If there is some limit, it is not a masterSessionKey
        if (_limit == ((2 ** 48) - 1)) {
            sessionKeys[_key].masterSessionKey = true;
        } else {
            sessionKeys[_key].masterSessionKey = false;
        }
    }

    sessionKeys[_key].validAfter = _validAfter;
    sessionKeys[_key].validUntil = _validUntil;
    sessionKeys[_key].limit = _limit;
    sessionKeys[_key].registrarAddress = owner();

    emit SessionKeyRegistered(_key);
}

/**
 * Revoke a session key from the account
 * @param _key session key to revoke
 */
function revokeSessionKey(address _key) external virtual {
    _requireFromOwner();
    if (sessionKeys[_key].validUntil != 0) {
        sessionKeys[_key].validUntil = 0;
        sessionKeys[_key].limit = 0;
        sessionKeys[_key].masterSessionKey = false;
        sessionKeys[_key].registrarAddress = address(0);
        emit SessionKeyRevoked(_key);
    }
}

Recommendation:

We advise the code to erase the whitelist as well, potentially by maintaining a whitelist nonce that is incremented each time the key has been revoked so as to permit fresh mapping declarations to be used each time.

[Haypierre]

ACK. Flagged as Minor in the previous audit from Certik: https://drive.google.com/drive/folders/1aoPgJD_oz1qagWflnO91ASlQo-2upjRL

"_Added the reset of sessionKeys[key].limit validAfter , whitelisting and whitelist have been excluded to
save gas."