reNFT - Jorgect's results

Lines of code

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

Vulnerability details

The Guard Policy serves as a crucial module within the SafeWallet, safeguarding against renters attempting to get away with the rent NFT.

The Guard Policy check if the transactions is a transfer or aprove among others selectors. In the event that the transaction pertains to an active rental, the Guard Policy promptly triggers a revert action, ensuring the prevention of any unauthorized actions.

function _checkTransaction(address from, address to, bytes memory data) private view {
        bytes4 selector;

        // Load in the function selector.
        assembly {
            selector := mload(add(data, 0x20))
        }

        ...

        } else if (selector == e1155_safe_transfer_from_selector) { <----

            // Load the token ID from calldata.
            uint256 tokenId = uint256(
                _loadValueFromCalldata(data, e1155_safe_transfer_from_token_id_offset)
            );

         ...
    }


function _revertSelectorOnActiveRental(
        bytes4 selector,
        address safe,
        address token,
        uint256 tokenId
    ) private view {
        // Check if the selector is allowed.
        if (STORE.isRentedOut(safe, token, tokenId)) {
            revert Errors.GuardPolicy_UnauthorizedSelector(selector);
        }

[Link] [Link]

The current logic is effective for ERC721 tokens. However, challenges arise when dealing with ERC1155 tokens since a user can possess multiple ERC1155 NFTs with the same ID. Even if a user rents only one NFT, the other NFTs sharing the same ID may become untransferable in the wallet for the duration of the rent period. Consider the following scenario:

In a scenario where a user needs two ERC1155 tokens to complete a mission in an NFT game, and they have the resources to mint one and rent another, the following issue arises. After the user mints the first NFT and subsequently rents another (keeping in mind ERC1155 tokens can share the same ID), upon completing the mission, they encounter difficulties when attempting to sell or transfer the minted NFT. This issue is triggered by the Guard Policy, even when the particular NFT was not rented.

This can happen not only minting the nft but also is they transfer the erc1155 to the safe wallet.

Impact

The impact of this issue is significant, especially for active web3 gamers who frequently engage with ERC1155 tokens in various web3 games. When a user rents an NFT, other ERC1155 tokens sharing the same ID become inaccessible and get stuck in the wallet. This limitation poses a serious challenge for gamers who rely on the constant renting and movement of NFTs between wallets for seamless gameplay experiences.

Note that this not happen with erc721.

Proof of Concept

First add the follow mint function in the mockErc1155 file:smart-contracts/test/mocks/tokens/standard /MockERC1155.sol

 function mintSameTokenId(address to, uint256 amount, uint256 tokenId) public {
        _mint(to, tokenId, amount, "");

    }

Next copy the next POC in the file:smart-contracts/test/integration /Rent.t.sol

function test_Success_Rent_BaseOrder_ERC1155_vul() public {
        // create a BASE order
        createOrder({
            offerer: alice,
            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: bob,
            order: order,
            orderHash: orderHash,
            metadata: metadata
        });

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


        //Mint another nft for bob

        erc1155s[0].mintSameTokenId(bob.addr,0, 1);

        vm.startPrank(bob.addr);

        vm.expectRevert();
        // The transaction should revert
        erc1155s[0].safeTransferFrom(bob.addr,address(this), 0, 1, "");

        vm.stopPrank();

    }

As you can see in the proof of concep user rent and the mint and the nft get stuck

Tools Used

Manual foundry

Recommended Mitigation Steps

Consider check the amount of the erc1155 that the order has and allow renters transfer is the balance of user for a givend id erc1155 is not less than the order

Assessed type

Other

Lines of code

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

Vulnerability details

When signing a rental order, the lender can decide to include an array of Hook structs along with it. These are bespoke restrictions or added functionality that can be applied to the rented token within the wallet. This protocol allows for flexibility in how these hooks are implemented and what they restrict. A common use-case for a hook is to prevent a call to a specific function selector on a contract when renting a particular token ID from an ERC721/ERC1155 collection.

hooks within the contract can be activated or deactivated through a bitmap. The contract subsequently verifies whether these hooks have been removed or are still in place:

function stopRent(RentalOrder calldata order) external {
        ...

        // Interaction: process hooks so they no longer exist for the renter.
        if (order.hooks.length > 0) {
            _removeHooks(order.hooks, order.items, order.rentalWallet);
        }

        ...
}

 function _removeHooks(
        Hook[] calldata hooks,
        Item[] calldata rentalItems,
        address rentalWallet
    ) internal {
        // Define hook target, item index, and item.
        address target;
        uint256 itemIndex;
        Item memory item;

        // Loop through each hook in the payload.
        for (uint256 i = 0; i < hooks.length; ++i) {
            // Get the hook address.
            target = hooks[i].target;

            // Check that the hook is reNFT-approved to execute on rental stop.
            if (!STORE.hookOnStop(target)) {  <-----------
                revert Errors.Shared_DisabledHook(target);
            }

         ...
}

[Link] [Link]

The issue arises when a user obtains a rented NFT with certain hooks. If these hooks encounter problems and the project decides to eliminate them, all NFTs rented with these hooks become immobilized in the safe wallet.

Impact

NFTs may become immobilized within a safe wallet, creating a scenario where renters are unable to transfer the NFTs due to the Guard module. Simultaneously, lenders are unable to stop the order as the function triggers a revert.

Proof of Concept

As you can see in the _removeHooks function which is triggered by the stopRent function.

 function _removeHooks(
        Hook[] calldata hooks,
        Item[] calldata rentalItems,
        address rentalWallet
    ) internal {
        // Define hook target, item index, and item.
        address target;
        uint256 itemIndex;
        Item memory item;

        // Loop through each hook in the payload.
        for (uint256 i = 0; i < hooks.length; ++i) {
            // Get the hook address.
            target = hooks[i].target;

            // Check that the hook is reNFT-approved to execute on rental stop.
            if (!STORE.hookOnStop(target)) {  <-----------
                revert Errors.Shared_DisabledHook(target);
            }

         ...
}

This presents a significant issue, as it could lead to the NFT getting stuck within the safe wallet.

Tools Used

Manual, Foundry

Recommended Mitigation Steps

Instead of triggering a revert when a hook is deactivated, consider allowing the renter to stop the transaction. This approach provides renters with the ability to manage and resolve issues related to deactivated hooks without encountering a transaction revert.

function _removeHooks(
        Hook[] calldata hooks,
        Item[] calldata rentalItems,
        address rentalWallet
    ) internal {
        // Define hook target, item index, and item.
        address target;
        uint256 itemIndex;
        Item memory item;

        // Loop through each hook in the payload.
        for (uint256 i = 0; i < hooks.length; ++i) {
            // Get the hook address.
            target = hooks[i].target;

            // Check that the hook is reNFT-approved to execute on rental stop.
            if (!STORE.hookOnStop(target)) {  
                return Errors.Shared_DisabledHook(target); <-----------
                // another logic can be added here
            }

         ...
}

This has to be evaluted and have to be carefully revised.

Assessed type

Other