Biconomy - Smart Contract Wallet contest - Jayus'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 https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccountFactory.sol#L53-L61 https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L127-L131

Vulnerability details

Impact

Smart Contract Wallet can be created and initialized with the wrong entry-point address. This will lead to the denial of wallet services and the loss of user funds sent to the wallet.

Proof of Concept

New smart contract wallets can be deployed by either calling deployCounterFactualWallet or deployWallet in SmartAccountFactory.sol but let's focus on the former.

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;
    }

The _entryPoint parameter which is supposed to be a contract address pointing to the entryPoint contract is not validated during Smart Contract Wallet creation. Though smartAccount.sol does have updateEntryPoint method, it equally lacks enough validation to ensure the right entryPoint address is set by the user, basically allowing users to shoot themselves in the foot. Aside that, a user could easily forget to update the entryPoint address in smartAccount.sol and calling the addDeposit method will lead to user's funds getting locked in the wrong entryPoint address.

Here is a sample test both checking wrong parameters can be used for SCW deployment and funds will be locked in the wrong entryPoint address if addDeposit is called and passed some ether.

Test file

/* eslint-disable prettier/prettier */
import { expect } from "chai";
import { ethers, waffle } from "hardhat";
import {
  SmartAccount,
  SmartAccountFactory,
  EntryPoint,
  DefaultCallbackHandler,
// eslint-disable-next-line node/no-missing-import
} from "../../typechain";


describe.only("Wallet Deployment vulnerabilities", function () {
  let baseImpl: SmartAccount;
  let walletFactory: SmartAccountFactory;
  let entryPoint: EntryPoint;
  let owner: string;
  let bob: string;
  let userSCW: any;
  let handler: DefaultCallbackHandler;
  let accounts: any;

  beforeEach(async () => {
    accounts = await ethers.getSigners();

    owner = await accounts[0].getAddress();
    bob = await accounts[1].getAddress();

    const BaseImplementation = await ethers.getContractFactory("SmartAccount");
    baseImpl = await BaseImplementation.deploy();
    await baseImpl.deployed();
    console.log("base wallet impl deployed at: ", baseImpl.address);

    const SmartAccountFactory = await ethers.getContractFactory(
      "SmartAccountFactory"
    );
    walletFactory = await SmartAccountFactory.deploy(baseImpl.address);
    await walletFactory.deployed();
    console.log("wallet factory deployed at: ", walletFactory.address);

    const EntryPoint = await ethers.getContractFactory("EntryPoint");
    entryPoint = await EntryPoint.deploy();
    await entryPoint.deployed();
    console.log("Entry point deployed at: ", entryPoint.address);

    const DefaultHandler = await ethers.getContractFactory(
      "DefaultCallbackHandler"
    );
    handler = await DefaultHandler.deploy();
    await handler.deployed();
    console.log("Default callback handler deployed at: ", handler.address);
  });

  it("Should check wrong parameters can be used for wallet deployment", async function () {
    const fakeEntryPoint = '0x000000000000000000000000000000000000FACE'
    const expected = await walletFactory.getAddressForCounterfactualWallet(
      owner,
      0
    );
    console.log("deploying new wallet..expected address: ", expected);

    await walletFactory.deployCounterFactualWallet(
      owner,
      fakeEntryPoint,
      handler.address,
      0
    );
   

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


    const deployedSCWEntryPoint = await userSCW.entryPoint();
    console.log('Deployed SCW entry-Point address: ', deployedSCWEntryPoint)
      
    expect(deployedSCWEntryPoint).to.not.equal(entryPoint.address);
  });
    
  it("Wrong entryPoint locks user funds", async function () {
    const fakeEntryPoint = '0x000000000000000000000000000000000000FACE'
    const expected = await walletFactory.getAddressForCounterfactualWallet(
        owner,
        0
    );
    console.log("deploying new wallet..expected address: ", expected);
 
    await walletFactory.deployCounterFactualWallet(
        owner,
        fakeEntryPoint,
        handler.address,
        0
    );

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

    console.log("userSCW address: ", userSCW.address)
    console.log('Deployed SCW entry-Point address: ', entryPoint)
    console.log('balance of entryPoint before transfer: ', await waffle.provider.getBalance(entryPoint))

    // funds sent to wrong entryPoint
    const depositTx = userSCW.addDeposit({from: owner, value: ethers.utils.parseEther('1')})
    await expect(depositTx).not.to.be.reverted;
    
    console.log('balance of entryPoint after transfer: ', await waffle.provider.getBalance(entryPoint))

    // owner cannot withdraw funds from wrong entry point address 
    const withdrawAmount = ethers.utils.parseEther('1')
      const withdrawTx = userSCW.withdrawDepositTo(bob, withdrawAmount, { from: owner })
      await expect(withdrawTx).to.be.reverted;
      console.log('balance of entryPoint after failed withdrawal: ', await waffle.provider.getBalance(entryPoint)) 
  })
    
});

To run the test above, simply copy and save to a file in 2023-01-biconomy/scw-contracts/test/smart-wallet folder. Run npx hardhat test

Output of test

  Wallet Deployment vulnerabilities
base wallet impl deployed at:  0x5FbDB2315678afecb367f032d93F642f64180aa3
wallet factory deployed at:  0xe7f1725E7734CE288F8367e1Bb143E90bb3F0512
Entry point deployed at:  0x9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0
Default callback handler deployed at:  0xCf7Ed3AccA5a467e9e704C703E8D87F634fB0Fc9
deploying new wallet..expected address:  0x47898d14b97867D0452504589497Ef23985cce4C
userSCW address:  0x47898d14b97867D0452504589497Ef23985cce4C
Deployed SCW entry-Point address:  0x000000000000000000000000000000000000Face
    ✔ Should check wrong parameters can be used for wallet deployment (159ms)
base wallet impl deployed at:  0x5FC8d32690cc91D4c39d9d3abcBD16989F875707
wallet factory deployed at:  0x0165878A594ca255338adfa4d48449f69242Eb8F
Entry point deployed at:  0xa513E6E4b8f2a923D98304ec87F64353C4D5C853
Default callback handler deployed at:  0x2279B7A0a67DB372996a5FaB50D91eAA73d2eBe6
deploying new wallet..expected address:  0x3eE85337b606b6d8F501b5bd498fEd26f56264b6
userSCW address:  0x3eE85337b606b6d8F501b5bd498fEd26f56264b6
Deployed SCW entry-Point address:  0x000000000000000000000000000000000000Face
balance of entryPoint before transfer:  BigNumber 0
balance of entryPoint after transfer:  BigNumber 1000000000000000000
balance of entryPoint after failed withdrawal:  BigNumber 1000000000000000000
    ✔ Wrong entry Point locks user funds (357ms)


  2 passing (3s)

Tools Used

Hardhat

Recommended Mitigation Steps

Considering only one entryPoint is needed per chain, the valid entry-point address should be declared in smartAccountFactory.sol. New deployments will have to have the entryPoint address parameter validated against it.

For updating the entryPoint address in smartAccount.sol, it should be checked for codesize > 0 or check if the new entryPoint being set is a valid contract on chain. This is not a 100% bulletproof solution but it does reduce the risk. The method could also be restricted to a multisig contract incharge of updating the entryPoint address or should be completely removed if feasible.