SafeWebAuthnSignerFactory.sol is prone to reorg issues.

[hats-bug-reporter[bot]]

Github username: @shealtielanz Twitter username: shealtielanz Submission hash (on-chain): 0x470e5d8ee1810a0dd7ff7bb7efe2c0a1778eb523764646cdf77811214855578e Severity: medium

Description: Description\ In the constructor of the factory contract during deployment, it uses the non-deterministic create method to deploy the SINGLETON allowing for reorg issues during creations of signers.

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

  SafeWebAuthnSignerSingleton public immutable SINGLETON;
  
  /**
   * @audit  comments.
   */
  constructor() {
      SINGLETON = new SafeWebAuthnSignerSingleton();
  }

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

  The SINGLETON is meant to be immutable/the same across all the signers to be created in the factory, however in a case of a reorg this would not be so making previous signers created to have an invalid/incorrect SINGLETON.

Bug Scenario

• In block 1 the factory is created with an immutable SINGLETON created also during construction.
  • In block 2 a signer is created using the SINGLETON address via create2.
  • A reorg occurs from block 1 and due to the use of create the address of the SINGLETON is change to a different address, now future signers created will have a different SINGLETON used during that creation making the first signers created before the reorg occurred to be outdated.

Severity

Low Likelihood - High Impact. I think a medium severity justifiable. Mitigation use create2 for the creation of the SINGLETON in the constructor.

[nlordell]

I don't think your described issue is possible. If I understand correctly, you are saying that, in case of a reorg, SINGLETON would end up at a different address? Can you please explain this more thoroughly?

[shealtielanz]

Hi @nlordell Good day, thanks for taking the time to review this. You're right! In the case of a reorg attack on the underlying blockchain the contract will be deployed in, the address will change due to the use of create because the address is created non-deterministically with the nonce of the contract. Example of an issue like this? take a look at this.

• https://medium.com/blockchain-new-normal/whats-reorg-attack-in-smart-contracts-2ab1b9c061d0
• https://github.com/code-423n4/2023-05-maia-findings/issues/861
• https://code4rena.com/reports/2023-01-rabbithole#m-01-questfactory-is-suspicious-of-the-reorg-attack
• https://github.com/code-423n4/2023-04-frankencoin-findings/issues/985

Point being, reorgs occur usually and that in such a case the nonce used will be different causing the address of the SINGLETON to change for future signers to be created, this is a common issue with create method and it is always advised to use create2 instead.

[0xEricTee]

@shealtielanz @nlordell Reorg is not possible here as the create opcode is in constructor and createSigner function cannot be called when the contract is not yet deployed.

[nlordell]

I don't see a security issue in this case. CREATE opcode and reorgs typically have issues when there are more than one contracts being deployed this way (in which case, reorgs can cause the contracts to not be on expected addresses). However, here, only the Singleton is deployed this way so I don't see an issue.

The only way I can see a reorg causing issues with the Singleton would be for an attacker to see the factory contract deployment being reorged, and have mined a CREATE2 address collision with the factory contract in order to deploy another contract first (this incrementing the nonce of the factory contract and causing the Singleton to land on a separate address). I don't think this is a possible attack and will mark this issue as invalid.

[shealtielanz]

Hey @nlordell Reorgs can occur in multiple blocks making the attack very feasible, I don’t see why this issue is marked invalid for any reason. I appeal a low severity atleast.

[nlordell]

Reorgs can occur in multiple blocks making the attack very feasible

I still don't understand how reorgs can be abused by bad actors. A CREATE address is computed based on two things: the address of the creator (which is the SafeWebAuthnSignerFactory contract, deployed with CREATE2 with a deterministic address) and its nonce.

• The first parameter (creator) cannot be influenced by a reorg
• The second parameter can theoretically be influenced in the case where we have a CREATE2 address collision. Seeing as this is not feasible (it would be a 160 bit collision - as the attacker does not have any influence over the deployment address of the factory contract), it also cannot be changed

All in all, this means that the SINGLETON will end up on the same address.

Furthermore, I believe that your statement here is false:

now future signers created will have a different SINGLETON used during that creation making the first signers created before the reorg occurred to be outdated.

Either the reorg will cause the transaction to play before the factory contract is created, in which case the reorg will cause the signer to no longer exist, or it will play after the factory contract is created, which would cause it to be deployed to a different address, but use the correct singleton, so, AFAIU, the signers will not "be outdated". Note that this is assuming that a reorg could be used to change the value of the SINGLETON address (which we argued above that it can't).

I believe this issue to be invalid.