Missing pause modifier on important `LensV2Migration` and `FollowNFT` functions

Question

Missing pause modifier on important `LensV2Migration` and `FollowNFT` functions

code423n4 opened this issue a year ago · 12 comments

Lines of code

Vulnerability details

Summary

Important user functions on Lens V2 are protected by pause modifiers like whenNotPaused, or whenPublishingEnabled.

This includes functions to follow, unfollow profiles, set follow modules, or create profiles among others.

Impact

Several important functions that modify contract storage by users, or perform actions on tokens are not protected by any pause modifiers. This allows users to interact with the contracts, although they should not be supposed to.

The most important ones being:

Unfollow actions can be performed via FollowNFT::removeFollower() despite LensHub::unfollow is protected with whenNotPaused
Migrations can be performed on LensHub via:
- LensV2Migration::batchMigrateProfiles()
- LensV2Migration::batchMigrateFollows()
- LensV2Migration::batchMigrateFollowModules()

In addition, some other FollowNFT functions could be considered for pause modifiers, as LensHub tokens have similar protections on the inherited LensProfiles contract:

FollowNFT::wrap()
FollowNFT::unwrap()
FollowNFT::burn()
FollowNFT::_beforeTokenTransfer()

Proof of Concept

Unfollow actions are prevented on paused contracts for LensHub::unfollow(), but can still be executed via FollowNFT::removeFollower():

    function unfollow(uint256 unfollowerProfileId, uint256[] calldata idsOfProfilesToUnfollow)
        external
        override
        whenNotPaused
        onlyProfileOwnerOrDelegatedExecutor(msg.sender, unfollowerProfileId)
    {

LensHub.sol#L368-L373

    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#L131-L138

LensV2Migration is inherited by LensHub and has the following functions missing any pause modifiers:

    function batchMigrateProfiles(uint256[] calldata profileIds) external {

    function batchMigrateFollows(
        uint256[] calldata followerProfileIds,
        uint256[] calldata idsOfProfileFollowed,
        uint256[] calldata followTokenIds
    ) external {

    function batchMigrateFollowModules(uint256[] calldata profileIds) external {

LensProfiles is inherited by LensHub and protects certain functions regarding the token like these ones:

    function burn(uint256 tokenId)
        public
        override(LensBaseERC721, IERC721Burnable)
@>      whenNotPaused
        onlyProfileOwner(msg.sender, tokenId)
    {

    function _beforeTokenTransfer(
        address from,
        address to,
        uint256 tokenId
@>  ) internal override whenNotPaused {

On the other hand, FollowNFT has no pause protection on functions related to its token. Such as:

    function wrap(uint256 followTokenId, address wrappedTokenReceiver) external override {

    function wrap(uint256 followTokenId) external override {

    function unwrap(uint256 followTokenId) external override {

    function burn(uint256 followTokenId) public override {

    function _beforeTokenTransfer(
        address from,
        address to,
        uint256 followTokenId
    ) internal override {

Tools Used

Manual Review

Recommended Mitigation Steps

Add the corresponding pause modifiers to the functions mentioned on the Impact section.

Assessed type

Other

Answer 1 · 2023-08-04T12:41:36.000Z

141345 marked the issue as primary issue

Answer 2 · 2023-08-07T15:55:03.000Z

We confirm the issue, although this might be rather considered as Low instead of Medium

Answer 3 · 2023-08-07T15:55:08.000Z

vicnaum marked the issue as disagree with severity

Answer 4 · 2023-08-28T13:42:47.000Z

Picodes marked the issue as satisfactory

Answer 5 · 2023-08-28T14:10:13.000Z

Downgrading to Low as assets nor availability of the protocol is at risk

Answer 6 · 2023-08-28T14:10:22.000Z

Picodes changed the severity to QA (Quality Assurance)

Answer 7 · 2023-08-29T03:42:13.000Z

Hi @Picodes. I'd like to ask if you could take a second look at this issue as a Medium risk finding, considering the Docs:

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted

In this case the "function of the protocol" is impacted, and availability can also be considered as well. Not for being unavailable, but because of functions being available in moments that they shouldn't be, allowing important actions for a social network that users should not be able to perform, out of the control of the protocol.

When the protocol is paused, it should not allow unfollow actions (via removeFollower()) or new follows (via batchMigrateProfiles() for example, among all the other mentioned functions on the Impact section.

In DeFi protocols, missing pause modifiers having been evaluated as Medium like here, and here. In the case of Lens Protocol, the functions mentioned on the Impact section should be considered of ultimate importance to have under control as a social network.

Answer 8 · 2023-08-30T02:10:44.000Z

Hi @Picodes, just wanted to mention that #144 is a duplicate of this issue in case it is upgraded. Thanks!

Answer 9 · 2023-08-30T16:15:42.000Z

Yeah, I think it is a fair argument and can be upgraded to Medium.

Answer 10 · 2023-08-31T16:29:12.000Z

My view on this is that it ultimately depends on the sponsor's intent. In this case, it seems clear by the above comment and what you highlighted that the intent was to be able to totally pause follows and unfollows, so you're right and I'll upgrade this to Med as a functionality is broken

Answer 11 · 2023-08-31T16:29:34.000Z

This previously downgraded issue has been upgraded by Picodes

Answer 12 · 2023-08-31T16:33:04.000Z

Picodes marked issue #144 as primary and marked this issue as a duplicate of 144