QA Report

[code423n4]

8 Low issue identified as depicted below:

Zero address checks missing on setting Governance

1. While setting Governance via _setGovernance on LensHub.sol#L850-L854, there is no check to see if new Governance is address(0)
2. This can be disastrous as multiple functions like whitelisting profile/module etc will become unusable

Remediation: Change the _setGovernance like below:

function _setGovernance(address newGovernance) internal {
require(newGovernance!=address(0), "Incorrect Governance");
        address prevGovernance = _governance;
        _governance = newGovernance;
        emit Events.GovernanceSet(msg.sender, prevGovernance, newGovernance, block.timestamp);
    }

Old users are not impacted on changing Follow module

1. User A creates a new Profile using createProfile at LensHub.sol#L142 and has set address(0) as vars.followModule. A new profile gets created
2. User B follows User A and is issued a NFT from User A. Since followModule for User A was address(0) so no checks were performed and no fees was involved
3. User A decides to change the follow Module using setFollowModule at PublishingLogic.sol#L80 and makes this to FeeFollowModule.sol
4. Now if anyone wants to follow User A they will need to pay fees.
5. However Any old user who were already following User A (without any fees) will not be asked for this fees and would remain following User A for eg User B

Remediation: If followModule is changed using setFollowModule then new followModule should run on existing followers

Publication/Comment/Mirror can be collected for deleted profile

1. User A decides to burn his profile using burn function of LensHub.sol#L493. User A expects all data to be deleted
2. User B can still collect publication, comment, mirror belonging to User A even though User A does not exist in the system. User A will still receive payment from collect module
3. Correct checks are implemented for Follow NFT which checks whether profile exists or not

Remediation: Similar to follow function, implement checks in comment, publication, mirror creation to see if target profile exists/active

Stack too deep checks missing

1. It was observed that the prevention applied for Stack too deep error was missing in _processCollect function of LimitedTimedFeeCollectModule.sol#L159. Even though the same prevention is applied in _processCollectWithReferral function at LimitedTimedFeeCollectModule.sol#L180-L188

Remediation: Change all collect module treasury fees calculation to below (Like change LimitedTimedFeeCollectModule.sol#L159 to below):

address treasury;
        uint256 treasuryAmount;

        // Avoids stack too deep
        {
            uint16 treasuryFee;
            (treasury, treasuryFee) = _treasuryData();
            treasuryAmount = (amount * treasuryFee) / BPS_MAX;
        }

Incorrect implementation of _setTreasuryFee

1. It was observed that if newTreasuryFee >= BPS_MAX / 2 then the tresaury fees will be rejected. However this also rejects if newTreasuryFee = BPS_MAX / 2 (50%) which should not be the case

Remediation: Change the formula to:

if (newTreasuryFee > BPS_MAX / 2) revert Errors.InitParamsInvalid();

Multiple NFT can have same symbol

1. The NFT symbol is derived by taking bytes4 on user handle in both follow and collect function of InteractionLogic.sol#L60-L67 (and L117-L128)
2. Assume User A has handle aaaaa and User B has handle aaaab
3. In this case both user will have same NFT symbol as first 4 bytes in both cases is "aaaa" (only 5th byte differ)

bytes4 firstBytes = bytes4(bytes(handle)); which means only first 4 bytes are considered which is "aaaa" in both cases

string memory followNFTSymbol = string(
                    abi.encodePacked(firstBytes, Constants.FOLLOW_NFT_SYMBOL_SUFFIX)
                );

4. This could confuse users

Remediation:

1. Generate a separate logic to aware users that 2 profiles or publications can have same NFT symbol or Create a separate logic to calculate NFT symbol

Incorrect License specified

1. It was observed that LensMultiState.sol was marked with license SPDX-License-Identifier: MIT. Although all remaining contracts are licensed under SPDX-License-Identifier: AGPL-3.0-only

Remediation: Remove MIT license and add AGPL-3.0-only. Modify LensMultiState.sol#L1 to below:

// SPDX-License-Identifier: AGPL-3.0-only 

Contract State should not change in Library

1. Ideally Library should perform only those operation which does not impact Contract state.
2. But all functions at PublishingLogic.sol & InteractionLogic.sol are changing the contract state (creating new profile/post, minting nft etc)

Remediation: Move all state changing functions to a contract

[Zer0dot]

With respect to collecting from deleted profiles, we really leave this up to the UIs. The only "break" that occurs from deleted profiles and collecting is when attempting to collect from the mirror of a deleted profile, in which case user interfaces have the option to direct the transaction directly towards collecting the mirrored publication.

State changing functions in the library are a remediation to code size concerns, the MIT license is a leftover from the modified Pausable contract, good catch! I believe the rest to be invalid.