c4-bot-1 commented 6 months ago

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:

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);
    }

At line 105 - the owner is set as abi.encode(address(0))
isValidSignatureNow() from Solady will return address(0) when signature is not valid
Since the owner (adddress(0)) is the same as the returned value from isValidSignatureNow() - address(0) - signature will be evaluated as valid
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 commented 6 months ago

raymondfam marked the issue as insufficient quality report

c4-pre-sort commented 6 months ago

raymondfam marked the issue as primary issue

raymondfam commented 6 months ago

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

c4-judge commented 6 months ago

3docSec changed the severity to QA (Quality Assurance)

3docSec commented 6 months ago

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.