reNFT - SBSecurity's results

Lines of code

https://github.com/re-nft/smart-contracts/blob/main/src/policies/Stop.sol#L194-L212

Vulnerability details

Impact

The protocol has admin-owned hooks, which can be used instead of _checkTransaction from GnosisSafe to disable transferring and approving the rented assets. There can be cases where hooks can contain vulnerable code in their onTransaction function and admins should disable them. Still, a malicious user can effectively block them from disabling by creating a rent to himself with a long rent period. As there will most likely be other rents using these hooks, admins will be forced to call updateHookStatus for onStop and all funds will be locked inside the PaymentEscrow contract, with no way to be recovered.

Proof of Concept

Stop.sol#L211

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); //@audit will revert if hook onStop function is disabled
        }

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding a blacklist to disable hooks only for certain renters, this will ensure that other users who are using the same hook will be able to stop their rents, without losing their funds.

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

ERC721 and ERC1155 tokens that implement ERC721Burnable can be rented for a short rent period and maliciously burned because burn function is not disabled in Guard::checkTransaction.

Guard.sol#L12-L31

import { //@audit burn selector is missing
    shared_set_approval_for_all_selector,
    e721_approve_selector,
    e721_safe_transfer_from_1_selector,
    e721_safe_transfer_from_2_selector,
    e721_transfer_from_selector,
    e721_approve_token_id_offset,
    e721_safe_transfer_from_1_token_id_offset,
    e721_safe_transfer_from_2_token_id_offset,
    e721_transfer_from_token_id_offset,
    e1155_safe_transfer_from_selector,
    e1155_safe_batch_transfer_from_selector,
    e1155_safe_transfer_from_token_id_offset,
    e1155_safe_batch_transfer_from_token_id_offset,
    gnosis_safe_set_guard_selector,
    gnosis_safe_enable_module_selector,
    gnosis_safe_disable_module_selector,
    gnosis_safe_enable_module_offset,
    gnosis_safe_disable_module_offset
} from "@src/libraries/RentalConstants.sol";

Burning will lead to loss of funds from lender as it will cost him the total price of the NFT + amount from the rent because Stop::stopRent() will revert when trying to transfer back the token to the lender. Renter will have to pay for the rent period, but there are ERC721 tokens that return the floor price of the NFT when burned.

Opensea allows NFT collections with a burn mechanism and most of the NFT users don’t have the appropriate knowledge given the fact that they are not technical users.

There are some NFT collections that support token burning:

https://opensea.io/learn/nft/what-is-nft-burning

As we can see there are NFTs with burn functionality and prices over $100k.

Proof of Concept

1. Alice has an NFT from a valuable collection.
2. She sees that Bob (owner of another NFT from the same collection) lists his NFT for renting and sees the opportunity to increase the value of her own NFT by burning his.
3. Fulfills the order and rents it for 1 day.
4. Effectively bypasses the Guard::checkTransaction() and burns the token.

Here is a test that demonstrates step 4, due to the off-chain nature of previous steps:

1. Navigate to test/unit/Guard/CheckTransaction.sol
2. Add new storage variable:

bytes4 public e721_burn_selector = 0x42966c68;

3. Paste the test and execute it with the following command:

forge test --match-test test_Success_CheckTransaction_ERC721_Burn

function test_Success_CheckTransaction_ERC721_Burn() 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); //@audit needed to mark the assets as rented which will trigger the checkTransaction execution
      bytes memory burnFromCalldata = abi.encodeWithSelector(bytes4(keccak256(bytes("burn(uint256)"))), 0); //@audit burn is not handled in checkTransaction allowing renters to burn tokens

      // Expect revert because of an unauthorized function selector
      _checkTransactionRevertUnauthorizedSelector(
          address(alice.safe), address(erc721s[0]), e721_burn_selector, burnFromCalldata
      );
  }

4. Expected outcome is: [FAIL. Reason: call did not revert as expected], because burn selector is not specified in the Guard contract and tx passes successfully.

Tools Used

Manual Review

Recommended Mitigation Steps

Add burn selector to the Guard and disable this functionality too, there are only negatives of allowing such transactions.

Guard.sol#L12-L31

