[BRA-03M] Inconsistent Recovery & Lock States

[Haypierre]

BRA-03M: Inconsistent Recovery & Lock States

Type	Severity	Location
Logical Fault		BaseRecoverableAccount.sol:L132-L134, L147-L151, L156-L160, L338-L346, L354-L370

Description:

The BaseRecoverableAccount implements inconsistencies in its recovery and lock states as the contract may become unlocked after being set to recovery mode, a mechanism that should be disallowed.

Impact:

The recovery and lock state management of the BaseRecoverableAccount is inconsistent, resulting in accounts that may be set to recovery mode, become unlocked, activate new signers, and pass a recovery attempt with an incorrect majority validation under specific circumstances.

Example:

/**
 * @notice Finalizes an ongoing recovery procedure if the security period (executeAfter) is over.
 * The method is public and callable by anyone.
 * @param _signatures Array of guardian signatures concatenated.
 * @notice The arguments should be ordered by the address of the guardian signing the message
 */
function completeRecovery(bytes[] calldata _signatures) external virtual {
    _requireRecovery(true);
    if (recoveryDetails.executeAfter > uint64(block.timestamp)) revert OngoingRecovery();

    require(recoveryDetails.guardiansRequired > 0, "No guardians set on wallet");
    if (recoveryDetails.guardiansRequired != _signatures.length) revert InvalidSignatureAmount();

    if (!_validateSignatures(_signatures)) revert InvalidRecoverySignatures();

    address recoveryOwner = recoveryDetails.recoveryAddress;
    delete recoveryDetails;

    _transferOwnership(recoveryOwner);
    _setLock(0);

    emit RecoveryCompleted(recoveryOwner);
}

Recommendation:

We advise the system's state management to be refactored so as to ensure the system remains locked for as long as a recovery is in effect, prevents unlocking when an unexpired recovery period is currently in effect, and allows a recovery period to be reset after it has expired.

[Haypierre]

Not sure to understand the vulnerability scenario

What does "activate new signers" refer to? If it means adding new guardians, please note that it's an action reserved for the account owner and enforced with the onlyOwner modifier.

The guardian's recovery logic is equivalent to BaseRecoverableAccount from already audited evm openfort evm contracts

see: BRA-03 DISCREPANCY IN WALLET LOCK STATE VERIFICATION