c4-bot-2 commented 9 months ago

Lines of code

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

Vulnerability details

Impact

PaymentEscrow::setFee() is vulnerable to sandwich attacks. Lenders and renters may frontrun it by ending the rent first with Stop::stopRent() or Stop::stopRentBatch() (if multiple) to get a larger payment distribution.

Lenders may choose to frontrun PaymentEscrow::setFee() specifically when the existing fee is lower than the intended new fee. Renters can also take advantage of this but only in BASE orders and when they have already expired.

Another impact of this arises when the protocol team decides to do some drastic change in the fee and lenders or renters happen to stop their rent shortly afterwards without being aware. This could lead to them getting rental amounts out of the escrow that might not align with their expectations.

Proof of Concept

Setting a new fee is handled from the following function in PaymentEscrow:

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

Then, the following function is used to calculate the fee:

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

This is then used in the following part of PaymentEscrow::_settlePayment(), just before distributing the rental payments for the specific order scenario:

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

Tools Used

Manual Analysis

Recommended Mitigation Steps

One way to fix this would be to store the fee into the order, so it is not sandwichable and users won't get unexpected results if a change occurs shortly before them stopping their rents. An alternative could involve implementing a pausing mechanism. This mechanism would restrict users from calling stop functions until the fee has been successfully changed, thereby preventing sandwich attacks.

Assessed type

MEV

c4-pre-sort commented 9 months ago

141345 marked the issue as sufficient quality report

c4-pre-sort commented 9 months ago

141345 marked the issue as duplicate of #463

c4-judge commented 9 months ago

0xean changed the severity to QA (Quality Assurance)