Checks in `isContract()` can be bypassed using CREATE2 which can break several functionalities

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/libs/LibAddress.sol#L11-L16 https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/paymasters/verifying/singleton/VerifyingSingletonPaymaster.sol#L48-L53 https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L120-L125

Vulnerability details

Proof of Concept

https://consensys.github.io/smart-contract-best-practices/development-recommendations/solidity-specific/extcodesize-checks/

The function isContract() just checks code length at the address provided.

https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/libs/LibAddress.sol#L11-L16

  11-16: function isContract(address account) internal view returns (bool) { 
    uint256 csize;
    // solhint-disable-next-line no-inline-assembly++
    assembly { csize := extcodesize(account) } 
    return csize != 0;
  }

Attacker can interact with the system and selfdestruct his contract, and with help of CREATE2 recreate it at same address when he needs to interact with the system again.

Several functionalities will break and will provide wrong results. For instance depositFor() function makes use of isContract() as follows:

https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/paymasters/verifying/singleton/VerifyingSingletonPaymaster.sol#L48-L53

function depositFor(address paymasterId) public payable {
        require(!Address.isContract(paymasterId), "Paymaster Id can not be smart contract address");
        require(paymasterId != address(0), "Paymaster Id can not be zero address");
        paymasterIdBalances[paymasterId] += msg.value;
        entryPoint.depositTo{value : msg.value}(address(this));
    }

As it can be observed paymasterId can not be a smart contract. However an attacker can interact with the system and selfdestruct his contract, pass the require check ( require(!Address.isContract(paymasterId)) and with help of CREATE2 recreate it at same address when he needs to interact with the system again.

similarly the following can also produce unexpected results:

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

    120-125: function updateImplementation(address _implementation) external mixedAuth {
        require(_implementation.isContract(), "INVALID_IMPLEMENTATION");
        _setImplementation(_implementation);
        // EOA + Version tracking
        emit ImplementationUpdated(address(this), VERSION, _implementation);
    }

Recommended Mitigation Steps

A whitelist of valid addresses can be maintained.

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory: Insufficient proof

[c4-sponsor]

livingrockrises marked the issue as disagree with severity

[c4-sponsor]

livingrockrises marked the issue as sponsor disputed