c4-bot-3 commented 10 months ago

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Create.sol#L733-L776 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L265-L285

Vulnerability details

Impact

Since the creation of a rental consumes less gas per rental NFT than stopping a rental, creating a rental with a lot of NFTs might be possible but stopping it might run out of gas. Not being able to stop a rental will lock the rented NFTs in the borrowers safe and prevent the lender to ever being paid.

Proof of Concept

To create a new rental the function Create:validateOrder() is called, to stop a rental Stop::stopRental() is called. Tests show that when creating a rental, every additional NFT added to the rental increases the gas cost of the transaction by at least 29,000 gas. When stopping a rental, every additional NFT adds at least 36,200 gas to the transaction.

For convenience here is a table of the gas cost per additional NFT for different numbers of NFTs in a rental:

Number of NFTs	Gas "validateOrder"	Gas per NFT	Gas "stopRental"	Gas per NFT
1	156,218		97,523
2	185,208	28,990	113,731	36,208
5	272,225	29,006	242,394	36,221
10	417,398	29,035	423,613	36,244
100	3,061,615	29,380	3,709,491	36,510
401	12,333,058	30,802	15,028,311	37,604

As can be seen in the table above, creating a rental of 401 NFTs would be possible but stopping the rental would cost over 15 million gas. This is more than the gas block limit of Avalanche, one of the chains the reNFT protocol will be deployed on and will lead to an OOG revert. Therfore the rental can never be stopped and the rented NFTs will be stuck in the borrowers safe and the lender will never get paid.

Even though the number of 401 NFTs is quite high, it is not impossible to reach, depending on the game and the NFTs that are rented (e.g cosmetics for land plots in an “Axie Infinity Land” kind of game). Also, this calculation is not considering any additional gas used for onStop hooks or multiple ERC20 tokens for payment. Such additional gas cost could dramatically reduce the number of NFTs needed for an OOG event.

Following is a test that can be added to StopRent.t.sol to check the gas cost for calling Create: validateOrder() and Stop::stopRental. To be able to run the test the number of tokens deployed must be changed in test/fixtures/protocol/AccountCreator.sol::setUp from 3 to 500:

        // deploy 3 erc20 tokens, 3 erc721 tokens, and 3 erc1155 tokens
-         _deployTokens(3);
+        _deployTokens(1000);

In the test itself, the number of NFTs in the rental can be changed by adjusting the numberOfNfts variable in the beginning of the test. To run the test run:

forge test -vvv --gas-report --mt test_OOG_StopOrder

In the gas report look for the gas cost for the call to Create:validateOrder() and Stop::stopRental.

function test_OOG_StopOrder() public {
        uint256 numberOfNfts = 100;

        createOrder({
            offerer: alice,
            orderType: OrderType.BASE,
            erc721Offers: numberOfNfts,
            erc1155Offers: 0,
            erc20Offers: 0,
            erc721Considerations: 0,
            erc1155Considerations: 0,
            erc20Considerations: 1
        });

        (
            Order memory order,
            bytes32 orderHash,
            OrderMetadata memory metadata
        ) = finalizeOrder();

        createOrderFulfillment({
            _fulfiller: bob,
            order: order,
            orderHash: orderHash,
            metadata: metadata
        });

        // finalize the base order fulfillment
        uint256 gasBefore4 = gasleft();

        RentalOrder memory rentalOrder = finalizeBaseOrderFulfillment();

        uint256 gasAfter4 = gasleft();
        console.log("Gas for creating rental", gasBefore4 - gasAfter4);

        // speed up in time past the rental expiration
        vm.warp(block.timestamp + 750);

        // stop the rental order
        vm.prank(alice.addr);
        uint256 gasBefore5 = gasleft();

        stop.stopRent(rentalOrder);

    }

Tools Used

Manual review Foundry

Recommended Mitigation Steps

To ensure all rentals can be stopped consider introduceing a cap for the number of NFTs that can be added to a single rental. If a user want to rent out more NFTs he can still create multiple rentals

Assessed type

DoS

c4-pre-sort commented 10 months ago

141345 marked the issue as duplicate of #544

c4-judge commented 9 months ago

0xean marked the issue as satisfactory