Proxy creation isn't check in `deployWallet` function of `SmartAccountFactory` contract

[code423n4]

Lines of code

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

Vulnerability details

The deployWallet function present in the SmartAccountFactory contract deploys a new wallet by creating a Proxy that points to a base implementation using assembly.

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

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

The call to create may fail, in which case the result of proxy will be 0. Citing evm.codes from CREATE opcode section:

Stack output
address: the address of the deployed contract, 0 if the deployment failed.

Since the result value of the create call isn't checked, the proxy creation can silently fail while the deployWallet function still succeeds.

Impact

If the call to create fails, then the wallet (the Proxy itself) won't exist while the enclosing call to deployWallet will be successful.

Note that the call to init in line 59 will succeed too, since the address(0) has no code. Citing again evm.codes from the CALL opcode section:

Creates a new sub context and execute the code of the given account, then resumes the current one. Note that an account with no code will return success as true.

This means that the call to deployWallet will succeed and return the address(0) as the wallet's address. This will potentially cause loss of funds, as the user or any other integration may inadvertently send funds to this address.

PoC

1. User calls SmartAccountFactory.deployWallet with valid parameters
2. Call to create on line 57 fails and returns 0.
3. Call to init on 59 still succeeds as the address(0) has no code.
4. Function terminates correctly, and returns address(0) as the result.

Recommendation

Add a check to verify that the call to create succeeded (proxy != address(0)).

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))
    }
    require(proxy != address(0), "Create call failed");
    BaseSmartAccount(proxy).init(_owner, _entryPoint, _handler);
    isAccountExist[proxy] = true;
}

[c4-judge]

gzeon-c4 marked the issue as primary issue

[gzeoneth]

I am not quite sure if CREATE will revert or return 0, but even then seems to be state handling issue and low risk.

[iFrostizz]

The only time a create will fail is that if the creation code reverts at some point. As it's not subject to change, then there is no instance when the create returns an address(0).

[c4-sponsor]

livingrockrises marked the issue as sponsor disputed

[c4-sponsor]

livingrockrises requested judge review

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory: Invalid