Lens Protocol V2 - MiloTruck's results

Lines of code

https://github.com/code-423n4/2023-07-lens/blob/main/contracts/libraries/ActionLib.sol#L23-L26

Vulnerability details

Bug Description

In the protocol, publications are uniquely identified through the publisher's profile ID and the publication's ID. For example, when a user calls act(), the publication being acted on is determined by publicationActedProfileId and publicationActedId:

ActionLib.sol#L23-L26

        Types.Publication storage _actedOnPublication = StorageLib.getPublication(
            publicationActionParams.publicationActedProfileId,
            publicationActionParams.publicationActedId
        );

However, as publication IDs are not based on the publication's data, this could cause users to act on the wrong publication in the event a blockchain re-org occurs.

For example:

• Assume the following transactions occur in separate blocks:
  • Block 1: Alice calls post() to create a post; its publication ID is 20.
  • Block 2: Bob is interested in the post, he calls act() with publicationActedId = 20 to act on the post.
  • Block 3: Alice calls comment() separately, which creates another publication; its publication ID is 21.
• A blockchain re-org occurs; block 1 is dropped in place of block 3:
  • Alice's comment now has the publication ID 20 instead of 21.
• Bob's call to act() in block 2 is applied on top of the re-orged blockchain:
  • This causes him to act on the comment instead of the post he intended to, as it now has the publication ID 20.

In this scenario, due to the blockchain re-org, Bob calls act() on a different publication than the one he wanted. This could have severe impacts depending on the action module being called; if the action module is used to collect and pay fees to the publisher and referrals (eg. MultirecipientFeeCollectModule.sol), Bob could have lost funds.

Note that this also applies to comment(), mirror and quote(), as they can be called with reference modules with sensitive logic as well.

Impact

If a blockchain re-org occurs, users could potentially act/comment/mirror/quote on the wrong publication, which has varying impacts depending on the action or reference module being used, such as a loss of funds due to paying fees.

Given that Lens Protocol is deployed on Poylgon, which has experienced large re-orgs in the past, the likelihood of the scenario described above occuring due to a blockchain re-org is not low.

Recommended Mitigation

Consider identifying publications with a method that is dependent on its contents. For example, users could be expected to provide the keccak256 hash of a publication's contents alongside its publication ID.

This would prevent users from acting on the wrong publication should a publication's contents change despite having the same ID.

Assessed type

Other

Lines of code

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

Vulnerability details

Bug Description

In FollowNFT.sol, the tryMigrate() function is used to migrate users who were following before the V2 upgrade. It does so by updating _followTokenIdByFollowerProfileId and _followDataByFollowTokenId, which are state variables introduced in the V2 upgrade:

FollowNFT.sol#L510-L516

        _followTokenIdByFollowerProfileId[followerProfileId] = followTokenId;

        uint48 mintTimestamp = uint48(StorageLib.getTokenData(followTokenId).mintTimestamp);

        _followDataByFollowTokenId[followTokenId].followerProfileId = uint160(followerProfileId);
        _followDataByFollowTokenId[followTokenId].originalFollowTimestamp = mintTimestamp;
        _followDataByFollowTokenId[followTokenId].followTimestamp = mintTimestamp;

Since _followTokenIdByFollowerProfileId is a new state variable, it will be set to 0 for users who were following before the V2 upgrade. This allows old followers to call follow() to follow the profile again before tryMigrate() is called:

FollowNFT.sol#L59-L66

    function follow(
        uint256 followerProfileId,
        address transactionExecutor,
        uint256 followTokenId
    ) external override onlyHub returns (uint256) {
        if (_followTokenIdByFollowerProfileId[followerProfileId] != 0) {
            revert AlreadyFollowing();
        }

Even if tryMigrate() is called by the protocol team immediately after the V2 upgrade, a malicious user can still call follow() before tryMigrate() by:

• Front-running the migration transaction.
• Holding his profile and follow NFT in different addresses, which causes tryMigrate() to return here.

As a profile should not be able to follow the same profile twice, tryMigrate() should then revert for old followers who have called follow(). However, this isn't enforced by tryMigrate() as there is no check that _followDataByFollowTokenId[followerProfileId] is 0.

As a result, if tryMigrate() is called after follow(), _followerCount will be incremented twice for a single profile:

FollowNFT.sol#L506-L510

        unchecked {
            ++_followerCount;
        }

        _followTokenIdByFollowerProfileId[followerProfileId] = followTokenId;

Additionally, even though _followTokenIdByFollowerProfileId points to a new followTokenId, _followDataByFollowTokenId will not be cleared for the previous follow token ID.

As the state of the FollowNFT contract is now corrupt, followers can perform functions that they normally should not be able to, such as unfollowing when their profile is not a follower (isFollowing() returns false).

Impact

Users who are followers before the V2 upgrade will be able to follow with a single profile twice, causing followerCount to be higher than the actual number of profiles following.

Addtionally, as _followDataByFollowTokenId is corrupted, followers might be able to call functions when they should not be allowed to, potentially leading to more severe impacts.

Proof of Concept

The Foundry test below demonstrates that tryMigrate() can be called although the user is already following, and how followerCount and _followDataByFollowTokenId will be corrupted as a result. It can be run with the following command:

forge test --match-test testCanMigrateWhileFollowing -vvv

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

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

contract FollowMigration_POC is BaseTest {
    address target = address(0x1337);
    address ALICE;

    uint256 targetProfileId;
    uint256 aliceProfileId;

    uint256 oldFollowTokenId;

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

        // Setup addresses for Alice
        ALICE = makeAddr("Alice");

        // Create profile for target and Alice 
        targetProfileId = _createProfile(target);
        aliceProfileId = _createProfile(ALICE);

        // Add simulateV1Follow() helper function to FollowNFT implementation
        FollowNFTHelper implementation = new FollowNFTHelper(address(hub));  
        vm.etch(hub.getFollowNFTImpl(), address(implementation).code);
        
        // Follow and unfollow to deploy target's FollowNFT contract
        vm.startPrank(defaultAccount.owner);
        hub.follow(
            defaultAccount.profileId,
            _toUint256Array(targetProfileId),
            _toUint256Array(0),
            _toBytesArray('')
        );
        hub.unfollow(defaultAccount.profileId, _toUint256Array(targetProfileId));
        vm.stopPrank();

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

        // Alice follows target before the V2 upgrade
        oldFollowTokenId = FollowNFTHelper(followNFTAddress).simulateV1Follow(ALICE);
    }

    function testCanMigrateWhileFollowing() public {
        // After the V2 upgrade, Alice calls follow() instead of migrating her follow
        vm.startPrank(ALICE);
        uint256 followTokenId = hub.follow(
            aliceProfileId,
            _toUint256Array(targetProfileId),
            _toUint256Array(0),
            _toBytesArray('')
        )[0];

        // Alice migrates her V1 follow even though her profile is already following
        hub.batchMigrateFollows(
            _toUint256Array(aliceProfileId),
            _toUint256Array(targetProfileId),
            _toUint256Array(oldFollowTokenId)
        );

        // followTokenId's followerProfileId points to Alice's profile
        assertEq(followNFT.getFollowerProfileId(followTokenId), aliceProfileId);

        // However, Alice's _followTokenIdByFollowerProfileId points to oldFollowTokenId
        assertEq(followNFT.getFollowTokenId(aliceProfileId), oldFollowTokenId);

        // Follower count is 2 although Alice is the only follower
        assertEq(followNFT.getFollowerCount(), 2);

        // Wrap both follow tokens
        vm.startPrank(ALICE);
        followNFT.wrap(followTokenId);
        followNFT.wrap(oldFollowTokenId);

        // Alice unfollows using removeFollower()
        vm.expectEmit();
        emit Events.Unfollowed(aliceProfileId, targetProfileId, 1);
        followNFT.removeFollower(followTokenId);

        // Alice is no longer following
        assertFalse(followNFT.isFollowing(aliceProfileId));

        // However, she is still able to unfollow for a second time
        vm.expectEmit();
        emit Events.Unfollowed(aliceProfileId, targetProfileId, 1);
        followNFT.removeFollower(oldFollowTokenId);
    }
}

