Do not set owners of pre-initialized wallets to address(0)

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2024-03-coinbase/blob/e0573369b865d47fed778de00a7b6df65ab1744e/src/SmartWallet/CoinbaseSmartWallet.sol#L102-L107

Vulnerability details

Impact

Function _validSignature() in ConibaseSmartWallet.sol utilizes isValidSignatureNow() from Solady implementation. If the signature is invalid, function returns address(0). For not-yet initialised implementation of CoinbaseSmartWallet the owner will be zero, thus every invalid signature generated by the attacker will be evaluated as valid ones (invalid signature returns address(0), the owner of uninitialized ConibaseSmartWallet is addresss(0), thus the address returned by isValidSignatureNow() will match the owner).

The very similar issue had been reported during the previous Code4rena contest and evaluated as High. Please review below links for further Impact reference and more detailed explaination:

• the implementation can be killed bricking all the smart wallets that use it as an implementation 2023-01-biconomy-findings#14

• Destruction of the SmartAccount implementation 2023-01-biconomy-findings#496 (comment)

Proof of Concept

File: ConibaseSmartWallet.sol

    constructor() {
        // Implementation should not be initializable (does not affect proxies which use their own storage).
        bytes[] memory owners = new bytes[](1);
        owners[0] = abi.encode(address(0));
        _initializeOwners(owners);
    }

1. At line 105 - the owner is set as abi.encode(address(0))
2. isValidSignatureNow() from Solady will return address(0) when signature is not valid
3. Since the owner (adddress(0)) is the same as the returned value from isValidSignatureNow() - address(0) - signature will be evaluated as valid
4. It's possible to bypass signature validation for not yet initialized CoinbaseSmartWallet.

Tools Used

Manual code review

Recommended Mitigation Steps

The owner of non-yet initialized CoinbaseSmartWallet should not be address(0). Set it to address(1) instead.

File: src/SmartWallet/CoinbaseSmartWallet.sol

102:     constructor() {
103:         // Implementation should not be initializable (does not affect proxies which use their own storage).
104:         bytes[] memory owners = new bytes[](1);
105:         owners[0] = abi.encode(address(0));
106:         _initializeOwners(owners);
107:     }

line 105, should be changed to: owners[0] = abi.encode(address(1));

Assessed type

Access Control

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

SCW is proxy-deployed whose initialization logic is different than what you have described here.

[c4-judge]

3docSec changed the severity to QA (Quality Assurance)

[3docSec]

Interesting finding. If we exclude user errors, it is safe to assume that wallets created by the factory are initialized with non-zero addresses because the factory creates and initializes wallets in the same call.

The only exception to the above is the implementation contract, that has its storage initialized in the constructor. For this one, the only consequence I can imagine is that the implementation wallet can accept UserOperations from anybody. This does not seem to be a big deal, apart from self-calls to upgradeToAndCall which are however secured by the onlyProxy modifier.

The similar finding (this and this) pointed by #90 as having previously been judged valid are much more impactful because they open a viable path for destructing the implementation - this is not true for this codebase.

Leaving this as QA because having address(1) as the implementation owner is a sound recommendation that would prevent the implementation contract from accepting UserOperations with malformed signatures.

[c4-judge]

3docSec marked the issue as grade-a