Attacker can brick the creation of signers rendering thr factory useless.

[hats-bug-reporter[bot]]

Github username: @shealtielanz Twitter username: shealtielanz Submission hash (on-chain): 0x206c952bef7f13b55cfc44a6bf5945ffd021bfaa60a8030113e1516614386332 Severity: high

Description: Description

On the call to createSigner() the function uses the opcode extecodesize() to check if the contract has been created, however, an attacker can trick the factory into thinking that the signer has already been created when actually it hasn't been created, thereby making the calls to createSigner() useless as the signer cannot be created anymore.

• https://github.com/safe-global/safe-modules/blob/9a18245f546bf2a8ed9bdc2b04aae44f949ec7a0/modules/passkey/contracts/SafeWebAuthnSignerFactory.sol#L51C1-L59C6

  function createSigner(uint256 x, uint256 y, P256.Verifiers verifiers) external returns (address signer) {
      signer = getSigner(x, y, verifiers);
  
  @>      if (_hasNoCode(signer)) {
          SafeWebAuthnSignerProxy created = new SafeWebAuthnSignerProxy{salt: bytes32(0)}(address(SINGLETON), x, y, verifiers);
          assert(address(created) == signer);
          emit Created(signer, x, y, verifiers);
      }
  }

• https://github.com/safe-global/safe-modules/blob/9a18245f546bf2a8ed9bdc2b04aae44f949ec7a0/modules/passkey/contracts/SafeWebAuthnSignerFactory.sol#L94C1-L99C6

  function _hasNoCode(address account) internal view returns (bool result) {
      // solhint-disable-next-line no-inline-assembly
      assembly ("memory-safe") {
  @>         result := iszero(extcodesize(account))
      }
  }

this iszero(extcodesize(account)) check is insufficient to determine if an address has an existing code. According to EIP-1052, addresses without code only return a 0x0 codehash when they are empty:

In case the account does not exist or is empty (as defined by EIP-161) 0 is pushed to the stack.

In case the account does not have code the keccak256 hash of empty data (i.e. c5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470) is pushed to the stack.

As seen from above, addresses without code can also return keccak256("") as its codehash if it is non-empty. EIP-161 states that an address must have a zero ETH balance for it to be empty:

An account is considered empty when it has no code and zero nonce and zero balance.

As such, if anyone transfers 1 wei to an address, .codehash will return keccak256("") instead of bytes32(0), making the checks shown above skip incorrectly.

Attack Scenario

• A caller calls createSigner() with the given parameters to create the signer contract.
• An attacker front runs and computes the signer's address since it is deterministic and deposits one wei into it.
• on the internal call to _hasNoCode() the contract will assume that the address already has a code and skip creation.
• An attacker can create a bot to compute addresses to be created and send 0ne wei to them before the function is called bricking the factory contract with little to no cost.

Mitigation

Fix for this is easy - just change from result := iszero(extcodesize(account)) to x.code.length != 0 (ie. Check the code length of the address instead.

[0xEricTee]

I've tested with foundry test, the mentioned issue does not exist.

EXTCODESIZE.t.sol:

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.24;

import {Test, console2} from "forge-std/Test.sol";

contract Testing {

}

contract EXTCODESIZETest is Test {

    address public user1;
    address public user2;
    Testing public testing_contract;

    function _hasNoCode(address account) public view returns (bool result) {
        // solhint-disable-next-line no-inline-assembly
        assembly ("memory-safe") {
            result := iszero(extcodesize(account))
        }
    }

    function setUp() public {
        user1 = makeAddr("USER1");
        user2 = makeAddr("USER2");

        deal(user2, 1); //fund user2 with 1 wei.
        testing_contract = new Testing();

    }

    function test_AccountHasNoCode() public {
        console2.log(_hasNoCode(user1)); //true

        console2.log("ETH balance of user 2: ", user2.balance);

        console2.log(_hasNoCode(user2));

        console2.log(_hasNoCode(address(testing_contract)));

    }

}

Result:

Running 1 test for test/EXTCODESIZE.t.sol:EXTCODESIZETest
[PASS] test_AccountHasNoCode() (gas: 18585)
Logs:
  true
  ETH balance of user 2:  1
  true
  false

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.82ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Noticed that the function _hasNoCode returned true for user2 with balance of 1 wei, therefore the DoS situation is not valid.

[nlordell]

extcodesize is for getting the code length of an external account, and thus equivalent to account.code.length.

I will mark this as invalid.