Biconomy - Smart Contract Wallet contest - BClabs's results

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccountFactory.sol#L33

Vulnerability details

Impact

SCW factory's main purpose is to deploy contracts for addresses through deployWallet and deployCounterFactualWallet. In the case of deployCounterFactualWallet, deployed contract's address depends on the salt which is provided in the arguments. Functions however don't take into account who the msg.caller is, which means consequentially anyone can deploy a contact for anyone and initialize it with his own parameters. It is true that user can just deploy another contract with a different index, but user might not know that this even happened.

When a user wants to deploy a contract, an attacker can front-run him and deploy a contract with having himself as the entrypoint, which more or less grants him access to the contract through execFromEntryPoint(). This would also mislead the user into thinking he actually deployed the contract.

The second danger here would be that since the factory also has a getAddressForCounterfactualWallet() function, the user could deposit funds to a non yet existing contract and an attacker could create a contract and take the funds by making himself entryPoint and then having access to execute() function.

Proof of Concept

1. User submits a deployCounterFactualWallet() transaction
2. Attacker frontrunns him and creates the contract having himself as _entryPoint.
3. Users transaction failed because the contract already exists with the given salt, so the user might think he created it sometime before, so he continues to use it.
4. At some point attacker steals all the funds from the contract using execute()

Tools Used

Manual review

Recommended Mitigation Steps

Only allow the actual owner and protocol contracts to deploy contracts for the owner.

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L192-L197

Vulnerability details

Impact

Purpose of execTransaction() is for user to be able to execute transactions through signatures. This means that either user or someone whom user authorised could executer SC wallet transactions. Consequentially execTransaction() checks whether the signature is authentic using the checkSignatures() function. The function does that, but does not check whether the signer is actually contract owner in the case where v=0. This means that an attacker could create a malicious contract and call this function.

What that means for the contract is that anyone could call execTransaction and as a transaction input add setOwner(), attackerAddress and thus take ownership of the SC wallet.

Proof of Concept

1. Attacker creates a malicious contract that implements an attack function and isValidSignature(data, contractSignature) function that only returns bytes4(0x20c13b0b) == EIP1271_MAGIC_VALUE.
2. Attacker calls execTransaction() through his contract using setOwner(), attackerAddress as transaction data, and signs the transaction using his contract, making it the signer.
3. Since he signed the data, the signer is indeed the one who signer tx, checkSignatures will therefore not revert and the require will pass.
4. Transaction will then get executed in execute()
5. Attacker gains ownership of the contract and is able to drain all funds now.

Tools Used

Manual review

Recommended Mitigation Steps

Check that _signer is indeed contract owner