reNFT - kaden's results

Lines of code

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

Vulnerability details

Impact

The gnosis_safe_disable_module_offset constant is incorrect, pointing to the disableModule's prevModule param rather than the intended module param. This allows an attacker to unexpectedly disable a module in the case that prevModule is whitelisted. This also may prevent disabling modules which should be allowed to be disabled.

Proof of Concept

If the disableModule selector is used in a rental safe transaction, it will get processed by Guard._checkTransaction, where if the address retrieved at the gnosis_safe_disable_module_offset is not a whitelisted module, execution will revert:

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

The intention of this logic is to revert if the module being disabled is whitelisted. We can see by looking at the disableModule function that the module to disable is the second param:

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

However, gnosis_safe_disable_module_offset actually points to the first param:

// @audit 0x20 offset points to selector then additional 0x04 only gives us the first param
uint256 constant gnosis_safe_disable_module_offset = 0x24;

As a result, the transaction will instead revert if the prevModule param is not a whitelisted module and unexpectedly succeed if the prevModule param is a whitelisted module. This may result in:

• Attackers being able to disable a module which is not whitelisted, or
• Users being unable to disable a module which is whitelisted

Tools Used

• Manual review

Recommended Mitigation Steps

To solve, we can simply properly set the gnosis_safe_disable_module_offset constant:

uint256 constant gnosis_safe_disable_module_offset = 0x44;

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

_deriveRentalOrderHash fails to include the rentalWallet field. This allows for malicious renters to DoS execution of stopRent.

Proof of Concept

We can see that the RentalOrder struct includes a rentalWallet field:

struct RentalOrder {
    bytes32 seaportOrderHash;
    Item[] items;
    Hook[] hooks;
    OrderType orderType;
    address lender;
    address renter;
    address rentalWallet;
    uint256 startTimestamp;
    uint256 endTimestamp;
}

We can also see that in _deriveRentalOrderHash that the rentalWallet is not included in the hash:

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

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

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

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

This means that the rentalWallet passed to stopRent can be any arbitrary address and validation logic will still succeed.

A malicious renter can take advantage of this by executing the following attack:

• Attacker takes out a rental (order B) containing multiple ERC1155 tokens of the same tokenId
• Attacker then creates their own rental (order A) on a different rental safe in which they rent just one ERC1155 token of the same tokenId as in order B
• Attacker calls stopRent, providing order A but with the rentalWallet associated with order B

Since order A contains a subset of rental items that order B contains, even though the rentalWallet provided was incorrect, all the checks pass because the provided rentalWallet and it's associated state contain the token which we're trying to withdraw.

Now order A has been stopped successfully but the quantity of tokens available in order B has decreased while the stored order hash contains the full amount of tokens. This leads to a DoS when trying to stop order B because the safe and its associated state don't include sufficient rental tokens to withdraw. Note that even if we transferred in the token needed, we still would be DoS'd since Storage.removeRentals already decreased the rentedAssets[asset.rentalId] when stopping order A.

// @audit this will underflow now for order B
rentedAssets[asset.rentalId] -= asset.amount;

We prove this attack using the following coded PoC in StopRent.t.sol:

First we must modify OrderCreator._createOfferItems such that we can reuse the same ERC1155 tokenId:

