code423n4 commented 1 year ago

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

donosonaumczuk commented 1 year ago

We disagree with the validity of this issue.

Transactions will have a nonce (even when meta-txs), so each profile will already have a specific order for each of their publications, which means that the IDs of the publications will be asigned correctly to those profiles, whenever the transactions get confirmed.

What can happen, is that the re-org ends up executing the "act" transaction before the "post/quote/comment" that is being acted on is created, leading the "act" transaction to revert. It shouldn't be likely to occur.

For the described issue to happen, a really weird edge case needs to occur, a re-org that also includes a transaction replacement (override) for the "post/quote/comment". This is very unlikely and the harm caused is also not clear.

c4-sponsor commented 1 year ago

donosonaumczuk marked the issue as sponsor disputed

Picodes commented 1 year ago

Transactions will have a nonce (even when meta-txs), so each profile will already have a specific order for each of their publications, which means that the IDs of the publications will be asigned correctly to those profiles, whenever the transactions get confirmed.

As it may happen that multiple addresses have the right to post (for example delegated executors and owners) I think the described scenario is valid and it's possible to have multiple publications being reordered in a different order. So to me this finding is valid. Let me know if I am missing something!

c4-judge commented 1 year ago

Picodes marked the issue as satisfactory

donosonaumczuk commented 1 year ago

As it may happen that multiple addresses have the right to post (for example delegated executors and owners) I think the described scenario is valid

Yes, I did not consider that detail. What you have said is correct and then the issue is valid