code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-07-lens/blob/main/contracts/FollowNFT.sol#L131-L138 https://github.com/code-423n4/2023-07-lens/blob/main/contracts/FollowNFT.sol#L255-L258

Vulnerability details

Bug Description

When the LensHub contract has been paused by governance (_state set to ProtocolState.Paused), users should not be able unfollow profiles. This can be inferred as the unfollow() function has the whenNotPaused modifier:

LensHub.sol#L368-L371

    function unfollow(uint256 unfollowerProfileId, uint256[] calldata idsOfProfilesToUnfollow)
        external
        override
        whenNotPaused

However, in the FollowNFT contract, which is deployed for each profile that has followers, the removeFollower() and burn() functions do not check if the LensHub contract is paused:

FollowNFT.sol#L131-L138

    function removeFollower(uint256 followTokenId) external override {
        address followTokenOwner = ownerOf(followTokenId);
        if (followTokenOwner == msg.sender || isApprovedForAll(followTokenOwner, msg.sender)) {
            _unfollowIfHasFollower(followTokenId);
        } else {
            revert DoesNotHavePermissions();
        }
    }

FollowNFT.sol#L255-L258

    function burn(uint256 followTokenId) public override {
        _unfollowIfHasFollower(followTokenId);
        super.burn(followTokenId);
    }

As such, whenever the system has been paused by governance, users will still be able to unfollow profiles by wrapping their followNFT and then calling either removeFollower() or burn().

Impact

Users are able to unfollow profiles when the system is paused, which they should not be able to do.

This could be problematic if governance ever needs to temporarily pause unfollow functionality (eg. for a future upgrade, or unfollowing functionality has a bug, etc...).

Proof of Concept

The Foundry test below demonstrates how users will still be able to unfollow profiles by calling wrap() and removeFollower(), even after the system has been paused by governance. It can be run with the following command:

forge test --match-test testCanUnfollowWhilePaused -vvv

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

import 'test/base/BaseTest.t.sol';

contract Unfollow_POC is BaseTest {
    address targetProfileOwner;
    uint256 targetProfileId;
    FollowNFT targetFollowNFT;

    address follower;
    uint256 followerProfileId;
    uint256 followTokenId;

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

        // Create profile for target
        targetProfileOwner = makeAddr("Target");
        targetProfileId = _createProfile(targetProfileOwner);

        // Create profile for follower
        follower = makeAddr("Follower");
        followerProfileId = _createProfile(follower);

        // Follower follows target
        vm.prank(follower);
        followTokenId = hub.follow(
            followerProfileId,
            _toUint256Array(targetProfileId),
            _toUint256Array(0),
            _toBytesArray('')
        )[0];
        targetFollowNFT = FollowNFT(hub.getProfile(targetProfileId).followNFT);
    }

    function testCanUnfollowWhilePaused() public {
        // Governance pauses system
        vm.prank(governance);
        hub.setState(Types.ProtocolState.Paused);
        assertEq(uint8(hub.getState()), uint8(Types.ProtocolState.Paused));

        // unfollow() reverts as system is paused
        vm.startPrank(follower);
        vm.expectRevert(Errors.Paused.selector);
        hub.unfollow(followerProfileId, _toUint256Array(targetProfileId));

        // However, follower can still unfollow through FollowNFT contract 
        targetFollowNFT.wrap(followTokenId);
        targetFollowNFT.removeFollower(followTokenId);        
        vm.stopPrank();

        // follower isn't following anymore
        assertFalse(targetFollowNFT.isFollowing(followerProfileId));
    }
}

Recommended Mitigation

All FollowNFT contracts should check that the LensHub contract isn't paused before allowing removeFollower() or burn() to be called. This can be achieved by doing the following:

Add a whenNotPaused modifier to FollowNFT.sol:

modifier whenNotPaused() {
    if (ILensHub(HUB).getState() == Types.ProtocolState.Paused) {
        revert Errors.Paused();
    }
    _;
}

Use the modifier on removeFollower() and burn():

FollowNFT.sol#L131-L138

-   function removeFollower(uint256 followTokenId) external override {
+   function removeFollower(uint256 followTokenId) external override whenNotPaused {
        // Some code here...
    }

FollowNFT.sol#L255-L258

-   function burn(uint256 followTokenId) public override {
+   function burn(uint256 followTokenId) public override whenNotPaused {
        // Some code here...
    }

Assessed type

Access Control

donosonaumczuk commented 1 year ago

This report is a subset of this #108 Same resolution, we accept it but we disagree with the severity, it should be Low.

c4-sponsor commented 1 year ago

donosonaumczuk marked the issue as disagree with severity