QA Report

[code423n4]

Title: Duplicates in array Severity: Low /Med Risk (put here for you to decide)

ERC721Enumerable._addTokenToAllTokensEnumeration pushed (tokenId)

Title: Mult instead div in compares Severity: Low Risk

To improve algorithm precision instead using division in comparison use multiplication in the following scenario:

    Instead a < b / c use a * c < b. 

In all of the big and trusted contracts this rule is maintained (for example look at AAVE codebase).

    ModuleGlobals.sol, 109, if (newTreasuryFee >= BPS_MAX / 2) revert Errors.InitParamsInvalid();

Title: Missing fee parameter validation Severity: Low Risk

Some fee parameters of functions are not checked for invalid values. Validate the parameters:

    ModuleGlobals._setTreasuryFee (newTreasuryFee)
    ModuleGlobals.setTreasuryFee (newTreasuryFee)

Title: Does not validate the input fee parameter Severity: Low Risk

Some fee parameters of functions are not checked for invalid values. Validate the parameters:

    ModuleGlobals.constructor (treasuryFee)

Title: Not verified input Severity: Low Risk

external / public functions parameters should be validated to make sure the address is not 0. Otherwise if not given the right input it can mistakenly lead to loss of user funds.

    LensNFTBase.sol._validateRecoveredAddress expectedAddress
    FeeCollectModule.sol.processCollect collector
    CollectNFT.sol.mint to
    InteractionLogic.sol.collect collector

Title: Treasury may be address(0) Severity: Low Risk

Make sure the treasury is not address(0).

    ModuleGlobals.sol._setTreasury newTreasury
    ModuleGlobals.sol.setTreasury newTreasury
    ModuleGlobals.sol.constructor treasury

Title: Solidity compiler versions mismatch Severity: Low Risk

The project is compiled with different versions of solidity, which is not recommended due ti undefined behaviors as a result of it.

Title: Not verified owner Severity: Low Risk

owner param should be validated to make sure the owner address is not address(0).
Otherwise if not given the right input all only owner accessible functions will be unaccessible.

    ERC721Enumerable.sol.tokenOfOwnerByIndex owner
    LensNFTBase.sol.permitForAll owner
    ERC721Time.sol.isApprovedForAll owner
    FollowNFT.sol._writeSnapshot owner
    ApprovalFollowModule.sol.isApproved profileOwner
    ERC721Time.sol._setOperatorApproval owner
    ERC721Time.sol.balanceOf owner
    ApprovalFollowModule.sol.isApprovedArray profileOwner

Title: Init frontrun Severity: Low Risk

Most contracts use an init pattern (instead of a constructor) to initialize contract parameters. Unless these are enforced to be atomic with contact deployment via deployment script or factory contracts, they are susceptible to front-running race conditions where an attacker/griefer can front-run (cannot access control because admin roles are not initialized) to initially with their own (malicious) parameters upon detecting (if an event is emitted) which the contract deployer has to redeploy wasting gas and risking other transactions from interacting with the attacker-initialized contract.

Many init functions do not have an explicit event emission which makes monitoring such scenarios harder. All of them have re-init checks; while many are explicit some (those in auction contracts) have implicit reinit checks in initAccessControls() which is better if converted to an explicit check in the main init function itself. (details credit to: https://github.com/code-423n4/2021-09-sushimiso-findings/issues/64) The vulnerable initialization functions in the codebase are:

    LensHub.sol, initialize, 63
    MockLensHubV2BadRevision.sol, initialize, 28
    MockLensHubV2.sol, initialize, 29

Title: Two Steps Verification before Transferring Ownership Severity: Low Risk

The following contracts have a function that allows them an admin to change it to a different address. If the admin accidentally uses an invalid address for which they do not have the private key, then the system gets locked. It is important to have two steps admin change where the first is announcing a pending new admin and the new address should then claim its ownership. A similar issue was reported in a previous contest and was assigned a severity of medium: code-423n4/2021-06-realitycards-findings#105

    IModuleGlobals.sol
    ModuleGlobals.sol
    ILensHub.sol
    LensHub.sol

[Zer0dot]

1) Invalid, doing so makes no sense as BPS_MAX is a constant, and this would increase code size and gas.

2) Invalid? I don't understand-- they are validated in the internal function.

3) Invalid, again, validated in the internal function.

4) Invalid (at least to my knowledge):

1. The function reverts if the recovered address is the zero address, so this appears to be pointless to me.
2. Collector can never be zero because it's either msg.sender or a recovered address which also cannot be zero as mentioned above.
3. This is only ever called with the collector as the to address and thus follows the same logic as the above point.
4. Again, same logic as the above point.

5) Invalid, again again, validated in the internal function.

6) Invalid, although different files may specify different versions (not specified in the finding but afaik this is referring to the OZ forked ERC721 contracts) the project uses a single compiler.

7) Invalid? Doesn't really make sense to me. Some of these use the aforementioned _validateRecoveredAddress() and others are view functions that should not revert on zero address params. Furthermore, _writeSnapshot() is only ever called if the owner is not the zero address.

8) Invalid, this is why the upgradeable proxies include an upgradeToAndCall() function.

9) Invalid, this is a known pattern, it is assumed that governance/emergency admin roles will be timelocked and thus the 2-step migration process is unnecessary.

[0xleastwood]

I'm not a fan of when wardens use automated tooling to generate their gas and QA reports. As such, I'm marking this as invalid. Please be more considerate when you write your reports.

[Zer0dot]

I'm not a fan of when wardens use automated tooling to generate their gas and QA reports. As such, I'm marking this as invalid. Please be more considerate when you write your reports.

^^^^^^^^^^^^^^^^