reNFT - cccz's results

Lines of code

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

Vulnerability details

Impact

The document describes PAY order as follows

PAY: This order type describes an order in which the lender wishes to pay the renter for renting out their asset. This order must contain at least one ERC721 or ERC1155 offer item, and at least one ERC20 offer item. It must contain 0 consideration items. This may sound counter-intuitive but the rationale is that some lenders may get benefit (tokens, rewards, etc) from allowing others to interact with contracts (on-chain games, etc) with their assets to extract some type of value from the lended asset.

When a PAY order expires, both the Lender and the Renter can stop the order, with the Lender receiving the NFT and the Renter receiving the ERC20 as payment.

        else if (orderType.isPayOrder()) {
            // If the stopper is the lender, then it doesnt matter whether the rental
            // has expired. But if the stopper is not the lender, then the rental must have expired.
            if (!isLender && (!hasExpired)) {
                revert Errors.StopPolicy_CannotStopOrder(block.timestamp, msg.sender);
            }
        }

The problem here is that when Lender receives the NFT, Lender can reject it in fallback, which will cause the transaction to fail, thus preventing Renter from receiving ERC20 as payment.

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

    /**
     * @dev Helper function to transfer an ERC1155 token.
     *
     * @param item      Item which will be transferred.
     * @param recipient Address which will receive the token.
     */
    function _transferERC1155(Item memory item, address recipient) private {
        IERC1155(item.token).safeTransferFrom(
            address(this),
            recipient,
            item.identifier,
            item.amount,
            ""
        );
    }

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Consider saving the NFT data when the NFT transfer fails and allow Lender to claim it later

Assessed type

ERC721

Lines of code

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

Vulnerability details

Impact

OpenSea now allows trading with USDC. When Lender creates a PAY order, USDC can be used as payment to Renter. When stopping the order, Lender receives NFT and Renter receives USDC.

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

The problem here is that when a USDC blacklisted Renter fulfills a PAY order, the transaction is successful because the fulfillment sends USDC to ESCROW instead of the Renter. Later, when the Lender stops the order, the transaction fails due to failure to send USDC to the Renter, preventing the Lender from receiving the NFT.

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

        // Because both reverting and returning false are allowed by the ERC20 standard
        // to indicate a failed transfer, we must handle both cases.
        //
        // If success is false, the ERC20 contract reverted.
        //
        // If success is true, we must check if return data was provided. If no return
        // data is provided, then no revert occurred. But, if return data is provided,
        // then it must be decoded into a bool which will indicate the success of the
        // transfer.
        if (!success || (data.length != 0 && !abi.decode(data, (bool)))) {
            revert Errors.PaymentEscrowModule_PaymentTransferFailed(token, to, value);
        }
    }

Consider the following scenario, where there are a large number of PAY orders in the market that are paid in USDC. Alice, a blacklisted USDC user, decides to fulfill these orders in bulk and does not perform any action, and when Lender tries to stop the order, it is unable to do so due to the USDC send failure, preventing Lender from getting the NFT back.

Proof of Concept

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#L190-L202 https://github.com/re-nft/smart-contracts//blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L100-L118

Tools Used

None

Recommended Mitigation Steps

Instead of sending ERC20 directly to the user, consider saving the data and waiting for the user to claim it later.

Assessed type

ERC20