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

[code423n4]

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:

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

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

FollowNFT.sol#L115-L125

            // 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

        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

// 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

            // 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();
            }

Assessed type

Access Control

[c4-pre-sort]

141345 marked the issue as primary issue

[donosonaumczuk]

We confirm the issue. However, we are still debating if it is a Medium severity one or if it should be classified as Low

[c4-sponsor]

donosonaumczuk marked the issue as disagree with severity

[donosonaumczuk]

We mark it as we disagree with the severity, so we can discuss it better with the judges.

[c4-judge]

Picodes marked the issue as selected for report

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

This previously downgraded issue has been upgraded by Picodes

[Picodes]

Some comments :

• the comments on approveFollow and unfollow are quite clear on the fact that approveFollow doesn't give approval to unfollow

• but the argument that a functionality is broken as the unfollowerProfileOwner should be able to perform the operation seems valid to me and would be of Med severity under function of the protocol or its availability could be impacted

  Overall Medium severity seems appropriate here

[c4-judge]

Picodes marked the issue as satisfactory