import {
    shared_set_approval_for_all_selector,
    e721_approve_selector,
    e721_safe_transfer_from_1_selector,
    e721_safe_transfer_from_2_selector,
    e721_transfer_from_selector,
    e721_approve_token_id_offset,
    e721_safe_transfer_from_1_token_id_offset,
    e721_safe_transfer_from_2_token_id_offset,
    e721_transfer_from_token_id_offset,
+   e721_burn_selector,
+   e1155_burn_selector,
+   e1155_burn_batch_selector,
    e1155_safe_transfer_from_selector,
    e1155_safe_batch_transfer_from_selector,
    e1155_safe_transfer_from_token_id_offset,
    e1155_safe_batch_transfer_from_token_id_offset,
    gnosis_safe_set_guard_selector,
    gnosis_safe_enable_module_selector,
    gnosis_safe_disable_module_selector,
    gnosis_safe_enable_module_offset,
    gnosis_safe_disable_module_offset
} from "@src/libraries/RentalConstants.sol";

RentalConstants.sol

+ // bytes4(keccak256("burn(uint256)"));
+ bytes4 constant e721_burn_selector = 0x42966c68;
+ uint256 constant e721_burn_token_id_offset = 0x24;

+ // bytes4(keccak256("burn(address,uint256,uint256)"));
+ bytes4 constant e1155_burn_selector = 0xf5298aca;
+ uint256 constant e1155_burn_token_id_offset = 0x24;

+ // bytes4(keccak256("burnBatch(address,uint256[],uint256[])"));
+ bytes4 constant e1155_burn_batch_selector = 0x6b20c454;

Guard.sol::_checkTransaction() add following code:

else if (selector == e721_burn_selector) {
      // Load the token ID from calldata.
      uint256 tokenId = uint256(
          _loadValueFromCalldata(data, e721_burn_token_id_offset)
      );

      // Check if the selector is allowed.
      _revertSelectorOnActiveRental(selector, from, to, tokenId);
		}
else if (selector == e1155_burn_selector) {
      // Load the token ID from calldata.
      uint256 tokenId = uint256(
          _loadValueFromCalldata(data, e1155_burn_token_id_offset)
      );

      // Check if the selector is allowed.
      _revertSelectorOnActiveRental(selector, from, to, tokenId);
		}
else {
      if (selector == e1155_burn_batch_selector) {
          revert Errors.GuardPolicy_UnauthorizedSelector(e1155_burn_batch_selector);
      }
  }

Assessed type

ERC721

Lines of code

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

Vulnerability details

Impact

OrderMetadata struct has nested an array of the Hook struct. This Hook struct is missing from the OrderMeta typehash, which makes the OrderMetadata not compliant with EIP712, and with that, the RentPayload is also not compliant because it has OrderMetadata in it.

Proof of Concept

The OrderMetadata typehash is composed in _deriveRentalTypehashes() as you see, the Hook typestring is missing. Also, you can see the RentalOrder struct, which has nested structs but composes the typehash correctly. https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L406

struct OrderMetadata {
    // Type of order being created.
    OrderType orderType;
    // Duration of the rental in seconds.
    uint256 rentDuration;
    // Hooks that will act as middleware for the items in the order.
    Hook[] hooks;
    // Any extra data to be emitted upon order fulfillment.
    bytes emittedExtraData;
}

function _deriveRentalTypehashes()
    internal
    pure
    returns (
        bytes32 itemTypeHash,
        bytes32 hookTypeHash,
        bytes32 rentalOrderTypeHash,
        bytes32 orderFulfillmentTypeHash,
        bytes32 orderMetadataTypeHash,
        bytes32 rentPayloadTypeHash
    )
{
    // Construct the Item type string.
    bytes memory itemTypeString = abi.encodePacked(
        "Item(uint8 itemType,uint8 settleTo,address token,uint256 amount,uint256 identifier)"
    );

    // Construct the Hook type string.
    bytes memory hookTypeString = abi.encodePacked(
        "Hook(address target,uint256 itemIndex,bytes extraData)"
    );

    // Construct the RentalOrder type string.
    bytes memory rentalOrderTypeString = abi.encodePacked(
        "RentalOrder(bytes32 seaportOrderHash,Item[] items,Hook[] hooks,uint8 orderType,address lender,address renter,address rentalWallet,uint256 startTimestamp,uint256 endTimestamp)"
    );

    // Derive the Item type hash using the corresponding type string.
    itemTypeHash = keccak256(itemTypeString);

    // Derive the Hook type hash using the corresponding type string.
    hookTypeHash = keccak256(hookTypeString);

    // Derive the RentalOrder type hash using the corresponding type string.
    rentalOrderTypeHash = keccak256(
        abi.encode(rentalOrderTypeString, hookTypeString, itemTypeString)
    );

    {
        // Construct the OrderFulfillment type string.
        bytes memory orderFulfillmentTypeString = abi.encodePacked(
            "OrderFulfillment(address recipient)"
        );

        // Construct the OrderMetadata type string.
        bytes memory orderMetadataTypeString = abi.encodePacked(
            "OrderMetadata(uint8 orderType,uint256 rentDuration,Hook[] hooks,bytes emittedExtraData)"
        );

        // Construct the RentPayload type string.
        bytes memory rentPayloadTypeString = abi.encodePacked(
            "RentPayload(OrderFulfillment fulfillment,OrderMetadata metadata,uint256 expiration,address intendedFulfiller)"
        );

        // Derive RentPayload type hash via combination of relevant type strings.
        rentPayloadTypeHash = keccak256(
            abi.encodePacked(
                rentPayloadTypeString,
                orderMetadataTypeString,
                orderFulfillmentTypeString
            )
        );

        // Derive the OrderFulfillment type hash using the corresponding type string.
        orderFulfillmentTypeHash = keccak256(orderFulfillmentTypeString);

        // Derive the OrderMetadata type hash using the corresponding type string.
				// @audit hookTypeString is missing
        orderMetadataTypeHash = keccak256(orderMetadataTypeString);
    }
}

