`_getReceiver()` will revert for burnt profiles due to use of `ownerOf()`

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-lens/blob/main/contracts/misc/LegacyCollectNFT.sol#L102-L106 https://github.com/code-423n4/2023-07-lens/blob/main/contracts/FollowNFT.sol#L449-L453

Vulnerability details

Bug Description

Contracts that inherit ERC2981CollectionRoyalties must override _getReceiver() to determine the royalty recipient returned by royaltyInfo().

For LegacyCollectNFT.sol and FollowNFT.sol, _getReceiver() returns the owner of _profileId and _followedProfileId respectively:

LegacyCollectNFT.sol#L102-L106

    function _getReceiver(
        uint256 /* tokenId */
    ) internal view override returns (address) {
        return IERC721(HUB).ownerOf(_profileId);
    }

FollowNFT.sol#L449-L453

    function _getReceiver(
        uint256 /* followTokenId */
    ) internal view override returns (address) {
        return IERC721(HUB).ownerOf(_followedProfileId);
    }

However, the ownerOf() function in Openzeppelin's ERC-721 implementation reverts when the token has no owner:

ERC721.sol#L70-L74

    function ownerOf(uint256 tokenId) public view virtual override returns (address) {
        address owner = _ownerOf(tokenId);
        require(owner != address(0), "ERC721: invalid token ID");
        return owner;
    }

As such, profile owners can burn their profile through the burn() function in LensProfiles.sol to force royaltyInfo() to revert.

This becomes problematic in NFT marketplaces that support ERC-2981 and call royaltyInfo() without handling reverts gracefully. For example, the Caviar NFT Marketplace calls royaltyInfo() whenever NFTs are bought or sold:

CaviarEthRoyaltyRouter.sol#L150-L152

        if (IERC2981(lookupAddress).supportsInterface(type(IERC2981).interfaceId)) {
            (recipient, royaltyAmount) = IERC2981(lookupAddress).royaltyInfo(tokenId, salePrice);
        }

Therefore, whenever a collect or follow NFT is bought through the NFT marketplace, the profile owner can burn his profile to DOS the swap, and potentially cause the NFT to become stuck in the contract.

Impact

In LegacyCollectNFT.sol and FollowNFT.sol, royaltyInfo() will revert when the corresponding profile is burned. This might break the functionality of contracts that make calls to royaltyInfo(), but do not expect it to revert.

Proof of Concept

The following Foundry test demonstrates that royaltyInfo() reverts after the corresponding profile has been burnt. It can be run using the following command:

forge test --match-test testCanMakeRoyaltyInfoRevert -vvv 

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.13;

import 'forge-std/Test.sol';
import 'test/LegacyCollectNFTTest.t.sol';

contract ERC2981Royalties_POC is LegacyCollectNFTTest {
    function testCanMakeRoyaltyInfoRevert() public {
        // royaltyInfo() returns the profile owner as expected
        (address receiver, uint256 amount) = collectNFT.royaltyInfo(firstCollectTokenId, 1e18);
        assertEq(receiver, defaultAccount.owner);

        // Profile owner disables tokenGuardian
        vm.startPrank(defaultAccount.owner);
        hub.DANGER__disableTokenGuardian();

        // Forward time until tokenGuardian is disabled
        vm.warp(hub.getTokenGuardianDisablingTimestamp(defaultAccount.owner));

        // Profile owner burns his profile
        hub.burn(defaultAccount.profileId);

        // royaltyInfo() reverts as the token doesn't exist anymore
        vm.expectRevert(Errors.TokenDoesNotExist.selector);
        (receiver, amount) = collectNFT.royaltyInfo(firstCollectTokenId, 1e18);
    }
}

Recommended Mitigation

Consider checking if the profile exists first and returning address(0) immediately if it doesn't:

LegacyCollectNFT.sol#L102-L106

    function _getReceiver(
        uint256 /* tokenId */
    ) internal view override returns (address) {
+       if (!_exists(_profileId)) return address(0);
        return IERC721(HUB).ownerOf(_profileId);
    }

FollowNFT.sol#L449-L453

    function _getReceiver(
        uint256 /* followTokenId */
    ) internal view override returns (address) {
+       if (!_exists(_followedProfileId)) return address(0);
        return IERC721(HUB).ownerOf(_followedProfileId);
    }

Assessed type

DoS

[donosonaumczuk]

We accept it. This should be Low severity.

We will see if there is a more interesting place where to send the royalties instead of returning address(0) in this case.

[c4-sponsor]

donosonaumczuk marked the issue as disagree with severity

[Picodes]

Unless I have missed it I don't see in the EIP that this function shouldn't revert, so it's more an issue at the marketplace level and a QA comment in the context of this contest in my opinion

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)