whole amount can be taken as a fee therefore 0 `paymentAmount` passes, which could calculate the wrong pro-rata payment for renter and lender

[c4-bot-2]

Lines of code

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

Vulnerability details

Impact

whole amount can be taken as a fee therefore 0 paymentAmount passed, which could calculate the wrong pro-rata payment for renter and lender.

Proof of Concept

PaymentEscrow.settlePayment function is used to Settles the payment for a rental order by transferring all items marked as payments to their destination accounts. During the settlement process, if active, a fee is taken on the payment. as per the comment on L314. settlePayment function calls the internal _settlePayment function and their the actual settlement is occurred where the item should be an ECR20 and if the fee is on it will calculate the fee via _calculateFee function and later deduct the paymentFee from the paymentAmount.

The fee is calculated as,

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

where amount = paymentAmount Now the issue occurs, when fee = 10000 when updated via the function setFee

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

As we can the check under the if condition will only revert if the feeNumerator > 10000 and not when feeNumerator == 10000 it means the fee taken on paymentAmount can be 10000 or lesser than this, now lets see how the fee is calculated, the fee is first multiplied to paymentAmount and than divided by denominator 10000 now if the fee is updated to 10000 the paymentFee under the _settlePayment would be same as the paymentAmount, i.e; now the paymentFee = paymentAmount,

 // Calculate the new fee.
                    uint256 paymentFee = _calculateFee(paymentAmount);

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

and when the paymentFee is calculated it adjust the payment amount by the fee, which might be zero as per the above condition(paymentFee == paymentAmount) that further could devastate the result under _settlePaymentProRata and mainly in _settlePaymentInFull that transfer the paymentAmount to the settler address,therefore the zero paymentAmount will be transferred every time to the lender or reenter, iff the fee is set to 10000.

Tools Used

manual

Recommended Mitigation Steps

Revisit the if condition under setFee and make the changes as required,

 -   if (feeNumerator > 10000) {
            revert Errors.PaymentEscrow_InvalidFeeNumerator();
        }
 +   if (feeNumerator >= 10000) {
            revert Errors.PaymentEscrow_InvalidFeeNumerator();
        }

Assessed type

Other

[c4-pre-sort]

141345 marked the issue as duplicate of #304

[c4-judge]

0xean marked the issue as unsatisfactory: Insufficient quality