If the struct type references other struct types (and these in turn reference even more struct types), then the set of referenced struct types is collected, sorted by name and appended to the encoding. An example encoding is Transaction(Person from,Person to,Asset tx)Asset(address token,uint256 amount)Person(address wallet,string name). - from EIP 712 (link)

Also read this Stack Overflow page - link

Tools Used

Manual Review

Recommended Mitigation Steps

Make sure to include the typestring for Hook when generating the typehash for OrderMetadata.

function _deriveRentalTypehashes()
    internal
    pure
    returns (
        bytes32 itemTypeHash,
        bytes32 hookTypeHash,
        bytes32 rentalOrderTypeHash,
        bytes32 orderFulfillmentTypeHash,
        bytes32 orderMetadataTypeHash,
        bytes32 rentPayloadTypeHash
    )
{
    // Construct the Item type string.
    bytes memory itemTypeString = abi.encodePacked(
        "Item(uint8 itemType,uint8 settleTo,address token,uint256 amount,uint256 identifier)"
    );

    // Construct the Hook type string.
    bytes memory hookTypeString = abi.encodePacked(
        "Hook(address target,uint256 itemIndex,bytes extraData)"
    );

    // Construct the RentalOrder type string.
    bytes memory rentalOrderTypeString = abi.encodePacked(
        "RentalOrder(bytes32 seaportOrderHash,Item[] items,Hook[] hooks,uint8 orderType,address lender,address renter,address rentalWallet,uint256 startTimestamp,uint256 endTimestamp)"
    );

    // Derive the Item type hash using the corresponding type string.
    itemTypeHash = keccak256(itemTypeString);

    // Derive the Hook type hash using the corresponding type string.
    hookTypeHash = keccak256(hookTypeString);

    // Derive the RentalOrder type hash using the corresponding type string.
    rentalOrderTypeHash = keccak256(
        abi.encode(rentalOrderTypeString, hookTypeString, itemTypeString)
    );

    {
        // Construct the OrderFulfillment type string.
        bytes memory orderFulfillmentTypeString = abi.encodePacked(
            "OrderFulfillment(address recipient)"
        );

        // Construct the OrderMetadata type string.
        bytes memory orderMetadataTypeString = abi.encodePacked(
            "OrderMetadata(uint8 orderType,uint256 rentDuration,Hook[] hooks,bytes emittedExtraData)"
        );

        // Construct the RentPayload type string.
        bytes memory rentPayloadTypeString = abi.encodePacked(
            "RentPayload(OrderFulfillment fulfillment,OrderMetadata metadata,uint256 expiration,address intendedFulfiller)"
        );

        // Derive RentPayload type hash via combination of relevant type strings.
        rentPayloadTypeHash = keccak256(
            abi.encodePacked(
                rentPayloadTypeString,
                orderMetadataTypeString,
                orderFulfillmentTypeString
            )
        );

        // Derive the OrderFulfillment type hash using the corresponding type string.
        orderFulfillmentTypeHash = keccak256(orderFulfillmentTypeString);

        // Derive the OrderMetadata type hash using the corresponding type string.
				// @audit hookTypeString is missing
-       orderMetadataTypeHash = keccak256(orderMetadataTypeString);
+       orderMetadataTypeHash = keccak256(abi.encode(orderMetadataTypeString, hookTypeString));
    }
}

Assessed type

en/de-code

Lines of code

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

Vulnerability details

Impact

The encoding of OrderMetadata encodedData is inaccurately performed, leading to issues in verifying EIP712 signed messages.

Signer::_deriveOrderMetadataHash() is used in Create::_isValidOrderMetadata() and Signer::_deriveOrderMetadataHash() to check if the hash of the OrderMetadata struct is valid. However, there's a mistake in the encoding process because it doesn't hash all the members of OrderMetadata.

