code-423n4 / 2024-01-renft-findings

2 stars 0 forks source link

Rented assets will be locked in safe forever if lender or renter is blacklisted by consideration token #628

Closed c4-bot-4 closed 10 months ago

c4-bot-4 commented 10 months ago

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L292-L296

Vulnerability details

Impact

The Stop policy provides functionality to stop a rental, which involves transferring the rented items back to the lender and the tokens in escrow to the lender, renter or both based on the type of order and point in time. This is implemented in the stopRent() and stopRentBatch() functions, which require both the transfers of the rented items and of the tokens in escrow to succeed:

function stopRent(RentalOrder calldata order) external {
    ...

    // Interaction: Transfer rentals from the renter back to lender.
    _reclaimRentedItems(order);

    // Interaction: Transfer ERC20 payments from the escrow contract to the respective recipients.
    ESCRW.settlePayment(order);
    ...

However, there is a potential issue if the lender's address is unable to receive a consideration ERC20, for example, due to being blacklisted. In such a case, when a BASE rental is stopped, the lender will not be able to recover the rented assets either as the transaction will revert.

The same applies for the renter in case of a PAY transaction, in which case the lender won't be able to recover the rented assets either.

This could potentially lead to a situation where rented assets are locked forever in the safe.

Proof of Concept

Consider the following scenario:

  1. Alice (the lender) creates a PAY order to rent out her NFTs and provides USDC as a consideration item.
  2. Bob fulfills the order and the NFTs are transferred to the rental safe while the USDC is held in escrow.
  3. Bob gets blacklisted in the USDC contract, possibly on purpose.
  4. Alice tries to stop the rental using the stopRent() function in the Stop.sol contract.
  5. The stopRent() function attempts to transfer the rented NFTs back to Alice and the USDC tokens in escrow, or part of them, to Bob.
  6. However, because Bob is blacklisted, the transfer of the USDC tokens to his address fails.
  7. As a result, the entire stopRent() transaction reverts, and Alice is unable to recover her rented NFTs.

Tools Used

Manual review

Recommended Mitigation Steps

A possible solution to this issue could be to allow the lender to forfeit the payment if they are the one calling stopRent(). This would ensure that even if any actor is blacklisted and cannot receive the consideration ERC20 token, they can still recover their rented assets.

Assessed type

ERC20

c4-pre-sort commented 10 months ago

141345 marked the issue as duplicate of #64

c4-judge commented 10 months ago

0xean marked the issue as satisfactory