Factory.deployRentalSafe transaction can be frontrun to prevent rental safe from being deployed

[c4-bot-4]

Lines of code

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/policies/Factory.sol#L138-L193
https://github.com/safe-global/safe-contracts/blob/eb93dbb0f62e2dc1b308ac4c110038062df0a8c9/contracts/proxies/SafeProxyFactory.sol#L52-L57
https://github.com/safe-global/safe-contracts/blob/eb93dbb0f62e2dc1b308ac4c110038062df0a8c9/contracts/proxies/SafeProxyFactory.sol#L26-L44

Vulnerability details

Impact

The Factory.deployRentalSafe transaction can be frontrun by calling the SafeProxyFactory.createProxyWithNonce function with the same address(safeSingleton), initializerPayload, uint256(keccak256(abi.encode(STORE.totalSafes() + 1, block.chainid))) being the inputs. Since this frontrunning attack can be repeated, none of the rental safes can be deployed.

Proof of Concept

When the following Factory.deployRentalSafe function is called, address(safeSingleton), initializerPayload, uint256(keccak256(abi.encode(STORE.totalSafes() + 1, block.chainid))), which are used for calling the SafeProxyFactory.createProxyWithNonce function, are all known.

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/policies/Factory.sol#L138-L193

    function deployRentalSafe(
        address[] calldata owners,
        uint256 threshold
    ) external returns (address safe) {
        ...

        // Delegate call from the safe so that the rental manager module can be enabled
        // right after the safe is deployed.
        bytes memory data = abi.encodeCall(
            Factory.initializeRentalSafe,
            (address(stopPolicy), address(guardPolicy))
        );

        // Create gnosis initializer payload.
        bytes memory initializerPayload = abi.encodeCall(
            ISafe.setup,
            (
                // owners array.
                owners,
                // number of signatures needed to execute transactions.
                threshold,
                // Address to direct the payload to.
                address(this),
                // Encoded call to execute.
                data,
                // Fallback manager address.
                address(fallbackHandler),
                // Payment token.
                address(0),
                // Payment amount.
                0,
                // Payment receiver
                payable(address(0))
            )
        );

        // Deploy a safe proxy using initializer values for the Safe.setup() call
        // with a salt nonce that is unique to each chain to guarantee cross-chain
        // unique safe addresses.
        safe = address(
            safeProxyFactory.createProxyWithNonce(
                address(safeSingleton),
                initializerPayload,
                uint256(keccak256(abi.encode(STORE.totalSafes() + 1, block.chainid)))
            )
        );

        ...
    }

After detecting a Factory.deployRentalSafe transaction in the mempool, a malicious actor can frontrun such transaction by directly calling the SafeProxyFactory.createProxyWithNonce function with the same address(safeSingleton), initializerPayload, uint256(keccak256(abi.encode(STORE.totalSafes() + 1, block.chainid))) being the inputs on the same chain. After the frontrunning, the address for the rental safe to be deployed is already taken, and address(proxy) != address(0) would become false in the SafeProxyFactory.deployProxy function to revert the Factory.deployRentalSafe transaction. Hence, the rental safe deployment fails. This frontrunning attack can be repeated each time the Factory.deployRentalSafe function is called, which means no rental safes can be deployed at all.

https://github.com/safe-global/safe-contracts/blob/eb93dbb0f62e2dc1b308ac4c110038062df0a8c9/contracts/proxies/SafeProxyFactory.sol#L52-L57

    function createProxyWithNonce(address _singleton, bytes memory initializer, uint256 saltNonce) public returns (SafeProxy proxy) {
        // If the initializer changes the proxy address should change too. Hashing the initializer data is cheaper than just concatinating it
        bytes32 salt = keccak256(abi.encodePacked(keccak256(initializer), saltNonce));
        proxy = deployProxy(_singleton, initializer, salt);
        emit ProxyCreation(proxy, _singleton);
    }

https://github.com/safe-global/safe-contracts/blob/eb93dbb0f62e2dc1b308ac4c110038062df0a8c9/contracts/proxies/SafeProxyFactory.sol#L26-L44

    function deployProxy(address _singleton, bytes memory initializer, bytes32 salt) internal returns (SafeProxy proxy) {
        require(isContract(_singleton), "Singleton contract not deployed");

        bytes memory deploymentData = abi.encodePacked(type(SafeProxy).creationCode, uint256(uint160(_singleton)));
        // solhint-disable-next-line no-inline-assembly
        assembly {
            proxy := create2(0x0, add(0x20, deploymentData), mload(deploymentData), salt)
        }
        require(address(proxy) != address(0), "Create2 call failed");

        if (initializer.length > 0) {
            // solhint-disable-next-line no-inline-assembly
            assembly {
                if eq(call(gas(), proxy, 0, add(initializer, 0x20), mload(initializer), 0, 0), 0) {
                    revert(0, 0)
                }
            }
        }
    }