function _deriveOrderMetadataHash(
    OrderMetadata memory metadata
) internal view returns (bytes32) {
    // Create array for hooks.
    bytes32[] memory hookHashes = new bytes32[](metadata.hooks.length);

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

    // Derive and return the metadata hash as specified by EIP-712.
    // @audit why not pass 'orderType' and 'emittedExtraData'
@>  return
        keccak256(
            abi.encode(
                _ORDER_METADATA_TYPEHASH,
                metadata.rentDuration,
                keccak256(abi.encodePacked(hookHashes))
            )
        );
}

As you can see the encodedData includes only OrderMetadata.rentDuration and OrderMetadata.hooks, while OrderMetadata.orderType and OrderMetadata.emittedExtraData are not included.

According to EIP712, all struct members should be hashed in the order that they are defined.

The encoding of a struct instance is enc(value₁) ‖ enc(value₂) ‖ … ‖ enc(valueₙ), i.e. the concatenation of the encoded member values in the order that they appear in the type. Each encoded member value is exactly 32-byte long.

Read More here

All of this will result in that only wrong hashed OrderMetadata though Signer::_deriveOrderMetadataHash() to pass when Seaport call validateOrder(). If the Lender pass correctly composed hash in Seaport::OrderComponents::zoneHash by himself and do not use Create::getOrderMetadataHash() for it, any rental will revert because in Create::_isValidOrderMetadata(). This situation leads to an issue where only an incorrectly hashed OrderMetadata can pass through Signer::_deriveOrderMetadataHash() during the Seaport validateOrder(). Even if the Lender provides the correct hash in Seaport::OrderComponents::zoneHash, any rental will still revert in Create::_isValidOrderMetadata().

Proof of Concept

In the tests, when the lender provides the zoneHash, it relies on the incorrect _deriveOrderMetadataHash(). As a result, all these tests will pass, even though the underlying issue exists.

For accurate testing, use the correct OrderMetadata hash. This hash is generated in Remix using the fixed function outlined in the Recommendations section.

Hash: 0x04d9e28e69367406d7fdf7aaeb155428b0555405dbdf6521bfc87f6d3d19867c

Insert it into the test/fixture/engine/OrderCreator.sol file at line 280 exactly as it is shown.

.withZoneHash(0x04d9e28e69367406d7fdf7aaeb155428b0555405dbdf6521bfc87f6d3d19867c)

Run the basic test in Rent::test_Success_Rent_BaseOrder_ERC721. The test will revert CreatePolicy_InvalidOrderMetadataHash.

Run with: forge test --match-test "test_Success_Rent_BaseOrder_ERC721" -vvvv

Note: The Reason: log != expected log message is a result of some expectEmit() calls, but it doesn't affect the test outcome.

Tools Used

Manual Review

Recommended Mitigation Steps

Add orderType and emittedExtraData when create the hashStruct.

function _deriveOrderMetadataHash(
    OrderMetadata memory metadata
) internal view returns (bytes32) {
    // Create array for hooks.
    bytes32[] memory hookHashes = new bytes32[](metadata.hooks.length);

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

    // Derive and return the metadata hash as specified by EIP-712.
    // @audit why not pass 'orderType' and 'emittedExtraData'
    return
        keccak256(
            abi.encode(
                _ORDER_METADATA_TYPEHASH,
+		metadata.orderType,
	        metadata.rentDuration,
                keccak256(abi.encodePacked(hookHashes)),
+	        metadata.emittedExtraData
            )
        );
}

Assessed type

en/de-code

Judge has assessed an item in Issue #604 as 2 risk. The relevant finding follows:

L3

Low Risk

Non-Critical

Low Risks

[L-01] CryptoPunk NFTs will be stolen

Issue Description:

CryptoPunks collection was created before EIP712 was introduced and has an additional function used to offer punk for sale to certain addresses: offerPunkForSaleToAddress.

function offerPunkForSaleToAddress(uint punkIndex, uint minSalePriceInWei, address toAddress) {
    if (!allPunksAssigned) throw;
    if (punkIndexToAddress[punkIndex] != msg.sender) throw;
    if (punkIndex >= 10000) throw;
    punksOfferedForSale[punkIndex] = Offer(true, punkIndex, msg.sender, minSalePriceInWei, toAddress);
    PunkOffered(punkIndex, minSalePriceInWei, toAddress);
}

CryptoPunk NFT renter can call freely this function because Guard is not handling it and pass 1 wei as a price.

Then after rental period is terminated he will still have approval to buy it without any interruptions.

