Coinbase Smart Wallet - SBSecurity's results

Low Risk

Non-Critical

Low Risks

[L-01] Upgrade to EntryPoint 0.7

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

Issue Description: CoinbaseSmartWallet uses EntryPoint 0.6, although EntryPoint 0.7 was released in the past months. EntryPoint 0.7 introduce more gas optimization, security and other interesting enhancements.

Can check here: https://blog.smart-contracts-developer.com/entrypoint-v0-7-0-the-new-era-of-account-abstraction-854f9f82912e

Recommendation: Upgrade the CoinbaseSmartWallet to use EntryPoint 0.7

[L-02] _initializeOwners is not capped at reasonable bounds

https://github.com/code-423n4/2024-03-coinbase/blob/e0573369b865d47fed778de00a7b6df65ab1744e/src/SmartWallet/MultiOwnable.sol#L162-L174

Issue Description: _initializeOwners is not capped and if the array is big can result in out of gas.

function _initializeOwners(bytes[] memory owners) internal virtual { 
    for (uint256 i; i < owners.length; i++) {
        if (owners[i].length != 32 && owners[i].length != 64) {
            revert InvalidOwnerBytesLength(owners[i]);
        }

        if (owners[i].length == 32 && uint256(bytes32(owners[i])) > type(uint160).max) {
            revert InvalidEthereumAddressOwner(owners[i]);
        }

        console.log(_getMultiOwnableStorage().nextOwnerIndex);
        _addOwnerAtIndex(owners[i], _getMultiOwnableStorage().nextOwnerIndex++);
    }
}

Recommendation: Cap the function at reasonable bounds, since then can add owners and always can add if miss someone at initialization.

[L-03] executeBatch is not capped at reasonable bounds

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

Issue Description: executeBatch is not capped and if the array of Calls is big can result in out of gas.

function executeBatch(Call[] calldata calls) public payable virtual onlyEntryPointOrOwner { 
    for (uint256 i; i < calls.length;) {
        _call(calls[i].target, calls[i].value, calls[i].data);
        unchecked {
            ++i;
        }
    }
}

Recommendation: Cap the function at reasonable bounds.

Non-Critical

[N‑01] Pass only nonce to _validateRequest

https://github.com/code-423n4/2024-03-coinbase/blob/e0573369b865d47fed778de00a7b6df65ab1744e/src/MagicSpend/MagicSpend.sol#L315-L324

Issue Description: In MagicSpend::_validateRequest only the nonce from WithdrawRequest was used.

function _validateRequest(address account, WithdrawRequest memory withdrawRequest) internal {
    if (_nonceUsed[withdrawRequest.nonce][account]) {
        revert InvalidNonce(withdrawRequest.nonce);
    }

    _nonceUsed[withdrawRequest.nonce][account] = true;

    // This is emitted ahead of fund transfer, but allows a consolidated code path
    emit MagicSpendWithdrawal(account, withdrawRequest.asset, withdrawRequest.amount, withdrawRequest.nonce);
}

Recommendation: In validatePaymasterUserOp() and withdraw() pass only nonce to _validateRequest and move the event in the upper function.

[N‑02] MultiOwnable contract need to be marked abstract

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

Issue Description: MultiOwnable contract is inherited from CoinbaseSmartWallet and all its functions are used there as inherited. No one will use MultiOwnable independently.

