QA Report

[code423n4]

• The LensNFTBase inherits a functionality of the Context contract of OpenZeppelin, which is designed to be used with Ethereum Gas Station Network (GSN):

  contract LensNFTBase is ERC721Enumerable
  contract ERC721Enumerable is ERC721Time
  contract ERC721Time is Context

Context wants _msgSender() as a replacement for msg.sender. For regular transactions it returns msg.sender and for meta transactions it can be used to return the end user rather than the relayer. So I think LensNFTBase and all the contracts that inherit from it should use _msgSender(), not msg.sender, e.g.:

LensNFTBase

  function burn(uint256 tokenId) public virtual override {
      if (!_isApprovedOrOwner(msg.sender, tokenId)) revert Errors.NotOwnerOrApproved();
      ...

CollectNFT

    function mint(address to) external override {
        if (msg.sender != HUB) revert Errors.NotHub();

FollowNFT

    function mint(address to) external override {
        if (msg.sender != HUB) revert Errors.NotHub();
        uint256 tokenId = ++_tokenIdCounter;
        _mint(to, tokenId);
    }
    function delegate(address delegatee) external override {
        _delegate(msg.sender, delegatee);
    }

And there are many usages of msg.sender in LensHub. If you really need Context there, consider replacing msg.sender with _msgSender().

• You could put this to Constants instead of duplicating it in multiple contracts:

FeeModuleBase

    uint16 internal constant BPS_MAX = 10000;

ModuleGlobals

    uint16 internal constant BPS_MAX = 10000;

• Either the comment is a bit misleading or the code is not entirely correct:

  @param PublishingPaused The state where only publication creation functions are paused.

  However, whenPublishingEnabled modifier is applied not only to post functions but also comment and mirror. But maybe it's just me misinterpreting the situation and comment/mirror are also considered as publication creation functions.

• Consider using 2-step governance change procedure to reduce a risk of accidentally setting a wrong address:

  function _setGovernance(address newGovernance) internal {
    if (newGovernance == address(0)) revert Errors.InitParamsInvalid();
    address prevGovernance = _governance;
    _governance = newGovernance;
    emit Events.ModuleGlobalsGovernanceSet(prevGovernance, newGovernance, block.timestamp);
  }

  function _setGovernance(address newGovernance) internal {
    address prevGovernance = _governance;
    _governance = newGovernance;
    emit Events.GovernanceSet(msg.sender, prevGovernance, newGovernance, block.timestamp);
  }

• It is not reliable to use bytes(str).length to check the string length because encoded strings can differ in length, e.g., a single character encoded in UTF-8 can be more than a byte long.

  function _validateHandle(string calldata handle) private pure {
    bytes memory byteHandle = bytes(handle);
    if (byteHandle.length == 0 || byteHandle.length > Constants.MAX_HANDLE_LENGTH

  More details: https://github.com/miguelmota/solidity-idiosyncrasies However, I do not see any serious issue with your implementation, because you allow only a whitelisted set of characters. so just added this FYI.

• Name _dataByPublicationByProfile is a bit misleading, as it is actually more like _dataByProfileByPublication:

  _dataByPublicationByProfile[profileId][pubId]

  but again, maybe you are reading it differently.

• contract LensHubStorage should be abstract cuz it doesn't make sense on its own.

• Fee modules have this comment, but there is no such thing as FeeCollectModuleBase:

  * @notice This is a simple Lens CollectModule implementation, inheriting from the ICollectModule interface and
  * the FeeCollectModuleBase abstract contract.

  I think it should be FeeModuleBase.

• Handle registrations can be frontrunned, but I am not sure if that's a big issue because onlyWhitelistedProfileCreator can do that, and the prevention would introduce unnecessary complexity.

• The same sigNonces mapping is used for all the actions. To prevent replay attacks, upon successful execution sigNonces is incremented. This means that to sign unrelated actions a signer needs to wait for one by one to execute it on-chain, it can't happen in parallel. As an active user I want to sign txs to create a post, follow, delegate, change dispatcher, etc independently without waiting for one to finish before signing another.

• Consider explicitly marking all the main user functions as non-reentrant, especially those that interact with modules, because many more modules can be whitelisted in the future and there is no guarantee that they will follow the Checks-Effects-Interactions pattern.

[Zer0dot]

These are mostly valid, and helpful. I think a lot of this is opinion, but the sigNonces point is interesting and quite relevant. It's kept this way for simplicity but basically for streamlined actions we have the dispatcher system. Still, this is a high quality report!

[0xleastwood]

Great findings! The warden has shown the issues clearly and all seems to valid from the sponsor's POV.