code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-07-lens/blob/main/contracts/libraries/FollowLib.sol#L35-L37 https://github.com/code-423n4/2023-07-lens/blob/main/contracts/FollowNFT.sol#L480-L520

Vulnerability details

Impact

Users are not supposed to be able to self-follow on Lens v2, but they are able to bypass the restriction. This can also affect modules or newer functionalities that count on this behaviour.

Migration is an Area of specific concern for the devs, and this can easily be prevented with a simple check.

This can't be undone without any upgrade.

Proof of Concept

FollowLib::follow() has a specific restriction to revert when a user tries to self-follow on Lens v2:

    if (followerProfileId == idsOfProfilesToFollow[i]) {
        revert Errors.SelfFollow();
    }

FollowLib.sol#L35-L37

However, users that own a follow NFT from V1 can execute FollowNFT::tryMigrate() to self-follow on V2, as there is no restriction to prevent it. A test proving it can be found on the next section.

FollowNFT.sol#L480-L520

Coded POC

Add this test to test/migrations/Migrations.t.sol and run TESTING_FORK=mainnet POLYGON_RPC_URL="https://polygon.llamarpc.com" forge test --mt "testSelfFollow".

Note: In case of a memory allocation error during the Forge test, please comment these lines. They are not used for the current test.

    function testSelfFollow() public onlyFork {
        uint256 selfFollowProfileId = 3659; // juancito.lens
        uint256 selfFollowTokenId = 42;     // juancito.lens follows juancito.lens on V1

        FollowNFT nft = FollowNFT(hub.getProfile(selfFollowProfileId).followNFT);
        address user = nft.ownerOf(selfFollowTokenId); // Owner of juancito.lens

        // 1. Migrate the self-follow
        uint256[] memory selfFollowProfileIdArray = new uint256[](1);
        uint256[] memory selfFollowTokenIdArray = new uint256[](1);

        selfFollowProfileIdArray[0] = selfFollowProfileId; // 3659
        selfFollowTokenIdArray[0] = selfFollowTokenId;     // 42

        hub.batchMigrateFollows(selfFollowProfileIdArray, selfFollowProfileIdArray, selfFollowTokenIdArray);

        // 2. The user is self-following on V2
        assertTrue(nft.isFollowing(selfFollowProfileId));
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Add the following validation to FollowNFT::tryMigrate():

+    if (followerProfileId == _followedProfileId) {
+        return 0;
+    }

Assessed type

Invalid Validation

c4-pre-sort commented 1 year ago

141345 marked the issue as primary issue

vicnaum commented 1 year ago

This seems like a sub-case of this: https://github.com/code-423n4/2023-07-lens-findings/issues/112 (But the mitigation is different for this case)

c4-sponsor commented 1 year ago

vicnaum marked the issue as sponsor confirmed

Picodes commented 1 year ago

The impact is the same but the issue is it seems different, as the mitigation suggested by #112 wouldn't prevent this from happening

c4-judge commented 1 year ago

Picodes marked the issue as satisfactory