QA Report

[code423n4]

1. Floating pragma is used in ERC721 contracts

As different compiler versions have critical behavior specifics, if the contracts get accidentally deployed using another compiler version compared to one they tested with, the various types of undesired behavior can be introduced

Proof of Concept

pragma solidity ^0.8.0 is used in the following ERC721 contracts:

ERC721Enumerable ERC721Time IERC721Time

Recommended Mitigation Steps

Consider fixing the version to 0.8.10 across all the codebase

2. Initializers have no access modifiers and can be front run

Impact

An attacker can front-run the initiallization whenever it is run not atomically with the construction (and given the contract code alone it is not guaranteed in any way), setting a malicious set of core configuration variables

Proof of Concept

FollowNFT:

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/FollowNFT.sol#L57

CollectNFT:

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/CollectNFT.sol#L39

LensHub:

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/LensHub.sol#L67

Recommended Mitigation Steps

Consider adding the access controls to the initialization functions so that only deployer contract could run them

3. LensNFTBase allows initialization with empty name and symbol

Impact

A token with empty name and symbol will not be convenient to manually operate, other contracts' malfunctions are possible

Proof of Concept

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/base/LensNFTBase.sol#L45

Recommended Mitigation Steps

Add validity checks

4. LensHub allows setting immutable core implementations to zero addresses

Impact

A range of malfunctions is possible if core configuration parameters are set by mistake to any values that make little sense. It is also not always right away evident that such a mistake took place

Proof of Concept

LensHub constructor doesn't check the addresses supplied:

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/LensHub.sol#L58-59

Recommended Mitigation Steps

Addresses to be set are controlled in the deploy script, but as that's core immutable configuration consider adding second layer of control in the constructor

5. InteractionLogic.follow emits vaguely related error on missing profile

Impact

TokenDoesNotExist is emitted which is ambigous, ProfileDoesNotExist is actual error there.

Proof of Concept

InteractionLogic.follow reverts with Errors.TokenDoesNotExist() is profile can't be found by handle hash (profile wasn't created or was burned):

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/libraries/InteractionLogic.sol#L50

Given how many tokens exist in the system, this looks not clear enough.

Recommended Mitigation Steps

Consider adding a dedicated error. This looks justified as equvalently rare situations already have particular errors (ProfileCreatorNotWhitelisted or HandleLengthInvalid, for example).

6. ApprovalFollowModule.approve emits unrelated error on array lengths mismatch

Impact

InitParamsInvalid error is emitted, which is ambigous as approve isn't related to initialization, the ArrayMismatch one looks to fit better.

Proof of Concept

ApprovalFollowModule.approve reverts with Errors.InitParamsInvalid if profileIds.length != followModuleDatas.length:

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/modules/follow/ApprovalFollowModule.sol#L37

Given that it's not an initialization, this doesn't look good enough.

Recommended Mitigation Steps

Consider reverting here with Errors.ArrayMismatch

7. Recipient can be set to zero, which disable core functionality if collect currency do not allow transfers to zero address

Impact

Address of recipient isn't checked to be non-zero, but a combination of zero recipient and currency that do not allow for transfers to a zero address will impact the protocol as all the attempts to collect will revert

Proof of Concept

FeeCollectModule, LimitedFeeCollectModule, TimedFeeCollectModule, LimitedTimedFeeCollectModule do not check the addresses on init:

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/modules/collect/FeeCollectModule.sol#L76

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/modules/collect/LimitedFeeCollectModule.sol#L85

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/modules/collect/TimedFeeCollectModule.sol#L86

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/modules/collect/LimitedTimedFeeCollectModule.sol#L92

The same in the FeeFollowModule:

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/modules/follow/FeeFollowModule.sol#L67

References

https://github.com/d-xo/weird-erc20#revert-on-transfer-to-the-zero-address

Recommended Mitigation Steps

Add the check for non-zero recipient address on the mentioned modules initialization (initializePublicationCollectModule for FeeCollectModule, as an example).

As this is one time action the additional gas cost is low and it's justified giving an enhancement of protocol stability

8. Consider removing temp comment as returning image URI looks to be the best option for profile token

Proof of Concept

imageURI as tokenURI marked as temp:

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/LensHub.sol#L843

While there looks to be no better option:

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/libraries/DataTypes.sol#L66-71

[Zer0dot]

Most of this is not really relevant, and in particular the recipient != address(0) check is actually present in the linked examples.