code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L784 https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L786

Vulnerability details

Impact

The bug report concerns an issue with NFTs that do not support interface lookup. In such cases, a transaction could revert. Additionally, external NFTs that can make the getRoyaltyLookupAddress(nft) query revert when blocking buy/sell trading. This vulnerability can lead to denial of service, which can significantly impact the users' ability to trade NFTs on the platform.

Proof of Concept

The issue lies in the _getRoyalty function in the code.

the code is used when settling the royalty fee payment

if (payRoyalties) {
    for (uint256 i = 0; i < tokenIds.length; i++) {
        // get the royalty fee for the NFT
        (uint256 royaltyFee, address recipient) = _getRoyalty(
            tokenIds[i],
            salePrice
        );

        // transfer the royalty fee to the recipient if it's greater than 0
        if (royaltyFee > 0 && recipient != address(0)) {
            if (baseToken != address(0)) {
                ERC20(baseToken).safeTransfer(recipient, royaltyFee);
            } else {
                recipient.safeTransferETH(royaltyFee);
            }
        }
    }
}

When the NFT does not support interface lookup, the transaction could revert. Furthermore, if external NFTs make the getRoyaltyLookupAddress(nft) query revert intentionally or unintentionally, it could lead to denial of service when trading NFTs.

    function _getRoyalty(
        uint256 tokenId,
        uint256 salePrice
    ) internal view returns (uint256 royaltyFee, address recipient) {
        // get the royalty lookup address
        address lookupAddress = IRoyaltyRegistry(royaltyRegistry)
            .getRoyaltyLookupAddress(nft);

        if (
            IERC2981(lookupAddress).supportsInterface(
                type(IERC2981).interfaceId
            )
        ) {
            // get the royalty fee from the registry
            (recipient, royaltyFee) = IERC2981(lookupAddress).royaltyInfo(
                tokenId,
                salePrice
            );

            // revert if the royalty fee is greater than the sale price
            if (royaltyFee > salePrice) revert InvalidRoyaltyFee();
        }
    }

Another line of code that can revert is

address lookupAddress = IRoyaltyRegistry(royaltyRegistry).getRoyaltyLookupAddress(nft);

if the lookupAddress is address(0), function call below woudl revert and block the transaction

if (IERC2981(lookupAddress).supportsInterface(type(IERC2981).interfaceId)) {

Tools Used

Manual Review

Recommended Mitigation Steps

The recommended mitigation steps include wrapping the royalty fee query in a try-catch block to avoid denial of service. This will prevent the transaction from reverting if the getRoyaltyLookupAddress(nft) query fails. Additionally, the platform can explore alternatives to prevent the getRoyaltyLookupAddress(nft) query from failing, such as adding support for interface lookup for NFTs that do not currently support it.

    function _getRoyalty(
        uint256 tokenId,
        uint256 salePrice
    ) internal view returns (uint256 royaltyFee, address recipient) {
        // get the royalty lookup address
        address lookupAddress = IRoyaltyRegistry(royaltyRegistry)
            .getRoyaltyLookupAddress(nft);

        try IERC2981(lookupAddress).supportsInterface(
            type(IERC2981).interfaceId
        ) returns (bool supported) {
            if (supported) {
                // get the royalty fee from the registry
                (recipient, royaltyFee) = IERC2981(lookupAddress).royaltyInfo(
                    tokenId,
                    salePrice
                );

                // revert if the royalty fee is greater than the sale price
                if (royaltyFee > salePrice) revert InvalidRoyaltyFee();
            }
        } catch {
            // handle the exception to avoid denial of service
        }
    }