deployCounterFactualWallet frontrunning/pre-deployment risks

[code423n4]

Lines of code

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

Vulnerability details

The SmartAccountFactory.deployCounterFactualWallet function creates a proxy for an owner in a deterministic way. Besides the owner address and index, which are used to define the proxy address, the caller can also supply the endpoint and handler addresses.
This means a malicious user can create a wallet for a target owner with custom endpoint and/or handler. Since there is only one endpoint per chain, changing the endpoint will basically break EIP4337 functionality, so this is pretty pointless.
Being able to set a different handler does allow various attack scenarios.

The _handler parameter sets the fallbackhandler address. When the proxy is called with a non existing function selector, it will be handled by the fallback manager function which forwards the call to the handler contract. By default this is set to DefaultCallbackHandler.

Impact, Severity and Risk

The basis is to set a token address or a dApp contract as a handler. The fallback will then directly call the functions in that contract.
So if for example the WETH contract is set as the handler, when the wallet has any WETH, we can directly call the WETH functions directly from the wallet via the fallback, making it possible to transfer or approve the tokens.

The impact depends a lot on the future dApps that will be using the wallets. In the 'ideal' scenario, it is possible to steal all or some of the assets in the wallet. For this to happen, a lot of conditions should be met, making it hard to succees in real life.
For this I have set the severity to Medium instead of High.

1. Need to choose a target owner beforehand.
   Before the wallet is created, it is hard to guess which owner will fund the wallet with which kind of assets, and how he will use it. This makes it hard to choose a possible victim
2. Create the wallet with modified handler without raising suspicion of the owner.
   It is possible to front-run wallet creation. If the owner creates the wallet himself, their transaction will fail and raise suspicion. If a dApp automatically creates wallets for its users it might go unnoticed.
   Is is also possible to pre-create the wallet, hoping that a user will later try to create one. This could be a scenario when a user is actively using a wallet on 1 chain. It is then possible to create the same wallet with different handler on other chains and hope that one day the user will also use the wallet on other chains.
   If the new handler does not have the ERC1155, ERC777, ERC721 and IERC165 receiver functions, receiving tokens which support any of those interfaces will fail, raising suspicion.
3. You have to guess/know how the wallet will be used to be able to setup the right handler (which token, protocol, etc).
4. There can be a long time between creating the wallet and possible abuse, making it easier to detect.

Impact and example scenarios

Specific app based:

Imagine an example company wich lets users buy USDC with creditcard.
The user only needs to connect to the website to verify their address. The platform creates SmartAccount wallets for its users and users can make transactions via their platform, paying the gas in USDC.
It is then very likely that new wallets of this app will be funded with USDC.
We frontrun deployCounterFactualWallet calls made by this platform and replace handler with the USDC contract address.
When USDC is received by a wallet, we can take it by simply calling SmartAccount.transfer(....).
Since the SmartAccount contract does not have a transfer function, this will fallback and forward the call to the handler contract (USDC).

User based example

A user with a well funded wallet is actively using it on the ethereum network. The user only has normal ERC20 tokens in the wallet, and often swaps them via uniswap.
We can create a wallet with the same address on other chains, using the uniswap router contract as handler.
If one day the user, or some of the dApps he uses, moves to another chain and starts to use the wallet there, it might be possible to call the contract via fallback and steal some of the funds.

Recommended Mitigation Steps

It might be an option to change fallback manager call to a staticcall.
This removes the possibility of an exploit.
Unwanted side effect might be that it is then not possible to add events to the handler contract.

[c4-judge]

gzeon-c4 marked the issue as duplicate of #460

[c4-sponsor]

livingrockrises marked the issue as sponsor confirmed

[c4-judge]

gzeon-c4 changed the severity to 3 (High Risk)

[c4-judge]

gzeon-c4 marked the issue as satisfactory