c4-bot-8 commented 8 months ago

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. if (alreadyDeployed == false) { account.initialize(owners); }

here we are not changing the alreadyDeployed after account.initialize(owners); people can again call account.initialize(owners).

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

function createAccount(bytes[] calldata owners, uint256 nonce) public payable 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);
    }
}

Tools Used

Recommended Mitigation Steps

if (alreadyDeployed == false) { account.initialize(owners); } alreadyDeployed=true;

Assessed type

Context

c4-pre-sort commented 8 months ago

raymondfam marked the issue as insufficient quality report

c4-pre-sort commented 8 months ago

raymondfam marked the issue as primary issue

raymondfam commented 8 months ago

This is a one time account creation. You do not need to do it because trying to call createAccount() again with the same parameters would have alreadyDeployed == true returned by LibClone.createDeterministicERC1967.

c4-judge commented 8 months ago

3docSec marked the issue as duplicate of #11