[WP-M1] Inappropriate handling of referralFee makes collecting Mirror fails without error when referrerProfileId is burned

[code423n4]

Lines of code

https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/modules/collect/FeeCollectModule.sol#L163-L172

Vulnerability details

In the current implementation, even when the profile's owner burnt the ProfileNFT, as the profile's legacy, the publications can still be collected.

However, if the publication is a Mirror and there is a referralFee set by the original publication, the user won't be able to collect from a Mirror that was published by a burned profile.

https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/modules/collect/FeeCollectModule.sol#L163-L172

if (referralFee != 0) {
    // The reason we levy the referral fee on the adjusted amount is so that referral fees
    // don't bypass the treasury fee, in essence referrals pay their fair share to the treasury.
    uint256 referralAmount = (adjustedAmount * referralFee) / BPS_MAX;
    adjustedAmount = adjustedAmount - referralAmount;

    address referralRecipient = IERC721(HUB).ownerOf(referrerProfileId);

    IERC20(currency).safeTransferFrom(collector, referralRecipient, referralAmount);
}

When a mirror is collected, what happens behind the scenes is the original, mirrored publication is collected, and the mirror publisher's profile ID is passed as a "referrer."

In _processCollectWithReferral(), if there is a referralFee, contract will read referralRecipient from IERC721(HUB).ownerOf(referrerProfileId), if referrerProfileId is burned, the IERC721(HUB).ownerOf(referrerProfileId) will revert with ERC721: owner query for nonexistent token.

However, since we wish to allow the content to be collected, we should just treat referrals as non-existent in this situation.

Recommendation

Change to:

try IERC721(HUB).ownerOf(referrerProfileId) returns (address referralRecipient) {
    uint256 referralAmount = (adjustedAmount * referralFee) / BPS_MAX;
    adjustedAmount = adjustedAmount - referralAmount;

    address referralRecipient = IERC721(HUB).ownerOf(referrerProfileId);

    IERC20(currency).safeTransferFrom(collector, referralRecipient, referralAmount);
} catch {
    emit LogNonExistingReferrer(referrerProfileId);
}

[Zer0dot]

This is valid! However, here's what I commented on #14 :

"This is only an issue when a profile is deleted (burned), in which case UIs have multiple choices:

1. Stop displaying all the burnt profile's publications
2. Redirect users, when collecting mirrors, to the original publication
3. Prevent all collects

I don't think this adds any risk to the protocol and although it's valid, we will not be taking any action."