wallets may be in risk if it have different owner of the fisrt owners when the wallet was deployed

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-03-coinbase/blob/e0573369b865d47fed778de00a7b6df65ab1744e/src/SmartWallet/CoinbaseSmartWalletFactory.sol#L49

Vulnerability details

When user wants create a wallet he has to call the createAccount function in the factory, this functions is taking the owner by a salt:

 function createAccount(bytes[] calldata owners, uint256 nonce)
        public
        payable
        virtual
        returns (CoinbaseSmartWallet account)
    {
        if (owners.length == 0) {
            revert OwnerRequired();
        }

        (bool alreadyDeployed, address accountAddress) =
            LibClone.createDeterministicERC1967(msg.value, implementation, _getSalt(owners, nonce));  <-----

        account = CoinbaseSmartWallet(payable(accountAddress));

        if (alreadyDeployed == false) {
            account.initialize(owners);
        }
    }

[Link]

Note that user have to deploy the wallet with the same owner and nonces if they want the same address for the wallet in others chain.

The problem is that if a user changed some owners (addOwnerAddress,removeOwnerAtIndex) whether the signature have chain id or not, the user need to pass the first owners to deploy the wallet in other chains.

Consider the next scenario:

1. user create a Coinbase wallet in all the chain availables with two owners.
2. One of this owners wallet get compromised, so the compromised owner get removed from the Coinbase wallet.
3. A new L2 get added in the protocol.
4. User want to deploy his wallet in the new chain but he notice that he need to pass the compromised owner wallet to create the wallet with the same address.

At this point the compromised owner wallet can do bad thing due he is an owner in other chain of the wallet, bad thing like stole money from the wallet, delete owners, etc

Impact

User may not be capable to create another wallet in other chain because he need the all owner of the wallet, one of this owners can be a compromised wallet who can steal funds.

Proof of Concept

As you can see below, the salt for a new wallet has to be the first owners of the wallet to get the same address:

 function createAccount(bytes[] calldata owners, uint256 nonce)
        public
        payable
        virtual
        returns (CoinbaseSmartWallet account)
    {
        if (owners.length == 0) {
            revert OwnerRequired();
        }

        (bool alreadyDeployed, address accountAddress) =
            LibClone.createDeterministicERC1967(msg.value, implementation, _getSalt(owners, nonce));  <-----

        account = CoinbaseSmartWallet(payable(accountAddress));

        if (alreadyDeployed == false) {
            account.initialize(owners);
        }
    }

[Link]

This can be problematic if the owners of the wallet was already changed.

Tools Used

Manual.

Recommended Mitigation Steps

The most straightforward solution if change the the way that the salt is got it instead of owner this can be some other string passed by the user.

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

Seems valid barring a secured and smooth SCW deployment on various chains for the same address.

[c4-pre-sort]

raymondfam marked the issue as duplicate of #68

[c4-judge]

3docSec marked the issue as not a duplicate

[c4-judge]

3docSec changed the severity to QA (Quality Assurance)

[3docSec]

In case of loss of trust in one of the original owners, users can (and should!) create a wallet with a different address in the new chain. The code in scope makes no assumption that a given user must have the same wallet on multiple chains, so the impact is purely UX.

The user privileging the UX of having the same address over the security of barring a disgraced owner would be a user mistake.

[c4-judge]

3docSec marked the issue as grade-b