Recommendation:

There is no universal mitigation for all these non-standard NFT implementations but care must be taken when dealing with such tokens and especially CryptoPunks as they are the foundation of the whole NFT ecosystem.

[L-02] Kernel deployment can be frontrunned

Issue Description:

Kernel relies on the deployer to specify addresses of the Admin and Executor.

//@audit frontrun
  constructor(address _executor, address _admin) {
      executor = _executor;
      admin = _admin;
  }

There is also functionality to migrate kernel to a new implementation.

function executeAction(Actions action_, address target_) external onlyExecutor {
  ...More code
      } else if (action_ == Actions.MigrateKernel) {
          ensureContract(target_);
          _migrateKernel(Kernel(target_));
			}
  ...More code
      emit Events.ActionExecuted(action_, target_);
  }

That way of setting roles which are critical for the protocol opens frontrun possibility by a person who will set himself as an executor since this is the person with most privileges across the reNFT contract.

Recommendation:

Consider adding Ownable2Step to the Kernel and assign the owner from there, after that have a separate onlyOwner function to set and change the executor role.

[L-03] ERC721Pausable tokens can block reclaiming

Issue Description:

Since ERC721Pausable is an extension to the EIP712 standard, there will be many tokens that are going to implement it.

One possible use case of renting an NFTs will be to show proof of an ownership to an specific game or organisation, for example Bored Ape or Axie Infinity game token.

If pausable token is being renter there is nothing to stop the renter from pausing the transfers of these NFTs.

He will be able to continue to use it, while the rent will be permanent and there will be no way to recover the rented items from the Stop module.

Recommendation:

Consider adding pause/unpause selectors to the Guard::checkTransaction, that will successfully mitigate the problem.

[L-04] Some NFTs will return the floor price when burned

Issue Description:

There are certain NFTs that will return the floor price to the owner when burned. One example is the [Burnables](https://opensea.io/collection/burnables-nft?embed%5B0%5D=test&embed%5B1%5D=test(.(%22()%27.%2C%2C). We can verify this statement from the description of the collection:

The first-ever refundable NFT. The owner of the token can burn their NFT at any time and have the original mint price sent to their wallet.

Users who have such a token and are non-technical can also rent their NFTs. Then renter who spotted it can rent it for short period and burn it to receive the mint price of the NFT.

This will result in a direct profit for renter, and big loss for the lender because rent payment won’t be distributed as well.

Recommendation:

Add burn function selector which will remove all the malicious use cases for such tokens.

[L-05] No validation if Order.orderType and OrderMetadata.orderType are equal

Issue Description:

The OrderMetadata hash, generated in _deriveOrderMetadataHash(), is missing two crucial members: orderType and emittedExtraData. When fulfilling an order, Seaport calls Create::validateOrder(), performing necessary logic on zones, conversion, and checks. The problem arises because the Lender sets Order.orderType initially, but the Renter can later provide a different value in OrderMetadata. As the hash doesn't consider orderType, _isValidOrderMetadata() passes, causing items to be converted based on the Renter's OrderMetadata.orderType, leading to potential inconsistency. For example, if the Lender sets PAY as the Order.orderType, the Renter can pass BASE in OrderMetadata, resulting in inconsistency.

As all PAY orders are provided with PAYEE by the reNFT backend and executed collectively, the second validateOrder() call in _processBaseOrderConsideration() reverts, because it will try to convert ERC721, but the function only work with ERC20. However, this scenario has no impact whatsoever, except for the pre-existing inconsistency.

Recommendation:

To avoid the inconsistency, consider checking whether Order.orderType equals OrderMetadata.orderType initially in validateOrder(). This step can help skip unnecessary computations or align all logic based on Order.orderType rather than relying on the fulfiller's OrderMetadata.orderType.

Non-Critical

[N‑01] Typos

Issue Description:

Various typos are presented in the codebase that should be fixed to improve the overall quality of the code and make the understanding easier:

as a a kernel → as a kernel

Kernel.sol#L28

- @dev Instantiate this contract as a a kernel adapter. When using a proxy, the kernel address
+ @dev Instantiate this contract as a kernel adapter. When using a proxy, the kernel address

as a a kernel → as a kernel

Kernel.sol#L66

- @dev Instantiate this contract as a a kernel adapter. When using a proxy, the kernel address
+ @dev Instantiate this contract as a kernel adapter. When using a proxy, the kernel address

seaport → Seaport

Create.sol#L408

- @param considerations Array of seaport consideration items.
+ @param considerations Array of Seaport consideration items.

[N‑02] CheckTransaction can be improved

Issue Description:

CheckTransaction is used to prevent transfers and approvals of rented assets originated from safe wallet which has enabled this guard.

Special case are the batch transactions: safeTransferBatch, safeTransferFromBatch. They are totally forbidden from the Guard even if they don’t contain rented assets, due to the complexity that they add - needing to extract the tokenId offset for every token passed in the ids array:

} else {
        // Revert if the `setApprovalForAll` selector is specified. This selector is
        // shared between ERC721 and ERC1155 tokens.
        if (selector == shared_set_approval_for_all_selector) {
            revert Errors.GuardPolicy_UnauthorizedSelector(
                shared_set_approval_for_all_selector
            );
        }

        // Revert if the `safeBatchTransferFrom` selector is specified. There's no
        // cheap way to check if individual items in the batch are rented out.
        // Each token ID would require a call to the storage contract to check
        // its rental status.
        if (selector == e1155_safe_batch_transfer_from_selector) {
            revert Errors.GuardPolicy_UnauthorizedSelector(
                e1155_safe_batch_transfer_from_selector
            );
        }

        // Revert if the `setGuard` selector is specified.
        if (selector == gnosis_safe_set_guard_selector) {
            revert Errors.GuardPolicy_UnauthorizedSelector(
                gnosis_safe_set_guard_selector
            );
        }
    }

