Denial-of-service issue on rental stop transactions

[c4-bot-6]

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L205 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L227-L233 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L210-L212 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L300 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/Storage.sol#L222 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L257-L264 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L271-L277 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L231 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L174-L178

Vulnerability details

Once a user executes the Stop::stopRent() (or Stop::stopRentBatch()) to stop a rental order. The function must process a handful of gas-exceeding significant-risk tasks.

In the worst-case scenario, the rental stop transaction will be reverted if the function consumes gas beyond the block gas limit. Consequently, all ERC-721/ERC-1155 rented items and ERC-20 payments will be frozen permanently.

Proof of Concept

This section explains only the Stop::stopRent() for brevity. The DoS issue on the Stop::stopRentBatch() should have similar details.

The following highlights a list of gas-exceeding significant-risk tasks of the stopRent():

1. Generating the rentalAssetUpdates array from all ERC-721/ERC-1155 rented items.
2. Processing of the onStop() of all Hook contracts.
3. Transferring all ERC-721/ERC-1155 rented items from a Gnosis Safe contract to a lender.
4. Settling and transferring all ERC-20 payments from the protocol's PaymentEscrow contract to all respective recipients.
5. Removing the rental order and all rented items from the protocol's Storage contract.

This report will analyze only the most significant risk tasks, including the processing of the onStop() of all Hook contracts (Task 2 above) and settling and transferring all ERC-20 payments from the protocol's PaymentEscrow contract to all respective recipients (Task 4). Furthermore, the gas risks when deploying the protocol to multiple chains are also analyzed in this report.

The remaining of this section will be categorized into three sub-sections as follows.

• Gas Exceeding Significant Risk #1 -- Processing Hooks' onStop()
• Gas Exceeding Significant Risk #2 -- Processing Payment Settlements
• Gas Exceeding Significant Risk #3 -- Multi-Chain Protocol Deployment

Gas Exceeding Significant Risk #1 -- Processing Hooks' onStop()

The Stop::_removeHooks() is responsible for processing and triggering all Hook contracts attached to the rental order. Therefore, the more Hooks to be processed, the more gas is required.

For each Hook contract, the _removeHooks() will trigger the onStop() to process the rental stop event. Even though the onStart() of each Hook in question would have successfully been executed on the rental start transaction, that cannot guarantee that the rental stop transaction will succeed because the stop transaction will call another callback function: onStop() (not the onStart()).

Specifically, the onStop() can consume more gas than the onStart() as the function can route the execution control flow to different logic branches.

If the Hooks consume gas beyond the block gas limit, the rental order will be permanently DoS'ed. The protocol admin cannot disable some Hooks to remedy the DoS issue because the stop transaction will be reverted when the _removeHooks() executes the Storage::hookOnStop() to check for disabled Hooks.

Furthermore, we cannot remove some Hooks from the rental order payload because the resulting rentalOrderHash derived from the Stop::_deriveRentalOrderHash() will be changed. Subsequently, the Storage::removeRentals()will revert the stop transaction, as the rentalOrderHash has changed.

    // FILE: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol
    function _removeHooks(
        Hook[] calldata hooks,
        Item[] calldata rentalItems,
        address rentalWallet
    ) internal {
        ...

        // Loop through each hook in the payload.
@1      for (uint256 i = 0; i < hooks.length; ++i) { //@audit -- Loop to trigger every attached hook (the more hooks to be processed, the more gas is required)
            // Get the hook address.
            target = hooks[i].target;

            ...
            ...

            // Call the hook with data about the rented item.
            try
@2              IHook(target).onStop( //@audit -- The hook's onStop() can consume more gas than the onStart(). This poses a significant risk to the issue: "gas consumption beyond the block gas limit"
@2                  rentalWallet,
@2                  item.token,
@2                  item.identifier,
@2                  item.amount,
@2                  hooks[i].extraData
@2              )
            {} ...
            ...
            ...
        }
    }

• @1 -- Loop to trigger every attached hook (the more hooks to be processed, the more gas is required): https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L205

• @2 -- The hook's onStop() can consume more gas than the onStart(). This poses a significant risk to the issue: "gas consumption beyond the block gas limit": https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L227-L233

