reNFT - zzebra83's results

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L147-L154

Vulnerability details

Impact

During rental creation, all data needed by third party integrators to construct the rental order, including their hash, is emitted as an event. The constructed rental order can then be used to stop rentals if they expired for instance.

    function _emitRentalOrderStarted(
    RentalOrder memory order,
    bytes32 orderHash,
    bytes memory extraData
) internal {
    // Emit the event.
    emit Events.RentalOrderStarted(
        orderHash, // @audit order hash is here, doesnt follow EIP712
        extraData,
        order.seaportOrderHash,
        order.items,
        order.hooks,
        order.orderType,
        order.lender,
        order.renter,
        order.rentalWallet,
        order.startTimestamp,
        order.endTimestamp
    );

during the stopping of the rental, the way the constructed rental order is validated is done by comparing it to the hash of the original order made when created.

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

    function removeRentals(
    bytes32 orderHash,
    RentalAssetUpdate[] calldata rentalAssetUpdates
) external onlyByProxy permissioned {
    // The order must exist to be deleted.
    if (!orders[orderHash]) { // @audit will revert if derived hash is not stored
        revert Errors.StorageModule_OrderDoesNotExist(orderHash);
    } else {
        // Delete the order from storage.
        delete orders[orderHash];
    }

A third party integrator can reconstruct the rental order using data from event, and for additional sanity checking, they can hash it according to EIP712 and compare it to the emitted hash in the event, before they pass it reNFT. this is a reasonable step because it ensures they validate their constructed order before they pass it to reNFT.

The problem however is that hash created during rental created does not adhere to EIP712 if there are hooks with the rental order. This can lead to unexpected integration failures. lets see how.

The _deriveRentalOrderHash from the signer contract is used to generate the hash. if the rental order has hooks, they are also hashed and added to hookhashes list which itself will be hashed as part of final hash.

        // Iterate over each hook.
    for (uint256 i = 0; i < order.hooks.length; ++i) {
        // Hash the hook.
        hookHashes[i] = _deriveHookHash(order.hooks[i]);
    }

The problem specifically arises in _deriveHookHash.

    function _deriveHookHash(Hook memory hook) internal view returns (bytes32) {
    // Derive and return the hook as specified by EIP-712.
    // @audit extraData is of type bytes, needs to be hashed with keccak
    return
        keccak256(
            abi.encode(_HOOK_TYPEHASH, hook.target, hook.itemIndex, hook.extraData)
        );
}

notice how hook extra data, which is of bytes type is not hashed. this is in violation of EIP712 standards, which states dynamic data needs to be hashed.

https://eips.ethereum.org/EIPS/eip-712#definition-of-encodedata

Implications of this is it is reasonable to assume third party integrators who attempt to stop rentals via the data in the emitted event, might reconstruct the rental order properly, and then compare it to the incorrect hash in the event, only to realize it does not match. This might cause the stop rental execution to fail on their end.

Proof of Concept

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

           // create a custom offer item that uses the game token
    OfferItem[] memory offers = new OfferItem[](1);
    offers[0] = OfferItem({
        itemType: ItemType.ERC1155,
        token: address(gameToken),
        identifierOrCriteria: 0,
        startAmount: 1e18,
        endAmount: 1e18
    });

    // Define the extra data to be used by the hook
    RevenueShare memory revenueShareData = RevenueShare({
        // lender address which will receive the rewards
        lender: alice.addr,
        // lender wants to take 70% of the revenue
        lenderShare: 70
    });

    // Define the hook for the rental
    Hook[] memory hooks = new Hook[](1);
    hooks[0] = Hook({
        // the hook contract to target
        target: address(hook),
        // index of the item in the order to apply the hook to
        itemIndex: 0,
        // any extra data that the hook will need.
        extraData: abi.encode(revenueShareData)
    });

    // use an amendment to switch the offer item to the game token
    withReplacedOfferItems(offers);

    // use an amendment to add hooks to the metadata
    withOrderMetadata(
        OrderMetadata({
            // the type of order being created
            orderType: OrderType.BASE,
            // the duration of the rental in seconds
            rentDuration: 500,
            // the hooks that will act as middleware for the items in the order
            hooks: hooks,
            // any extra data to be emitted upon order fulfillment
            emittedExtraData: bytes("")
        })
    );

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

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

    // @audit hash using hashed hooks does not equal that of current invalid implementation
    assertTrue(create.getRentalOrderHash(rentalOrder) != create.getRentalOrderHashUpdated(rentalOrder));

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

    // stop the rental order
    vm.prank(alice.addr);
    stop.stopRent(rentalOrder);

    // assert that the rental order doesnt exist in storage
    assertEq(STORE.orders(orderHash), false);

    // assert that the token is no longer rented out in storage
    assertEq(STORE.isRentedOut(address(bob.safe), address(erc721s[0]), 0), false);

    // assert that the ERC721 is back to its original owner
    assertEq(erc721s[0].ownerOf(0), address(alice.addr));

    // assert that the offerer received a payment
    assertEq(erc20s[0].balanceOf(alice.addr), uint256(10100));

    // assert that the fulfiller made a payment
    assertEq(erc20s[0].balanceOf(bob.addr), uint256(9900));

    // assert that a payment was pulled from the escrow contract
    assertEq(erc20s[0].balanceOf(address(ESCRW)), uint256(0));
}

To get the test above working, add it to ERC20RewardHook.t.sol test file. You will also need to modify the signers.sol to add additional functions:

    function _deriveHookHashUpdated(Hook memory hook) internal view returns (bytes32) {
    // Derive and return the hook as specified by EIP-712.
    return
        keccak256(
            abi.encode(_HOOK_TYPEHASH, hook.target, hook.itemIndex, keccak256(hook.extraData))
        );
}

    function _deriveRentalOrderHashUpdated(
    RentalOrder memory order
) internal view returns (bytes32) {
    // Create arrays for items and hooks.
    bytes32[] memory itemHashes = new bytes32[](order.items.length);
    bytes32[] memory hookHashes = new bytes32[](order.hooks.length);

    // Iterate over each item.
    for (uint256 i = 0; i < order.items.length; ++i) {
        // Hash the item.
        itemHashes[i] = _deriveItemHash(order.items[i]);
    }

    // Iterate over each hook.
    for (uint256 i = 0; i < order.hooks.length; ++i) {
        // Hash the hook.
        hookHashes[i] = _deriveHookHashUpdated(order.hooks[i]);
    }

    return
        keccak256(
            abi.encode(
                _RENTAL_ORDER_TYPEHASH,
                order.seaportOrderHash,
                keccak256(abi.encodePacked(itemHashes)),
                keccak256(abi.encodePacked(hookHashes)),
                order.orderType,
                order.lender,
                order.renter,
                order.startTimestamp,
                order.endTimestamp
            )
        );
}

finally add this function to create.sol

    function getRentalOrderHashUpdated(
    RentalOrder memory order
) external view returns (bytes32) {
    return _deriveRentalOrderHashUpdated(order);
}

Basically, the crux of the test is in this line:

        // @audit hash using hashed hooks does not equal that of current invalid implementation
    assertTrue(create.getRentalOrderHash(rentalOrder) != create.getRentalOrderHashUpdated(rentalOrder));

it compares the hash generated by the faulty hook method with one that adheres to EIP712, they dont match, which like mentioned before, can be problematic for third party integrators that do adhere to EIP712.

Tools Used

Manual Review

Recommended Mitigation Steps

update _deriveHookHashUpdated as so:

    function _deriveHookHashUpdated(Hook memory hook) internal view returns (bytes32) {
    // Derive and return the hook as specified by EIP-712.
    return
        keccak256(
            abi.encode(_HOOK_TYPEHASH, hook.target, hook.itemIndex, keccak256(hook.extraData))
        );
}

Assessed type

en/de-code