reNFT - DeFiHackLabs's results

Lines of code

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

Vulnerability details

Impact

If the order is Base or Pay, ESCRW will increase the deposit.

// PAYEE orders are considered mirror-images of a PAY order. So, PAYEE orders
        // do not need to be processed in the same way that other order types do.
        if (
            payload.metadata.orderType.isBaseOrder() ||
            payload.metadata.orderType.isPayOrder()
        ) {
            /* .... */

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

            // Interaction: Increase the deposit value on the payment escrow so
            // it knows how many tokens were sent to it.
            for (uint256 i = 0; i < items.length; ++i) {
                if (items[i].isERC20()) {
                    ESCRW.increaseDeposit(items[i].token, items[i].amount);
                }
            }

The way Seaport matches orders is as follows: Calling one of two "match" functions, matchOrders and matchAdvancedOrders, where a group of explicit orders are supplied alongside a group of fulfillments specifying which offer items to apply to which consideration items。 Note that orders fulfilled in this manner do not have an explicit fulfiller; instead, Seaport will simply ensure coincidence of wants across each order. Consider the following scenario:

The PAY order rented from the BASE order and corresponds with two PAYEE orders. BASE order increases 10 in escrow. Pay order increases 15 in escrow. In this case, PAY only gives 15 to escrow. The escrow's accounting will be incorrect. This would cause a loss for the escrow, and in the worst-case scenario, it could lead to the escrow being completely depleted.

Proof of Concept

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

Tools Used

Recommended Mitigation Steps

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

PAY order have at least one ERC721/ERC1155, at least one ERC20. These orders are paired with PAYEE orders. PAYEE order have at least one ERC721/ERC1155, at least one ERC20. In Create.sol, when utilizing validateOrder(), the execution of _rentFromZone initiates a rental order.

function validateOrder(
        ZoneParameters calldata zoneParams
    ) external override onlyRole("SEAPORT") returns (bytes4 validOrderMagicValue) {
        /* ... */
        // Create a payload of seaport data.
        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
        });

        // Check: The signature from the protocol signer has not expired.
        _validateProtocolSignatureExpiration(payload.expiration);

        // Check: The fulfiller is the intended fulfiller.
        _validateFulfiller(payload.intendedFulfiller, seaportPayload.fulfiller);

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

        // Initiate the rental using the rental manager.
        _rentFromZone(payload, seaportPayload);
/* ... */
    }

Create a PAYEE order, where the ERC20 is identical to the PAY order. And set the recipient to be the same as the offerer. Can pass _executionInvariantChecks.

function _executionInvariantChecks(
        ReceivedItem[] memory executions,
        address expectedRentalSafe
    ) internal view {
        for (uint256 i = 0; i < executions.length; ++i) {
            ReceivedItem memory execution = executions[i];

            // ERC20 invariant where the recipient must be the payment escrow.
            if (execution.isERC20()) {
                _checkExpectedRecipient(execution, address(ESCRW));
            }
            // ERC721 and ERC1155 invariants where the recipient must
            // be the expected rental safe.
            else if (execution.isRental()) {
                _checkExpectedRecipient(execution, expectedRentalSafe);
            }
            // Revert if unsupported item type.
            else {
                revert Errors.CreatePolicy_SeaportItemTypeNotSupported(
                    execution.itemType
                );
            }
        }
    }

In OrderCombiner.sol, _isFilterableExecution() returns true when the offerer and recipient have the same address.

function _isFilterableExecution(Execution memory execution) internal pure returns (bool filterable) {
        // Utilize assembly to efficiently determine if execution is filterable.
        assembly {
            // Retrieve the received item referenced by the execution.
            let item := mload(execution)

            // Determine whether the execution is filterable.
            filterable :=
                and(
                    // Determine if offerer and recipient are the same address.
                    eq(
                        // Retrieve the recipient's address from the received item.
                        mload(add(item, ReceivedItem_recipient_offset)),
                        // Retrieve the offerer's address from the execution.
                        mload(add(execution, Execution_offerer_offset))
                    ),
                    // Determine if received item's item type is non-zero, thereby
                    // indicating that the execution does not involve native tokens.
                    iszero(iszero(mload(item)))
                )
        }
    }

The renter of the pay order can retrieve the escrowed rent, and the lender of the payee can borrow the NFT without paying any ERC20 tokens.

In the worst-case scenario, it could lead to the depletion of the escrow.

Proof of Concept

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

Tools Used

Recommended Mitigation Steps

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