function _createOfferItems(
    uint256 erc721Offers,
    uint256 erc1155Offers,
    uint256 erc20Offers
) private {
    // generate the ERC721 offer items
    for (uint256 i = 0; i < erc721Offers; ++i) {
        // create the offer item
        orderToCreate.offerItems.push(
            OfferItemLib
                .empty()
                .withItemType(ItemType.ERC721)
                .withToken(address(erc721s[i]))
                .withIdentifierOrCriteria(usedOfferERC721s[i])
                .withStartAmount(1)
                .withEndAmount(1)
        );

        // mint an erc721 to the offerer
        erc721s[i].mint(orderToCreate.offerer.addr);

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

    // generate the ERC1155 offer items
    for (uint256 i = 0; i < erc1155Offers; ++i) {
        // create the offer item
        orderToCreate.offerItems.push(
            OfferItemLib
                .empty()
                .withItemType(ItemType.ERC1155)
                // @audit always use token 0
                .withToken(address(erc1155s[0]))
                // @audit always use tokenId 0
                .withIdentifierOrCriteria(0)
                .withStartAmount(100)
                .withEndAmount(100)
        );

        // mint an erc1155 to the offerer
        // @audit minting extra of tokenId 0
        erc1155s[0].mint(orderToCreate.offerer.addr, 300);

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

    // generate the ERC20 offer items
    for (uint256 i = 0; i < erc20Offers; ++i) {
        // create the offer item
        orderToCreate.offerItems.push(
            OfferItemLib
                .empty()
                .withItemType(ItemType.ERC20)
                .withToken(address(erc20s[i]))
                .withStartAmount(100)
                .withEndAmount(100)
        );
    }
}

Then we add the following in StopRent.t.sol:

error StopPolicy_ReclaimFailed();

function test_StopRent_DoS() public {
    // Create order B (order to be DoS'd)
    // ==================================

    // create a BASE order
    createOrder({
        offerer: alice,
        orderType: OrderType.BASE,
        erc721Offers: 0,
        erc1155Offers: 2,
        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 rentalOrderB = finalizeBaseOrderFulfillment();

    // Create order A (attacker order)
    // ===============================

    // 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,
        orderHash,
        metadata
    ) = finalizeOrder();

    // create an order fulfillment
    createOrderFulfillment({
        _fulfiller: alice,
        order: order,
        orderHash: orderHash,
        metadata: metadata
    });

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

    // Check that respective safes get expected amounts
    assertEq(erc1155s[0].balanceOf(address(bob.safe), 0), uint256(200));
    assertEq(erc1155s[0].balanceOf(address(alice.safe), 0), uint256(100));

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

    // Adjust order A to use bob's safe as rental wallet
    rentalOrderA.rentalWallet = address(bob.safe);

    // Stop order A
    stop.stopRent(rentalOrderA);

    // Try to stop order B, DoS!
    vm.expectRevert(StopPolicy_ReclaimFailed.selector);
    stop.stopRent(rentalOrderB);
}

As we can see above, by creating order A with a subset of rental items from order B and stopping it with the rentalWallet from order B, order A succeeds and order B is permanently DoS'd.

Tools Used

• Manual review
• Forge

Recommended Mitigation Steps

Include the rentalWallet in _deriveRentalOrderHash.

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

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

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

  return
      keccak256(
          abi.encode(
              _RENTAL_ORDER_TYPEHASH,
              order.seaportOrderHash,
              keccak256(abi.encodePacked(itemHashes)),
              keccak256(abi.encodePacked(hookHashes)),
              order.orderType,
              order.lender,
              order.renter,
              // @audit include rentalWallet in hash
              order.rentalWallet,
              order.startTimestamp,
              order.endTimestamp
          )
      );
}

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

_deriveOrderMetadataHash fails to include the orderType. This causes order metadata with different orderType's to have the same hash, which is signed by the protocol signer, thus allowing users to use what should be an unsigned payload to create a new rental.

Proof of Concept

OrderMetadata includes an orderType field:

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

However, _deriveOrderMetadataHash fails to include the orderType in the hash:

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.
    return
        keccak256(
            abi.encode(
                _ORDER_METADATA_TYPEHASH,
                metadata.rentDuration,
                keccak256(abi.encodePacked(hookHashes))
            )
        );
}

The order metadata hash is also used in _deriveRentPayloadHash, so both the order metadata hash and rent payload hashes both fail to include the orderType:

function _deriveRentPayloadHash(
    RentPayload memory payload
) internal view returns (bytes32) {
    // Derive and return the rent payload hash as specified by EIP-712.
    return
        keccak256(
            abi.encode(
                _RENT_PAYLOAD_TYPEHASH,
                _deriveOrderFulfillmentHash(payload.fulfillment),
                _deriveOrderMetadataHash(payload.metadata),
                payload.expiration,
                payload.intendedFulfiller
            )
        );
}

The rent payload must be signed by the CREATE_SIGNER prior to order execution since it's validated in validateOrder:

// validateOrder()
(RentPayload memory payload, bytes memory signature) = abi.decode(
    zoneParams.extraData,
    (RentPayload, bytes)
);

...

// Recover the signer from the payload.
address signer = _recoverSignerFromPayload(
    _deriveRentPayloadHash(payload),
    signature
);

// Check: The data matches the signature and that the protocol signer is the one that signed.
if (!kernel.hasRole(signer, toRole("CREATE_SIGNER"))) {
    revert Errors.CreatePolicy_UnauthorizedCreatePolicySigner();
}

Since the orderType is not included in the rent payload hash, a signed payload which is expected to have a specific orderType can actually have any orderType. This allows users to create orders of an orderType which is unvalidated by the CREATE_SIGNER.

Tools Used

• Manual review

Recommended Mitigation Steps

Include the orderType in _deriveOrderMetadataHash, e.g.:

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.
    return
        keccak256(
            abi.encode(
                _ORDER_METADATA_TYPEHASH,
                // @audit include orderType
                metadata.orderType,
                metadata.rentDuration,
                keccak256(abi.encodePacked(hookHashes))
            )
        );
}

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

