Loss of ETher and DOS when calling createAccount() function can be caused by an attacker

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-03-coinbase/blob/main/src/SmartWallet/CoinbaseSmartWalletFactory.sol#L38-L56

Vulnerability details

Impact

• Loss of Ether and Denial of service can be caused by an attacker.

Root cause

1. This is because an attacker can frontrun create2 deterministic deployment to cause DOS.
2. Because createAccount(...) is payable and does not revert when deterministic deployment fails, the ETh sent with createAccount can be lost and stuck in the contract when create2 is front-ran.

Note that the revert in LibClone.createDeterministicERC1967(...) is not bubbled up so the need to check the returned alreadyCreated and revert is necessary.

Proof of Concept

The LibClone.createDeterministicERC1967(...) function used in the createAccount(...) function uses create2(...) to deterministically deploy a contract. The create2(...) deterministic deployment can be frontrun by anyone to cause denial of service.

The createAccount(...) function is payable and allows users to create and send ETher to the created account in the same transaction.

When a user calls the createAccount(...) function and an attacker frontruns that transaction to create a contract at the same address, the alreadyCreated returned variable is true. This is due to the fact that create2 used in the LibClone.createDeterministicERC1967 function creates a deterministic address which anyone can frontrun to take up the address.

The main issue is the the transaction that calls the createAccount(...) function does not revert when alreadyCreated returned variable is true. This will cause the ETher sent along with the transaction to be stuck in the CoinbaseSmartWallet contract forever.

File: CoinbaseSmartWalletFactory.sol
function createAccount(bytes[] calldata owners, uint256 nonce)
        public
        payable //@audit loss of value when already created.
        virtual
        returns (CoinbaseSmartWallet account)
    {
        if (owners.length == 0) {
            revert OwnerRequired();
        }

        (bool alreadyDeployed, address accountAddress) =//@audit revert not bubbled up so check alreadyDeployed and revert.
            LibClone.createDeterministicERC1967(msg.value, implementation, _getSalt(owners, nonce));

        account = CoinbaseSmartWallet(payable(accountAddress));

        if (alreadyDeployed == false) {//@audit ETh lost when alreadDeployed is true
            account.initialize(owners);
        }
    }

The createDeterministicERC1967 function

function createDeterministicERC1967(uint256 value, address implementation, bytes32 salt)
        internal
        returns (bool alreadyDeployed, address instance)
    {
        /// @solidity memory-safe-assembly
        assembly {
            let m := mload(0x40) // Cache the free memory pointer.
            mstore(0x60, 0xcc3735a920a3ca505d382bbc545af43d6000803e6038573d6000fd5b3d6000f3)
            mstore(0x40, 0x5155f3363d3d373d3d363d7f360894a13ba1a3210667c828492db98dca3e2076)
            mstore(0x20, 0x6009)
            mstore(0x1e, implementation)
            mstore(0x0a, 0x603d3d8160223d3973)
            // Compute and store the bytecode hash.
            mstore(add(m, 0x35), keccak256(0x21, 0x5f))
            mstore(m, shl(88, address()))
            mstore8(m, 0xff) // Write the prefix.
            mstore(add(m, 0x15), salt)
            instance := keccak256(m, 0x55)
            for {} 1 {} {
                if iszero(extcodesize(instance)) {
@>                    instance := create2(value, 0x21, 0x5f, salt)//@audit create2
                    if iszero(instance) {
                        mstore(0x00, 0x30116425) // `DeploymentFailed()`.
                        revert(0x1c, 0x04)
                    }
                    break
                }
                alreadyDeployed := 1
                if iszero(value) { break }
                if iszero(call(gas(), instance, value, codesize(), 0x00, codesize(), 0x00)) {
                    mstore(0x00, 0xb12d13eb) // `ETHTransferFailed()`.
                    revert(0x1c, 0x04)
                }
                break
            }
            mstore(0x40, m) // Restore the free memory pointer.
            mstore(0x60, 0) // Restore the zero slot.
        }
    }

Exploit Scenario

1. Alice calls the createAccount(...) function with 1 ETH
2. Bob frontruns Alice's transaction with the same parameters as Alice's parameters to createAccount(...)
3. Alice's account is not created and the transaction did not revert
4. Since Alice's transaction did not revert, the 1 ETH she sent gets stuck in the contract.
5. Alice lost 1ETH without successfully creating an account.
6. Bob the front-runner gets his account contract created.

Bob can setup bots to monitor the mempool for this type of exploit. This would cause users to lose ETH and also unable to create Account.

Tools Used

create2

Recommended Mitigation Steps

1. Consider reverting the transaction when alreadyCreated is false like below. But this can still be DOSed by frontrunning but will not lose ETH so its better to go with the second recommendation below this code.

function createAccount(bytes[] calldata owners, uint256 nonce)
        public
        payable //@audit loss of value when already created.
        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);
--        }
++       if (alreadyDeployed == true) {
++            revert AccountCreationFailed();
++        } else {
++         account.initialize(owners);
++        }
    }

2. Use a deployment function that uses create instead of create2 like the LibClone.deployERC1967(...) function. This is because create2 can be DOSed through front-running but create cannot be DOSed.

Assessed type

DoS

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[raymondfam]

Could have had a coded POC. Seems infeasible with SCW proxy and backend controlled but will let the sponsor review.

[wilsoncusack]

Right, is something we discussed but we forgot clean up @xenoliss. Agree this can lead to loss of funds, if calling directly and not using ERC-4337. We should probably just make this function not payable.

Bob the front-runner gets his account contract created.

This is not really correct. Bob just deployed Alice's account, which presumably has owners only she controls.

[wilsoncusack]

Could have had a coded POC. Seems infeasible with SCW proxy and backend controlled but will let the sponsor review.

There's no way to exploit this with how we use it--because we use ERC-4337 and users can't send ETH on this create call--but it is still legitimate, in abstract.

[c4-sponsor]

wilsoncusack (sponsor) confirmed

[3docSec]

The finding is technically invalid. When the clone is not created, LibClone.createDeterministicERC1967 backs up by sending the value that would have been sent at creation:

File: LibClone.sol
850:                 if iszero(extcodesize(instance)) {
851:  @>                 instance := create2(value, 0x21, 0x5f, salt)
852:                     if iszero(instance) {
853:                         mstore(0x00, 0x30116425) // `DeploymentFailed()`.
854:                         revert(0x1c, 0x04)
855:                     }
856:                     break
857:                 }
858:                 alreadyDeployed := 1
859:                 if iszero(value) { break }
860: @>              if iszero(call(gas(), instance, value, codesize(), 0x00, codesize(), 0x00)) {
861:                     mstore(0x00, 0xb12d13eb) // `ETHTransferFailed()`.
862:                     revert(0x1c, 0x04)
863:                 }
864:                 break

[3docSec]

So if no value is lost, in the proposed exploit scenario, there is no damage to Alice, because Bob's frontrun created the wallet for her. The wallet that Bob created for Alice is identical to what Alice would have created (Alice is the owner, and it's not compromised in any way), so it appears to be safe and the only visible effect of the attack is that Bob offered Alice the gas required to create her wallet.

[c4-judge]

3docSec marked the issue as unsatisfactory: Invalid