reNFT - 0xDING99YA's results

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L265-L306 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L215-L283

Vulnerability details

Impact

In reNFT, when a rental is created, NFT will be transferred to safe wallet and ERC20 tokens will be transferred to payment escrow, when rental expires and stop the rent, ERC20 tokens will then be transferred to lender or renter accordingly and NFT will be transferred back to lender. However, those ERC20 tokens may include some malicious tokens due to the "tipping" feature of Seaport, an attacker can intentionally creates a token which will always revert when transferred from PaymentEscrow, as a result, the rental cannot be stopped and lender will lose their NFT.

Proof of Concept

To understand the issue, let's first go over what will happen in stopRent() in Stop.sol. Within stopRent(), it will call PaymentEscrow to settle the payment:

ESCRW.settlePayment(order);

settlePayment(order) basically just calls an internal function:

function settlePayment(RentalOrder calldata order) external onlyByProxy permissioned {
        // Settle all payments for the order.
        _settlePayment(
            order.items,
            order.orderType,
            order.lender,
            order.renter,
            order.startTimestamp,
            order.endTimestamp
        );
    }

_settlePayment() will iterate over all order.items and, do the transfer if finding any ERC20 token.

for (uint256 i = 0; i < items.length; ++i) {
            // Get the item.
            Item memory item = items[i];

            // Check that the item is a payment.
            if (item.isERC20()) {
                // ...Do Transfer...
            }

So we can see, if there is any malicious ERC20 token in the order.items, it can be made to revert the transaction, making the rental cannot be stopped and NFT get stuck.

order.items is created in Create.sol, let's see how it is constructed. RentalOrder is constructed in _rentFromZone() within Create.sol:

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

and items is constructed in this way:

Item[] memory items = _convertToItems(
            seaportPayload.offer,
            seaportPayload.consideration,
            payload.metadata.orderType
        );

seaportPayload is a struct constructed when validateOrder() get called by Seaport contract and constructed with zoneParams.

SeaportPayload memory seaportPayload = SeaportPayload({
            orderHash: zoneParams.orderHash,
            zoneHash: zoneParams.zoneHash,
            offer: zoneParams.offer,
            consideration: zoneParams.consideration,
            totalExecutions: zoneParams.totalExecutions,
            fulfiller: zoneParams.fulfiller,
            offerer: zoneParams.offerer
        });

Now it turns out as long as we can insert malicious ERC20 token into zoneParams.consideration, we can achieve the goal to revert the rental stopping.

Actually, Seaport implements a tipping feature, so that when an order is fulfilled, the fulfiller can provide additional consideration item as a tip, so attacker can exploit this to insert the malicious token to consideration. The official docs talking about this is here: 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). https://docs.opensea.io/reference/seaport-overview

Now let's see how this tipping feature is implemented in Seaport. One can call fulfillOrder() within Seaport.sol which inherits from Consideration.sol: https://github.com/ProjectOpenSea/seaport-core/blob/2f546b9a0d61a70e1632445cbcb108149a9369ae/src/lib/Consideration.sol#L166-L180

function fulfillOrder(
        /**
         * @custom:name order
         */
        Order calldata,
        bytes32 fulfillerConduitKey
    ) external payable override returns (bool fulfilled) {
        // Convert order to "advanced" order, then validate and fulfill it.
        fulfilled = _validateAndFulfillAdvancedOrder(
            _toAdvancedOrderReturnType(_decodeOrderAsAdvancedOrder)(CalldataStart.pptr()),
            new CriteriaResolver[](0), // No criteria resolvers supplied.
            fulfillerConduitKey,
            msg.sender
        );
    }

Then _validateAndFulfillAdvancedOrder() will validate the order that the fulfiller provides is correct and valid. https://github.com/re-nft/seaport-core/blob/3bccb8e1da43cbd9925e97cf59cb17c25d1eaf95/src/lib/OrderFulfiller.sol#L78-L136

The most important step in _validateAndFulfillAdvancedOrder() is the call to _validateOrderAndUpdateStatus().

// Validate order, update status, and determine fraction to fill.
        (bytes32 orderHash, uint256 fillNumerator, uint256 fillDenominator) =
            _validateOrderAndUpdateStatus(advancedOrder, true);

