Biconomy - Smart Contract Wallet contest - wait's results

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.

Lines of code

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

Vulnerability details

Impact

All SmartAccount contracts can be hacked, assets can be stolen, arbitrary functions can be called by hackers.

Proof of Concept

A hacker can hack any SmartAccount by calling SmartAccount.sol#execTransaction. All the hacker needs to do is to deploy an eip-1271 contract and assemble a signature that meets the structural requirements of SmartAccount.sol#checkSignatures.

The hacker was able to call execTransaction successfully because SmartAccount.sol#checkSignatures does not check that the _signer is the owner when v is 0.

Using this vulnerability and the power of execTransaction, the hacker can make the SmartAccount to call or delegatecall any interface of any contract, including but not limited to:

• call SmartAccount.sol#setOwner to take the ownership.
• call SmartAccount.sol#transfer after setOwner to drain ETH in the wallet.
• call transfer or transferFrom of ERC20/ERC721 token contracts directly to drain assets in the wallet.
• etc

Tools Used

Manual

Recommended Mitigation Steps

I recommend to checking in SmartAccount.sol#checkSignatures to make sure _signer is the owner:

function checkSignatures(
        bytes32 dataHash,
        bytes memory data,
        bytes memory signatures
    ) public view virtual {
    // omitted
    if(v == 0) {
        // omitted
        _signer = address(uint160(uint256(r)));
        // -- added --
        require(_signer == owner, "INVALID_SIGNATURE");
        // omitted
    }
    // omitted
}

Lines of code

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

Vulnerability details

Impact

Transaction will always revert if EntryPoint invokes SmartAccount.sol#execute or SmartAccount.sol#executeBatch.

Proof of Concept

The SmartAccount.sol#execute contais a checking _requireFromEntryPointOrOwner(), while this checking will always succeed and invoking from EntryPoint will always fail because the modifier onlyOwner restricts that only owner can invoke the function:

function execute(address dest, uint value, bytes calldata func) external onlyOwner{
    _requireFromEntryPointOrOwner();
    _call(dest, value, func);
}

The SmartAccount.sol#executeBatch has the same problem.

Tools Used

Manual

Recommended Mitigation Steps

I Recommend removing the onlyOwner from SmartAccount.sol#execute and SmartAccount.sol#executeBatch, just keeping and using the checking _requireFromEntryPointOrOwner().