Skip to content

Latest commit

 

History

History
113 lines (91 loc) · 5.35 KB

M-04.md

File metadata and controls

113 lines (91 loc) · 5.35 KB
Raw

Checking for interface support while fetching royalties might revert operation

Impact

NFT trading can incur in royalty fees. Private pools that have the feature enabled and public pools via the EthRouter support paying royalty fees by querying a lookup address using a registry.

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L778-L793

778:     function _getRoyalty(uint256 tokenId, uint256 salePrice)
779:         internal
780:         view
781:         returns (uint256 royaltyFee, address recipient)
782:     {
783:         // get the royalty lookup address
784:         address lookupAddress = IRoyaltyRegistry(royaltyRegistry).getRoyaltyLookupAddress(nft);
785: 
786:         if (IERC2981(lookupAddress).supportsInterface(type(IERC2981).interfaceId)) {
787:             // get the royalty fee from the registry
788:             (recipient, royaltyFee) = IERC2981(lookupAddress).royaltyInfo(tokenId, salePrice);
789: 
790:             // revert if the royalty fee is greater than the sale price
791:             if (royaltyFee > salePrice) revert InvalidRoyaltyFee();
792:         }
793:     }

https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L301-L316

301:     function getRoyalty(address nft, uint256 tokenId, uint256 salePrice)
302:         public
303:         view
304:         returns (uint256 royaltyFee, address recipient)
305:     {
306:         // get the royalty lookup address
307:         address lookupAddress = IRoyaltyRegistry(royaltyRegistry).getRoyaltyLookupAddress(nft);
308: 
309:         if (IERC2981(lookupAddress).supportsInterface(type(IERC2981).interfaceId)) {
310:             // get the royalty fee from the registry
311:             (recipient, royaltyFee) = IERC2981(lookupAddress).royaltyInfo(tokenId, salePrice);
312: 
313:             // revert if the royalty fee is greater than the sale price
314:             if (royaltyFee > salePrice) revert InvalidRoyaltyFee();
315:         }
316:     }

The implementation in both cases is exactly the same. First a lookupAddress is fetched from the registry and then this address is used to query the royalty information by using the royaltyInfo function. Before making this call, the function checks that the lookupAddress address supports the IERC2981 interface by using the ERC165 interface detection process (supportsInterface).

However, if the lookupAddress contract doesn't support ERC165, then the supportsInterface call itself will fail, causing the whole transaction (buy and sell operations in PrivatePool and EthRouter) to revert.

Proof of Concept

In the following test we setup a royalty lookup contract that doesn't support ERC165 (RoyaltyLookupNoERC165) and define this contract as the lookup address for the milady collection in the RoyaltyRegistry contract. When Alice tries to execute a buy operation in the pool, the transaction will be reverted as the call to supportsInterface to the RoyaltyLookupNoERC165 contract will fail.

Note: the snippet shows only the relevant code for the test. Full test file can be found here.

function test_PrivatePool_getRoyalty_RevertCheckInterface() public {
    RoyaltyLookupNoERC165 royaltyLookup = new RoyaltyLookupNoERC165();

    // Register royalty lookup for milady collection
    vm.prank(royaltyRegistryOwner);
    royaltyRegistry.setRoyaltyLookupAddress(
        address(milady),
        address(royaltyLookup)
    );

    // Setup pool
    PrivatePool privatePool = new PrivatePool(
        address(factory),
        address(royaltyRegistry),
        address(stolenNftOracle)
    );
    privatePool.initialize(
        address(0), // address _baseToken,
        address(milady), // address _nft,
        100e18, // uint128 _virtualBaseTokenReserves,
        10e18, // uint128 _virtualNftReserves,
        500, // uint56 _changeFee,
        100, // uint16 _feeRate,
        bytes32(0), // bytes32 _merkleRoot,
        false, // bool _useStolenNftOracle,
        true // bool _payRoyalties
    );

    // Add NFT to pool
    uint256 tokenId = 0;
    milady.mint(address(privatePool), tokenId);

    // Alice tries to buy the NFT
    vm.startPrank(alice);

    (uint256 netInputAmount, , ) = privatePool.buyQuote(1e18);
    vm.deal(alice, netInputAmount);

    uint256[] memory tokenIds = new uint256[](1);
    tokenIds[0] = tokenId;
    uint256[] memory tokenWeights = new uint256[](0);
    PrivatePool.MerkleMultiProof memory proof;

    // The following will fail because `_getRoyalty` will try to call function `supportsInterface` in RoyaltyLookupNoERC165
    vm.expectRevert();
    privatePool.buy{value: netInputAmount}(tokenIds, tokenWeights, proof);

    vm.stopPrank();
}

Recommendation

The getRoyalty function should use a safer and more resilient method for ERC165 interface detection. One alternative would be to use OpenZeppelin ERC165Checker, which also checks if the target supports ERC165 and will eventually return false instead of reverting in case the lookup address doesn't implement the supportsInterface function. Another possible solution would be to directly execute the royaltyInfo function by wrapping it in a try/catch block and defaulting to empty values in case the call fails.