A SINGLE MALICOUS LENDER CAN REVERT THE `Stop.stopRentBatch` TRANSACTION INSIDE THE `onERC721Received` HOOK WHILE RECEIVING THE `ERC721` TOKENS BACK

[c4-bot-7]

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L353 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Reclaimer.sol#L90-L100 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Reclaimer.sol#L32-L34

Vulnerability details

Impact

The Stop.stopRentBatch function is used to stop a batch of rentals by providing an array of RentalOrder structs to the function.

For each of the RentalOrders in the struct the Stop._reclaimRentedItems function is called as shown below:

            _reclaimRentedItems(orders[i]);

The _reclaimRentedItems function calls the Stop.reclaimRentalOrder function via a delegateCall from the Gnosis safe. In the reclaimRentalOrder function the ERC721 or ERC1155 is transferred to the lender for each of the Items as shown below:

        // Transfer each item if it is a rented asset.
        for (uint256 i = 0; i < itemCount; ++i) { //@audit-info - struct Item { ItemType itemType; SettleTo settleTo; address token; uint256 amount; uint256 identifier; }
            Item memory item = rentalOrder.items[i]; //@audit-info - cache each item

            // Check if the item is an ERC721.
            if (item.itemType == ItemType.ERC721) //@audit-info - enum ItemType { ERC721, ERC1155, ERC20 }
                _transferERC721(item, rentalOrder.lender); //@audit-info - transfer the ERC721 to the lender

            // check if the item is an ERC1155.
            if (item.itemType == ItemType.ERC1155)
                _transferERC1155(item, rentalOrder.lender); //@audit-info - transfer the ERC1155
        }

Let's consider the case of ERC721. Here the _transferERC721 function call is as follows:

    function _transferERC721(Item memory item, address recipient) private {
        IERC721(item.token).safeTransferFrom(address(this), recipient, item.identifier);
    }

The safeTransferFrom is called on the ERC721 contract to transfer the token to the lender. Since the ERC721.safeTransferFrom calls the onERC721Received hook on the lender, a malicious lender can revert this transaction by calling revert inside the onERC721Received hook. This will revert the entire Stop.stopRentBatch batch transaction thus not allowing other lenders to receive their respective ERC721 and ERC1155 tokens.

As a result each of the rentalOrders will have to be stopped by calling them individually via the Stop.stopRent function. Hence there will be delay in lenders getting their NFTs back since each order has to be stopped individually. This could incur monetary loss to the lender since he could have used the NFTs for another purpose (such as gaming) if he received the NFT earlier. And this is not good for the user experience as well. The users might lose the trust on the reNFT if they are unable to use the stopRentBatch feature and having to go through the cumbersome process of having to stop one rentalOrder at a time.

Proof of Concept

            _reclaimRentedItems(orders[i]);

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

        for (uint256 i = 0; i < itemCount; ++i) {
            Item memory item = rentalOrder.items[i];

            // Check if the item is an ERC721.
            if (item.itemType == ItemType.ERC721)
                _transferERC721(item, rentalOrder.lender);

            // check if the item is an ERC1155.
            if (item.itemType == ItemType.ERC1155)
                _transferERC1155(item, rentalOrder.lender);
        }

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

    function _transferERC721(Item memory item, address recipient) private {
        IERC721(item.token).safeTransferFrom(address(this), recipient, item.identifier);
    }

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

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to execute the _reclaimRentedItems(orders[i]) function call inside a try-catch block and handle the single rentalOrder reverts via a fallback appropriately. This will ensure a single malicious lender is unable to deprive other lenders getting their ERC721 and ERC1155 tokens on time and also unable to deprive renters getting their ERC20 tokens (In the event of a PAY order) when the Stop.stopRentBatch is called.

Assessed type

DoS

[c4-pre-sort]

141345 marked the issue as duplicate of #65

[c4-judge]

0xean marked the issue as satisfactory

[c4-judge]

0xean changed the severity to 3 (High Risk)

[c4-judge]

0xean changed the severity to 2 (Med Risk)