reNFT - Krace's results

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/libraries/RentalUtils.sol#L38-L44 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/Storage.sol#L118-L128 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L126-L136

Vulnerability details

Impact

The rentalId is derived from the combination of recipient, token, and identifier. While this approach is suitable for ERC721, where each token has a unique ID, it poses challenges for ERC1155. In ERC1155, tokens may share the same ID, leading to potential issues in the contract when different users create orders with identical rentalId if there are same recipient.

Proof of Concept

The rentalId is computed within the getItemPointer function using recipient, token, and identifier. In the context of ERC1155, where tokens can have shared IDs, situations may arise where different users create orders with the same ERC1155 and the same ID. This scenario results in identical rentalId values when the recipient is also the same.

    function getItemPointer(
        address recipient,
        address token,
        uint256 identifier
    ) internal pure returns (RentalId id) {
        id = RentalId.wrap(keccak256(abi.encodePacked(recipient, token, identifier)));
    }

The rentalId serves as a means to track the quantity of lent tokens in the safe. When initiating a rental, the corresponding amount is added to the rentedAssets, and when concluding a rental, the amount is deducted from the rentedAssets.

    function addRentals(
        bytes32 orderHash,
        RentalAssetUpdate[] memory rentalAssetUpdates
    ) external onlyByProxy permissioned {
        // Add the order to storage.
        orders[orderHash] = true;

        // Add the rented items to storage.
        for (uint256 i = 0; i < rentalAssetUpdates.length; ++i) {
            RentalAssetUpdate memory asset = rentalAssetUpdates[i];

            // Update the order hash for that item.
            rentedAssets[asset.rentalId] += asset.amount;
        }
    }

    function removeRentals(
        bytes32 orderHash,
        RentalAssetUpdate[] calldata rentalAssetUpdates
    ) external onlyByProxy permissioned {
        // The order must exist to be deleted.
        if (!orders[orderHash]) {
            revert Errors.StorageModule_OrderDoesNotExist(orderHash);
        } else {
            // Delete the order from storage.
            delete orders[orderHash];
        }

        // Process each rental asset.
        for (uint256 i = 0; i < rentalAssetUpdates.length; ++i) {
            RentalAssetUpdate memory asset = rentalAssetUpdates[i];

            // Reduce the amount of tokens for the particular rental ID.
            rentedAssets[asset.rentalId] -= asset.amount;
        }
    }

If the amount in the safe is not zero, attempting to transfer these tokens will result in failure. Consequently, if there are multiple orders with the same rentalId, none of the orders can be terminated until all of them have expired, so we can remove them all via removeRentalsBatch.

    function isRentedOut(
        address recipient,
        address token,
        uint256 identifier
    ) external view returns (bool) {
        // calculate the rental ID
        RentalId rentalId = RentalUtils.getItemPointer(recipient, token, identifier);

        // Determine if there is a positive amount
        return rentedAssets[rentalId] != 0;
    }

    function _revertSelectorOnActiveRental(
        bytes4 selector,
        address safe,
        address token,
        uint256 tokenId
    ) private view {
        // Check if the selector is allowed.
        if (STORE.isRentedOut(safe, token, tokenId)) {
            revert Errors.GuardPolicy_UnauthorizedSelector(selector);
        }
    }

Example:

1. Alice lends her ERC1155 token with ID 0, amount 100, with an expiration time set at 100.
2. Bob also lends his ERC1155 token with ID 0, amount 100, with an expiration time set at 200.
3. Carol rents the ERC1155 token with ID 0 from both Alice and Bob. Carol can only cancel the order for ERC1155 with ID 0 at time 200 using removeRentalsBatch. If Carol only removes Alice's order, there will still be tokens in rentedAssets[rentalId], leading to a failed transfer.

Tools Used

Manual Review

Recommended Mitigation Steps

It's recommended to use more info to generate the rentalId, like the lender's address.

Assessed type

Context

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L215-L283 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L100-L118

Vulnerability details

Impact

In cases where a rental order involves ERC20 tokens with a blacklist or hook, such as USDT or ERC777, the lender faces the challenge of being unable to reclaim their NFTs. This is due to the renter's ability to consistently revert the _safeTransfer during payment settlement.

Proof of Concept

When a lender wants to stop the order, he needs to invoke stopRent and settle the payment in function _settlePayment.

    function _settlePayment(
        Item[] calldata items,
        OrderType orderType,
        address lender,
        address renter,
        uint256 start,
        uint256 end
    ) internal {
        // Calculate the time values.
        uint256 elapsedTime = block.timestamp - start;
        uint256 totalTime = end - start;

        // Determine whether the rental order has ended.
        bool isRentalOver = elapsedTime >= totalTime;

        // Loop through each item in the order.
        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()) {
                // Set a placeholder payment amount which can be reduced in the
                // presence of a fee.
                uint256 paymentAmount = item.amount;

                // Take a fee on the payment amount if the fee is on.
                if (fee != 0) {
                    // Calculate the new fee.
                    uint256 paymentFee = _calculateFee(paymentAmount);

                    // Adjust the payment amount by the fee.
                    paymentAmount -= paymentFee;
                }

                // Effect: Decrease the token balance. Use the payment amount pre-fee
                // so that fees can be taken.
                _decreaseDeposit(item.token, item.amount);

                // If its a PAY order but the rental hasn't ended yet.
                if (orderType.isPayOrder() && !isRentalOver) {
                    // Interaction: a PAY order which hasnt ended yet. Payout is pro-rata.
                    _settlePaymentProRata(
                        item.token,
                        paymentAmount,
                        lender,
                        renter,
                        elapsedTime,
                        totalTime
                    );
                }
                // If its a PAY order and the rental is over, or, if its a BASE order.
                else if (
                    (orderType.isPayOrder() && isRentalOver) || orderType.isBaseOrder()
                ) {
                    // Interaction: a pay order or base order which has ended. Payout is in full.
                    _settlePaymentInFull(
                        item.token,
                        paymentAmount,
                        item.settleTo,
                        lender,
                        renter
                    );
                } else {
                    revert Errors.Shared_OrderTypeNotSupported(uint8(orderType));
                }
            }
        }
    }

The _settlePayment function facilitates the transfer of ERC20 tokens to both the lender and renter through either _settlePaymentProRata or _settlePaymentInFull. Both of these functions employ the _safeTransfer function for token transfer.

However, with certain types of ERC20, the recipient has the ability to consistently revert the transfer, resulting in a false return. Examples include USDT with its blacklist or ERC777 with its hook. In any such scenario where the renter fails the transfer, the lender remains unable to reclaim their NFT, allowing the renter to persist in using 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);
        }
    }

Tools Used

Manual Review

Recommended Mitigation Steps

It's recommended to lock the ERC20 in ESCRW and let the renter or lender claim ERC20 for themselves.

Assessed type

DoS