Biconomy - Smart Contract Wallet contest - HE1M's results

Lines of code

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

Vulnerability details

Impact

Uninitialized implementation can lead to self destruction, and stop the proxies from normal working.

Proof of Concept

The SmartAccount.sol is not initialized during construction (no constructor is available). So, anybody can call the function init for the first time and set the important variables _owner, _entryPointAddress, and _handler to a malicious address.

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(""));
    }

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

Then, the malicious address can call the function execFromEntryPoint:

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

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

This function will call the function execute in which it delegate calls to an address deployed by the attacker which has selfdestruct function. So, the SmartAccount.sol will be destructed, and all the proxies who point to this contract as an implementation will not working.

function execute(
        address to,
        uint256 value,
        bytes memory data,
        Enum.Operation operation,
        uint256 txGas
    ) internal returns (bool success) {
        if (operation == Enum.Operation.DelegateCall) {
            // solhint-disable-next-line no-inline-assembly
            assembly {
                success := delegatecall(txGas, to, add(data, 0x20), mload(data), 0, 0)
            }
        } else {
            // solhint-disable-next-line no-inline-assembly
            assembly {
                success := call(txGas, to, value, add(data, 0x20), mload(data), 0, 0)
            }
        }
        // Emit events here..
        if (success) emit ExecutionSuccess(to, value, data, operation, txGas);
        else emit ExecutionFailure(to, value, data, operation, txGas);
    }

https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/base/Executor.sol#L13

Tools Used

Recommended Mitigation Steps

The following piece of code should be added to the contract SmartAccount.sol:

constructor(){
        init(address(this), address(this), address(this));
    }

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/SmartAccountFactory.sol#L43

Vulnerability details

Impact

It is possible to intermediate in creation of a wallet and sets the entryPoint to a malicious address and then stealing funds or self destructing the wallet by invoking the functions which are only allowed to be accessed by entryPoint.

Proof of Concept

Since the smart contract wallet address is counterfactual i.e., it can be generated off-chain without actually deploying the code, user onboarding is super frictionless where the user is shown the smart wallet address as soon as she connects her wallet (EOA).

In the case user pays the wallet creation cost, the user needs to deposit the funds in the smart contract address to be able to pay for the smart contract wallet creation fee.

The user deposits funds using a fiat-onramp, centralized exchange, or from any other EOA on the same chain. In this case, the wallet will be created when the user does his first transaction from the wallet. Biconomy SDK will identify this case and appends a wallet creation transaction along with the user’s first action.

Suppose Alice (an honest user) deposits funds into her smart wallet address (which is not deployed yet. This is a normal behavior of the protocol as described in the documentation and referenced above). After some time, she sends the signed userOp to be handled by the entryPoint.

Bob (a malicious user) immediately deploys the smart wallet of Alice on the expected address with malicious _entryPoint parameter by calling deployCounterFactualWallet or deployWallet.

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

https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/SmartAccountFactory.sol#L33

function deployWallet(address _owner, address _entryPoint, address _handler) public returns(address proxy){ 
        bytes memory deploymentData = abi.encodePacked(type(Proxy).creationCode, uint(uint160(_defaultImpl)));
        // solhint-disable-next-line no-inline-assembly
        assembly {
            proxy := create(0x0, add(0x20, deploymentData), mload(deploymentData))
        }
        BaseSmartAccount(proxy).init(_owner, _entryPoint, _handler);
        isAccountExist[proxy] = true;
    }

https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/SmartAccountFactory.sol#L53

The parameters provided by Bob are:

• _owner: Alice's address
• _entryPoint: Bob's address
• _handler: Bob's address
• _index: the index used by the protocol when predicting the address of Alice's wallet (it can be captured by Bob also through front-running the relayer during creating the Alice's wallet).

Please note that it is possible to give Bob's address as _entryPoint parameter, because changing _entryPoint or _handler does not change the address of the smart wallet. They are only parameters for initializing the wallet.

After Bob deploys the smart wallet for Alice with his own address as entryPoint, he can call the functions that are only allowed by entryPoint:

• Bob can call the function validateUserOp with the parameters and signatures already provided by Alice to pass the checks inside this function. Then, he can steal the already deposited fund in this wallet by giving a nonzero value to missingAccountFunds as parameter.

function validateUserOp(UserOperation calldata userOp, bytes32 userOpHash, address aggregator, uint256 missingAccountFunds)
    external override virtual returns (uint256 deadline) {
        _requireFromEntryPoint();
        deadline = _validateSignature(userOp, userOpHash, aggregator);
        if (userOp.initCode.length == 0) {
            _validateAndUpdateNonce(userOp);
        }
        _payPrefund(missingAccountFunds);
    }

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