PAY order have at least one ERC721/ERC1155, at least one ERC20. These orders are paired with PAYEE orders. PAYEE order have at least one ERC721/ERC1155, at least one ERC20. In Create.sol, when utilizing validateOrder(), the execution of _rentFromZone initiates a rental order.

function validateOrder(
        ZoneParameters calldata zoneParams
    ) external override onlyRole("SEAPORT") returns (bytes4 validOrderMagicValue) {
        /* ... */
        // Create a payload of seaport data.
        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
        });

        // Check: The signature from the protocol signer has not expired.
        _validateProtocolSignatureExpiration(payload.expiration);

        // Check: The fulfiller is the intended fulfiller.
        _validateFulfiller(payload.intendedFulfiller, seaportPayload.fulfiller);

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

        // Initiate the rental using the rental manager.
        _rentFromZone(payload, seaportPayload);
/* ... */
    }

Create a PAYEE order, where the ERC721 is identical to the PAY order. And set the recipient to be the same as the offerer. Can pass _executionInvariantChecks.

function _executionInvariantChecks(
        ReceivedItem[] memory executions,
        address expectedRentalSafe
    ) internal view {
        for (uint256 i = 0; i < executions.length; ++i) {
            ReceivedItem memory execution = executions[i];

            // ERC20 invariant where the recipient must be the payment escrow.
            if (execution.isERC20()) {
                _checkExpectedRecipient(execution, address(ESCRW));
            }
            // ERC721 and ERC1155 invariants where the recipient must
            // be the expected rental safe.
            else if (execution.isRental()) {
                _checkExpectedRecipient(execution, expectedRentalSafe);
            }
            // Revert if unsupported item type.
            else {
                revert Errors.CreatePolicy_SeaportItemTypeNotSupported(
                    execution.itemType
                );
            }
        }
    }

In OrderCombiner.sol, _isFilterableExecution() returns true when the offerer and recipient have the same address.

function _isFilterableExecution(Execution memory execution) internal pure returns (bool filterable) {
        // Utilize assembly to efficiently determine if execution is filterable.
        assembly {
            // Retrieve the received item referenced by the execution.
            let item := mload(execution)

            // Determine whether the execution is filterable.
            filterable :=
                and(
                    // Determine if offerer and recipient are the same address.
                    eq(
                        // Retrieve the recipient's address from the received item.
                        mload(add(item, ReceivedItem_recipient_offset)),
                        // Retrieve the offerer's address from the execution.
                        mload(add(execution, Execution_offerer_offset))
                    ),
                    // Determine if received item's item type is non-zero, thereby
                    // indicating that the execution does not involve native tokens.
                    iszero(iszero(mload(item)))
                )
        }
    }

The lender of the payee order didn't successfully borrow the NFT but still made the ERC20 payment for the pay order.

Proof of Concept

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

Tools Used

Recommended Mitigation Steps

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

In case of pay and payee order, lender pays the renter. if the the transfer fails, lenders cannot call stopOrders to retrieve their NFT. There are two scenarios:

• When the payment token implements a blacklist, which is common for tokens on the Ethereum network (e.g. USDC/USDT implementing blacklist/blocklist; See: https://github.com/d-xo/weird-erc20). The following steps describe this issue:

1. A blacklisted renter fulfills a PAY order. ERC20 token is transferred from lender to escrow successfully.
2. When the rent ended (or lender wants to stop the order before the end), In paymentEscrow, settlePayment() is used to settle all payments contained in the given item.

  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);
        /* ...*/
        // Emit rental order stopped.
        _emitRentalOrderStopped(order.seaportOrderHash, msg.sender);
    }

3. _safeTransfer fails, and the call will revert. the lender cannot stopRent and reclaim their item.

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

• In case of ERC20-compatible ERC777 token, similarly the renter can choose to reject the payment and lenders cannot call stopOrders to retrieve their NFT. See https://docs.openzeppelin.com/contracts/2.x/api/token/erc777 for details on ERC777 tokens. The following steps describe this issue:

1. The lender uses ERC777 tokens, and the contract can accept/reject the transfer of tokens to it.
2. The lender can reject any transfer of tokens sent to it.
3. Due to the failed token transfer by the lender (ERC777 tokens invoke the tokensReceived function in the smart contract to complete the token transfer and revert), the lender is unable to receive the expected payment, also loss their NFT.

Proof of Concept

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

Tools Used

Recommended Mitigation Steps

Assessed type

Token-Transfer