Potential revenue loss due to balance desynchronization in PaymentEscrow

[c4-bot-8]

Lines of code

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

Vulnerability details

Impact

Balance desynchronization leads to a situation where the contract does not check the actual tokens held.
Which results in the loss of protocol assets, eroding the contract's financial stability.

Proof of Concept

Any losses or wrong calculations between the increaseDeposit amount and _settlePayment will not be caught, because after we settle a payment, we are missing a check 'are the expected fees/assets inside the contract.'

Calculating the fees and later not synchronizing them with the actual balance gives us a pretty nice gap for losing assets. Because when we settle the payment and make the transfer of the assets, logically, we sent the last amount directly, assuming we already took the fee, which is added to the contract balance:

_settlePayment

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

            // Effect: Decrease the token balance. Use the payment amount pre-fee
            // so that fees can be taken.
            _decreaseDeposit(item.token, item.amount);

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

We have a few examples of losing assets because the denominator doesn't handle all scenarios.
One is when we calculate the fees, which happens exactly between increasing the deposit amount and settling the payment.

In the passed test case below, let's assume we loop through 100 items in the order, and the escrow (ESCRW) received 14 tokens pro item because the item.amount was 14, for example.

This discrepancy becomes serious when considering the fee mechanism. With a 7% fee set, the contract should retain a portion of the transactions as fees, but as we can see, it has 0 after settling the payment, because after 'settling the payment', we don't check if we have the expected fee amount left in the contract:

function test_Success_SettlePayment_BaseOrder_WithFee_100Items() public {
    // determine an amount of one item
    uint256 amount = 14;

    // determine a rent duration
    uint rentDuration = 100;

    // mint tokens to the payment escrow
    erc20s[0].mint(address(ESCRW), amount * 100); // Minting for 100 items

    // impersonate an address with permissions
    vm.prank(address(create));

    // increase the deposit
    ESCRW.increaseDeposit(address(erc20s[0]), amount * 100); // Increasing deposit for 100 items

    // impersonate an address with permissions
    vm.prank(address(admin));

    // set the fee to 7% of the payment
    ESCRW.setFee(700);

    // create rental items for 100 different items
    Item[] memory items = new Item[](100);
    for (uint i = 0; i < 100; i++) {
        items[i] = Item({
            itemType: ItemType.ERC20,
            settleTo: SettleTo.LENDER,
            token: address(erc20s[0]),
            amount: amount,
            identifier: i // Assuming unique identifier for each item
        });
    }

    // create a rental order
    RentalOrder memory rentalOrder = RentalOrder({
        seaportOrderHash: keccak256(abi.encode("someSeaportOrderHash")),
        items: items,
        hooks: new Hook[](0),
        orderType: OrderType.BASE,
        lender: alice.addr,
        renter: bob.addr,
        rentalWallet: address(bob.safe),
        startTimestamp: block.timestamp,
        endTimestamp: block.timestamp + rentDuration
    });

    // warp ahead until the rent duration is over
    vm.warp(block.timestamp + rentDuration);

    // impersonate an address with permissions
    vm.prank(address(stop));

    // settle the payment
    ESCRW.settlePayment(rentalOrder);

    // assert only the fee is left in the escrow
    assertEq(erc20s[0].balanceOf(address(ESCRW)), 0); // THE FEE NEED TO BE 98 , but we have 0 !!!

    // assert the payment was synced
    assertEq(ESCRW.balanceOf(address(erc20s[0])), 0);

    // assert the lender address received the payment in full (MINUS THE FEE) for all 100 items
    assertEq(erc20s[0].balanceOf(alice.addr), 11400); // NEED TO BE 11302
}

However, other posibilities of losing assets are around _calculatePaymentProRata and using block.timestamp for the elapsed time.

Tools Used

Manual review

Recommended Mitigation Steps

Add a check after _settlePayment to ensure the fees left match with the expected true token balance of the escrow.

Assessed type

Other

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-sponsor]

Alec1017 (sponsor) disputed

[Alec1017]

Ran the provided PoC. This is intended behavior because there isnt much we can do about users trading very small amounts. In the example provided, 14 wei is used, which is a very small number of tokens. In this case, the accounting is correct that a fee is not taken here.

The expected amount of fees taken for the transaction in the PoC is indeed 0

[0xean]

Warden's POC shows loss of dust amounts with no proof of how this occurs with real values.

[c4-judge]

0xean marked the issue as unsatisfactory: Insufficient proof