Failure to check the amount of ERC1155 tokens being rented and the amount to transfer results in additional ERC1155 tokens of the same tokenId as those being rented being unable to be transferred out of the rental safe.

Proof of Concept

When calling ERC1155 safeTransferFrom, if the address and tokenId to be transferred matches a tokenId being rented, execution is always reverted.

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

    // Check if the selector is allowed.
    _revertSelectorOnActiveRental(selector, from, to, tokenId);
}

The problem with this logic is that an account can hold many ERC1155 tokens of the same address and tokenId, and the logic doesn't consider how many tokens of the given tokenId that the rental safe is actually renting. As a result, it's possible that the rental safe has additional ERC1155 tokens of the same tokenId transferred to it, but they can't be withdrawn until the rental is stopped.

Tools Used

• Manual review

Recommended Mitigation Steps

Rather than simply preventing all transfers of ERC1155 tokens matching the given tokenId, the total amount of tokens rented matching that tokenId should be tracked such that we can withdraw additional tokens not being rented. Note that this tracked amount of tokens must be the sum of all rentals or an exploit may occur.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

In Create._addHooks and Stop._removeHooks, we loop through all the hooks associated with the rental and revert if any of the hooks are not hookOnStart or if any of the hooks are not hookOnStop, respectively. As a result, contrary to the intended effect, hooks can only be used throughout the lifecycle of a rental if they're marked as both hookOnStart and hookOnStop. Otherwise, in the worst case hooks will DoS stopRent logic, causing all rental items and associated tokens in PaymentEscrow to be permanently locked.

Proof of Concept

When an order is created and processed through validateOrder, on to _rentFromZone, if there are any hooks included, we include those hooks in the stored orderHash, then we run _addHooks with them.

// Generate the rental order.
RentalOrder memory order = RentalOrder({
  seaportOrderHash: seaportPayload.orderHash,
  items: items,
  hooks: payload.metadata.hooks,
  orderType: payload.metadata.orderType,
  lender: seaportPayload.offerer,
  renter: payload.intendedFulfiller,
  rentalWallet: payload.fulfillment.recipient,
  startTimestamp: block.timestamp,
  endTimestamp: block.timestamp + payload.metadata.rentDuration
});

// Compute the order hash.
bytes32 orderHash = _deriveRentalOrderHash(order);

// Interaction: Update storage only if the order is a Base Order or Pay order.
STORE.addRentals(orderHash, _convertToStatic(rentalAssetUpdates));

if (payload.metadata.hooks.length > 0) {
  _addHooks(
      payload.metadata.hooks,
      seaportPayload.offer,
      payload.fulfillment.recipient
  );
}

In _addHooks, if any of the hooks are not hookOnStart, execution reverts:

// @audit if any non-onStart hooks exist, this reverts
if (!STORE.hookOnStart(target)) {
  revert Errors.Shared_DisabledHook(target);
}

The problem is that the same hooks which are passed to _addHooks are also included in the stored order hash, meaning that they're also used later on in the lifecycle of the rental. We can see how this manifests when the order hooks are used in Stop._removeHooks.

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

In _removeHooks, if any of the hooks are not hookOnStop, execution reverts:

if (!STORE.hookOnStop(target)) {
  revert Errors.Shared_DisabledHook(target);
}

As proved above, the following outcomes will occur:

• If a hook is hookOnStop but not hookOnStart, the creation process will revert
• If a hook is hookOnStart but not hookOnStop, the stopRent process will revert

Neither of these outcomes are intended, and the latter results in rental items being permanently stuck in the rental safe and its associated ERC20 payment being permanently stuck in PaymentEscrow.

Tools Used

• Manual review

Recommended Mitigation Steps

Rather than reverting if the hook should not currently be run, logic should continue to the next iteration of the loop, e.g. in _addHooks:

if (!STORE.hookOnStart(target)) {
  continue
}

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Rent payload signatures lack a mechanism to prevent replay attacks. As a result, it's possible to reuse signatures to execute unauthorized rentals.

Proof of Concept

In Create.validateOrder, the fulfiller must pass extraData containing the rentPayload and an associated signature.

// Decode the signed rental zone payload from the extra data.
(RentPayload memory payload, bytes memory signature) = abi.decode(
    zoneParams.extraData,
    (RentPayload, bytes)
);

We then recover the signer and revert if it was not signed by the CREATE_SIGNER role:

// Recover the signer from the payload.
address signer = _recoverSignerFromPayload(
    _deriveRentPayloadHash(payload),
    signature
);

// Check: The data matches the signature and that the protocol signer is the one that signed.
if (!kernel.hasRole(signer, toRole("CREATE_SIGNER"))) {
    revert Errors.CreatePolicy_UnauthorizedCreatePolicySigner();
}

This signing logic fails however to prevent the signature from being reused. As long as replays are executed before the signature expiration, it's possible to execute unsigned rentals.

Tools Used

• Manual review

Recommended Mitigation Steps

Include a nonce and chainId as part of the rent payload then validate that the nonce has not yet been used and that the chainId is correct.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

Use of ERC721/ERC1155 safeTransferFrom to reclaim rental items from stopRent allows the lender to DoS execution by using a contract which doesn't support the onERC721Received callback.

Proof of Concept

When a rental is stopped, the rental safe delegatecall's reclaimRentalOrder to reclaim the rental items to the lender address. Item reclaim uses ERC721/ERC1155 safeTransferFrom.

// reclaimRentalOrder
for (uint256 i = 0; i < itemCount; ++i) {
  Item memory item = rentalOrder.items[i];

  // Check if the item is an ERC721.
  if (item.itemType == ItemType.ERC721)
      _transferERC721(item, rentalOrder.lender);

  // check if the item is an ERC1155.
  if (item.itemType == ItemType.ERC1155)
      _transferERC1155(item, rentalOrder.lender);
}

function _transferERC721(Item memory item, address recipient) private {
    IERC721(item.token).safeTransferFrom(address(this), recipient, item.identifier);
}

function _transferERC1155(Item memory item, address recipient) private {
    IERC1155(item.token).safeTransferFrom(
        address(this),
        recipient,
        item.identifier,
        item.amount,
        ""
    );
}

safeTransferFrom checks if the recipient address is a contract and if so, checks that the contract contains the onERC721Received callback, and reverts if not:

function _checkOnERC721Received(
    address from,
    address to,
    uint256 tokenId,
    bytes memory data
) private returns (bool) {
    if (to.code.length > 0) {
        try IERC721Receiver(to).onERC721Received(_msgSender(), from, tokenId, data) returns (bytes4 retval) {
            return retval == IERC721Receiver.onERC721Received.selector;
        } catch (bytes memory reason) {
            if (reason.length == 0) {
                revert ERC721InvalidReceiver(to);
            } else {
                /// @solidity memory-safe-assembly
                assembly {
                    revert(add(32, reason), mload(reason))
                }
            }
        }
    } else {
        return true;
    }
}

This allows the lender to use a contract which doesn't contain the onERC721Received callback to completely DoS stopping their rentals. This can be used to grief renters of PAY orders from receiving payment.

Listing as medium severity since the lender must also permanently lock their rental tokens.

Tools Used

• Manual review

Recommended Mitigation Steps

Rather than using safeTransferFrom, transferFrom should be used such that there is no callback check and thus the lender can't DoS the protocol.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Rental safes cannot call the receive ether function of other contracts because a function selector must be provided.

Proof of Concept

Guard.checkTransaction prevents any safe execution where data.length < 4:

if (data.length < 4) {
    revert Errors.GuardPolicy_FunctionSelectorRequired();
}

This means that calldata must be provided. However, there is a function which only executes if no calldata is provided. "The receive function is executed on a call to the contract with empty calldata."

There is no reason to prevent execution of receive functions, and the inability to do so causes a DoS of rental safe users under reasonable usage.

Tools Used

• Manual review

Recommended Mitigation Steps

There is no need to enforce that calldata is provided as it's impossible to call any guarded functions without calldata anyway. Simply remove the following statement from Guard.checkTransaction:

if (data.length < 4) {
    revert Errors.GuardPolicy_FunctionSelectorRequired();
}

Assessed type

DoS