This checks can be simplified by placing all the batch transactions into the else if:

else if (
            selector == shared_set_approval_for_all_selector || selector == e1155_safe_batch_transfer_from_selector
                || selector == gnosis_safe_set_guard_selector
        ) {
            revert Errors.GuardPolicy_UnauthorizedSelector(selector);
        }

This will make the whole function shorter and easier to spot that this types of transactions are not supported for the entire safe wallet.

[N‑03] setActiveStatus can implement check for status beforehand

Issue Description:

setActiveStatus function can check for the status of the policy before setting it, that will improve the flow:

function setActiveStatus(bool activate_) external onlyKernel {
+       require(activate_ != isActive, "Cannot re-set same status");
        isActive = activate_;
    }

📝 Analysis - reNFT

Overview

reNFT protocol is focused on lending and collateral-free rentals of ERC721 and ERC1155 tokens in integration with OpenSea’s engine - Seaport and GnosisSafe to prevent theft of rented assets. The protocol allows collateral-free in-house renting, lending, and reward sharing, focusing on scholarship automation.

The key contracts of reNFT protocol for this audit are:

• Create: Contract serving as an entry point for renters, this is their entry point for renting assets (ERC721, ERC1155). There OpenSea orders are fulfilled, rentals are registered as rentals to the wallets of the users, and payments are escrowed to PaymentEscrow.
• Stop: Contract that allows all users to stop rent, distribute the payments either pro rata (PayOrder) or in full (BaseOrder and expired PayOrder), and transfer back the rental assets to the lender.
• PaymentEscrow: Contract that is used for internal ERC20 accounting, payment calculation, and distribution between involved parties.
• Storage: Contract used as storage - keeping, and maintaining information about hooks, rentals, and safes. Additions and removals are routed to functions in this contract.
• Factory: Contract used to deploy rental safes (GnosisSafe) on behalf of renters with hardcoded module and guard policy which prevent users from stealing the NFTs.
• Guard: Contract which serves as SafeWallet guard, all the transactions (except the ones from Stop policy) are routed and checked in the checkTransaction function.

System Overview

Scope

1. Core

• Kernel - core contract used as a registry, manages permissions, upgrades, activates, and deactivates policies and modules.
• Create2Deploy - contract used for a deterministic salt-based deployment with added cross-chain safety.

2. Modules

• PaymentEscrow - internal ERC20 accounting proxy contract, also calculates involved parties’ payments.
• Storage - module proxy contract used as storage - keeping, and maintaining information about hooks, rentals, and safes. Additions and removals are routed to functions in this contract.

3. Policies

• Admin - contract responsible for fee management, whitelisting delegates and extensions, and upgrading modules.
• Create - contract serving as a main entry point for renters after the order is being fulfilled, used to convert Order and Consideration structs to reNFT native and save it into memory allocated mapping.
• Factory - contract used as deployment agent for Gnosis safe wallets and setting predefined wallet module (Stop) and guard (Guard.sol) to prevent malicious transactions from being executed.
• Guard - contract inheriting from Gnosis Safe’s Guard interface, used to verify all the transactions originated from a wallet that involves rented assets.
• Stop - contract used to reclaim rental assets and release the funds from the escrow contract.

4. Packages

• Accumulator - contract used to handle the manipulation of dynamically allocated arrays of data structures directly in memory.
• Reclaimer - contract used to transfer back rental assets to the proper recipient when the period has expired or the lender terminated the rent.
• Signer - contract used to create type hashes and signature verification when creating rentals.

