Aave Lens contest - hyh's results

Lines of code

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

Vulnerability details

Impact

Base fee modules require minimum fixed fee amount to be at least BPS_MAX, which is hard coded to be 10000.

This turns out to be a functionality restricting requirement for some currencies.

For example, WBTC (https://etherscan.io/token/0x2260fac5e5542a773aa44fbcfedf7c193bc2c599, #10 in ERC20 token rankings), has decimals of 8 and current market rate around $40k, i.e. if you want to use any WBTC based collect fee, it has to be at least $4 per collect or fee enabled follow.

Tether and USDC (https://etherscan.io/token/0xdac17f958d2ee523a2206206994597c13d831ec7 and https://etherscan.io/token/0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48, #1 and #3) have decimals of 6, so it is at least $0.01 per collect/follow, which also looks a bit tight for a hard floor minimum.

Proof of Concept

BPS_MAX is a system wide constant, now 10000:

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

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

This is correct for any fees defined in basis point terms.

When it comes to the nominal amount, 10000 can be too loose or too tight depending on a currency used, as there can be various combinations of decimals and market rates.

The following base collect module implementations require fee amount to be at least BPS_MAX (initialization reverts when amount < BPS_MAX):

All collect module implementations use the same check:

FeeCollectModule:

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

LimitedFeeCollectModule:

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

TimedFeeCollectModule:

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

LimitedTimedFeeCollectModule:

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

FeeFollowModule also uses the same approach:

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

Recommended Mitigation Steps

As a simplest solution consider adding a separate constant for minimum fee amount in nominal terms, say 1 or 10

Lines of code

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

Vulnerability details

Impact

Treasury fee can be zero, while collect modules do attempt to send it in such a case anyway as there is no check in place. Some ERC20 tokens do not allow zero value transfers, reverting such attempts.

This way, a combination of zero treasury fee and such a token set as a collect fee currency will revert any collect operations, rendering collect functionality unavailable

Proof of Concept

Treasury fee can be set to zero:

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

Treasury fee transfer attempts are now done uncoditionally in all the collect modules.

Namely, FeeCollectModule, LimitedFeeCollectModule, TimedFeeCollectModule and LimitedTimedFeeCollectModule do not check the treasury fee to be send, treasuryAmount, before transferring:

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

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

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

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

The same happens in the FeeFollowModule:

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

References

Some ERC20 tokens revert on zero value transfers:

https://github.com/d-xo/weird-erc20#revert-on-zero-value-transfers

Recommended Mitigation Steps

Consider checking the treasury fee amount and do transfer only when it is positive.

Now:

IERC20(currency).safeTransferFrom(follower, treasury, treasuryAmount);

To be:

if (treasuryAmount > 0)
	IERC20(currency).safeTransferFrom(follower, treasury, treasuryAmount);

Lines of code

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

Vulnerability details

Impact

In case when zero collection module be white listed and then zero collection module set to a post (done by different actors), its functionality will be partially broken: every collecting and mirroring of it will be reverted with Errors.PublicationDoesNotExist, while getPubType will return its type as a Mirror.

Setting severity to Medium per 'function of the protocol or its availability could be impacted', as either the behavior of rejecting collects/mirrors is desired and to be implemented explicitly, or such a scenario shouldn't exist in a system, i.e. now zero address absence in white list cannot be guaranteed as its setting is manual and possible

Proof of Concept

Collect module isn't checked additionally on init, any white listed address can be set:

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

While zero address also can be white listed:

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

If zero address is white listed and then set as collectModule for a post, a getPointedIfMirror helper function called on it will revert with Errors.PublicationDoesNotExist:

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

This way all attempts to collect and mirror this post will also revert:

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

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

Also, getContentURI view will fail with the same error:

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

And getPubType will return Mirror as a publication type:

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

Recommended Mitigation Steps

Consider prohibiting zero collection module to be white listed.

If rejecting collects/mirrors is desired for a special post type, the corresponding error path can be implemented

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