User can block another user safe creation

[c4-bot-4]

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Factory.sol#L180-L186

Vulnerability details

Impact

The factory doesn't use any user-inputted salt. This opens the door for a DOS.

Proof of Concept

When the factory generates a safe, it uses the following Solidity code:

safeProxyFactory.createProxyWithNonce(
     address(safeSingleton),
     initializerPayload,
     uint256(keccak256(abi.encode(STORE.totalSafes() + 1, block.chainid)))
)

safeProxyFactory utilizes create2 (createProxyWithNonce -> deployProxy) with the aforementioned inputs. Here, safeSingleton is a constant address, initializerPayload is predictable and constant for different users, and the "salt" is simply the number of safes incremented by one.

Our attack specifically targets the salt. If we deploy a safe at an address where our victim is going to deploy it, create2 will revert, leading to the victim's transaction reverting. This will not increase STORE.totalSafes(), meaning that the "salt" will remain the same. Subsequent transactions will also revert. The only way for our victim to deploy a safe is if someone else uses the Factory to deploy one.

Example:

1. ProjectX is a competitor of re-NFT.
2. It sees that Bob wants to rent a few NFTs.
3. ProjectX front runs Bob's TX and deploys a safe on its to be deployment address.
4. Bob's TX reverts.
5. Bob does another TX, but it also reverts.
6. Bob fires a third TX which also reverts, he quits.
7. Bob rents NFTs at ProjectX's site.

POC

Gist - https://gist.github.com/0x3b33/9baf7167e800b7a3b46b74ad3b12f982
Place in - smart-contracts/test/unit/<name>.sol
Run with - forge test --match-test test_FRfactory

Tools Used

Manual review and Джиджи, the gangster dog.

Recommended Mitigation Steps

Add a salt that the user can input, this way even if someone FR and reverts his TX, he can deploy another safe.

-   function deployRentalSafe(... , uint256 salt) external returns (address safe) {
+   function deployRentalSafe(... , uint256 salt) external returns (address safe) {
        ...
        safe = address(
            safeProxyFactory.createProxyWithNonce(
               address(safeSingleton),
               initializerPayload,
-              uint256(keccak256(abi.encode(STORE.totalSafes() + 1, block.chainid)))
+              uint256(keccak256(abi.encode(STORE.totalSafes() + 1, block.chainid, salt)))
            )
         ...
       );

Assessed type

DoS

[c4-pre-sort]

141345 marked the issue as duplicate of #443

[c4-judge]

0xean marked the issue as satisfactory

[c4-judge]

0xean changed the severity to QA (Quality Assurance)