Tools Used

Manual Review

Recommended Mitigation Steps

The Factory.deployRentalSafe function can be updated to additionally hash msg.sender as a part of the saltNonce input for calling the SafeProxyFactory.createProxyWithNonce function.

Assessed type

DoS

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-sponsor]

Alec1017 (sponsor) acknowledged

[0xean]

Dubious this should qualify as M. Its an expensive attack vector with limited economic value to be gained. Users would certainly begin to leverage more private transaction methods to avoid the front running. Welcome more discussion during PJQA, but may decide to downgrade.

[c4-judge]

0xean marked the issue as satisfactory

[c4-judge]

0xean marked the issue as selected for report

[kadenzipfel]

This is invalid. We can see that the incremented value of Store.totalSafes() is passed as part of the saltNonce to createProxyWithNonce:

safe = address(
            safeProxyFactory.createProxyWithNonce(
                address(safeSingleton),
                initializerPayload,
                // @audit totalSafes() + 1 included in saltNonce
                uint256(keccak256(abi.encode(STORE.totalSafes() + 1, block.chainid)))
            )
        );

And STORE.addRentalSafe, called in deployRentalSafe, increments totalSafes:

function addRentalSafe(address safe) external onlyByProxy permissioned {
        // Get the new safe count.
        uint256 newSafeCount = totalSafes + 1;

        // Register the safe as deployed.
        deployedSafes[safe] = newSafeCount;

        // Increment nonce.
        totalSafes = newSafeCount;
    }

We can see in createProxyWithNonce that the saltNonce changes the salt and thus the deployed address:

function createProxyWithNonce(address _singleton, bytes memory initializer, uint256 saltNonce) public returns (SafeProxy proxy) {
        // If the initializer changes the proxy address should change too. Hashing the initializer data is cheaper than just concatinating it
        bytes32 salt = keccak256(abi.encodePacked(keccak256(initializer), saltNonce));
        proxy = deployProxy(_singleton, initializer, salt);
        emit ProxyCreation(proxy, _singleton);
    }

Therefore, frontrunning deployRentalSafe will not cause any problems since both transactions will have different saltNonce's and therefore different deployment addresses.

[k4zanmalay]

Hey @0xean! I'd like to add that this issue is not only about the frontrunning, griefer can pass the same salt to the gnosis safe factory as the reNFT factory. If we know the victim's address and the current total number of safes, it's quite easy to predict the salt and create multiple safes in advance. For example Alice wants to create a reNFT safe, salt is calculated with: address - 0xa11ce, threshold - 1, totalSafes - 10, other parameters are always the same. The attacker can use these parameters and create 5 safes in the gnosis safe factory, therefore blocking Alice from creation of the new safe until other reNFT users will create 5 safes in the protocol to set totalSafes counter to 15.

All in all, the feasibility of this attack vector depends on the popularity of the protocol, how many new safes will be created every day; if totalsSafes counter moves slow enough, it can be pretty significant.

[k4zanmalay]

@kadenzipfel hi, the idea is to call gnosis safe factory directly, bypassing the reNFT factory

[akshaysrivastav]

No loss of funds, no permanent loss of protocol functionality (only temporary), high cost of attack, no profit.
Seems a Low/QA.

[k4zanmalay]

Also I'd argue about the cost since the sponsor plans to deploy on polygon network and transactions there are quite cheap, here is the hash of Create proxy with nonce of the local factory (only $0.01)
https://polygonscan.com/tx/0xdf0ebb56d6698d95a326e48ca68d92c318ce2b01cc1d246da83fe5d215ac6388

[0xA5DF]

We have an open org issue on this type of attack: code-423n4/org#143
I personally believe it should be a low (I've also reported it as a low at #330)

[rbserver]

Hi @0xean ,

A similar finding, code-423n4/2023-04-caviar-findings#419, is considered as a medium risk in a previous contest though such DOS issue could be sidestepped according to code-423n4/2023-04-caviar-findings#419 (comment). Due to the similarity, would this finding be considered as a medium issue as well? Thanks again for judging!

[0xean]

gonna stand with @dmvt on this one. Downgrading all of these to QA. Final ruling.

[c4-judge]

0xean changed the severity to QA (Quality Assurance)

[rbserver]

Hi @0xean,

After this finding is downgraded to QA, 7 findings of mine are considered as QA now, which are this one (#443), #436, #438, #446, #447, #452, and #455. Comparing to some other QA reports that have A grades, such as #293, my number of QA items is higher. Hence, would it be possible for my QA items to be graded A instead of B? Thanks!

[0xean]

@rbserver - @141345 will recheck your score based on downgrades.

[141345]

@rbserver - @141345 will recheck your score based on downgrades.

Yes, 7 low should be ranked 1st for QA

[c4-judge]

0xean marked the issue as grade-a

[c4-judge]

0xean marked the issue as not selected for report