An order can be created before and stopped after a `fee` increase, which is unfair to lender and renter of such order

[c4-bot-10]

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#L88-L91 https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/modules/PaymentEscrow.sol#L380-L388

Vulnerability details

Impact

The PaymentEscrow.setFee function can be called to increase fee when the protocol wants to charge a higher percentage fee for orders created after such fee change. However, an order can be created before and stopped after such fee increase. This is unfair to the lender and renter of such order because the payment amounts received by them are reduced by the new higher fee and are lower than expected.

Proof of Concept

When an order is stopped, calling the following PaymentEscrow._settlePayment function would charge a percentage fee according to fee so the payment amounts received by the lender and renter of such order are reduced by the fee already.

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
                    );
                }
                // If its a PAY order and the rental is over, or, if its a BASE order.
                else if (
                    (orderType.isPayOrder() && isRentalOver) || orderType.isBaseOrder()
                ) {
                    // Interaction: a pay order or base order which has ended. Payout is in full.
                    _settlePaymentInFull(
                        item.token,
                        paymentAmount,
                        item.settleTo,
                        lender,
                        renter
                    );
                } else {
                    revert Errors.Shared_OrderTypeNotSupported(uint8(orderType));
                }
            }
        }
    }

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

    function _calculateFee(uint256 amount) internal view returns (uint256) {
        // Uses 10,000 as a denominator for the fee.
        return (amount * fee) / 10000;
    }

The following PaymentEscrow.setFee function can be called to increase fee when the protocol wants to charge a higher percentage fee for orders created after such fee change. If the order is stopped before such fee increase, it would be charged the previous lower fee. If the order is stopped after such fee increase, even if it has expired before such fee increase, it would be charged the new higher fee; in this case, the payment amounts received by the lender and renter of such order are reduced by the new higher fee and are lower than expected. Hence, it is unfair to the lender and renter of such order that is created before but stopped after such fee increase.

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

    function setFee(uint256 feeNumerator) external onlyByProxy permissioned {
        // Cannot accept a fee numerator greater than 10000.
        if (feeNumerator > 10000) {
            revert Errors.PaymentEscrow_InvalidFeeNumerator();
        }

        // Set the fee.
        fee = feeNumerator;
    }

Tools Used

Manual Review

Recommended Mitigation Steps

When creating an order, the fee at that moment can be recorded for such order. When stopping such order, it can be charged a fee amount that corresponds to its recorded fee value.

Assessed type

Timing

[c4-pre-sort]

141345 marked the issue as duplicate of #463

[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