Privileged Roles

The approach that reNFT team has taken is unique, they are configuring permissions to policies based on functions that need to be called.

There are standard trusted roles assigned to real people:

**ADMIN** - role used to grant and revoke other roles.
**SEAPORT** - singleton role granted to Seaport, which will make it possible to call **validateOrder** function in **Create** policy.
**CREATE_SIGNER** - privileged EOA which will live on the ReNFT backend, owned by ReNFT.
**ADMIN_ADMIN** - admin of the protocol, can skim funds, and manage critical params through **Admin** policy
**GUARD_ADMIN** - role given to EOA, can update hook status and path.
**EXECUTOR** - role used only for action execution in the **Kernel**

Approach Taken-in Evaluating The reNFT

Codebase Explanation & Examples Scenarios of Intended Protocol Flow

All possible Actions and Flows in reNFT:

1. Safe wallet deployment

Rental process cannot be finalized without a valid safe gnosis wallet deployed by the renter.

This can be done through Factory policy as it contains all the needed configurations for a safe deployment and transaction validation. Pre deployed Gnosis contracts, such as FallbackHandler, SafeProxyFactory and SafeSingleton, are being used. Additionally, Guard policy is set as guard, that is used to prevent malicious transactions with rental assets involved, such as transfers and approvals, Stop policy is set as wallet module as it is used to reclaim the rented assets without routing the transaction through the Guard’s checkTransaction. TokenCallbackHandler is added as a wallet fallback handler, to be able to handle rental assets as this adds callback functionality.

2. Rental initiation

When a user wants to rent an NFT he interacts only with the backend of the protocol by choosing the token that he want to rent and the rent period. Then fulfillAdvancedOrder in Seaport is called which does various validations and actions, one of them to transfer the token to the recipient specified in the ZoneParameters which is passed to the reNFT’s Create policy through the validateOrder function. This function is the entry point for the reNFT rental creation as it does all the needed actions to ensure information is saved and can be tracked properly in the system:

• Check that the payload is not expired.

• Validates if the intended fulfiller by the lender is the same as the actual fulfiller.

• Recovers the RentPayload signer, who is a reNFT person who is granted the CREATE_SIGNER role.

• Valid safe wallet is required (should be deployed through Factory policy).

• SpentItem and ReceivedItem, aka offer and consideration items from a fulfiller’s perspective are converted to protocol native struct: Item. Then rental assets (ERC721, ERC1155) which are hashed with the help of Accumulator (package used for in memory structure, holding unknown size of elements) added as rentals to the Storage module. (This is valid only for Base and Pay order types. You can read more about different order types here).

• ERC20 items are used to increase the internal balance of the PaymentEscrow module.

  PaymentEscrow module is passed as a conduit recipient and receives all the token payments when rent is initiated.

• If renter has specified hooks, mostly used for Pay order types, their onStart function is being executed (execution can be disabled by an admin).

• Event is emitted and order is saved off-chain.

• Lastly, valid order magic value is returned in order Seaport to consider the execution successful.

Note about why seaport was forked:

reNFT uses validateOrder with modified ZoneParameters struct that contains totalExecutions used to validate the recipient for both ERC721 and ERC20 token which has to be Gnosis Safe wallet of the renter and PaymentEscrow module respectively. This information is missing in the original OpenSea code.

3. Safe transaction execution

First we need to understand how gnosis safes generally works - their architecture is modular, which allows owners to specify various add-ons on top of a wallet.

Some of them are guards and modules:

• Guard - this is a special EIP165 compliant contract that is executed for all normal transactions, initiated from a wallet and does various checks and validations according to the needs of the owners.
• Module - contract that adds additional logic to an existing wallet - in reNFT Stop policy is configured as a wallet module. Note that modules can execute transactions without validating them through the Guard which is crucial for the seamless rental reclaiming.

Transaction is initiated from a wallet and if it’s not originated from a module, Guard’s checkTransaction is executed. This function is used to prevent renters from executing transactions that involve approvals or transfers of rented assets.

On top of that, if renter has specified hooks on rental initiation process and they are activated by an admin, their onExecution function is called giving an additional extendability to the rental process.

Note: delegate call functionality offered by Gnosis is disabled by default, unless explicitly enabled by an admin for specific transaction recipients.

4. Rental termination

Process of stopping a rent can be initiated by everyone, but there are caveats - in case orderType is Base order it will only succeed if rent period has passed, not matter who the caller is. For Pay order if lender initiated the reclaim action, rent period is not taken into account, for non-lender users the rule above applies.