Within _validateOrderAndUpdateStatus(), it will call _assertConsiderationLengthAndGetOrderHash() to make some verification and generate the orderHash. https://github.com/re-nft/seaport-core/blob/3bccb8e1da43cbd9925e97cf59cb17c25d1eaf95/src/lib/OrderValidator.sol#L198

// Retrieve current counter & use it w/ parameters to derive order hash.
        orderHash = _assertConsiderationLengthAndGetOrderHash(orderParameters);

And finally, within _assertConsiderationLengthAndGetOrderHash() it will make sure the considerationItem.length provided by the fulfiller is not less then the considerationItem length by the offerer, this is where indicating that the fulfiller can add malicious ERC20 token freely.

https://github.com/re-nft/seaport-core/blob/3bccb8e1da43cbd9925e97cf59cb17c25d1eaf95/src/lib/Assertions.sol#L69-L81 https://github.com/re-nft/seaport-core/blob/3bccb8e1da43cbd9925e97cf59cb17c25d1eaf95/src/lib/Assertions.sol#L93-L101

function _assertConsiderationLengthAndGetOrderHash(OrderParameters memory orderParameters)
        internal
        view
        returns (bytes32)
    {
        // Ensure supplied consideration array length is not less than original.
        _assertConsiderationLengthIsNotLessThanOriginalConsiderationLength(
            orderParameters.consideration.length, orderParameters.totalOriginalConsiderationItems
        );

        // Derive and return order hash using current counter for the offerer.
        return _deriveOrderHash(orderParameters, _getCounter(orderParameters.offerer));
    }

function _assertConsiderationLengthIsNotLessThanOriginalConsiderationLength(
        uint256 suppliedConsiderationItemTotal,
        uint256 originalConsiderationItemTotal
    ) internal pure {
        // Ensure supplied consideration array length is not less than original.
        if (suppliedConsiderationItemTotal < originalConsiderationItemTotal) {
            _revertMissingOriginalConsiderationItems();
        }
    }

So based on the analysis from above, the tipping feature allows attacker to give malicious token to PaymentEscrow when fulfilling an order and make the rental cannot be stopped and NFT will get stuck.

Tools Used

Manual Review

Recommended Mitigation Steps

When forking the Seaport, only allowing the offered specified consideration items to be given by the fulfiller and disable this tipping feature

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Currently Guard.sol allows the owner of a safe wallet to call setFallbackHandler(), which updating the fallback handler of the safe wallet. Attacker can reset the fallback handler to the rental NFT contract and directly call "transfer" or "approval" to the safe wallet, since there is no such function in the safe wallet, safe wallet will directly call the fallback handler (NFT contract), as a result, rental NFT can be stolen.

Proof of Concept

Under current implementation, rental NFT will be transferred to protocol created Safe Wallet. When the wallet owner wants to execute any call data by execTransaction(), Guard.sol is used to make sure the call cannot steal the NFT. The restricted calls are those:

e721_safe_transfer_from_1_selector e721_safe_transfer_from_2_selector e721_transfer_from_selector e721_approve_selector e1155_safe_transfer_from_selector gnosis_safe_enable_module_selector gnosis_safe_disable_module_selector shared_set_approval_for_all_selector e1155_safe_batch_transfer_from_selector gnosis_safe_set_guard_selector

As we can see here, the call to reset the fallback handler setFallbackHandler is not banned. So the owner can simply reset the fallback handler by execTransaction

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

The owner can set the handler to a rental NFT contract, then directly call "transfer" or "approval" function to the Safe Wallet. Safe Wallet doesn't have these two functions, so it will forward the calldata to fallback():

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

As we can see, the fallback() function will use "call" opcode to call handler, since the handler is now reset to the NFT contract, the Safe Wallet will call "transfer" or "approve", and directly transfer the rental NFT.

Tools Used

Manual Review

Recommended Mitigation Steps

Disable the call to setFallbackHandler() in Guard.sol.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Some functions in "Signer.sol" which are used to derive hash don't strictly follow EIP-712, those generated hash are used when validating a Seaport order, as a result, a Seaport order with correct EIP-712 hash may not be executed and cause integration issues.

Proof of Concept

