search

code-423n4 / 2023-01-biconomy-findings

10 stars 10 forks source link

A malicious user can make other's smart wallet invoke any external contract by deploying the smart wallet #407

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

A malicious user can initiate an arbitrary external call in the role of other user's smart wallet, which is equivalent to stealing a smart wallet and sending a transaction with it. This can lead to fund being stolen from the smart wallet directly, may also lead to authority, funds, function problems of other contracts.

Proof of Concept

If a smart wallet has not been deployed or is being deployed, a hacker can launch an attack by deploying the wallet directly or front-running the deployment.

The following lists some possible attack scenarios.

If a hacker wants to steal USDT from a smart wallet:

  1. The hacker invokes SmartAccountFactory.sol#deployCounterFactualWallet to deploy the smart wallet contract and sets _handler to the USDT contract address.
  2. The hacker invokes the ERC20 - approve function of the smart contract wallet.
  3. The hacker invokes the ERC20 - transferFrom function of the USDT contract to steal all the USDT from the smart wallet.

The principle of step 2 is: The smart contract wallet itself does not implement the ERC20 - approve function, so the invoking will go into FallbackManager.sol#fallback. Because in step 1, the hacker has already set the fallback handler to the USDT contract address when deploying the smart wallet, so eventually the smart wallet will call the approve function of USDT contract in FallbackManager.sol#fallback.

If a hacker wants to steal some ERC721 assets from a smart wallet: Similarly, the hacker just need to set the fallback handler to the ERC721 contract address in step 1, and invokes ERC721 - setApprovalForAll or ERC721 - transferFrom in step 2.

If a smart wallet manages the assets or functions of another contract: Similarly, the hacker just need to set the fallback handler to the target contract address in step 1, and invokes the specific function in step 2.

Tools Used

Manual

Recommended Mitigation Steps

I recommend removing the input parameter _handler from SmartAccountFactory.sol#deployCounterFactualWallet and SmartAccountFactory.sol#deployWallet, and using a fixed verified DefaultCallbackHandler contract instead.

c4-judge commented 1 year ago

gzeon-c4 marked the issue as duplicate of #460

c4-sponsor commented 1 year ago

livingrockrises marked the issue as sponsor confirmed

c4-judge commented 1 year ago

gzeon-c4 marked the issue as satisfactory