Pay order, which has a very long rental duration while lender's intended rental duration is very short, can be unfair to renter

[c4-bot-4]

Lines of code

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/modules/PaymentEscrow.sol#L215-L283 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#L132-L146

Vulnerability details

Impact

A lender can submit a pay order with a very long rental duration even though her or his intended rental duration is very short for getting a benefit. After the intended rental duration, which is very short, is reached after a renter's fulfillment for the corresponding order, the lender can stop such order. This can cause the renter to receive no payment while not have enough time to utilize the rented ERC721/ERC1155 token even though she or he has rented such token for some time. This is unfair to the renter because the renter could receive the entire payment amount when the order is stopped at the time that the intended duration is reached if the lender could honestly submit the pay order with the rental duration being the intended rental duration that is very short.

Proof of Concept

When a lender can get benefit by allowing others to rent her or his ERC721/ERC1155 token for a very short time, this lender can still submit a pay order with a very long rental duration. When the very short intended rental duration is reached after a renter fulfills the corresponding order and before such order expires, the lender can stop it, which would eventually call the PaymentEscrow._settlePayment, PaymentEscrow._settlePaymentProRata, and PaymentEscrow._calculatePaymentProRata functions below.

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

    function _settlePayment(
        Item[] calldata items,
        OrderType orderType,
        address lender,
        address renter,
        uint256 start,
        uint256 end
    ) internal {
        // Calculate the time values.
        uint256 elapsedTime = block.timestamp - start;
        uint256 totalTime = end - start;

        // Determine whether the rental order has ended.
        bool isRentalOver = elapsedTime >= totalTime;

        // Loop through each item in the order.
        for (uint256 i = 0; i < items.length; ++i) {
            // Get the item.
            Item memory item = items[i];

            // Check that the item is a payment.
            if (item.isERC20()) {
                // Set a placeholder payment amount which can be reduced in the
                // presence of a fee.
                uint256 paymentAmount = item.amount;

                // Take a fee on the payment amount if the fee is on.
                if (fee != 0) {
                    // Calculate the new fee.
                    uint256 paymentFee = _calculateFee(paymentAmount);

                    // Adjust the payment amount by the fee.
                    paymentAmount -= paymentFee;
                }

                ...

                // If its a PAY order but the rental hasn't ended yet.
                if (orderType.isPayOrder() && !isRentalOver) {
                    // Interaction: a PAY order which hasnt ended yet. Payout is pro-rata.
                    _settlePaymentProRata(
                        item.token,
                        paymentAmount,
                        lender,
                        renter,
                        elapsedTime,
                        totalTime
                    );
                }
                ...
            }
        }
    }

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

When calling the PaymentEscrow._calculatePaymentProRata function, it is possible that amount * elapsedTime < 500 * totalTime / 1000 is true because elapsedTime is very small while totalTime is very big, which would cause renterAmount to be 0. Hence, the renter would not receive any payment even though she or he has rented the lender's ERC721/ERC1155 token for some time. Moreover, since the actual rental duration represented by elapsedTime is much shorter than expected for the renter, the renter might not have a chance to utilize such rented ERC721/ERC1155 token. Hence, the renter is not compensated by any rental payment or usage of such ERC721/ERC1155 token.

If the lender could honestly submit the pay order with the rental duration being the intended rental duration that is very short, the renter, who fulfills such order, would receive the entire payment amount instead of 0 when the order is stopped at the time that the intended rental duration is reached. Thus, such pay order is unfair to the renter.

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

    function _calculatePaymentProRata(
        uint256 amount,
        uint256 elapsedTime,
        uint256 totalTime
    ) internal pure returns (uint256 renterAmount, uint256 lenderAmount) {
        // Calculate the numerator and adjust by a multiple of 1000.
        uint256 numerator = (amount * elapsedTime) * 1000;

        // Calculate the result, but bump by 500 to add a rounding adjustment. Then,
        // reduce by a multiple of 1000.
        renterAmount = ((numerator / totalTime) + 500) / 1000;

        // Calculate lender amount from renter amount so no tokens are left behind.
        lenderAmount = amount - renterAmount;
    }

Tools Used

Manual Review

Recommended Mitigation

Each pay order can be updated to include a minimum payment percentage that the lender is willing to pay the renter. When stopping a pay order before its expiration, the renter would receive at least such minimum percentage of the payment amount.

Assessed type

Context

[c4-pre-sort]

141345 marked the issue as duplicate of #409

[c4-judge]

0xean changed the severity to QA (Quality Assurance)

[c4-judge]

0xean marked the issue as grade-b

[c4-judge]

0xean marked the issue as grade-a