SmartAccountFactory.sol - Account can be created for an owner and setting any entryPoint

[code423n4]

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding.

Proof of Concept

The entrypoint is being set in the init(args...) function of SmartAccount.sol. The problem is that the malicious users could create wallets for legitimate owners of wallets and set the entrypoint contract to be anything they want.

What's more, a wallet creation for a user could be frontran by a malicious actor, returning exactly the same address as expected, but with a different entry point. The reason is that the entryPoint is not being used as the salt.

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

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

Recommended Mitigation Steps

Add entrypoint to salt.

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

[c4-judge]

gzeon-c4 marked the issue as duplicate of #460

[livingrockrises]

If you recommend to add entryPoint to the salt then handler should also be added to the salt.

Additional notes: If the owner address is changed it doesn't affect the user as only that specific owner will be the controller of the smart account. If for a counterfactual wallet entry point initialised is different than intended, then the owner can update the entry point on their own smart account to a right one. But I agree this should be evaluated..

[c4-sponsor]

livingrockrises marked the issue as disagree with severity

[c4-sponsor]

livingrockrises marked the issue as sponsor confirmed

[livingrockrises]

agree with severity as per proof shown in issues like #460 but lack of proof here

[c4-sponsor]

livingrockrises requested judge review

[c4-judge]

gzeon-c4 marked the issue as partial-50

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[c4-judge]

gzeon-c4 changed the severity to 3 (High Risk)