Aave Lens contest - hubble's results

Summary of Findings

Issue#1 : Null address check missing while assigning new Governance address in LensHub.sol Issue#2 : Donot allow any transactions when state is not changed in ModuleGlobals.sol Issue#3 : Implementation contracts of FollowNFT and CollectNFT deployed, remains open for initialization

Details Issue#1

Title : Null address check missing while assigning new Governance address in LensHub.sol

Impact

In the event of wrongly assigning ZERO address to the governance address, the governance control of the Lens protocol will be lost.

Proof of Concept

Contract : LensHub.sol

line 79 :

function setGovernance(address newGovernance) external override onlyGov {
    _setGovernance(newGovernance);
}

line 850 :

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

Recommended Mitigation Steps

Add a require statement to check for null address, in function _setGovernance(). As a best practice the two step standard process for changing the Governance address is recommended.

Details Issue#2

Title : Donot allow any transactions when state is not changed in ModuleGlobals.sol

In the set functions in ModuleGlobals.sol, there is no check if the new Value being set is different than previous Value. Allow changing to new value only if its different than prev value.

Impact

Unnecessary event generated even if no state is changed

Proof of Concept

Contract : ModuleGlobals.sol
line 94 : function _setGovernance(address newGovernance) line 101 : function _setTreasury(address newTreasury) line 108 : function _setTreasuryFee(uint16 newTreasuryFee) line 115 : function _whitelistCurrency(address currency, bool toWhitelist)

Recommended Mitigation Steps

Add a require statement to check if prev value is same as new value, example below. if (prevWhitelisted == toWhitelist ) revert Errors.ChangeParamsInvalid()

Details Issue#3

Title : Implementation contracts of FollowNFT and CollectNFT deployed, remains open for initialization.

Risk:

The implementation contracts of FollowNFT and CollectNFT, that is passed as constructor parameter of LensHub, they remain uninitialized and open for initialization. Since these contracts are used extensively for Cloning, it is better to avoid its initialization and keep its state as it was originally deployed.

Proof of Concept

Contract : FollowNFT.sol and CollectNFT.sol Function : initialize()

Recommended Mitigation Steps

Mitigation: Add the following lines of code to FollowNFT and CollectorNFT's constructor

constructor(address hub) {
    HUB = hub;
    _initialized = true;  // Avoids initialization of originally deployed contract. Does not affect the cloned contracts.
}