search

code-423n4 / 2023-07-lens-findings

0 stars 0 forks source link

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

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

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:

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 commented 1 year ago

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 commented 1 year ago

vicnaum marked the issue as disagree with severity

c4-judge commented 1 year ago

Picodes changed the severity to 2 (Med Risk)

c4-judge commented 1 year ago

Picodes marked the issue as primary issue

c4-judge commented 1 year ago

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

c4-judge commented 1 year ago

Picodes marked the issue as satisfactory

c4-judge commented 1 year ago

Picodes marked the issue as not a duplicate

c4-judge commented 1 year ago

Picodes changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

This previously downgraded issue has been upgraded by Picodes

c4-judge commented 1 year ago

This previously downgraded issue has been upgraded by Picodes

c4-judge commented 1 year ago

Picodes marked the issue as duplicate of #104

MiloTruck commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

Thanks for your effort!

vicnaum commented 1 year ago

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

Picodes commented 1 year ago

So before the QA period, I had split this issue in 2: one dup of #104 and one dup of #106 (https://github.com/code-423n4/2023-07-lens-findings/issues/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:

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

c4-judge commented 1 year ago

Picodes marked the issue as not a duplicate

c4-judge commented 1 year ago

Picodes changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

This previously downgraded issue has been upgraded by Picodes

c4-judge commented 1 year ago

This previously downgraded issue has been upgraded by Picodes

c4-judge commented 1 year ago

Picodes marked the issue as duplicate of #104