Improper signature verification in recovery spell allowing incompatible smart contract owners

[howlbot-integration]

Lines of code

https://github.com/solidity-labs-io/kleidi/blob/0d72b6cb5725c1380212dc76257da96fcfacf22f/src/RecoverySpell.sol#L118
https://github.com/solidity-labs-io/kleidi/blob/0d72b6cb5725c1380212dc76257da96fcfacf22f/src/RecoverySpell.sol#L215

Vulnerability details

Proof Of Concept

The vulnerability exists in the constructor of the RecoverySpell contract, which allows the addition of any Ethereum addresses, including smart contract wallets, as owners. The issue arises during recovery when the signature verification process checks owners' signatures using the ECDSA.recover function, which is designed for EOA.

We can read in the documentation:

The protocol is opinionated and is designed to be used with Gnosis Safe multisigs

However, if one of the owners is a smart contract wallet (like a Gnosis Safe), signature verification should follow EIP-1271, which specifies how smart contracts should validate signatures. Since the current implementation does not account for smart contract wallets, this can cause invalid signature verification for such owners, potentially preventing legitimate recovery actions.

The constructor for RecoverySpell does not validate that the owners are regular EOA (Externally Owned Accounts). Safe wallets, for instance, allow smart contract wallets to act as owners, so a user may assume that the RecoverySpell works the same way.

constructor(
    address[] memory _owners,
    address _safe,
    uint256 _safeThreshold,
    uint256 _recoveryThreshold,
    uint256 _delay
) {
    owners = _owners;
    safe = Safe(payable(_safe));
    threshold = _safeThreshold;
    recoveryThreshold = _recoveryThreshold;
    delay = _delay;

    recoveryInitiated = block.timestamp;

    emit RecoveryInitiated(block.timestamp, msg.sender);
}

In the recovery process, the signature is validated using the ECDSA.recover method:

address recoveredAddress = ECDSA.recover(digest, v[i], r[i], s[i]);

This only works for EOAs, not smart contract wallets, which should use isValidSignatureNow from SignatureChecker.sol to account for smart contracts that implement EIP-1271.

The impact is that recovery attempts will fail if any of the owners is a smart contract wallet, as their signatures will not be correctly validated. This could leave users unable to recover control of their wallet in emergency situations if they have added smart contract wallets as owners.

Recommended Mitigation Steps

To properly validate signatures from smart contract wallets and EOAs alike, the SignatureChecker.sol library from OpenZeppelin should be used. Specifically, the isValidSignatureNow function should replace the current ECDSA.recover mechanism to ensure compliance with EIP-1271.

This modification ensures that smart contract wallets, which implement EIP-1271, can also validate their signatures during the recovery process, making the system compatible with both EOAs and smart contract owners.

Moreover, I recommend to make sure that reentrancy is not possible since EIP-1271 involve external calls.

Assessed type

DoS

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[c4-judge]

GalloDaSballo marked the issue as grade-c