c4-bot-1 commented 10 months ago

Lines of code

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/modules/PaymentEscrow.sol#L159-L179 https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/modules/PaymentEscrow.sol#L190-L202 https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/modules/PaymentEscrow.sol#L100-L118 https://etherscan.io/address/0x43506849d7c04f9138d1a2050bbf3a0c054402dd#code#F15#L50 https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/policies/Stop.sol#L265-L306 https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/policies/Stop.sol#L313-L364

Vulnerability details

Impact

When the ERC20 payment token, such as USDC, implements a blocklist, the renter of a pay order can perform abusive actions to make the token's admin to block her or him. Afterwards, sending any amounts of such token to the renter reverts so stopping such pay order always reverts. As a result, such pay order can never be stopped, and the lender can never get back the lent ERC721/ERC1155 tokens.

Proof of Concept

Stopping a pay order would call the following PaymentEscrow._settlePaymentProRata or PaymentEscrow._settlePaymentInFull function, which further calls the PaymentEscrow._safeTransfer function below, depending on whether such order is expired or not.

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/modules/PaymentEscrow.sol#L159-L179

    function _settlePaymentProRata(
        address token,
        uint256 amount,
        address lender,
        address renter,
        uint256 elapsedTime,
        uint256 totalTime
    ) internal {
        // Calculate the pro-rata payment for renter and lender.
        (uint256 renterAmount, uint256 lenderAmount) = _calculatePaymentProRata(
            amount,
            elapsedTime,
            totalTime
        );

        // Send the lender portion of the payment.
        _safeTransfer(token, lender, lenderAmount);

        // Send the renter portion of the payment.
        _safeTransfer(token, renter, renterAmount);
    }

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/modules/PaymentEscrow.sol#L190-L202

    function _settlePaymentInFull(
        address token,
        uint256 amount,
        SettleTo settleTo,
        address lender,
        address renter
    ) internal {
        // Determine the address that this payment will settle to.
        address settleToAddress = settleTo == SettleTo.LENDER ? lender : renter;

        // Send the payment.
        _safeTransfer(token, settleToAddress, amount);
    }

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

    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);
        }
    }

If the ERC20 payment token is a token, such as USDC, that implements a blocklist, the renter can perform abusive actions to make herself or himself to become blocked by such token's admin. Afterwards, calling the PaymentEscrow._safeTransfer function to send some of such token to the renter reverts because the renter is already blocked to receive any of such token. For example, USDC implements the following notBlacklisted modifier that would revert any token-transfer function calls if the receiver of USDC is blocked. When settling the payment through transferring a token amount to the renter reverts, stopping the pay order also reverts as shown by the Stop.stopRent and Stop.stopRentBatch functions below. As a result, such pay order cannot be stopped, and the lender can never get back the lent ERC721/ERC1155 tokens.

https://etherscan.io/address/0x43506849d7c04f9138d1a2050bbf3a0c054402dd#code#F15#L50

    modifier notBlacklisted(address _account) {
        require(
            !_isBlacklisted(_account),
            "Blacklistable: account is blacklisted"
        );
        _;
    }

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/policies/Stop.sol#L265-L306

    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);

        ...
    }

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/policies/Stop.sol#L313-L364

    function stopRentBatch(RentalOrder[] calldata orders) external {
        ...

        // Process each rental order.
        // Memory will become safe after this block.
        for (uint256 i = 0; i < orders.length; ++i) {
            ...

            // Interaction: Transfer rental assets from the renter back to lender.
            _reclaimRentedItems(orders[i]);

            ...
        }

        // Interaction: Transfer ERC20 payments from the escrow contract to the respective recipients.
        ESCRW.settlePaymentBatch(orders);
        ...
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Instead of directly pushing the payments to the lender and renter, the PaymentEscrow contract can be updated to record the payment amounts that the lender and renter are entitled to when stopping an order. Then, the PaymentEscrow contract can be updated to add a function for the lender and renter to pull the respective recorded amount of the payment token.

Assessed type

ERC20

c4-pre-sort commented 9 months ago

141345 marked the issue as duplicate of #64

c4-judge commented 9 months ago

0xean changed the severity to 2 (Med Risk)