function _payPrefund(uint256 missingAccountFunds) internal virtual {
        if (missingAccountFunds != 0) {
            (bool success,) = payable(msg.sender).call{value : missingAccountFunds, gas : type(uint256).max}("");
            (success);
            //ignore failure (its EntryPoint's job to verify, not account.)
        }
    }

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

• Bob can call the function execFromEntryPoint to steal all the funds and even self destruct the wallet contract.

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

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

function execute(
        address to,
        uint256 value,
        bytes memory data,
        Enum.Operation operation,
        uint256 txGas
    ) internal returns (bool success) {
        if (operation == Enum.Operation.DelegateCall) {
            // solhint-disable-next-line no-inline-assembly
            assembly {
                success := delegatecall(txGas, to, add(data, 0x20), mload(data), 0, 0)
            }
        } else {
            // solhint-disable-next-line no-inline-assembly
            assembly {
                success := call(txGas, to, value, add(data, 0x20), mload(data), 0, 0)
            }
        }
        // Emit events here..
        if (success) emit ExecutionSuccess(to, value, data, operation, txGas);
        else emit ExecutionFailure(to, value, data, operation, txGas);
    }

https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/base/Executor.sol#L13

Tools Used

Recommended Mitigation Steps

Maybe it is better to include _entryPoint and _handler in the salt as well, so if Bob gives his own address as entryPoint, the created address will be different than the address Alice is expecting (and depositing fund to).

function deployCounterFactualWallet(address _owner, address _entryPoint, address _handler, uint _index) public returns(address proxy){
        bytes32 salt = keccak256(abi.encodePacked(_owner, address(uint160(_index)), _entryPoint, _handler));
        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;
    }

Lines of code

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

Vulnerability details

Impact

Replay attack protection is not used for 2D nonces, so a malicious user can replay the user's transactions many times.

Proof of Concept

• Suppose Alice (an honest user) signed 2 transactions TX0 and TX1, with nonces 0 and 1, respectively.

• Then the relayer executes these transactions one by one by assigning the batchId equal to 10 for example. So, the parameters for these calls will be:

  first call:

  • _tx: TX0
  • batchId: 10
  • refundInfo: the valid data provided by Alice
  • signatures: Alice's signature of TX0 with nonce 0

  second call:

  • _tx: TX1
  • batchId: 10
  • refundInfo: the valid data provided by Alice
  • signatures: Alice's signature of TX1 with nonce 1

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

• Please note that during the first call we have: nonces[batchId] = nonces[10] = 0 and during the second call (as the nonce for the same batch id is incremented by one) we have: nonces[batchId] = nonces[10] = 1. Since Alice signed the transaction data, refund data, and nonce, the checkSignatures will pass.

{
            bytes memory txHashData =
                encodeTransactionData(
                    // Transaction info
                    _tx,
                    // Payment info
                    refundInfo,
                    // Signature info
                    nonces[batchId]
                );
            // Increase nonce and execute transaction.
            // Default space aka batchId is 0
            nonces[batchId]++;
            txHash = keccak256(txHashData);
            checkSignatures(txHash, txHashData, signatures);
        }

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