contract MultiOwnable {

Recommendation: Mark MultiOwnable abstract.

[N‑03] Wallets with same owners and same nonce can be deployed

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

Issue Description: When deploy wallets, owners and nonce is passed. Nonce is used to allow multiple accounts with same owners to be deployed, because its used to generate the salt.

function createAccount(bytes[] calldata owners, uint256 nonce)

But if reorder the owners, another wallet can be deployed with same nonce, since when generate the salt, owners and nonce is used.

function _getSalt(bytes[] calldata owners, uint256 nonce) internal pure returns (bytes32 salt) {
    salt = keccak256(abi.encode(owners, nonce));
}

Recommendation: Change the way salt is generated or limit to pass the nonce from the deployer and user auto-incrementing nonce.

[N‑04] Do not explicitly compare bool variables to bool expression

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

Issue Description: In createAccount there is check if the account is already deployed and if so, it doesn’t deploy it, but just return the address. The if statement explicitly compare the alreadyDeployed with false.

if (alreadyDeployed == false) { // @audit - !alreadyDeployed
    account.initialize(owners);
}

Recommendation: Remove == false, and change to if (!alreadyDeployed).

[N‑05] Typos

Issue Description: There are several typos in the codebase:

MultiOwner.sol

bytes32 private constant MUTLI_OWNABLE_STORAGE_LOCATION =
        0x97e2c6aad4ce5d562ebfaa00db6b9e0fb66ea5d8162ed5b243f51a2e03086f00; // @audit - MUTLI -> MULTI

/// @notice Checks if the sender is an owner of this contract or the contract itself.
///
/// @dev Revert if the sender is not an owner fo the contract itself. // @audit - typo | fo -> of
function _checkOwner() internal view virtual {
    if (isOwnerAddress(msg.sender) || (msg.sender == address(this))) {
        return;
    }

    revert Unauthorized();
}

36, 200

ERC1271.sol

/// @dev To prevent the same signature from being validated on different accounts owned by the samer signer, // @audit - samer -> same

9

🛠️ Analysis - Coinbase

Overview

Coinbase Smart Wallet contracts are focused mainly on ERC4337 and making it compliant with the Coinbase infrastructure. With his custom Paymaster implementation, the flow where sponsoring transactions from the executor’s off-chain wallet is already possible. The end goal of the product is to completely remove the need for EOAs, thus attracting non-technical users without exposing them to the underlying complexity. WebAuthnSol is a crucial contract validating the signatures provided off-chain by passkey owners (passwords, biometrics, and PINs).

Key contracts of Coinbase Smart Wallet for this audit are:

• CoinbaseSmartWallet: ERC4337-compliant smart wallet supporting multiple owners and two types of signatures
• CoinbaseSmartWalletFactory: ERC4337-compliant smart wallet factory, used to deploy CoinbaseSmartWallet deterministically using the UUPS pattern.
• WebAuthn: Library for verifying WebAuthn Authentication assertions (off-chain chain biometric data validation).
• MagicSpend: ERC4337-compliant Paymaster contract, used to sponsor user transactions from Coinbase wallet.

System Overview

We can observe 3 main parts in the Coinbase Smart Wallet system:

• CoinbaseSmartWallet - Constructed from:
  • MultiOwnable - Contract that makes wallet to support multiple owners and their management, also ensures that storage is consistent throughout the upgrades, follows ERC-7201.
  • UUPSUpgradeable - Solady implementation contract makes it possible to deploy many instances in a gas-efficient manner and upgrade the deployable implementation when needed.
  • Receiver - Solady fallback extension to support token transfers with callbacks (ERC721, ERC1155).
  • ERC1271 - Signature validation contract, defining the domain and version of the contract to prevent signature replays.

Is a smart wallet implementation with added functionality to have repayable admin transactions across different chains.

• CoinbaseSmartWalletFactory - Standard factory contract that can precompute address and deploy them through CREATE2 when needed.
• MagicSpend - Paymaster implementation to sponsor smart wallet transactions and allowing users to deposit and withdraw from EntryPoint .
• WebAuthn - Biometric validation contract, used to verify and backup through various options (fingerprints, Face ID, password).

Approach Taken in Evaluating WiseLending

Architecture Recommendations

CoinbaseSmartWallet:

Developers did a great job, maximally sticking to the EIP4337, without introducing unnecessary complexity. Additions, such as multi-owners and signatures without chainId are well implemented, using assembly where possible. EntryPoint address is hardcoded, but this does not pose any problem since the wallet is upgradeable.

All access modifiers are applied properly, owner management is as described. Signatures cannot be replayed, containing all the crucial information hashed, only exception is for admins.

Notes for improvement:

• Replace setting owners at 1st index to address(0) with disableInitializer() in the constructor as it is more widely used:

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

• Fix typos:

bytes32 private constant MUTLI_OWNABLE_STORAGE_LOCATION //MULTI

• Some functions can be simplified, such as canSkipChainIdValidation to return the result without explicitly use true/false.
• MultiOwner can be made abstract since there is no purpose to be deployed separately.
• Functions to deposit and withdraw from EntryPoint can be added instead of relying on users to call them on behalf of the smart wallet.
• executeBatch is not capped to reasonable amount of transactions potentially leading to out of gas exceptions.

CoinbaseSmartWalletFactory:

Standard implementation of ERC4337 factory, has all the needed functions to precompute and deploy the wallet when needed, as well as support just in time ETH transfers for new wallets.

Salt creation can be more specific as now same nonce and different owners combination will deploy new wallet every time, eventually misleading non-technical users.

Notes for improvement:

• Owners array can be sorted to prevent deployment of duplicate wallets.

MagicSpend:

Simple paymaster implementation, contains all the needed functions to operate effectively and integrate with EntryPoint, paying the smart wallet transactions.

Notes for improvement:

• Remove asset from the WithdrawRequest, since only ETH is supported as payment token for now.

WebAuthn:

1:1 with Daimo’s implementation, with some modified steps. Following the official w3 documentation.

Notes for improvement:

• In verify() remove step 17 as the requireUV is always passed as false and no need to check for the user verification flag.

// 17. If user verification is required for this assertion, verify that the User Verified bit of the flags in authData is set.
if (requireUV && (webAuthnAuth.authenticatorData[32] & AUTH_DATA_FLAGS_UV) != AUTH_DATA_FLAGS_UV) {
    return false;
}

Codebase Quality Analysis

We consider the quality of Coinbase Smart Wallet codebase as a exceptional, without introducing unnecessary complexity and error prone code.

Contracts strictly follow the desired standards (4337, 1271, 712).

Unique feature represented by WebAuthn and FCL, used to solve the problem of the signatures is hard to be understood but on the other hand, there is no way to be changed due to the increased gas cost.

Tests are not well developed, with validating mainly the happy path and not testing for use cases that might cause harm to the contracts in scope. They are modular with a good structure and setup, but due to not utilizing CoinbaseSmartWalletFactory in SmartWalletTestBase to deploy accounts, tests for UUPS upgrades were failing, because of using a concrete implementation instead of a proxy.

Test analysis

The overall test coverage is high, most of them covering only happy path scenarios. Not all the functions were tested. For example upgradeAndCall called from EntryPoint contract was missing.

It was failing because onlyProxy modifier was reverting since the account was not deployed through factory, test was indicating passed but when the verbosity was increased it appeared.

Consider updating the setUp function in SmartWalletTestBase in order to properly test the functionality and avoid scenarios where concrete implementations work, but proxies don’t.

function setUp() public virtual {
    CoinbaseSmartWalletFactory factory = new CoinbaseSmartWalletFactory(address(new CoinbaseSmartWallet()));
    owners.push(abi.encode(signer));
    owners.push(passkeyOwner);
    account = factory.createAccount(owners, 1);
}

It will be beneficial for the project if end-to-end tests are introduced validating example use cases of the wallet-paymaster integration.

Systemic & Centralization Risks

Regarding the centralization risks involved in the CoinbaseSmartWallet contract, it is important to mention that owners can remove each other freely, potentially leading to scenarios where if one of them gets compromised it can handily remove other owners and steal the assets held in the wallet.

Functions allowed to have replayable signatures are well protected with proper access modifiers applied.

Important _authorizeUpgrade function is overriden with applied onlyOwner modifier, but the risk of a one of the admins to upgrade the wallet to a non-ERC1967 compliant contract is presented which can have serious security implications.

Another inconsistency can be the non-sequential nonce tracking in MagicSpend allowing bundlers to execute multiple userOp transactions of the same sender in their desired order, possibly extracting value from them.

Notes for improvements:

• Threshold for admin actions can be introduced, although it will require significant amount of development it can increase the security of the wallet, in exchange for making it less robust.
  • Middle ground can be to apply it only on the upgradeToAndCall function from UUPSUpgradeable

Team Findings

New insights and learning from this audit

Learned about:

• ERC4337
  • Resources
  • Ambire
  • Infinitism

Conclusion

In general, CoinbaseSmartWallet protocol presents a well-designed architecture. It exhibits, such as modularity, entity separation, also strictly following ERCs and we believe the team has done a good job regarding the code. However, there are areas for improvement, including tests, and some minor inconsistencies, mentioned in the Architecture Recommendations section. Systemic and Centralization risks do not pose any significant impact to the system.

Main code recommendations include simplifying functions, fixing typos, adding functions to ease the usage of the contracts.

It is also highly recommended that the team continues to invest in security measures such as mitigation reviews, audits, and bug bounty programs to maintain the security and reliability of the project.

Time spent:

25 hours