Then for rental items (ERC721 and ERC1155) same dynamic memory structure is created as the one in Rental initiation is created. In case there are hooks specified their onStop function is being executed (execution can be disabled by an admin).

Transaction from the Gnosis safe module (Stop) is executed. This is done to prevent the Guard::checkTransaction from being called (that’s how modules in gnosis safe work) and assets are successfully returned to the renter.

PaymentEscrow settles the payments in either full or pro rata, depending if the rent period is passed and the order type:

• For non-expired Pay orders:
  • Payment is split between lender and renter equally(slightly in favor of the renter because of rounding protection) based on the elapsed time.
• For expired Pay orders:
  • Full payment is settled to the renter due to the nature of this orderType.
• For Base orders:
  • Full payment is settled to the lender.

Rental references are removed from the Storage module which prevents from claiming the tokens twice.

Lastly, event is emitted to notify the off-chain that the rental order has stopped.

Architecture Recommendations

Architecture of the protocol is simple and robust, there is no big complexity for the main tasks that will be performed - renting and executing transactions. There are other parts of the code that are requiring more attention such as upgrades and dynamically allocated data structs kept in memory.

Contracts are split by context which makes the understanding easier.

The idea of hooks is essential as it will extend significantly the use case of the protocol.

Create2Deployer is gas efficient due to the amount of assembly used, would be great if same effect is applied to Factory policy, because it will be used quite often.

Important notes regarding the codebase and its sustainability:

• Guard policy does not prevent important functions from being executed such as burn, it is important to note that all the rent request will be approved by reNFT signer, but handling such function will definitely prevent from malicious actions.
• Most of the hashes are constructed wrong, which will DoS the rentals.

Codebase Quality Analysis

We consider the codebase of the reNFT simple and versatile, hooks greatly fit the main idea of the team but also does not limit the working capabilities of the protocol as not being mandatory and having the go-to checkTranscation function giving the same or even bigger sense of security. Safe wallets deployment is utilised to automatically deploy one on behalf of renter. The rent stop process is not as good as the other parts of the code, as it heavily depends on the hooks’ (if order uses any) onStop not being disabled, otherwise NFTs will be stuck in rental safes.

Tests have high abstraction which is a double-edged sword as they require more time to be completely understood, but due to Seaport integration this can’t be mitigated.

Test analysis

The audit scope of the contracts to be audited is 80% most of them cover the whole flow of the actions that are available in the system without testing in isolation.

However, development team did a great job by not creating mocks for most of the contracts but fixtures with the real logic of the contracts were used.

This approach deviates a little from the initial idea of unit testing - to test only small modules, but gives context what happens in the both protocols that are integrated in the system - GnosisSafe and Seaport.

Thing to consider about the tests is how the hashes in fixtures are constructed. Same function is used to hash both structs and this gives false sense of security and most of our reported vulnerabilities are from these parts of the code. Currently their representation is something like this: _deriveOrderMetadataHash(arg) == _deriveOrderMetadataHash(arg), which will always return true.

Systemic & Centralization Risks

reNFT utilises contract based authorization which gives the policy contracts (contracts where users interact with the system) certain function selectors from the module contracts that can be called from these contracts. This approach lowers the centralization risk of the protocol.

There are other roles as well, such as EXECUTOR and ADMIN that will be given to EOAs and they poses serious centralisation risk, especially the EXECUTOR.

If it turns out to be given to a malicious user, this can lead to catastrophic consequences as he can change the Kernel, Policies and Modules implementation which will directly affect both lenders and renters.

Team Findings

Findings were identified in the Signer and Guard contracts.

They cause significant impact because:

• Signer policy malfunction which leads to wrongly hashed structs.
• Burn function selector is not handled and can result in 100%+ loss for lender and in some cases profit for renter who burned because NFT refunds the floor price.

New insights and learning from this audit

Learned about:

• Seaport
  • Zones
  • Order fulfillment
  • Order creation
  • Conduits
  • TokenTransferrer from Seaport fulfillment
• GnosisSafe
  • Guard
  • Modules
  • Safe transaction execution
• EIP 712 in depth

Conclusion

In general, the reNFT project exhibits an standard and well-developed architecture we believe the team has done a good job regarding the code, but the identified risks need to be addressed, and measures should be implemented to protect the protocol from potential malicious use cases. Additionally, it is recommended to improve the documentation as there is a need of proper understanding of the underlying architecture interacting with the integrated protocols. It is also highly recommended that the team continues to invest in security measures such as mitigation reviews, audits, and bug bounty programs to maintain the security and reliability of the project.

Time spent:

55 hours