Adression collision attack due to wrong salt implementation

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x773bcc52f96102a014ce73376bf0c1369d4171c684b105f2ef0e9a703b3ebc25 Severity: medium

Description: Description\

SafeWebAuthnSignerFactory.createSigner is prone to address collision attack. Since the SafeWebAuthnSignerProxy to be created is preknown, any attacker can brute force deploy some code at that address change a state or sign smart account signature or any allowance change from the address is possible. Root cause id using the 0 salt which makes easy for the attacker. Recommendation is to use a salt that includes tx.origin and a user provided signature hash as a salt. So this randomness will redce the probability/likelihood of this attack to zero.

Previous issues explaing this attack

1. https://github.com/sherlock-audit/2023-07-kyber-swap-judging/issues/90
2. https://github.com/sherlock-audit/2023-12-arcadia-judging/issues/59
3. https://github.com/sherlock-audit/2024-01-napier-judging/issues/111

Attack Scenario\

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

safe-modules/modules/passkey/contracts/SafeWebAuthnSignerFactory.sol

51:     function createSigner(uint256 x, uint256 y, P256.Verifiers verifiers) external returns (address signer) {
52:         signer = getSigner(x, y, verifiers);
53: 
54:         if (_hasNoCode(signer)) {
55:   >>>       SafeWebAuthnSignerProxy created = new SafeWebAuthnSignerProxy{salt: bytes32(0)}(address(SINGLETON), x, y, verifiers);
56:             assert(address(created) == signer);
57:             emit Created(signer, x, y, verifiers);
58:         }
59:     }

1. as we know create2 being used in L55 above, an attacker can deterministically predict the evey contract that will be created with the 0 salt.
2. So he deploys a code that signs a smart wallet signatures, increase token allowances, also chnage the storage slots that does maximum damage to the safe wallet system.

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

[nlordell]

Thank you for the submission. I thought long and hard about this one, but AFAICT this is not an issue for us (beyond finding a hash collision over 2^160 bits which is not really possible at the moment).

To address the first link in particular: https://github.com/sherlock-audit/2023-07-kyber-swap-judging/issues/90

In this case, the difference was that it was much easier to find some collision with a CREATE2 contract that would be considered a pool by the router when in fact it wasn't one. Critically, it didn't really matter what tokens were involved, as it allowed you to call the swapCallback function as an EOA which shouldn't be allowed.

Here, however, you would need to find a hash collision for a specific public key for the owner that is configured by the account, and not any public key. So, I do believe that the address being determined by a public key pair makes mining of hash collisions harder and any attack not really feasible with current computing power.

I don't want to rule out this submission quite yet - but can you please provide more details on how exactly the hash attack collision would work here?

[nlordell]

After some further research, from what I understand an attacker would have to influence one of the user inputs to the hash used for the CREATE2 address collision (they would have to influence either the public key or verifiers parameter). A collision of a user-chosen parameters is not really feasible AFAIU.

[nlordell]

I will mark this invalid for now. If you believe this to be invalid, and have a concrete attack please describe it here.