code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-07-lens/blob/main/contracts/libraries/ProfileLib.sol#L100

Vulnerability details

Bug Description

In ProfileLib.sol, the setFollowModule() function calls _initFollowModule() with msg.sender as the transactionExecutor:

ProfileLib.sol#L100

            followModuleReturnData = _initFollowModule(profileId, msg.sender, followModule, followModuleInitData);

_initFollowModule() then initializes the follow module with transactionExecutor:

ProfileLib.sol#L110-L118

    function _initFollowModule(
        uint256 profileId,
        address transactionExecutor,
        address followModule,
        bytes memory followModuleInitData
    ) private returns (bytes memory) {
        ValidationLib.validateFollowModuleWhitelisted(followModule);
        return IFollowModule(followModule).initializeFollowModule(profileId, transactionExecutor, followModuleInitData);
    }

However, as setFollowModule() is called by setFollowModuleWithSig(), the implementation above is incorrect. When setFollowModuleWithSig() is called during a meta-transaction, the relayer's address will be passed to initializeFollowModule() as the transactionExecutor, instead of the signer.

This contradicts the contest's README, which states that transactionExecutor should be set to the signer in a meta-transaction:

Transaction Executor: The msg.sender in a regular transaction, the signer in a meta-transaction. Take into account that this can be either the owner of a profile, as well as one of its delegated executors.

Impact

When setFollowModuleWithSig() is called during a meta-transaction, the follow module will be initialized with the relayer as the transaction executor instead of the signer.

This could become problematic for follow modules that use transactionExecutor during initialization (eg. a module that pays profiles to perform a follow, thus requiring the transaction executor to deposit an initial amount of tokens).

Proof of Concept

The Foundry test below demonstrates how setFollowModuleWithSig() incorrectly initializes follow modules with the relayer as the transaction executor. It can be run with the following command:

forge test --match-test testSetFollowModuleWithSigWrongTransactionExecutor -vvv

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

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

contract MockFollowModule {
    address public initializedAddress;

    function initializeFollowModule(
        uint256 /* profileId */,
        address transactionExecutor,
        bytes calldata /* data */
    ) external returns (bytes memory) {
        initializedAddress = transactionExecutor;
    }
}

contract SetFollowModuleWithSig_POC is BaseTest {
    MockFollowModule mockFollowModule;

    address RELAYER;

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

        // Deploy MockFollowModule
        mockFollowModule = new MockFollowModule();

        // Whitelist MockFollowModule
        vm.prank(governance);
        hub.whitelistFollowModule(address(mockFollowModule), true);

        // Setup relayer address
        RELAYER = makeAddr("Relayer");
    }

    function testSetFollowModuleWithSigWrongTransactionExecutor() public {
        // Profile owner generates signature to set follow module
        uint256 nonce = hub.nonces(defaultAccount.owner);
        uint256 deadline = type(uint256).max;
        bytes32 digest = _getSetFollowModuleTypedDataHash(
            defaultAccount.profileId,
            address(mockFollowModule),
            "",
            nonce,
            deadline
        );

        Types.EIP712Signature memory signature = _getSigStruct({
            signer: defaultAccount.owner,
            pKey: defaultAccount.ownerPk,
            digest: digest,
            deadline: deadline
        });

        // Relayer calls setFollowModuleWithSig() with profile owner's signature
        vm.prank(RELAYER);        
        hub.setFollowModuleWithSig({
            profileId: defaultAccount.profileId,
            followModule: address(mockFollowModule),
            followModuleInitData: "",
            signature: signature
        });

        // transactionExecutor is set to relayer instead of profile owner
        assertEq(mockFollowModule.initializedAddress(), RELAYER);
    }
}

Recommended Mitigation

Consider adding a transactionExecutor parameter to setFollowModule() and passing it to _initFollowModule() instead of msg.sender:

ProfileLib.sol#L92-L101

    function setFollowModule(
        uint256 profileId,
+       address transactionExecutor,
        address followModule,
        bytes calldata followModuleInitData
    ) external {
        StorageLib.getProfile(profileId).followModule = followModule;
        bytes memory followModuleReturnData;
        if (followModule != address(0)) {
-           followModuleReturnData = _initFollowModule(profileId, msg.sender, followModule, followModuleInitData);
+           followModuleReturnData = _initFollowModule(profileId, transactionExecutor, followModule, followModuleInitData);
        }

In setFollowModuleWithSig(), pass signature.signer as the transactionExecutor:

LensHub.sol#L138-L146

    function setFollowModuleWithSig(
        uint256 profileId,
        address followModule,
        bytes calldata followModuleInitData,
        Types.EIP712Signature calldata signature
    ) external override whenNotPaused onlyProfileOwnerOrDelegatedExecutor(signature.signer, profileId) {
        MetaTxLib.validateSetFollowModuleSignature(signature, profileId, followModule, followModuleInitData);
-       ProfileLib.setFollowModule(profileId, followModule, followModuleInitData);
+       ProfileLib.setFollowModule(profileId, signature.signer, followModule, followModuleInitData);
    }

Assessed type

Context

donosonaumczuk commented 1 year ago

We accept the issue, we are not sure about its severity

c4-sponsor commented 1 year ago

donosonaumczuk marked the issue as disagree with severity

Picodes commented 1 year ago

This is somewhere between "function incorrect as to spec" and "the function of the protocol or its availability could be impacted". As the given example ("a module that pays profiles to perform a follow, thus requiring the transaction executor to deposit an initial amount of tokens") relies on a module who's dev team would have checked the doc and not the code, I think it's an instance of "function incorrect as to spec", so of Low severity.

c4-judge commented 1 year ago

Picodes changed the severity to QA (Quality Assurance)

MiloTruck commented 1 year ago

Hi @Picodes, I don't really understand why this issue isn't qualified for medium severity.

If you take a look at all the other meta-transaction related functions, such as postWithSig() or commentWithSig(), it becomes quite apparent that transactionExecutor is always meant to be the signer instead of the relayer.

This issue has pointed out an error in the code that causes transactionExecutor to be set to the wrong address in setFollowModule(), which contradicts all the other meta-transaction related functions in the protocol. Therefore, in my opinion, the function is working incorrectly/differently than intended due to an error, which would fall under "function of the protocol could be impacted".

I don't think it would be fair to consider this "function incorrect as to spec", since even if the contest's README didn't exist and there was no specification, this would still have been considered a bug.

Hope you could reconsider your judgement here, thanks!