• Bob (a malicious user) calls this function with the following parameters:

  • _tx: TX0
  • batchId: 50 (any batchId that gives nonces[batchId] = 0, I am assuming the batck id 50 was never used before so it's nonce will be zero)
  • refundInfo: the valid data provided by Alice
  • signatures: Alice's signature of TX0 with nonce 0

• This transaction will be executed and the checkSignatures will pass. Although the batch id is different, the signature is passed as the nonce is important (because Alice signs the transaction including the nonce not the batch id).

• Bob can replay the second transaction as well with batch id 50 , because the nonce now is 1, so it matches to what Alice singed for TX1.

• Bob can replay this transaction many times because the mapping mapping(uint256 => uint256) public nonces; has a lot of keys with zero value.

The vulnerability is that the replay attack protection is not used for 2D nonces.

Tools Used

Recommended Mitigation Steps

Maybe it is better to track the consumed transactions:

function execTransaction(
        Transaction memory _tx,
        uint256 batchId,
        FeeRefund memory refundInfo,
        bytes memory signatures
    ) public payable virtual override returns (bool success) {
        // ...
        require(!isConsumed[txHash], "it is already consumed");
        isConsumed[txHash] = true;
    }

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L68

Vulnerability details

Impact

In summary, a malicious user can add his UserOperation to the mempool of bundler, and then by front-running the bundler and withdrawing the gas refund required for his operation to be executed, he can cause that the bundler lose huge gas. If his operation is going to be refunded by his smart account, then by front-running and withdrawing his deposit, all the gas consumed for all ops account prepayment validation will be deducted from the bundler without refund. If his operation is going to be refunded by paymaster, then by front-running and withdrawing his deposit as paymaster, all the gas consumed for all ops account prepayment validation and paymaster prepayment validation will be deducted from the bundler without refund.

Proof of Concept

A bundler is going to relay UserOperation to the EntryPoint contract. The bundler picks UserOperations from the high-level UserOperation mempool (which is not the same as the regular transaction mempool). Suppose there are 50 UserOpertions from various sources (ops1 for user1, ops2 for user2, ...., and ops50 for Bob as a malicious user). So, the function handleOps will be called to validate and execute these operations. https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L68

Please note that the function simulateValidation is to check whether the account and paymaster prepayment are valid before the function handleOps is called. https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L226

There are two scenarios:

First Scenario:

Bob has already provided the required gas refund for his operation (ops50) by calling the function depositTo(address account) in which the account is the address of Bob's smart account. So, we will have deposits[Bob's account].deposit = gas refund.

https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/StakeManager.sol#L48

Bob has sets paymaster = address(0) in his ops50 that means the smart account is going to refund the gas not the paymaster. Since there is enough deposit in the EntryPoint contract by Bob, the simulation of validation shows that ops50 is a valid operation. So, the function handleOps will be called.

Bob, notices this function call in the mempool, and applies front-run attack and calls the function execute.

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

He provides the following parameters:

