Users cannot unfollow if they do not own the FollowNFT of the `followTokenId` used for their profile

mediumCode4rena

Lines of code

https://github.com/code-423n4/2023-07-lens/blob/main/contracts/FollowNFT.sol#L115-L125

Vulnerability details

Bug Description

If the followTokenId of a profile is wrapped, users will only be able to unfollow if they are either:

The owner of the follow NFT.
An approved operator of the follow NFT's owner.

This can be seen in the unfollow() function of FollowNFT.sol:

FollowNFT.sol#L115-L125

solidity
            // Follow token is wrapped.
            address unfollowerProfileOwner = IERC721(HUB).ownerOf(unfollowerProfileId);
            // Follower profile owner or its approved delegated executor must hold the token or be approved-for-all.
            if (
                (followTokenOwner != unfollowerProfileOwner) &&
                (followTokenOwner != transactionExecutor) &&
                !isApprovedForAll(followTokenOwner, transactionExecutor) &&
                !isApprovedForAll(followTokenOwner, unfollowerProfileOwner)
            ) {
                revert DoesNotHavePermissions();
            }

As seen from above, users that are not the owner or do not have approval for the wrapped follow NFT will not be able to unfollow. This is problematic as users are able to follow with a followTokenId without owning the corresponding follow NFT.

For example, someone who holds a follow NFT can call approveFollow() for a user. The user can then call follow() with the corresponding followTokenId, which works as _followWithWrappedToken() checks for follow approval:

FollowNFT.sol#L317-L327

solidity
        bool isFollowApproved = _followApprovalByFollowTokenId[followTokenId] == followerProfileId;
        address followerProfileOwner = IERC721(HUB).ownerOf(followerProfileId);
        if (
            !isFollowApproved &&
            followTokenOwner != followerProfileOwner &&
            followTokenOwner != transactionExecutor &&
            !isApprovedForAll(followTokenOwner, transactionExecutor) &&
            !isApprovedForAll(followTokenOwner, followerProfileOwner)
        ) {
            revert DoesNotHavePermissions();
        }

Now, if the user wants to unfollow, he will be unable to do so by himself, and is forced to rely on the follow NFT owner to unfollow for his profile.

Impact

Users that follow using a wrapped followTokenId that they do not own will not be unfollow the profile. This is incorrect as a profile owner should have full control over who the profile does/does not follow.

Proof of Concept

The Foundry test below demonstrates that unfollow() will revert when users do not own the FollowNFT, even when unfollowing with their own profile. It can be run with the following command:

forge test --match-test testCannotUnfollowWithoutFollowNFT -vvv

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

import 'test/base/BaseTest.t.sol';
import 'contracts/interfaces/IFollowNFT.sol';

contract Unfollow_POC is BaseTest {
    address targetProfileOwner;
    address ALICE;
    address BOB;

    uint256 targetProfileId;
    uint256 aliceProfileId;
    uint256 bobProfileId;

    function setUp() public override {
        super.setUp();

        // Setup addresses for target, Alice and Bob
        targetProfileOwner = makeAddr("Target");
        ALICE = makeAddr("Alice");
        BOB = makeAddr("Bob");

        // Create profile for target, Alice and Bob 
        targetProfileId = _createProfile(targetProfileOwner);
        aliceProfileId = _createProfile(ALICE);
        bobProfileId = _createProfile(BOB);
    }

    function testCannotUnfollowWithoutFollowNFT() public {
        // Alice follows target
        vm.startPrank(ALICE);
        uint256 followTokenId = hub.follow(
            aliceProfileId,
            _toUint256Array(targetProfileId),
            _toUint256Array(0),
            _toBytesArray('')
        )[0];

        // Get followNFT contract created
        FollowNFT followNFT = FollowNFT(hub.getProfile(targetProfileId).followNFT);

        // Alice lets Bob follow using her followTokenId
        followNFT.wrap(followTokenId);
        followNFT.approveFollow(bobProfileId, followTokenId);
        vm.stopPrank();

        // Bob follows using her followTokenId        
        vm.startPrank(BOB);
        hub.follow(
            bobProfileId,
            _toUint256Array(targetProfileId),
            _toUint256Array(followTokenId),
            _toBytesArray('')
        );
        assertTrue(followNFT.isFollowing(bobProfileId));

        // After a while, Bob wants to unfollow. 
        // However, unfollow() reverts as he doesn't own the followNFT
        vm.expectRevert(IFollowNFT.DoesNotHavePermissions.selector);
        hub.unfollow(bobProfileId, _toUint256Array(targetProfileId));
        vm.stopPrank();
    }
}

Recommended Mitigation

In unfollow(), consider allowing the owner of unfollowerProfileId to unfollow as well:

FollowNFT.sol#L115-L125

diff
            // Follow token is wrapped.
            address unfollowerProfileOwner = IERC721(HUB).ownerOf(unfollowerProfileId);
            // Follower profile owner or its approved delegated executor must hold the token or be approved-for-all.
            if (
+               transactionExecutor != unfollowerProfileOwner && 
                (followTokenOwner != unfollowerProfileOwner) &&
                (followTokenOwner != transactionExecutor) &&
                !isApprovedForAll(followTokenOwner, transactionExecutor) &&
                !isApprovedForAll(followTokenOwner, unfollowerProfileOwner)
            ) {
                revert DoesNotHavePermissions();
            }