Asset can be stuck in renters safe and funds stolen from the escrow preventing other lenders from settling their positions.

[c4-bot-9]

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L320-L330 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L265-L306

Vulnerability details

if ESCRW::settlePayment(...) is called before Stop.stopRent(...) for a given Base rental order, funds can be stolen from the escrow preventing other users from stopping their rental leading to loss of funds for lenders and asset stuck in renters safe.

Vulnerability Description

This is a different finding I submitted for a reentrancy in the ESCRW::settlePayment(...) as this one is a different attack path although a related underlying issue

POC Summary

settlePayment(...) is an external function that has a wrongly implemented check

Summary

• Alice creates a BASE aliceRentalOrder with 100 wei, for 500s
• Carol creates a BASE `carolRentalOrdercarolRentalOrder with 100 wei, for 500s
• Bob fulfills both orders and 200wei is held to the ESCRW
• Authorised policy calls settlePayment(...) with Alice’s aliceRentalOrder after 200s
• Alice is settled with 100 wei
• 500s pass, both rentals have expired
• Alice calls Stop.stopRent(...) with aliceRentalOrder, she is settled with another 100 wei and now she has a balance of 200 wei
• Carol calls Stop.stopRent(...) with carolRentalOrder and it reverts because the ESCROW does not have enough balance to settle Carol and the call reverts.

Because ESCRW::settlePayment(...) can be called before Stop.stopRent(...) is called, it can lead to other lenders being unable to settle their positions leading to a loss of funds from the escrow and

function stopRent(RentalOrder calldata order) external {
        // Check that the rental can be stopped.
        _validateRentalCanBeStoped(order.orderType, order.endTimestamp, order.lender);

        ...
        // Interaction: Transfer ERC20 payments from the escrow contract to the respective recipients.
        ESCRW.settlePayment(order);

        // Interaction: Remove rentals from storage by computing the order hash.
        STORE.removeRentals(
            _deriveRentalOrderHash(order),
            _convertToStatic(rentalAssetUpdates)
        );

        // Emit rental order stopped.
        _emitRentalOrderStopped(order.seaportOrderHash, msg.sender);
    }

the _settlePayment(...) does not remove rentals after settling due payment which in itself will be disastrous if the payment is settled and rentalOrder is removed without settliing asset

function _settlePayment(
        Item[] calldata items,
        OrderType orderType,
        address lender,
        address renter,
        uint256 start,
        uint256 end
    ) internal {
        ...
        for (uint256 i = 0; i < items.length; ++i) {
            // Get the item.
            Item memory item = items[i];

            ...
            }
        }
    }

CODED POC

Add the test case below to the test/integration/StopRent.t.sol file and run forge test --mt test_Settle_And_StopRent_BaseOrder -vvv

function test_Settle_And_StopRent_BaseOrder() public {

        // create a BASE order for alice
        createOrder({
            offerer: alice,
            orderType: OrderType.BASE,
            erc721Offers: 1,
            erc1155Offers: 0,
            erc20Offers: 0,
            erc721Considerations: 0,
            erc1155Considerations: 0,
            erc20Considerations: 1
        });

        // finalize alice order creation
        (
            Order memory aliceOrder,
            bytes32 aliceOrderHash,
            OrderMetadata memory aliceMetadata
        ) = finalizeOrder();

        // create an order fulfillment for alice order
        createOrderFulfillment({
            _fulfiller: bob,
            order: aliceOrder,
            orderHash: aliceOrderHash,
            metadata: aliceMetadata
        });

        // finalize Alice base order fulfillment
        RentalOrder memory aliceRentalOrder = finalizeBaseOrderFulfillment();

        assertEq(erc20s[0].balanceOf(address(ESCRW)), 100);

        // create a BASE order for carol
        createOrder({
            offerer: carol,
            orderType: OrderType.BASE,
            erc721Offers: 1,
            erc1155Offers: 0,
            erc20Offers: 0,
            erc721Considerations: 0,
            erc1155Considerations: 0,
            erc20Considerations: 1
        });

        // finalize carol order creation
        (
            Order memory carolOrder,
            bytes32 carolOrderHash,
            OrderMetadata memory carolMetadata
        ) = finalizeOrder();

        // create an order fulfillment for alice order
        createOrderFulfillment({
            _fulfiller: bob,
            order: carolOrder,
            orderHash: carolOrderHash,
            metadata: carolMetadata
        });

        // finalize Alice base order fulfillment for carol
        RentalOrder memory carolRentalOrder = finalizeBaseOrderFulfillment();

        // extend time but before the end of the rental end time
        vm.warp(block.timestamp + 200);

        assertEq(erc20s[0].balanceOf(address(ESCRW)), 200);
        assertEq(erc20s[0].balanceOf(address(alice.addr)), 10000);
        emit log_named_uint("Escrows Balance before settling payment", erc20s[0].balanceOf(address(ESCRW)));
        emit log_named_uint("Alice Balance before settling payment", erc20s[0].balanceOf(address(alice.addr)));

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

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

        assertEq(erc20s[0].balanceOf(address(ESCRW)), 100);
        assertEq(erc20s[0].balanceOf(address(alice.addr)), 10100);

        // speed up in time past the rental expiration
        vm.warp(block.timestamp + 750);

        emit log_named_uint("Escrows Balance after settling payment", erc20s[0].balanceOf(address(ESCRW)));
        emit log_named_uint("Alice Balance after settling payment", erc20s[0].balanceOf(address(alice.addr)));

        // Alice stops her rental order
        vm.prank(alice.addr);
        stop.stopRent(aliceRentalOrder);

        assertEq(erc20s[0].balanceOf(address(ESCRW)), 0);
        assertEq(erc20s[0].balanceOf(address(alice.addr)), 10200);
        emit log_named_uint("Escrows Balance after stoping rent", erc20s[0].balanceOf(address(ESCRW)));
        emit log_named_uint("Alice Balance after stoping rent", erc20s[0].balanceOf(address(alice.addr)));

        // Carol tries stop her rental order but it reverts due to insufficient balance in the ESCRW
        vm.startPrank(carol.addr);
        vm.expectRevert();
        stop.stopRent(carolRentalOrder);
        vm.stopPrank();

    }

TOOLS USED

Foundry

RECOMMENDATION

Ensure that ESCRW.settlePayment(...) is only callable in the flow of execution of Stop.stopRent(...)

Assessed type

Invalid Validation

[c4-pre-sort]

141345 marked the issue as duplicate of #237

[c4-judge]

0xean marked the issue as satisfactory

[c4-judge]

0xean removed the grade

[c4-judge]

0xean marked the issue as unsatisfactory: Insufficient proof

[0xean]

invalid POC

[c4-judge]

0xean changed the severity to 2 (Med Risk)