Reclaimer contract may block asset recovery due to failed transfers

[c4-bot-4]

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Reclaimer.sol#L89-L100

Vulnerability details

Impact

The Reclaimer contract provides functionality for retrieving rented assets from a wallet contract once a rental has been stopped, and transferring them to the proper recipient. This is achieved by means of the reclaimRentalOrder() function being invoked as a DelegateCall from the renter's safe.

This function requires all transfers to succeed or the transaction reverts. However, if something goes wrong and an item is blocked or removed from the Safe during the rental, the lender will not be able to retrieve any of his other assets lent in the same order. This is because the reclaimRentalOrder() function iterates over all items in the rental order and attempts to transfer them. If any of these transfers fail, the entire transaction is reverted, leaving the lender unable to recover any of their assets.

This could lead to a situation where a lender's assets are locked indefinitely if a single item in a rental order becomes untransferable for any reason.

Proof of Concept

Consider the following scenario:

1. Alice rents multiple items to Bob.
2. During the rental period, one of the items becomes blocked or is removed from the Safe.
3. Alice attempts to stop the rental and recover her assets by calling the reclaimRentalOrder() function.
4. The reclaimRentalOrder() function attempts to transfer all items in the rental order back to Alice.
5. The transfer of the blocked item fails, causing the entire transaction to revert.
6. As a result, Alice is unable to recover any of her assets, even those that are not blocked.

Tools Used

Manual review

Recommended Mitigation Steps

A potential solution to this issue could be to allow the lender to pass a list of items to ignore as a parameter to the Stop.stopRent() function if he's the one calling. This would allow the lender to recover their other assets even if one item in the rental order is untransferable.

Assessed type

ERC721

[c4-pre-sort]

141345 marked the issue as duplicate of #65

[c4-judge]

0xean marked the issue as satisfactory

[c4-judge]

0xean marked the issue as partial-25

[c4-judge]

0xean changed the severity to 3 (High Risk)

[c4-judge]

0xean changed the severity to 2 (Med Risk)

[0xEVom]

Hi @0xean, this is a very basic finding but I believe in its core it is a valid concern that leads to loss of funds. It boils down to the following idea:

If multiple assets are rented out in one order and one of them becomes untransferable for any reason, all of them will be locked in the safe.

Either way, it is unrelated to #65. This issue is not about the interplay of payment transfers and rental item transfers, but concerns itself with the fact that the success of transferring rental assets as a whole is contingent on each individual rental asset transfer being successful. In other words, if any single asset transfer fails, all asset transfers fail.

[stalinMacias]

@0xean I believe this one is different from #65, I also think this is a great candidate for grade-a/b QA for the following reasons:

1. Assets aren't expected to be "removed" out of the safe, if that happens, that'd be a separate bug in and out of itself. The issue raised here doesn't demonstrate a way to steal a rented NFT.
2. It doesn't demonstrate a way in which the borrower can prevent the lender from claiming the order (temporarily or permanently)
3. It doesn't demonstrate a way in which a borrower can use his NFT bypassing restrictions placed by hooks (like #466)
4. It also doesn't impact the other rental orders made by the borrower OR impact the functionality of the protocol as a whole.

Given that it doesn't meet any of the 4 [med/high] impact criterias I listed, I believe this is much closer to it being a great suggestion / QA than it is to being a med.

[0xean]

During the rental period, one of the items becomes blocked or is removed from the Safe.

closing this as insufficient proof.

[c4-judge]

0xean marked the issue as unsatisfactory: Insufficient proof