contract FollowNFTHelper is FollowNFT {
    constructor(address hub) FollowNFT(hub) {}

    /*
    Helper function to mimic a V1 follow, which does the following:
    - Increment _tokenIdCounter
    - Mint a followNFT
    */ 
    function simulateV1Follow(address follower) external returns (uint256 followTokenId) {
        followTokenId = ++_lastFollowTokenId;
        _mint(follower, followTokenId);
    }
}

Recommended Mitigation

FollowNFT.sol#L480-L489

    function tryMigrate(
        uint256 followerProfileId,
        address followerProfileOwner,
        uint256 idOfProfileFollowed,
        uint256 followTokenId
    ) external onlyHub returns (uint48) {
+       if (_followTokenIdByFollowerProfileId[followerProfileId] != 0) {
+           return 0;
+       }
+
        // Migrated FollowNFTs should have `originalFollowTimestamp` set
        if (_followDataByFollowTokenId[followTokenId].originalFollowTimestamp != 0) {
            return 0; // Already migrated
        }

Assessed type

Upgradable

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

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:

1. Add a whenNotPaused modifier to FollowNFT.sol:

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

2. 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

Lines of code

https://github.com/code-423n4/2023-07-lens/blob/main/contracts/libraries/MigrationLib.sol#L69-L85

Vulnerability details

Bug Description

Profiles that exist before the V2 upgrade are migrated using the batchMigrateProfiles() function, which works by minting the profile's handle and linking it to their profile:

MigrationLib.sol#L69-L85

            string memory handle = StorageLib.getProfile(profileId).__DEPRECATED__handle;
            if (bytes(handle).length == 0) {
                return; // Already migrated
            }
            bytes32 handleHash = keccak256(bytes(handle));
            // We check if the profile is the "lensprotocol" profile by checking profileId != 1.
            // "lensprotocol" is the only edge case without the .lens suffix:
            if (profileId != LENS_PROTOCOL_PROFILE_ID) {
                assembly {
                    let handle_length := mload(handle)
                    mstore(handle, sub(handle_length, DOT_LENS_SUFFIX_LENGTH)) // Cut 5 chars (.lens) from the end
                }
            }
            // We mint a new handle on the LensHandles contract. The resulting handle NFT is sent to the profile owner.
            uint256 handleId = lensHandles.migrateHandle(profileOwner, handle);
            // We link it to the profile in the TokenHandleRegistry contract.
            tokenHandleRegistry.migrationLink(handleId, profileId);

For example, a profile with the handle "alice.lens" will receive a "alice" LensHandles NFT post-migration.

However, whitelisted profile creators are able to mint any handle using mintHandle() in the LensHandles contract. This makes it possible for any whitelisted profile creator to mint a handle corresponding to a V1 profile before the profile is migrated.

If this occurs, batchMigrateProfiles() will always revert for the corresponding V1 profile as the same handle cannot be minted twice, thereby breaking migration for that profile.

Impact

If a whitelisted profile creator accidentally mints a handle that already belongs to a V1 profile, that profile cannot be migrated.

Proof of Concept

The Foundry test below demonstrates how batchMigrateProfiles() will revert if a V1 profile's handle has already been minted. It can be run with the following command:

forge test --match-test testProfileCreatorCanBreakProfileMigration -vvv

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

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

contract ProfileMigration_POC is BaseTest {
    LensHubHelper hubProxy;

    function setUp() public override {
        super.setUp();
        
        // Add toLegacyV1Profile() function to LensHub
        LensHubHelper implementation = new LensHubHelper({
            lensHandlesAddress: address(lensHandles),
            tokenHandleRegistryAddress: address(tokenHandleRegistry),
            tokenGuardianCooldown: PROFILE_GUARDIAN_COOLDOWN
        });
        vm.prank(deployer);
        hubAsProxy.upgradeTo(address(implementation));

        // Cast proxy to LensHubHelper interface
        hubProxy = LensHubHelper(address(hubAsProxy));
    }

    function testProfileCreatorCanBreakProfileMigration() public {
        // Create a v1 profile with the handle "alice.lens"
        uint256 profileId = _createProfile(address(this));
        hubProxy.toLegacyV1Profile(profileId, "alice.lens");

        // Whitelisted profile creator accidentally mints a "alice.lens" handle
        vm.prank(lensHandles.OWNER());
        lensHandles.mintHandle(address(this), "alice");

        // V1 profile will revert when migrated as the handle already exists
        vm.expectRevert("ERC721: token already minted");
        hubProxy.batchMigrateProfiles(_toUint256Array(profileId));
    }
}

contract LensHubHelper is LensHub {
    constructor(
        address lensHandlesAddress,
        address tokenHandleRegistryAddress,
        uint256 tokenGuardianCooldown
    ) LensHub(
        address(0),
        address(0),
        address(0),
        lensHandlesAddress,
        tokenHandleRegistryAddress,
        address(0),
        address(0),
        address(0),
        tokenGuardianCooldown
    ) {}


    function toLegacyV1Profile(uint256 profileId, string memory handle) external {
        Types.Profile storage profile = StorageLib.getProfile(profileId);
        profile.__DEPRECATED__handle = handle;
        delete profile.metadataURI;
    }
}

Recommended Mitigation

Ensure that the handle of a V1 profile cannot be minted through mintHandle(). This validation will probably have to be done off-chain, as it is unfeasible to check all existing handles on-chain with a reasonable gas cost.

Assessed type

Upgradable

    function getProfileIdByHandleHash(bytes32 handleHash) external view returns (uint256) {
        return StorageLib.profileIdByHandleHash()[handleHash];
    }

    function proxyCreateProfileWithHandle(
        Types.CreateProfileParams memory createProfileParams,
        string calldata handle
    ) external onlyOwner returns (uint256, uint256) {
        // Check if LensHubV1 already has a profile with this handle that was not migrated yet:
        bytes32 handleHash = keccak256(bytes(string.concat(handle, '.lens')));
        if (LensV2Migration(LENS_HUB).getProfileIdByHandleHash(handleHash) != 0) {
            revert ProfileAlreadyExists();
        }
        
        // ...
     }

Lines of code

https://github.com/code-423n4/2023-07-lens/blob/main/contracts/libraries/MetaTxLib.sol#L160-L170

Vulnerability details

Bug Description

The _hashActionModulesInitDatas() function is used to encode the actionModulesInitDatas parameter when verifying signatures in accordance to EIP-712:

MetaTxLib.sol#L160-L170

    function _hashActionModulesInitDatas(bytes[] memory actionModulesInitDatas) private pure returns (bytes32) {
        bytes32[] memory actionModulesInitDatasHashes = new bytes32[](actionModulesInitDatas.length);
        uint256 i;
        while (i < actionModulesInitDatas.length) {
            actionModulesInitDatasHashes[i] = keccak256(abi.encode(actionModulesInitDatas[i]));
            unchecked {
                ++i;
            }
        }
        return keccak256(abi.encodePacked(actionModulesInitDatasHashes));
    }

As seen from above, it calls abi.encode() on every bytes element in the array before encoding the entire array. However, according to EIP-712, bytes should be passed to keccak256 directly, without abi.encode():

The dynamic values bytes and string are encoded as a keccak256 hash of their contents.

When abi.encode() is called on each bytes element, the offset and length of the bytes object will also be included in the data that is passed to keccak256. For example:

bytes memory b = "AAAA";

// abi.encode(b) will give:
[00]: 0000000000000000000000000000000000000000000000000000000000000020 // offset
[32]: 0000000000000000000000000000000000000000000000000000000000000004 // length
[64]: 4141414100000000000000000000000000000000000000000000000000000000 // contents

Therefore, the following functions do not comply with EIP-712 as they use _hashActionModulesInitDatas() when verifying signatures:

• validatePostSignature()
• validateCommentSignature()
• validateQuoteSignature()

A correct example of encoding a bytes array can be seen in validateFollowSignature(), where each element in the bytes array is passed to keccak256 directly.

Impact

Contracts or dapps/backends that encode actionModulesInitDatas according to the rules specified in EIP-712 will end up with different signatures, causing any of the functions listed above to revert when called.

Recommended Mitigation

In _hashActionModulesInitDatas(), pass actionModulesInitDatas[i] directly to keccak256 instead of using abi.encode():

MetaTxLib.sol#L160-L170

    function _hashActionModulesInitDatas(bytes[] memory actionModulesInitDatas) private pure returns (bytes32) {
        bytes32[] memory actionModulesInitDatasHashes = new bytes32[](actionModulesInitDatas.length);
        uint256 i;
        while (i < actionModulesInitDatas.length) {
-           actionModulesInitDatasHashes[i] = keccak256(abi.encode(actionModulesInitDatas[i]));
+           actionModulesInitDatasHashes[i] = keccak256(actionModulesInitDatas[i]);
            unchecked {
                ++i;
            }
        }
        return keccak256(abi.encodePacked(actionModulesInitDatasHashes));
    }

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-07-lens/blob/main/contracts/libraries/MetaTxLib.sol#L143-L153 https://github.com/code-423n4/2023-07-lens/blob/main/contracts/libraries/MetaTxLib.sol#L100-L109

Vulnerability details

Bug Description

According to the EIP-712 specification, arrays are encoded by concatenating its elements and passing the result to keccak256:

The array values are encoded as the keccak256 hash of the concatenated encodeData of their contents (i.e. the encoding of SomeType[5] is identical to that of a struct containing five members of type SomeType).

An example of a correct implementation can be seen in validateUnfollowSignature(), where the idsOfProfilesToUnfollow array is passed to keccak256 after using abi.encodePacked():

MetaTxLib.sol#L368

                        keccak256(abi.encodePacked(idsOfProfilesToUnfollow)),

However, some other functions in MetaTxLib encode arrays differently, which differs from the EIP-712 specification.

Some functions do not encode the array at all, and pass the array to abi.encode() alongside other arguments in its struct. For example, in validatePostSignature(), the postParams.actionModules array is not encoded by itself:

MetaTxLib.sol#L143-L153

                    abi.encode(
                        Typehash.POST,
                        postParams.profileId,
                        keccak256(bytes(postParams.contentURI)),
                        postParams.actionModules,
                        _hashActionModulesInitDatas(postParams.actionModulesInitDatas),
                        postParams.referenceModule,
                        keccak256(postParams.referenceModuleInitData),
                        _getAndIncrementNonce(signature.signer),
                        signature.deadline
                    )

Other instances of this include:

• mirrorParams.referrerProfileIds and mirrorParams.referrerPubIds in validateMirrorSignature()
• publicationActionParams.referrerProfileIds and publicationActionParams.referrerPubIds in validateActSignature()
• All functions that use _abiEncode(), namely validateCommentSignature() and validateQuoteSignature()

Secondly, the validateChangeDelegatedExecutorsConfigSignature() function encodes the delegatedExecutors and approvals arrays using abi.encodePacked(), but do not pass it to keccak256:

MetaTxLib.sol#L100-L109

                    abi.encode(
                        Typehash.CHANGE_DELEGATED_EXECUTORS_CONFIG,
                        delegatorProfileId,
                        abi.encodePacked(delegatedExecutors),
                        abi.encodePacked(approvals),
                        configNumber,
                        switchToGivenConfig,
                        nonce,
                        deadline
                    )

Impact

As arrays are encoded incorrectly, the signature verification in the functions listed above is not EIP-712 compliant.

Contracts or dapps/backends that encode arrays according to the rules specified in EIP-712 will end up with different signatures, causing any of the functions listed above to revert when called.

Moreover, the inconsistent encoding of arrays might be extremely confusing to developers who wish to use these functions to implement meta-transactions.

Recommended Mitigation

Consider encoding arrays correctly in the functions listed above, which can be achieved by calling abi.encodePacked() on the array and passing its results to keccak256.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-07-lens/blob/main/contracts/libraries/constants/Typehash.sol#L33 https://github.com/code-423n4/2023-07-lens/blob/main/contracts/libraries/constants/Typehash.sol#L23 https://github.com/code-423n4/2023-07-lens/blob/main/contracts/libraries/constants/Typehash.sol#L25 https://github.com/code-423n4/2023-07-lens/blob/main/contracts/libraries/constants/Typehash.sol#L15 https://github.com/code-423n4/2023-07-lens/blob/main/contracts/libraries/constants/Typehash.sol#L21

Vulnerability details

Bug Description

In LensHub.sol, the second parameter of setProfileMetadataURIWithSig() is declared as metadataURI:

LensHub.sol#L119-L123

    function setProfileMetadataURIWithSig(
        uint256 profileId,
        string calldata metadataURI,
        Types.EIP712Signature calldata signature
    ) external override whenNotPaused onlyProfileOwnerOrDelegatedExecutor(signature.signer, profileId) {

However, its EIP-712 typehash stores the parameter as metadata instead:

Typehash.sol#L33

bytes32 constant SET_PROFILE_METADATA_URI = keccak256('SetProfileMetadataURI(uint256 profileId,string metadata,uint256 nonce,uint256 deadline)');

---

The PostParams struct (which is used for postWithSig()) has address[] actionModules and bytes[] actionModulesInitDatas as its third and fourth fields:

Types.sol#L178-L185

    struct PostParams {
        uint256 profileId;
        string contentURI;
        address[] actionModules;
        bytes[] actionModulesInitDatas;
        address referenceModule;
        bytes referenceModuleInitData;
    }

However, the third and fourth fields in its typehash are declared as address collectModule and bytes collectModuleInitData instead:

Typehash.sol#L23

bytes32 constant POST = keccak256('Post(uint256 profileId,string contentURI,address collectModule,bytes collectModuleInitData,address referenceModule,bytes referenceModuleInitData,uint256 nonce,uint256 deadline)');

This occurs for the commentWithSig() and quoteWithSig() functions as well:

Typehash.sol#L25

bytes32 constant QUOTE = keccak256('Quote(uint256 profileId,string contentURI,uint256 pointedProfileId,uint256 pointedPubId,uint256[] referrerProfileIds,uint256[] referrerPubIds,bytes referenceModuleData,address collectModule,bytes collectModuleInitData,address referenceModule,bytes referenceModuleInitData,uint256 nonce,uint256 deadline)');

Typehash.sol#L15

bytes32 constant COMMENT = keccak256('Comment(uint256 profileId,string contentURI,uint256 pointedProfileId,uint256 pointedPubId,uint256[] referrerProfileIds,uint256[] referrerPubIds,bytes referenceModuleData,address collectModule,bytes collectModuleInitData,address referenceModule,bytes referenceModuleInitData,uint256 nonce,uint256 deadline)');

---

The fourth and fifth fields in the MirrorParams struct (which is used for mirrorWithSig()) are declared as referrerProfileIds and referrerPubIds:

Types.sol#L282-L289

    struct MirrorParams {
        uint256 profileId;
        uint256 pointedProfileId;
        uint256 pointedPubId;
        uint256[] referrerProfileIds;
        uint256[] referrerPubIds;
        bytes referenceModuleData;
    }

However, its EIP-712 typehash declares these fields as referrerProfileId and referrerPubId instead:

Typehash.sol#L21

bytes32 constant MIRROR = keccak256('Mirror(uint256 profileId,uint256 pointedProfileId,uint256 pointedPubId,uint256[] referrerProfileId,uint256[] referrerPubId,bytes referenceModuleData,uint256 nonce,uint256 deadline)');

Impact

Due to the use of incorrect typehashes, the signature verification in the functions listed above is not EIP-712 compliant.

Contracts or dapps/backends that use "correct" typehashes that match the parameters of these functions will end up generating different signatures, causing them to revert when called.

Recommended Mitigation

Amend the typehashes shown above to have matching parameters with their respective functions.

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-07-lens/blob/main/contracts/namespaces/LensHandles.sol#L139-L145

Vulnerability details

Bug Description

According to the README, when an address has token guardian enabled, approvals should not work for the tokens owned by that address:

Token Guardian: Protection mechanism for the tokens held by an address, which restricts transfers and approvals when enabled. See LIP-4 for more.

In LensHandles.sol, token guardian is enforced by the _hasTokenGuardianEnabled() check in the approve() function:

LensHandles.sol#L139-L145

    function approve(address to, uint256 tokenId) public override(IERC721, ERC721) {
        // We allow removing approvals even if the wallet has the token guardian enabled
        if (to != address(0) && _hasTokenGuardianEnabled(msg.sender)) {
            revert HandlesErrors.GuardianEnabled();
        }
        super.approve(to, tokenId);
    }

However, this check is inadequate as approved operators (addresses approved using setApprovalForAll() by the owner) are also allowed to call approve(). We can see this in Openzeppelin's ERC-721 implementation:

ERC721.sol#L116-L119

        require(
            _msgSender() == owner || isApprovedForAll(owner, _msgSender()),
            "ERC721: approve caller is not token owner or approved for all"
        );

As such, even if an owner has token guardian enabled, approvals can still be set for his tokens by other approved operators, leaving the owner's tokens vulnerable. For example:

• Alice sets Bob as an approved operator using setApprovalForAll().
• Alice enables token guardian using enableTokenGuardian().
• If Bob wants to set approvals for Alice's tokens, he can do so by:
  • Disabling his own token guardian using DANGER__disableTokenGuardian().
  • Calling approve() for Alice's tokens. This will still work, even though Alice has token guardian enabled.

Note that the approve() function in LensProfiles.sol also has the same vulnerability.

Impact

As token guardian protection in approve() does not account for approved operators, although an owner has token guardian enabled, approved operators will still be able to set approvals for his tokens.

Proof of Concept

The following Foundry test demonstrates the example above:

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

import 'forge-std/Test.sol';
import '../contracts/namespaces/LensHandles.sol';

contract TokenGuardian_POC is Test {
    LensHandles lensHandles;

    address ALICE;
    address BOB;
    uint256 tokenId;

    function setUp() public {
        // Setup LensHandles contract
        lensHandles = new LensHandles(address(this), address(0), 0);

        // Setup Alice and Bob addresses
        ALICE = makeAddr("Alice");
        BOB = makeAddr("Bob");

        // Mint "alice.lens" to Alice
        tokenId = lensHandles.mintHandle(ALICE, "alice");
    }
    
    function testCanApproveWhileTokenGuardianEnabled() public {
        // Alice disables tokenGuardian to set Bob as an approved operator
        vm.startPrank(ALICE);
        lensHandles.DANGER__disableTokenGuardian();
        lensHandles.setApprovalForAll(BOB, true);

        // Alice re-enables tokenGuardian
        lensHandles.enableTokenGuardian();
        vm.stopPrank();

        // Bob disables tokenGuardian for himself
        vm.startPrank(BOB);
        lensHandles.DANGER__disableTokenGuardian();

        // Alice still has tokenGuardian enabled
        assertEq(lensHandles.getTokenGuardianDisablingTimestamp(ALICE), 0);

        // However, Bob can still set approvals for Alice's handle
        lensHandles.approve(address(0x1337), tokenId);
        vm.stopPrank();
        assertEq(lensHandles.getApproved(tokenId), address(0x1337));
    }
}

Recommended Mitigation

Consider checking if the token's owner has token guardian enabled as well:

LensHandles.sol#L139-L145

    function approve(address to, uint256 tokenId) public override(IERC721, ERC721) {
        // We allow removing approvals even if the wallet has the token guardian enabled
-       if (to != address(0) && _hasTokenGuardianEnabled(msg.sender)) {
+       if (to != address(0) && (_hasTokenGuardianEnabled(msg.sender) || _hasTokenGuardianEnabled(_ownerOf(tokenId)))) {
            revert HandlesErrors.GuardianEnabled();
        }
        super.approve(to, tokenId);
    }

Assessed type

Access Control

Finding Summary

[L-01] Avoid directly minting follow NFTs to the profile owner in processBlock()

In FollowNFT.sol, whenever a follower is blocked by a profile and his followTokenId is unwrapped, processBlock() will mint the follow NFT to the follower's address:

FollowNFT.sol#L198-L203

        uint256 followTokenId = _followTokenIdByFollowerProfileId[followerProfileId];
        if (followTokenId != 0) {
            if (!_isFollowTokenWrapped(followTokenId)) {
                // Wrap it first, so the user stops following but does not lose the token when being blocked.
                _mint(IERC721(HUB).ownerOf(followerProfileId), followTokenId);
            }

However, if the profile owner's address isn't able to follow NFTs, such as profiles held in a secure wallet (e.g. hardware wallet or multisig), the minted follow NFT would become permanently stuck.

Recommendation

Consider allowing the follower to recover the NFT by himself by assigning profileIdAllowedToRecover to followerProfileId:

FollowNFT.sol#L198-L203

        uint256 followTokenId = _followTokenIdByFollowerProfileId[followerProfileId];
        if (followTokenId != 0) {
            if (!_isFollowTokenWrapped(followTokenId)) {
                // Wrap it first, so the user stops following but does not lose the token when being blocked.
-               _mint(IERC721(HUB).ownerOf(followerProfileId), followTokenId);
+               _followDataByFollowTokenId[followTokenId].profileIdAllowedToRecover = followerProfileId;
            }

[L-02] Regular approvals are not used in access controls in FollowNFT.sol

Throughout FollowNFT.sol, regular ERC-721 approvals are not used for access controls. For example, these are the access control checks when follow() is called with a wrapped follow token:

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

Apart from follow approvals, the function only allows the token owner or approved operators (address approved using setApprovalForAll()). If a user is approved by the token owner using approve() he will be unable to call follow() despite having control over the token.

This isn't a major issue as the approved address can sidestep this by doing the following:

• Transfer the follow NFT to himself.
• Call follow().
• Transfer the follow NFT back to the original address.

However, this could potentially be extremely inconvenient for the approved address.

Recommendation

Consider allowing addresses approved using approve() to call the following functions as well:

• follow()
• unfollow()
• removeFollower()
• approveFollow()

[L-03] Users have no way of revoking their signatures in LensHub

In the LensHub contract, users can provide signatures to allow relayers to call functions on their behalf in meta-transactions, such as followWithSig().

However, there is currently no way for the user to revoke their signatures. This could become a problem is the user needs to revoke their signatures (eg. action or reference module turns malicious, user doesn't want to use the module anymore).

Recommendation

In the Lenshub contract, implement a way for users to revoke their signatures. One way of achieving this is to add a function that increments the caller's nonce:

function incrementNonce() external {
    StorageLib.nonces()[msg.sender]++;
}

[L-04] Removing ERC721Enumerable functionality might break composability with other protocols

In LensBaseERC721.sol, the ERC721Enumerable extension is no longer supported. This can be seen in its supportsInterface() function, which no longer checks for type(IERC721Enumerable).interfaceId:

IERC721Enumerable.interfaceId

    function supportsInterface(bytes4 interfaceId) public view virtual override(ERC165, IERC165) returns (bool) {
        return
            interfaceId == type(IERC721).interfaceId ||
            interfaceId == type(IERC721Timestamped).interfaceId ||
            interfaceId == type(IERC721Burnable).interfaceId ||
            interfaceId == type(IERC721MetaTx).interfaceId ||
            interfaceId == type(IERC721Metadata).interfaceId ||
            super.supportsInterface(interfaceId);
    }

As LensBaseERC721 is inherited by LensProfiles, profiles will no longer support the ERC721Enumerable extension. This might break the functionality of other protocols that rely on ERC721Enumerable's functions.

Recommendation

Consider documenting/announcing that the V2 upgrade will remove ERC721Enumerable functionality from profiles.

[L-05] Delegated executor configs are not cleared on transfer

When profiles are transferred, its delegated executor config is not cleared. Instead, the function switches its config to a new config:

LensProfiles.sol#L165-L177

    function _beforeTokenTransfer(
        address from,
        address to,
        uint256 tokenId
    ) internal override whenNotPaused {
        if (from != address(0) && _hasTokenGuardianEnabled(from)) {
            // Cannot transfer profile if the guardian is enabled, except at minting time.
            revert Errors.GuardianEnabled();
        }
        // Switches to new fresh delegated executors configuration (except on minting, as it already has a fresh setup).
        if (from != address(0)) {
            ProfileLib.switchToNewFreshDelegatedExecutorsConfig(tokenId);
        }

This could potentially be dangerous as users are able to switch back to previous configs using changeDelegatedExecutorsConfig(). If a previous owner had added himself to previous configs, switching back to a previous config might potentially give the previous owner the ability to steal the profile, or execute malicious functions as a delegated executor.

Recommendation

Consider warning users about the danger of switching to previous configs in the documentation.

[L-06] act() in ActionLib.sol doesn't check if the target publication is a mirror

According to the Referral System Rules, mirrors cannot be a target publication. However, the act() function does not ensure that the target publication is not a mirror.

This is currently unexploitable as mirrors cannot be initialized with action modules, therefore the _isActionEnabled check will always revert when called with a mirror. However, this could potentially become exploitable if an attacker finds a way to corrupt the enabledActionModulesBitmap of a mirror publication.

Recommendation

Consider validating that the target publication is not a mirror in act().

[L-07] LensHub contract cannot be unpaused if emergency admin and governance is the same address

In GovernanceLib.sol, the setState() function is used by both the emergency admin and governance to change the state of the contract:

GovernanceLib.sol#L50-L60

    function setState(Types.ProtocolState newState) external {
        // NOTE: This does not follow the CEI-pattern, but there is no interaction and this allows to abstract `_setState` logic.
        Types.ProtocolState prevState = _setState(newState);
        // If the sender is the emergency admin, prevent them from reducing restrictions.
        if (msg.sender == StorageLib.getEmergencyAdmin()) {
            if (newState <= prevState) {
                revert Errors.EmergencyAdminCanOnlyPauseFurther();
            }
        } else if (msg.sender != StorageLib.getGovernance()) {
            revert Errors.NotGovernanceOrEmergencyAdmin();
        }

As seen from above, the emergency admin can only pause further, whereas governance can set any state.

However, if emergency admin and governance happens to be the same address, msg.sender == StorageLib.getEmergencyAdmin() will always be true, which only allows the system to be paused further. This could become a problem if Lens Protocol decides to set both to the same address.

Recommendation

Consider making the following change to the if-statement:

GovernanceLib.sol#L50-L60

    function setState(Types.ProtocolState newState) external {
        // NOTE: This does not follow the CEI-pattern, but there is no interaction and this allows to abstract `_setState` logic.
        Types.ProtocolState prevState = _setState(newState);
        // If the sender is the emergency admin, prevent them from reducing restrictions.
-       if (msg.sender == StorageLib.getEmergencyAdmin()) {
+       if (msg.sender != StorageLib.getGovernance() && msg.sender == StorageLib.getEmergencyAdmin()) {
-           if (newState <= prevState) {
-               revert Errors.EmergencyAdminCanOnlyPauseFurther();
-           }
-       } else if (msg.sender != StorageLib.getGovernance()) {
-           revert Errors.NotGovernanceOrEmergencyAdmin();
-       }

[L-08] Only 255 action modules can ever be whitelisted

When an action module is whitelisted, incrementMaxActionModuleIdUsed() is called, which increments _maxActionModuleIdUsed and checks that it does not exceed MAX_ACTION_MODULE_ID_SUPPORTED:

StorageLib.sol#L165-L175

    function incrementMaxActionModuleIdUsed() internal returns (uint256) {
        uint256 incrementedId;
        assembly {
            incrementedId := add(sload(MAX_ACTION_MODULE_ID_USED_SLOT), 1)
            sstore(MAX_ACTION_MODULE_ID_USED_SLOT, incrementedId)
        }
        if (incrementedId > MAX_ACTION_MODULE_ID_SUPPORTED) {
            revert Errors.MaxActionModuleIdReached();
        }
        return incrementedId;
    }

MAX_ACTION_MODULE_ID_SUPPORTED is currently declared as 255:

StorageLib.sol#L47

uint256 constant MAX_ACTION_MODULE_ID_SUPPORTED = 255;

This becomes an issue as _maxActionModuleIdUsed does not changed when action modules are un-whitelisted, which means that action module IDs cannot be reused:

GovernanceLib.sol#L105-L107

            // The action module with the given address was already whitelisted before, it has an ID already assigned.
            StorageLib.actionModuleWhitelistData()[actionModule].isWhitelisted = whitelist;
            id = actionModuleWhitelistData.id;

This means that only 255 action modules can ever be whitelisted, and governance will not be able to whitelist action modules forever after this limit is exceeded.

[L-09] Setting transactionExecutor to msg.sender in createProfile() might limit functionality

When createProfile() is called, the profile's follow module is initialized with msg.sender as its transactionExecutor:

ProfileLib.sol#L56-L61

            followModuleReturnData = _initFollowModule({
                profileId: profileId,
                transactionExecutor: msg.sender,
                followModule: createProfileParams.followModule,
                followModuleInitData: createProfileParams.followModuleInitData
            });

This could be extremely limiting if the follow module uses transactionExecutor during its initialization.

For example, consider a follow module that needs to be initialized with a certain amount of tokens:

• If these tokens are transferred from transactionExecutor, the profile creator will bear the cost of the initialization.
• If the profile's owner is meant to provide funds, he will have to transfer the tokens to the profile creator, which creates risk as he has to trust that the profile creator won't rug.

Recommendation

Consider setting transactionExecutor to createProfileParams.to instead, which is the owner of the newly created profile:

ProfileLib.sol#L56-L61

            followModuleReturnData = _initFollowModule({
                profileId: profileId,
-               transactionExecutor: msg.sender,
+               transactionExecutor: createProfileParams.to,
                followModule: createProfileParams.followModule,
                followModuleInitData: createProfileParams.followModuleInitData
            });

[L-10] Un-migratable handles might exist in Lens Protocol V1

In LensHandles.sol, the _validateLocalNameMigration() function is used to validate migrated handles:

LensHandles.sol#L210-L223

        bytes1 firstByte = localNameAsBytes[0];
        if (firstByte == '-' || firstByte == '_') {
            revert HandlesErrors.HandleFirstCharInvalid();
        }

        uint256 i;
        while (i < localNameLength) {
            if (!_isAlphaNumeric(localNameAsBytes[i]) && localNameAsBytes[i] != '-' && localNameAsBytes[i] != '_') {
                revert HandlesErrors.HandleContainsInvalidCharacters();
            }
            unchecked {
                ++i;
            }
        }

As seen from above, handles can only be migrated if:

• Its first byte is alphanumeric.
• Only contains alphanumeric characters, '-' or "_".

Additionally, the _migrateProfile() function in MigrationLib.sol removes the last 5 bytes of each handle:

MigrationLib.sol#L77-L80

                assembly {
                    let handle_length := mload(handle)
                    mstore(handle, sub(handle_length, DOT_LENS_SUFFIX_LENGTH)) // Cut 5 chars (.lens) from the end
                }

This means that any V1 handle that does not contan ".lens" and is shorter than 5 bytes cannot be migrated.

However, the rules mentioned above are not strictly enforced in Lens Protocol V1:

PublishingLogic.sol#L391

    function _validateHandle(string calldata handle) private pure {
        bytes memory byteHandle = bytes(handle);
        if (byteHandle.length == 0 || byteHandle.length > Constants.MAX_HANDLE_LENGTH)
            revert Errors.HandleLengthInvalid();

        uint256 byteHandleLength = byteHandle.length;
        for (uint256 i = 0; i < byteHandleLength; ) {
            if (
                (byteHandle[i] < '0' ||
                    byteHandle[i] > 'z' ||
                    (byteHandle[i] > '9' && byteHandle[i] < 'a')) &&
                byteHandle[i] != '.' &&
                byteHandle[i] != '-' &&
                byteHandle[i] != '_'
            ) revert Errors.HandleContainsInvalidCharacters();
            unchecked {
                ++i;
            }
        }
    }

As seen from above, V1 handles can also contain ".", and its first byte is not only restricted to alphanumeric characters. Additionally, handles do not have to end with the ".lens" suffix. This means that it might be possible to create a handle that cannot be migrated after the V2 upgrade.

Recommendation

Ensure all profile handles obey the following rules before the V2 upgrade:

• First byte is alphanumeric.
• Only contains alphanumeric characters, '-' or "_".
• Ends with the ".lens" suffix.

[L-11] Avoid minting handles that have a tokenId of 0 in mintHandle()

When a handle is minted, its tokenId is computed as the keccak256 hash of the handle name:

LensHandles.sol#L182-L184

    function getTokenId(string memory localName) public pure returns (uint256) {
        return uint256(keccak256(bytes(localName)));
    }

However, _mintHandle() does not ensure that the tokenId minted is not 0:

LensHandles.sol#L194-L200

    function _mintHandle(address to, string calldata localName) internal returns (uint256) {
        uint256 tokenId = getTokenId(localName);
        _mint(to, tokenId);
        _localNames[tokenId] = localName;
        emit HandlesEvents.HandleMinted(localName, NAMESPACE, tokenId, to, block.timestamp);
        return tokenId;
    }

This makes it theoretically possible to mint a handle with a tokenId of 0, which is problematic as tokenId == 0 is treated as a default value in the protocol. For example, getDefaultHandle() in TokenHandleRegistry returns profile ID as 0 when the handle tokenId is 0:

TokenHandleRegistry.sol#L100-L102

        if (resolvedTokenId == 0 || !ILensHub(LENS_HUB).exists(resolvedTokenId)) {
            return 0;
        }

Recommendation

In _mintHandle(), consider checking that tokenId is non-zero to protect against the extremely unlikely event where a localName results in tokenId = 0:

LensHandles.sol#L194-L200

    function _mintHandle(address to, string calldata localName) internal returns (uint256) {
        uint256 tokenId = getTokenId(localName);
+       if (tokenId == 0) revert InvalidHandle();
        _mint(to, tokenId);
        _localNames[tokenId] = localName;
        emit HandlesEvents.HandleMinted(localName, NAMESPACE, tokenId, to, block.timestamp);
        return tokenId;
    }

[L-12] Relayer can choose amount of gas when calling function in meta-transactions

The LensHub contract supports the relaying of calls for several functions using a supplied signature. For example, users can provide a signature alongside the usual parameters in setProfileMetadataURIWithSig() for a relayer to call the function on his behalf:

LensHub.sol#L119-L123

    function setProfileMetadataURIWithSig(
        uint256 profileId,
        string calldata metadataURI,
        Types.EIP712Signature calldata signature
    ) external override whenNotPaused onlyProfileOwnerOrDelegatedExecutor(signature.signer, profileId) {

However, the parameters above do not include a gas parameter, which means the relayer can specify any gas amount.

If the provided gas amount is insufficient, the entire transaction will revert. However, if the function exhibits different behaviour depending on the supplied gas (which might occur in modules), a relayer could potentially influence the outcome of the call by manipulating the gas amount.

Recommendation

For all functions used for meta-transactions, consider adding a gas parameter that allows users to specify the gas amount the function should be called with. This gas parameter should be included in the hash digest when verifying the user's signature.

[N-01] approveFollow() should allow followerProfileId = 0 to cancel follow approvals

In FollowNFT.sol, approveFollow() only allows profile IDs that exist to be set as followerProfileId:

FollowNFT.sol#L141-L153

    function approveFollow(uint256 followerProfileId, uint256 followTokenId) external override {
        if (!IERC721Timestamped(HUB).exists(followerProfileId)) {
            revert Errors.TokenDoesNotExist();
        }

As seen from above, the function does not allow followerProfileId to be 0. This forces users to set followerProfileId to their own address if they wish to cancel a follow approval, which could pose problems.

Recommendation

Allow approveFollow() to be called withfollowerProfileId = 0 as profiles will never have the ID 0:

FollowNFT.sol#L141-L153

    function approveFollow(uint256 followerProfileId, uint256 followTokenId) external override {
-       if (!IERC721Timestamped(HUB).exists(followerProfileId)) {
+       if (followerProfileId != 0 && !IERC721Timestamped(HUB).exists(followerProfileId)) {
            revert Errors.TokenDoesNotExist();
        }

[N-02] Redundant check in _isApprovedOrOwner() can be removed

In LensBaseERC721.sol, the _isApprovedOrOwner() functions contains an if-statement that checks if tokenId exists (_tokenData[tokenId].owner != 0):

LensBaseERC721.sol#L333-L339

    function _isApprovedOrOwner(address spender, uint256 tokenId) internal view virtual returns (bool) {
        if (!_exists(tokenId)) {
            revert Errors.TokenDoesNotExist();
        }
        address owner = ownerOf(tokenId);
        return (spender == owner || getApproved(tokenId) == spender || isApprovedForAll(owner, spender));
    }

However, this check is redundant as ownerOf also contains the same check:

LensBaseERC721.sol#L116-L122

    function ownerOf(uint256 tokenId) public view virtual override returns (address) {
        address owner = _tokenData[tokenId].owner;
        if (owner == address(0)) {
            revert Errors.TokenDoesNotExist();
        }
        return owner;
    }

Recommendation

Consider removing the if statement from _isApprovedOrOwner():

LensBaseERC721.sol#L333-L339

    function _isApprovedOrOwner(address spender, uint256 tokenId) internal view virtual returns (bool) {
-       if (!_exists(tokenId)) {
-           revert Errors.TokenDoesNotExist();
-       }
        address owner = ownerOf(tokenId);
        return (spender == owner || getApproved(tokenId) == spender || isApprovedForAll(owner, spender));
    }

[N-03] whenNotPaused modifier is redundant for burn() in LensProfiles.sol

In LensProfiles.sol, the burn() function has the whenNotPaused modifier:

LensProfiles.sol#L93-L96

    function burn(uint256 tokenId)
        public
        override(LensBaseERC721, IERC721Burnable)
        whenNotPaused

However, the modifier is redundant as the _beforeTokenTransfer hook also has the whenNotPaused modifier:

LensProfiles.sol#L165-L169

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

Recommendation

Consider removing the whenNotPaused modifier from burn():

LensProfiles.sol#L93-L96

    function burn(uint256 tokenId)
        public
        override(LensBaseERC721, IERC721Burnable)
-       whenNotPaused

[N-04] unfollow() should be allowed for burnt profiles

In the LensHub contract, unfollow() will revert if the profile to unfollow is burnt, due to the following check:

FollowLib.sol#L54-L62

    function unfollow(
        uint256 unfollowerProfileId,
        address transactionExecutor,
        uint256[] calldata idsOfProfilesToUnfollow
    ) external {
        uint256 i;
        while (i < idsOfProfilesToUnfollow.length) {
            uint256 idOfProfileToUnfollow = idsOfProfilesToUnfollow[i];
            ValidationLib.validateProfileExists(idOfProfileToUnfollow); // auditor: This check

However, this implementation seems incorrect as users should be able to unfollow burnt profiles.

This issue can be sidestepped by calling removeFollower() or burn() in the burnt profile's FollowNFT contract, but could be extremely inconvenient as users will have to first wrap their follow token.

Recommendation

Consider removing the validateProfileExists() check in unfollow():

FollowLib.sol#L54-L62

    function unfollow(
        uint256 unfollowerProfileId,
        address transactionExecutor,
        uint256[] calldata idsOfProfilesToUnfollow
    ) external {
        uint256 i;
        while (i < idsOfProfilesToUnfollow.length) {
            uint256 idOfProfileToUnfollow = idsOfProfilesToUnfollow[i];
-           ValidationLib.validateProfileExists(idOfProfileToUnfollow); // auditor: This check

This is possible as the followNFT addrress of non-existent profiles (profiles that are not minted yet) is 0, which causes the function to revert later on during execution.