code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-07-lens/blob/main/contracts/libraries/ActionLib.sol#L28-L38 https://github.com/code-423n4/2023-07-lens/blob/main/contracts/libraries/PublicationLib.sol#L318-L321

Vulnerability details

Bug Description

In the LensHub contract, action and reference modules can be whitelisted/un-whitelisted by governance using whitelistActionModule() and whitelistReferenceModule() respectively.

When publications are acted on using the act() function, ActionLib validates that the chosen module was added to the publication being acted on:

ActionLib.sol#L28-L38

        address actionModuleAddress = publicationActionParams.actionModuleAddress;
        uint256 actionModuleId = StorageLib.actionModuleWhitelistData()[actionModuleAddress].id;

        if (!_isActionEnabled(_actedOnPublication, actionModuleId)) {
            // This will also revert for:
            //   - Non-existent action modules
            //   - Non-existent publications
            //   - Legacy V1 publications
            // Because the storage will be empty.
            revert Errors.ActionNotAllowed();
        }

However, the function does not check if StorageLib.actionModuleWhitelistData()[actionModuleAddress].isWhitelisted is true, which means that there is no check if the module is currently whitelisted. Therefore, if a module was previously whitelisted but subsequently removed from the whitelist by governance, it will still remain executable.

Similarly, when publications with referrals are created (comment, mirror or quote), the following functions will call the reference module without validating that it is currently whitelisted:

Therefore, as long as the pointed publication was initialized with a reference module, it can still be used for referrals even after being un-whitelisted.

Impact

Action and reference modules that have been removed from the whitelist by governance will still remain executable after being un-whitelisted.

This becomes an issue if governance ever needs to prevent users from executing a certain module (eg. module has a bug or turns malicious), as they have no way of doing so.

Proof of Concept

The Foundry code below contains two tests:

testActionModuleStillExecutableWhenUnwhitelisted() demonstrates how action modules can still be executed through act() after being removed from the whitelist using whitelistActionModule().
testReferenceModuleStillExecutableWhenUnwhitelisted() shows that reference modules can still be used after being un-whitelisted using whitelistReferenceModule().

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

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

contract PublicationAction_POC is BaseTest {
    ActionModule actionModule;
    ReferenceModule referenceModule;

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

        // Deploy and whitelist action module
        vm.startPrank(governance);
        actionModule = new ActionModule();
        hub.whitelistActionModule(address(actionModule), true);

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

        // Create post with action and reference module
        Types.PostParams memory postParams = _getDefaultPostParams();
        postParams.actionModules = _toAddressArray(address(actionModule));
        postParams.actionModulesInitDatas = new bytes[](1);
        postParams.referenceModule = address(referenceModule);

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

    function testActionModuleStillExecutableWhenUnwhitelisted() public {
        // Action module removed from the whitelist
        vm.prank(governance);
        hub.whitelistActionModule(address(actionModule), false);

        Types.ActionModuleWhitelistData memory actionModuleData = hub.getActionModuleWhitelistData(address(actionModule));
        assertFalse(actionModuleData.isWhitelisted);

        // Action module hasn't been executed before
        assertFalse(actionModule.processPublicationActionExecuted());

        // However, action module can still be executed
        Types.PublicationActionParams memory actionParams = _getDefaultPublicationActionParams();
        actionParams.actionModuleAddress = address(actionModule);
        actionParams.actionModuleData = "";

        vm.prank(defaultAccount.owner);
        hub.act(actionParams);

        // Action module was executed
        assertTrue(actionModule.processPublicationActionExecuted());
    }

    function testReferenceModuleStillExecutableWhenUnwhitelisted() public {
        // Reference module removed from the whitelist
        vm.prank(governance);
        hub.whitelistReferenceModule(address(referenceModule), false);
        assertFalse(hub.isReferenceModuleWhitelisted(address(referenceModule)));

        // Reference module hasn't been executed before
        assertFalse(referenceModule.processMirrorExecuted());

        // However, reference module can still be executed
        vm.prank(defaultAccount.owner);
        hub.mirror(_getDefaultMirrorParams());

        // Reference module was executed
        assertTrue(referenceModule.processMirrorExecuted());
    }
}

contract ActionModule {
    bool public processPublicationActionExecuted;

    function initializePublicationAction(
        uint256, uint256, address, bytes calldata
    ) external pure returns (bytes memory) {}

    function processPublicationAction(
        Types.ProcessActionParams calldata
    ) external returns (bytes memory) {
        processPublicationActionExecuted = true;
    }
}

contract ReferenceModule {
    bool public processMirrorExecuted;

    function initializeReferenceModule(
        uint256, uint256, address, bytes calldata
    ) external pure returns (bytes memory) {}

    function processMirror(Types.ProcessMirrorParams calldata) external returns (bytes memory) {
        processMirrorExecuted = true;
    }
}

Recommended Mitigation

Consider ensuring that the action/reference module is whitelisted before executing it:

ActionLib.sol#L28-L38

        address actionModuleAddress = publicationActionParams.actionModuleAddress;
-       uint256 actionModuleId = StorageLib.actionModuleWhitelistData()[actionModuleAddress].id;
+       Types.ActionModuleWhitelistData memory actionModuleData = StorageLib.actionModuleWhitelistData()[actionModuleAddress];

-       if (!_isActionEnabled(_actedOnPublication, actionModuleId)) {
+       if (!_isActionEnabled(_actedOnPublication, actionModuleData.id) || !actionModuleData.isWhitelisted) {
            // This will also revert for:
            //   - Non-existent action modules
            //   - Non-existent publications
            //   - Legacy V1 publications
            // Because the storage will be empty.
            revert Errors.ActionNotAllowed();
        }

PublicationLib.sol#L318-L321

        address refModule = StorageLib
            .getPublication(commentParams.pointedProfileId, commentParams.pointedPubId)
            .referenceModule;
+       bool whitelisted = StorageLib.referenceModuleWhitelisted()[refModule];
-       if (refModule != address(0)) {
+       if (refModule != address(0) || !whitelisted) {

Assessed type

Invalid Validation

donosonaumczuk commented 1 year ago

We were aware of this, we think for now the whitelist it's just to prevent creating new publications with the modules, but we allow to keep executing old publications with their old modules as a "backwards compatibility" thing.

We would dispute validity, but given we didn't add this as known issue, we can accept it but as QA/Low severity.