Aave Lens contest - pauliax's results

• 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.

• function _validatePublishingEnabled() would be cheapier if it did not check all the other states, but just Unpaused:

  if (_state != DataTypes.ProtocolState.Unpaused) {
    revert Errors...();
  }

• Because MAX_HANDLE_LENGTH is 31, I think you can use bytes32 to store the handles. This would be cheaper than dynamic size strings or bytes.

• The same storage is accessed many times in a function, e.g. _dataByPublicationByProfile[profileId][pubId] is accessed 3 times in _processCollect:

  uint256 amount = _dataByPublicationByProfile[profileId][pubId].amount;
  address currency = _dataByPublicationByProfile[profileId][pubId].currency;
  ...
  address recipient = _dataByPublicationByProfile[profileId][pubId].recipient;

Would be better to cache this in a variable and re-use.

• I think it would be cheaper here to first increment the pubCount and the pass the updated value (avoid repeated add operation):

  PublishingLogic.createComment(
      vars,
      _profileById[vars.profileId].pubCount + 1,
      _profileById,
      _pubByIdByProfile,
      _collectModuleWhitelisted,
      _referenceModuleWhitelisted
  );
  _profileById[vars.profileId].pubCount++;

• I wonder do you really need to pass the whole mapping to another function if you use just one element from it, e.g. function createComment in PublishingLogic library takes the whole _profileById mapping but uses only pubCount of one element:

   mapping(uint256 => DataTypes.ProfileStruct) storage _profileById
   ...
  // Validate existence of the pointed publication
  uint256 pubCount = _profileById[vars.profileIdPointed].pubCount;
  if (pubCount < vars.pubIdPointed || vars.pubIdPointed == 0)
    revert Errors.PublicationDoesNotExist();

Can't you just pass only data that you actually need and use, or even pass this pubCount of profileIdPointed from the caller? There are many more functions that take these big storage pointers as parameters, so in my opinion, you should consider optimizing this.

• ++ and -- are a bit cheaper than += 1 and -= 1 :

_balances[to] += 1;
_balances[owner] -= 1;
_balances[from] -= 1;