executeWithoutChainIdValidation can lead to fund theft and control compromise.

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-03-coinbase/blob/e0573369b865d47fed778de00a7b6df65ab1744e/src/SmartWallet/CoinbaseSmartWallet.sol#L180-L187 https://github.com/code-423n4/2024-03-coinbase/blob/e0573369b865d47fed778de00a7b6df65ab1744e/src/SmartWallet/CoinbaseSmartWallet.sol#L252-L262 https://github.com/code-423n4/2024-03-coinbase/blob/e0573369b865d47fed778de00a7b6df65ab1744e/src/SmartWallet/CoinbaseSmartWallet.sol#L257

Vulnerability details

executeWithoutChainIdValidation is intended to allow specific functions to be executed on contract without validating the chain ID. It takes the function selector from the calldata and checks if it is allowed to skip chain ID validation using the canSkipChainIdValidation function. If allowed, it calls the function on the contract itself using _call.

executeWithoutChainIdValidation

function executeWithoutChainIdValidation(bytes calldata data) public payable virtual onlyEntryPoint {
    bytes4 selector = bytes4(data[0:4]);
    if (!canSkipChainIdValidation(selector)) {
        revert SelectorNotAllowed(selector);
    }
    _call(address(this), 0, data);
}

This can only be called by the EntryPoint contract, as enforced by the onlyEntryPoint modifier. It extracts the function selector from the calldata and checks if it's allowed to skip chain ID validation using canSkipChainIdValidation. If not allowed, it reverts. If allowed, it calls the function on itself using _call.

canSkipChainIdValidation

function canSkipChainIdValidation(bytes4 functionSelector) public pure returns (bool) {
    if (
        functionSelector == MultiOwnable.addOwnerPublicKey.selector
            || functionSelector == MultiOwnable.addOwnerAddress.selector
            || functionSelector == MultiOwnable.removeOwnerAtIndex.selector
            || functionSelector == UUPSUpgradeable.upgradeToAndCall.selector
    ) {
        return true;
    }
    return false;
}

Note: The issue lies in the inclusion of the upgradeToAndCall function selector in the canSkipChainIdValidation check. This allows the upgradeToAndCall function to be executed without validating the chain ID.

If a transaction calling executeWithoutChainIdValidation with upgradeToAndCall is signed by a valid owner on one chain and then replayed on another chain where the wallet has the same address, it could upgrade the wallet implementation on the second chain to a malicious version without the owners' intention.

The issue is an edge case responsible for enabling this vulnerability.

|| functionSelector == UUPSUpgradeable.upgradeToAndCall.selector

It is expected to allow only safe and non-critical functions to be executed without chain ID validation. These should not have any unintended consequences when replayed across different chains.

But because of the inclusion of upgradeToAndCall in the allowed functions, an attacker can potentially trick an owner into signing a transaction that upgrades the wallet implementation on one chain, and then replay that transaction on other chains to upgrade the wallet to a malicious implementation. This malicious implementation could allow the attacker to steal funds or control the wallet on those chains.

Impact

If exploited, it could lead to the compromise of the smart wallet on multiple chains, allowing an attacker to steal funds or gain unauthorized control over the wallet.

Recommended Mitigation Steps

Consider removing upgradeToAndCall from the allowed list in canSkipChainIdValidation. It is too risky to allow this to be replayed across chains, as it could lead to wallet compromise.

function canSkipChainIdValidation(bytes4 functionSelector) public pure returns (bool) {
    if (
        functionSelector == MultiOwnable.addOwnerPublicKey.selector
            || functionSelector == MultiOwnable.addOwnerAddress.selector
            || functionSelector == MultiOwnable.removeOwnerAtIndex.selector
    ) {
        return true;
    }
    return false;
}

Assessed type

Access Control

[raymondfam]

Owners are typically controlled by the SCW creator.

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-judge]

3docSec changed the severity to QA (Quality Assurance)

[3docSec]

Interesting finding, an exploit of the deliberate design of excluding signature validation from certain calls. So user A upgrades their wallet on chain X to contract B that looks alright on chain X. Their transaction is then replayed, without A knowing, on chain Y to upgrade their wallet to contract B, which however on chain Y has a different implementation, and a malicious one.

The key point to consider for judging is that to own the same B address in chain Y, one must own the B address in chain X as well.

This reduces the root cause to either:

• a reckless operation if the user signs the upgrade to a non-trusted implementation
• a centralization issue, if the user signs an upgrade recommended by Coinbase

both cases falling in QA territory.

[c4-judge]

3docSec marked the issue as grade-a