When deriving the Domain Separator, according to EIP-712 (https://eips.ethereum.org/EIPS/eip-712), name, version, chainId, verifyingContract and salt are required. However, the _deriveDomainSeparator() lacks a salt:

function _deriveDomainSeparator(
        bytes32 _eip712DomainTypeHash,
        bytes32 _nameHash,
        bytes32 _versionHash
    ) internal view virtual returns (bytes32) {
        return
            keccak256(
                abi.encode(
                    _eip712DomainTypeHash,
                    _nameHash,
                    _versionHash,
                    block.chainid,
                    address(this)
                )
            );
    }

When deriving rentalOrderHash, it doesn't consider the RentalOrder.rentalWallet:

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

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

When deriving orderMetadataHash, it doesn't consider OrderMetadata.OrderType and OrderMetadata.emittedExtraData:

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

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

Those generated hash can be used to validate if an order should be executed. For example, _rentFromZone() will invoke _isValidOrderMetadata():

function _isValidOrderMetadata(
        OrderMetadata memory metadata,
        bytes32 zoneHash
    ) internal view {
        // Check that the rent duration specified is not zero.
        if (metadata.rentDuration == 0) {
            revert Errors.CreatePolicy_RentDurationZero();
        }

        // Check that the zone hash is equal to the derived hash of the metadata.
        if (_deriveOrderMetadataHash(metadata) != zoneHash) {
            revert Errors.CreatePolicy_InvalidOrderMetadataHash();
        }
    }

It uses the derived orderMetadataHash and compares it with zoneHash from Seaport order, note zoneHash will follow the EIP-712 while orderMetadataHash doesn't, as a result, a correct order can not be executed.

Tools Used

Manual Review

Recommended Mitigation Steps

Ensure the derived data hash strictly follows EIP-712

Assessed type

Other

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/Storage.sol#L294-L303 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/Storage.sol#L313-L326

Vulnerability details

Impact

When trying to stop a rental order, if the order contains any hooks, Stop.sol will iterate over it to make sure STORE.hookOnStop() returns true. The hooks of a order are determined by an offerer when creating a Seaport order, it's possible when creating Seaport order hookOnStop() returns true but during the rental duration updated to false, thus result in the revert when stopping the order.

Proof of Concept

First we can see in "Storage.sol" hook status can be updated freely by admin role:

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

It only validates the input params, then update the hookStatus, meaning the status of a hook can be updated anytime even when the hook is currently used for a rental order.

When stop an order, the hooks will be iterated to ensure STORE.hookOnStop() returns true:

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

However, since the hook status can be updated anytime, it's possible the hook will not be executed when stop, thus invalid and revert the whole transaction.

Note: it's possible for admin role to make the hookOnStop returns true again so that an order to be stopped. However, given that there can be many orders using the same hook, and each has a different rental expiration time, and the reason to shut down a hook is because it's vulnerable, the scenario here will become very complicated and vulnerable, so I think this is a medium issue.

Tools Used

Manual Review

Recommended Mitigation Steps

When updating hook status, check if it's being used for any rental order. If it must be shut down due to security reason, add an emergency function to terminate those rental orders immediately.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

When creating an order on Seaport, the lender only needs to give approval to Seaport to transfer his NFT, no necessary to implement "onERC721Received()" if the lender itself is a smart contract. However, when stopping a rental and get the NFT transferred back, it uses "safeTransferFrom()", which means if the lender is a smart contract, it must implements the "onERC721Received()". This difference between creating rental order and stopping rental order can result in the revert when stopping the rental, and NFT will get stuck.

Proof of Concept

According to the official docs from Seaport, when creating an order the offerer (the lender in our case) only needs to give approval to the Seaport so that when order is fulfilled the NFT can be transferred. See "Balance and Approval Requirements" in this page: https://docs.opensea.io/reference/seaport-overview This means that even if the lender is a contract, it doesn't need to implement "onERC721Received()" function.

However, when stopping the rental, in Reclaimer.sol, it uses "safeTransferFrom()", which means the lender must implement "onERC721Received()" function.

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

A lender contract may successfully create a rental order and get the order fulfilled, but when rental expires and the lender wants to get the NFT back, the transaction will revert.

Tools Used

Manual Review

Recommended Mitigation Steps

Just use "transferFrom()" when stopping the rental order instead of using "safeTransferFrom()".

Assessed type

Other