Gas Exceeding Significant Risk #2 -- Processing Payment Settlements

The PaymentEscrow::_settlePayment() is responsible for processing and transferring all ERC-20 payments from the protocol's PaymentEscrow contract to all respective recipients.

The settlement can be categorized into two scenarios.

1. Pro-rata split payment where ERC-20 token will be transferred to both lender and renter.
2. Full-amount payment where ERC-20 token will be transferred to one respective recipient.

Token transfers are external calls that can consume a considerable amount of gas. Hence, the more ERC-20 items to be transferred, the more gas is required. Moreover, in the case of a pro-rata split payment, the gas cost required for each settlement will be doubled since the token must be transferred to both lender and renter.

If the payment settlements consume gas beyond the block gas limit, the rental order will be permanently DoS'ed.

We cannot remove some items from the rental order payload because the resulting rentalOrderHash derived from the Stop::_deriveRentalOrderHash() will be changed. Subsequently, the Storage::removeRentals()will revert the stop transaction, as the rentalOrderHash has changed.

    // FILE: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol
    function _settlePayment(
        Item[] calldata items,
        OrderType orderType,
        address lender,
        address renter,
        uint256 start,
        uint256 end
    ) internal {
        ...

        // Loop through each item in the order.
@3      for (uint256 i = 0; i < items.length; ++i) { //@audit -- Loop to process every ERC-20 item (the more items to be processed, the more gas is required)
            // Get the item.
            Item memory item = items[i];

            // Check that the item is a payment.
            if (item.isERC20()) {
                ...
                ...

                // If its a PAY order but the rental hasn't ended yet.
                if (orderType.isPayOrder() && !isRentalOver) {
                    // Interaction: a PAY order which hasnt ended yet. Payout is pro-rata.
@4                  _settlePaymentProRata(
@4                      item.token,
@4                      paymentAmount,
@4                      lender,
@4                      renter,
@4                      elapsedTime,
@4                      totalTime
@4                  ); //@audit -- In the case of a pro-rata split payment, 2x gas cost is required for each payment settlement (transfer token to both lender and renter)
                }
                // If its a PAY order and the rental is over, or, if its a BASE order.
                else if (
                    (orderType.isPayOrder() && isRentalOver) || orderType.isBaseOrder()
                ) {
                    // Interaction: a pay order or base order which has ended. Payout is in full.
@5                  _settlePaymentInFull(
@5                      item.token,
@5                      paymentAmount,
@5                      item.settleTo,
@5                      lender,
@5                      renter
@5                  ); //@audit -- In the case of a full-amount payment, 1x gas cost is required for each payment settlement (transfer token to one recipient)
                } else {
                    revert Errors.Shared_OrderTypeNotSupported(uint8(orderType));
                }
            }
        }
    }

• @3 -- Loop to process every ERC-20 item (the more items to be processed, the more gas is required): https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L231

• @4 -- In the case of a pro-rata split payment, 2x gas cost is required for each payment settlement (transfer token to both lender and renter): https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L257-L264

• @5 -- In the case of a full-amount payment, 1x gas cost is required for each payment settlement (transfer token to one recipient): https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L271-L277

Gas Exceeding Significant Risk #3 -- Multi-Chain Protocol Deployment

The reNFT protocol is planned to be deployed on multiple chains. Each chain can have a different block gas limit and gas calculation scheme. Some chains have a lower block gas limit than others.

Therefore, rental orders containing certain Hooks and rental items successfully executed on one chain cannot guarantee that they will work on other chains.

Tools Used

Manual Review

Recommended Mitigation Steps

To mitigate the DoS issue, limit each rental order's number of Hooks and rental items (ERC-721, ERC-1155, and ERC-20 items). Also, the protocol's contracts should be performed intensively for gas testing when deploying to a new chain.

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]

Without more proof, this is QA. The potential is clearly shown by the warden but for this to be M the issue would need to show a specific scenario that exceeds the gas limit and is within reasonable usage of the protocol.

[c4-judge]

0xean marked issue #538 as primary and marked this issue as a duplicate of 538

[c4-judge]

0xean marked the issue as partial-25

[c4-judge]

0xean marked the issue as satisfactory