Adversary can force any profile to follow a profile by sending followNFT to address that owns the profile and calling `LensV2Migration#batchMigrateFollows`

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-lens/blob/main/contracts/misc/LensV2Migration.sol#L37
https://github.com/code-423n4/2023-07-lens/blob/main/contracts/FollowNFT.sol#L502

Vulnerability details

Impact

Adversary that possesses a followNFT that was minted before the upgrade, can send it to a target address that owns a particular profile and then execute the LensV2Migration#batchMigrateFollows function. By providing the necessary inputs such as the profileId, idOfProfileFollowed, and the transferred tokenId, the adversary successfully enforces the target profile to follow the specified profile.

This introduces several potential attacks:

• An adversary can force any profile they wish to follow a particular profile.
• The adversary can forcefully change the tokenId that an already migrated user or post-upgrade minter is registered with.
• If a profile gets blocked, the user can refollow by accepting a pre-upgrade followNFT and then using the batchMigrateFollows function.
• A profile can follow itself by receiving a pre-upgrade followNFT and then using the batchMigrateFollows function."

Proof of Concept

The tryMigrate function does not consider the fact that any address will be forced to be the ownerOf a tokenId if it gets sent that token. This is common to all NFTs. Alice cannot stop Bob from sending her his NFTs.

For follow tokens that were minted before the upgrade, the batchMigrateFollows which downstream calls the tryMigrate function, is used to re-register the profileId that owns those tokens and grant them functionalities equivalent to those of tokens minted after the upgrade.

As an owner of a pre-upgrade followNFT, you can forcefully register ANY address with your tokenId. This can be achieved by simply sending your followNFT to an address that owns a specific profileId and then calling the LensV2Migration#batchMigrateFollows function, providing that profileId, idOfProfileFollowed, and the tokenId you just transferred.

Here are the checks that tryMigrate does:

// Migrated FollowNFTs should have `originalFollowTimestamp` set
if (_followDataByFollowTokenId[followTokenId].originalFollowTimestamp != 0) {
    return 0; // Already migrated
}

if (_followedProfileId != idOfProfileFollowed) {
    revert Errors.InvalidParameter();
}

if (!_exists(followTokenId)) {
    return 0; // Doesn't exist
}

address followTokenOwner = ownerOf(followTokenId);

// ProfileNFT and FollowNFT should be in the same account
if (followerProfileOwner != followTokenOwner) {
    return 0; // Not holding both Profile & Follow NFTs together
}

If the tokenId has not been used in migration before, and the adversary's input parameters are valid, and the tokenId exists, the first three checks will pass. The last check ensures that the owner of the followTokenId is the same as the followerProfileOwner. Since the adversary can control the ownerOf a followTokenId (by sending it to any address they wish) and can also provide any profile as an input parameter, this check will pass. Consequently, it would lead to the registration of that profile as a follower of any profile the adversary desires.

Tools Used

Manual Review

Recommended Mitigation Steps

I suggest allowing only the protocol or current owner of a followNFT to migrate his followNFT. Normal users should not be able to migrate another profile's followNFT unless with a signature.
This way, if a malicious user sends his followNFT to another address, he won't be able to tryMigrate because he is not the current owner of the token, while the token recipient can tryMigrate at will.

Assessed type

Invalid Validation

[vicnaum]

This is a valid issue and should be fixed before the upgrade. There are several similar issued in the duplicates section which we also confirm. This shouldn't be High because users assets are not at risk, it's rather a way of minting unexpected assets (and also all other duplicate issues are ranked as Medium).

[c4-sponsor]

vicnaum marked the issue as disagree with severity

[c4-judge]

Picodes changed the severity to 2 (Med Risk)

[c4-judge]

Picodes marked the issue as primary issue

[c4-judge]

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

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as not a duplicate

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

This previously downgraded issue has been upgraded by Picodes

[c4-judge]

This previously downgraded issue has been upgraded by Picodes

[c4-judge]

Picodes marked the issue as duplicate of #104

[MiloTruck]

Hi @Picodes,

This isn't my issue but wouldn't it be fairer towards the warden if it was split into three with partial rewards?

The finding here has grouped all findings related to tryMigrate() under one bug, which is the function has missing checks. As seen below, it has correctly identified the potential impacts mentioned in #40 and #106 in point 3 and 4:

• An adversary can force any profile they wish to follow a particular profile.
• The adversary can forcefully change the tokenId that an already migrated user or post-upgrade minter is registered with.
• If a profile gets blocked, the user can refollow by accepting a pre-upgrade followNFT and then using the batchMigrateFollows function.
• A profile can follow itself by receiving a pre-upgrade followNFT and then using the batchMigrateFollows function."

Therefore, I think it would be fair for it to be split into three and duplicated to #40 and #106 as well, perhaps with partial rewards due to the lack of explanation for each root cause and mitigation.

The alternative would be to group all three findings together and duplicate them under this issue, but I don't think that's correct given that their mitigations are different.

Also, impact 2 seems to be an overlap of impact 1 and #146 (fixing either of these would fix impact 2 as well), so I don't think it should be separated into an individual finding.

Thanks!

[0xJuancito]

Hi @Picodes. As the author of both #104 and #106, I would like to point out the difference in root and mitigations for both issues, as demonstrated on their respective Coded POCs on each issue + the Recommended Mitigation Steps, making them independent.

As for #40 and #146 they are also independent issues as well, with their own root and mitigations.

[Emedudu]

Hi @Picodes ,

This issue mentioned 4 different issues, as can be found under the impact section:

1. An adversary can force any profile they wish to follow a particular profile.==issue 104
2. The adversary can forcefully change the tokenId that an already migrated user or post-upgrade minter is registered with.(No other issue mentioned this)
3. If a profile gets blocked, the user can refollow by accepting a pre-upgrade followNFT and then using the batchMigrateFollows function.==issue 40
4. A profile can follow itself by receiving a pre-upgrade followNFT and then using the batchMigrateFollows function.==issue 106

Personally, I'm not sure of the best way to judge this to be fair to all the wardens.
If judges are allowed to split a finding, then this should probably be split into 4 findings.

• the first should be a dup of issue 104
• the second should be a separate finding
• the third should be a dup of issue 40
• the fourth should be a dup of issue 106

Thanks for your effort!

[vicnaum]

Me and @donosonaumczuk also agree with splitting this into 4 separate issues as proposed above

[Picodes]

So before the QA period, I had split this issue in 2: one dup of #104 and one dup of #106 (#189).

To me, this report is primarily a duplicate of #104 as the same scenario is described. Then, it seems that the warden correctly identified scenarios #40 and #106 but doesn't go into details and doesn't provide a mitigation.

I see 3 bugs:

• you can send and migrate someone without its consent: that is #104, #112 and covers scenario 2 of this report
• when migrating yourself you could self-follow: that is #106 and is evocated by this issue but no mitigation / proper scenario is described so I'll correct to partial-50
• you could bypass blocking: that is #40 and would be a different mitigation as well but is mentioned here. I'll create a duplicate of this report and mark it partial-50.

Then, for scenario 2 described here, it's another impact but not another issue in my opinion.

[c4-judge]

Picodes marked the issue as not a duplicate

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

This previously downgraded issue has been upgraded by Picodes

[c4-judge]

This previously downgraded issue has been upgraded by Picodes

[c4-judge]

Picodes marked the issue as duplicate of #104