Missing pause modifier on important LensV2Migration and FollowNFT functions

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-lens/blob/main/contracts/LensHub.sol#L368-L373
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/misc/LensV2Migration.sol#L33
https://github.com/code-423n4/2023-07-lens/blob/main/contracts/misc/LensV2Migration.sol#L37-L41
https://github.com/code-423n4/2023-07-lens/blob/main/contracts/misc/LensV2Migration.sol#L45
https://github.com/code-423n4/2023-07-lens/blob/main/contracts/base/LensProfiles.sol#L93-L98
https://github.com/code-423n4/2023-07-lens/blob/main/contracts/base/LensProfiles.sol#L165-L169
https://github.com/code-423n4/2023-07-lens/blob/main/contracts/FollowNFT.sol#L156
https://github.com/code-423n4/2023-07-lens/blob/main/contracts/FollowNFT.sol#L164
https://github.com/code-423n4/2023-07-lens/blob/main/contracts/FollowNFT.sol#L188
https://github.com/code-423n4/2023-07-lens/blob/main/contracts/FollowNFT.sol#L255
https://github.com/code-423n4/2023-07-lens/blob/main/contracts/FollowNFT.sol#L436-L440

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 {

• LensV2Migration.sol#L33
• LensV2Migration.sol#L37-L41
• LensV2Migration.sol#L45

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 {

• LensProfiles.sol#L93-L98
• LensProfiles.sol#L165-L169

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 {

• FollowNFT.sol#L156
• FollowNFT.sol#L164
• FollowNFT.sol#L188
• FollowNFT.sol#L255
• FollowNFT.sol#L436-L440

Tools Used

Manual Review

Recommended Mitigation Steps

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

Assessed type

Other

[c4-pre-sort]

141345 marked the issue as primary issue

[vicnaum]

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

[c4-sponsor]

vicnaum marked the issue as disagree with severity

[c4-judge]

Picodes marked the issue as satisfactory

[Picodes]

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

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[0xJuancito]

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.

[MiloTruck]

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

[donosonaumczuk]

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

[Picodes]

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

[c4-judge]

This previously downgraded issue has been upgraded by Picodes

[c4-judge]

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