Inability to Remove Compromised Owners Due to Data Loss and Lack of Backup Records

[c4-bot-7]

Lines of code

https://github.com/coinbase/smart-wallet/blob/95c8ab2777fa69243a56a5f4d1ce8ce629d707f3/src/MultiOwnable.sol#L109

Vulnerability details

Impact

The vulnerability arises specifically because the MultiOwnable contract's security model does not account for the real-world possibility of data loss. It assumes that owners will always have access to and the ability to reproduce the exact bytes representing their public keys.

An owner who has lost control over their private key will remain listed. This could leave the door open for attackers if the private key is compromised, as the legitimate participants would have no means to revoke the compromised owner's access.

see code below:

function removeOwnerAtIndex(uint256 index, bytes calldata owner_) public virtual onlyOwner {
        bytes memory owner = ownerAtIndex(index);
        if (owner.length == 0) revert NoOwnerAtIndex(index);
        if (keccak256(owner) != keccak256(owner_)) revert WrongOwnerAtIndex(index, owner_);

        delete _getMultiOwnableStorage().isOwner[owner];
        delete _getMultiOwnableStorage().ownerAtIndex[index];

        emit RemoveOwner(index, owner);
    }

In the case where data is not managed efficiently through use of backups and publicly accessible accurate records it might be impossible to remove owner hence compromising the function utility and ownership all together.

Poc Scenario

• Owner A and Owner B are added using their Ethereum addresses.
• Owner C is added using their public key, represented by (x, y) coordinates encoded as bytes, to utilize the passkey feature for additional security measures.
• Owners A and B are registered with their Ethereum addresses.
• Owner C generates a new Ethereum key pair specifically for this contract, uses the public key derived from this pair for registration, and is added with the public key bytes.
• Owner C loses the device containing their private key. This loss includes the inability to access the Ethereum account and, critically, the inability to regenerate the exact public key bytes used for their registration as an owner in the MultiOwnable contract.
• Due to the loss of the private key, Owner C can no longer participate in the governance of the DeFi application. The remaining owners decide to remove Owner C from the contract.
• The contract requires the exact bytes of Owner C's public key for removal. However, without access to the private key or an external, accurate record of the public key bytes, the remaining owners cannot generate the required data to invoke removeOwnerAtIndex successfully.
• Owner C remains a registered owner, despite no longer having control over their associated private key. This situation presents a serious security risk and operational challenge, as Owner C can be potentially compromised and can't be removed.

Severity Assesment: Medium to High

The severity depends on actions taken by compromised owner C and threat response. It becomes a high severity bug if malicious owner does the following actions:

1. Add unauthorized or malicious addresses as new owners

2. Remove legitimate owners from the contract gaining full control

Mitigation Strategies

• Introduce a mechanism that allows for owner removal under specific conditions without requiring the exact bytes, such as a time-based lock that allows an owner to be marked for removal if certain conditions are met.

• Introduce a dead man's switch mechanism where owners can periodically confirm their active control over their keys. Failure to do so could trigger a recovery process, allowing for their eventual removal.

• Require owners to store backup copies of their public key bytes in a secure yet accessible manner. This could involve off-chain storage solutions that are still secure and private.

• Have a multi-signature administrative override for critical situations like owner removal.

Assessed type

Access Control

[McCoady]

Loss of keys falls very much under "user error" and definitely out of scope of the smart contract review.

Additionally a missing owner public key can be found via the following function in MultiOwnable:

function ownerAtIndex(uint256 index) public view virtual returns (bytes memory) {
        return _getMultiOwnableStorage().ownerAtIndex[index];
}

[stevieraykatz]

Agreed with @McCoady here. This form of user error is out of scope for our contracts and the complexity from the mitigations is non-negligible.

[anthonyshiks]

Main invariants SmartWallet Only current owners or EntryPoint can make calls that Decrease account balance. Add or remove owner. Upgrade account. Any current owner can add or remove any other owner....this is a main invariant in contest readme.. not to get into semantics but I'd like to bring to attention the use of the word current...this clearly shows intent that owners should be currently recognized as owners to carry out this actions..but this bug clearly shows that stale or compromised owners still have control...it could be a user error due to lack of knowledge about such issues but can easily escalate to losing control of the contract given the way contract logic is setup with lack of timelocks for other owners to intervene in time when compromised owner carries out sensitive actions...The dead man's switch mitigation I suggested is a rather less complicated approach in mitigating this issue... furthermore owners should have offchain mechanisms of public owner key retrieval as backup to prevent this issue.

[3docSec]

I agree this is clearly a design choice rather than a vulnerability. You have the simplicity and reduced attack surface of a full-power-owner wallet over the flexibility (and the complexity, and the attack surface) of a multisig.

[c4-judge]

3docSec marked the issue as unsatisfactory: Out of scope

[anthonyshiks]

Hi sorry for the inconveniences with many disputes just bear with me I can prove this finding.. okay so the issue raised is being invalidated because of loss of keys is considered out of scope...but I think there's a misunderstanding the bug per se is not loss of keys but the inability of the function to be useful when keys are lost...this at the very least is medium finding according to c4 standards.

[3docSec]

the bug per se is not loss of keys but the inability of the function to be useful when keys are lost

This was clear already. The when keys are lost is a condition that is not in scope for consideration.

A quick refresher on the c4 standards:

Findings that require the user to be careless or enter the wrong information into a contract call are QA or Invalid. Non privileged users are expected to preview their transactions to protect against input mistakes. Anyone using modern tooling will have previews built into their toolset. Phishing attacks, and improper caution on using a protocol fall under this rule.

source

and it's hard to believe that losing private keys is not an "improper caution" prerequisite.

[anthonyshiks]

Then that would H-01 is invalid with that reasoning.

[anthonyshiks]

Findings that require the user to be careless or enter the wrong information into a contract call are QA or Invalid. Non privileged users are expected to preview their transactions to protect against input mistakes. Anyone using modern tooling will have previews built into their toolset. Phishing attacks, and improper caution on using a protocol fall under this rule.

Not the same as this report.

[3docSec]

Then that would H-01 is invalid with that reasoning.

Not the same as this report.

A quick refresher @anthonyshiks on the backstage guidelines:

All comments should present fact-based evidence, rather than opinion.

and

At the point that a judge provides a response, the conversation is over, unless you wish to provide additional facts which demonstrate inaccurate assumptions in the judge's response. Further pursuit of the conversation will result in having backstage access suspended.

Please comply, first and last warning.

[3docSec]

So, can you provide fact-based evidence on how this out-of-scope criteria should apply to H-01?

[anthonyshiks]

Ok agree to disagree..I have provided sufficient proof to the best of my ability and will have to respect judge's decision.

[anthonyshiks]

The owners carelessness in losing the keys is just but one of the factors that contribute to this bug although a primary one so I see you point but there are other important factors as well such as offchain owner byte retrieval. Thank you for the opportunity in sharing my findings.