Frontrunning and DOS possible with signer creation in `SafeWebAuthnSignerFactory.sol`

[hats-bug-reporter[bot]]

Github username: @0xRizwan Twitter username: 0xRizwann Submission hash (on-chain): 0x9fc9deb56a92f2424bc44de75850d1599eb64cf924da445f14082b576b757278 Severity: medium

Description: Description\

In SafeWebAuthnSignerFactory.sol contract, createSigner() is used to create the signer.

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

createSigner() function has used CREATE2. This function has used salt which means that a malicious actor can prevent a user from deploying a boreWell creation by frontrunning it with the same "salt".

Additionally, In future if this protocol is being deployed on Multichains like optimism, arbitrum, polygon additional, more-critical vulnerabilities become possible via reorg attacks.

The "salt" is a bytes32 value that is used in signer creation call and its hardcoded to bytes32(0). This enables frontrunning to occur in the following way:

1) When the createSigner is called by user. An attacker monitors the mempool for pending transactions that involve with signer creation via SafeWebAuthnSignerFactory.sol 2) Upon spotting such a transaction, The attacker quickly submits their own transaction with a higher gas price, attempting to create the signer with the same input arguments provided by users and with same hardcoded salt.

3) If the transaction got successful, the attacker's transaction is mined first, and the signers are created with orginal inputs provided by users. The original transaction will likely fail.

Recommendations\ Use a salt that includes the msg.sender. That way it is not possible to front-run the transaction.

    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);
+            SafeWebAuthnSignerProxy created = new SafeWebAuthnSignerProxy{salt: keccak256(abi.encode(msg.sender)}(address(SINGLETON), x, y, verifiers);
            assert(address(created) == signer);
            emit Created(signer, x, y, verifiers);
        }
    }

[nlordell]

Hmm, I'm trying to understand how this front-running could be harmful to the user in this case. In the steps you highlighted above, I would expect that the if (_hasNoCode(signer)) branch would be skipped, but the call would return the same address (it should be idempotent). In this case, the user should be happy that they pay less gas fees right?

[nlordell]

As discussed, I do not think this is an issue, and lacking further clarifications I will mark this as invalid for now. Please comment if you believe me to be mistaken.

[0xRizwan]

@nlordell Sorry for late reply, for the condition if (_hasNoCode(signer)), signer is wallet address it deploys SafeWebAuthnSignerProxy contract with CREATE2 which basically creates a new WebAuthn Safe Signer Proxy with input x and y public key coordinates and verfiers. When the users pass all these details and then someone front run them with these details with higher fees then their transaction would be possibly fail.This will create short term DOS for users trying create signers. Such CREATE2 Front running/dos issues are confirmed at Code4rena and other audit platforms.

In addition, the salt is hardcoded to bytes32(0) which is not random and such implementation of CREATE2 contract deployment is not recommended.

The recommendation is simply adding msg.sender with salt so such easily possible attacks can be prevented. I see, there is more benefit with using msg.sender along with salt then with current implementation. I would agree with your final decision on this issue.

CREATE2 frontrunning issue better explained here- https://github.com/martinetlee/create2-snippets?tab=readme-ov-file#oops-we-got-a-problem-front-running

[nlordell]

transaction would be possibly fail

I don't understand how the transaction would fail in this case (as it is idempotent). The second call would succeed and just not deploy a contract (but also not revert).

[0xRizwan]

yeah, the values would be same. I was bit skeptical about transaction failure so used possibly fail but the users is being affected with this issue can not be ruled out. I mean, why allow someone else to front run the signer creation transaction when it can be restricted with msg.sender in salt of CREATE2 deployement. Thats all from my side, would respect any decision for this issue.

[nlordell]

Thank you for your feedback it is very much appreciated.

I mean, why allow someone else to front run the signer creation transaction when it can be restricted with msg.sender in salt of CREATE2 deployement

One downside to having salt be msg.sender is that it does not work well with counterfactual Safe deployments. In particular, imagine you want to deploy a Safe that is owned by a WebAuthn signer that is also counterfactually deployed, you would do something like this:

address[] memory owners = new address[](1);
{
    owners[0] = computeWebAuthnSignerAddress()
}
bytes memory initializer = abi.encodeWithSignature(
    "setup(address[],uint256,address,bytes,address,address,uint256,address)",
    owners,
    threshold,
    multiSendCallOnly,
    encodeMultiSend({
        op: 0,
        to: safeWebAuthnSignerFactory,
        value: 0,
        data: abi.encodeCall("createSigner(uint256,uint256,uint176)", x, y, verifiers)
    }),
    compatibilityFallbackHander,
    address(0),
    0,
    address(0)
);

address safe = proxyFactory.createProxyWithNonce(safeSingleton, initializer, saltNonce);

Now - how do you implement computeWebAuthnSignerAddress if the salt is msg.sender? It isn't possible in this case as the salt needed to compute the address of the signer depends on the final safe address which, in turn, depends on the address of the shared signer.

[nlordell]

Also, for some additional context - this was our stance for a similar issue reported to the Safe contracts:

https://github.com/safe-global/safe-smart-account/issues/321

[0xRizwan]

@nlordell Thanks for sharing additional context and this link.

You can see that the two transactions are from different EOAs, and are both trying to createProxyWithNonce with the same args (including salt). The second txn fails because its create2 generates the same address as the first txn, and create2 throws when it generates an address that's already in use.

This is exactly the case here. Both of these issues have stressed on front-running and proposed similar mitigation which is commonly used in CREATE2 salts, However, the point raised with counterfactual Safe deployments seems to be correct and would prove to be downside. Current implementation hardcoded salt is not random, That salt can be easily used, to get a random salt, i have seen protocols using function callers incrementing nonce in case of deployment factories, not sure it would work here.

Since the attack vector, frontrunning the signer creation is confirmed here. The above issue link confirms same. The issue is to follow proper mitigation so it would not affect anything(including countrfactual) so i believe, this can be considered an issue and should be acknowledged as a part of contest. Thats all from my side, would respect final decision on this issue.

[nlordell]

Since the attack vector, frontrunning the signer creation is confirmed here. The above issue link confirms same.

The link was for additional context from previous such submissions which were considered invalid. Note that there is an important difference with the Safe factory in that the WebAuthn signer creation function is idempotent while the Safe creation function is not. As such, I still don't believe that there is a DoS vector here (since the transactions does not revert if an attacker races to create the signer before the user in the mempool).

[0xEricTee]

@0xRizwan Thanks for your submission.

@nlordell The DOS scenario is not possible, if an attacker frontrun createSigner function, the second call will just returned the signer address. As a result, I believe this issue is invalid.