• dest: address of EntryPoint
• value: 0
• func: abi.encodeWithSignature("withdrawTo(address,uint256)", Bob's EOA address, the amount was deposited before)

By doing so, Bob withdraws the deposits done before. So, we will have deposits[Bob's account].deposit = 0.

Then, it is the turn of the handleOps transaction to be executed. It goes over account prepayment validation process of all the ops from 1 to 49, and when it reaches to ops50, it sees that the paymaster is zero-address, so it checks the balance of Bob's account to transfer fund to EntryPoint contract for gas refund is enough or not.

https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L316 https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L319 https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/BaseSmartAccount.sol#L67 https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/BaseSmartAccount.sol#L108

Since the balance of Bob's account is zero, the call will be unsuccessful, so no fund will be transferred to EntryPoint contract. Finally, the validation of Bob's ops50 will fail. This failure, will revert all the validation of the previous ops from 1 to 49. So, the bundler has lost lots of gas during validation of ops 1 to 50. https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L334

Second Scenario:

Bob (the malicious user) has already provided the required gas refund for his operation (ops50) by calling the function depositFor(address paymasterId) in which the paymasterId is the address of Bob's EOA. So, we will have deposits[VerifyingSingletonPaymaster contract].deposit += gas refund in the EntryPoint contract and paymasterIdBalances[Bob's EOA] = gas refund in VerifyingSingletonPaymaster contract.

https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/paymasters/verifying/singleton/VerifyingSingletonPaymaster.sol#L48 https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/StakeManager.sol#L48

Bob has sets paymaster = VerifyingSingletonPaymaster contract in the ops50 that means the paymaster is going to refund the gas not the smart account. Since there is enough deposit in the EntryPoint contract by other paymasters as well as Bob's EOA as paymaster, the simulation of validation shows that it is a valid operation. So, the function handleOps will be called.

Bob, notices this function call in the mempool, and applies front-run attack and calls the function withdrawTo to withdraw his deposit as a paymaster. So, we will have deposits[VerifyingSingletonPaymaster contract].deposit -= gas refund in the EntryPoint contract and paymasterIdBalances[Bob's EOA] = 0 in VerifyingSingletonPaymaster contract.

Then, it is the turn of the handleOps transaction to be executed. After it goes over account prepayment validation, it goes over paymaster prepayment validation process of all the ops from 1 to 49, and when it reaches to ops50, it checks whether there is enough total deposit in EntryPoint contract or not (The transaction may revert here. But it most probably will not revert here, because there are other paymasters that have already deposited into EntryPoint contract, which are more than enough). https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L359

After the paymaster prepayment validation is done successfully, the function _executeUserOp is called for all 50 ops, in which it calls innerHandleOp, then it calls _handlePostOp, then postOp, and finally _postOp. https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L48 https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L168 https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L440 https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/paymasters/BasePaymaster.sol#L31 https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/paymasters/verifying/singleton/VerifyingSingletonPaymaster.sol#L119

In the function _postOp, the transaction will fail because the paymasterIdBalances[Bob's EOA] = 0, and underflow error happens. This failure reverts all other executed ops 1 to 49, which results in loss of huge gas from bundler.

Third Scenario:

Bob, notices handleOps function call in the mempool, and applies front-run attack and calls the function handleOps to execute only the ops50. After the ops50 is executed, the nonce of Bob's smart account will be incremented. This failure reverts all the account prepayment validation process and the gas consumed will not be refunded to the bundler.

Then, it is the turn of the handleOps transaction to be executed. It goes over account prepayment validation process of all ops from 1 to 49. When it reaches to ops50, it fails because this operation does not match the expected nonce.

https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L319 https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/BaseSmartAccount.sol#L60 https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L501

Tools Used

Recommended Mitigation Steps

try-catch is needed for validation and execution of each UserOperation in the function handleOps: https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L68

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L460 https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L465 https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/aa-4337/samples/SimpleAccount.sol#L60 https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/aa-4337/samples/SimpleAccount.sol#L68

Vulnerability details

Impact

The wrong extra modifier onlyOwner() caused that the functions execute and executeBatch are not accessible through the entryPoint. While this modifier is removed from these functions in the contract SimpleAccount.sol (which is used for tests), it is not removed from these functions in the contract SmartAccount.sol (which is the main contract).

Proof of Concept

The modifier onlyOwner() only accepts the owner as a sender, so the check _requireFromEntryPointOrOwner is useless. Since these functions should be allowed to be called by entryPoint as well, the modifier onlyOwner() should be removed.

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

    function executeBatch(address[] calldata dest, bytes[] calldata func) external onlyOwner{
        _requireFromEntryPointOrOwner();
        require(dest.length == func.length, "wrong array lengths");
        for (uint i = 0; i < dest.length;) {
            _call(dest[i], 0, func[i]);
            unchecked {
                ++i;
            }
        }
    }

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

Tools Used

Recommended Mitigation Steps

Removing the modifier OnlyOwner() from these functions.

No. 1 Two-step authentication

Setting the owner should be done through two-step.

function setOwner(address _newOwner) external mixedAuth {
        require(_newOwner != address(0), "Smart Account:: new Signatory address cannot be zero");
        address oldOwner = owner;
        owner = _newOwner;
        emit EOAChanged(address(this), oldOwner, _newOwner);
    }

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

Should be:

function setPendingOwner(address _newPendingOwner) external mixedAuth {
    require(_newPendingOwner != address(0));
    pendingOwner = _newPendingOwner;
}

function acceptOwnership() external {
    require(msg.sender == pendingOwner);
    owner = msg.sender;
    pendingOwner = address(0);
}

No. 2 Grieving by already deployed smart wallet contract

In the normal scenario, Alice sends fund to her smart wallet (which is not created yet) and then after some time sends the dapp transaction to the SDK. The SDK sends batch of transactions (create wallet and dapp transaction) to the relayer. The relayer first creates the wallet by calling the SmartAccountFactory.sol, and then sends the dapp transaction to the created wallet.

In the malicious scenario, Alice sends fund to her smart wallet (which is not created yet). Then Bob (a malicious user) calls the function deployCounterFactualWallet to create a smart wallet for Alice (with the same parameters that are used to predict the Alice's smart wallet address), so the smart wallet is deployed for Alice on the expected address. Then after some time Alice sends the dapp transaction to the SDK. The SDK sends batch of transactions (create wallet and dapp transaction) to the relayer. When the ralayer tries to create the wallet for Alice, it reverts, because it is already created by Bob.

Better to modify as:

function deployCounterFactualWallet(address _owner, address _entryPoint, address _handler, uint _index) public returns(address){
        bytes32 salt = keccak256(abi.encodePacked(_owner, address(uint160(_index))));
        bytes memory deploymentData = abi.encodePacked(type(Proxy).creationCode, uint(uint160(_defaultImpl)));

        bytes32 hash = keccak256(abi.encodePacked(bytes1(0xff), address(this), salt, keccak256(deploymentData)));
        address _wallet = address(uint160(uint(hash)));
        if(isAccountExist[_wallet]){
            return _wallet;
        }

        // solhint-disable-next-line no-inline-assembly
        address proxy;
        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;
        return proxy;
    }