Constructor in LensHub contract when used with proxy makes protocol unusable

[code423n4]

Lines of code

https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/LensHub.sol#L57-L60 https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/LensHub.sol#L32-L33

Vulnerability details

Impact

You will be running your protocol with parameters FOLLOW_NFT_IMPL and COLLECT_NFT_IMPL at zero address.

Proof of Concept

You cannot add constructors in upgradeable contracts because storage variables are inside the proxy. Proxies are unaware of constructors and when you run them you will only initialize storage inside the logic contract. As a rule of thumb, you cannot use immutable variables in upgradeable contracts either, there is a discussion in OpenZeppelin forum about that. https://forum.openzeppelin.com/t/immutable-variables-cannot-be-initialized/7927/4

Recommended Mitigation Steps

I think you have two options to fix this issue .

Solution 1: Move constructor logic to initialize() function and remove "immutable" from variables

Change this

address internal immutable FOLLOW_NFT_IMPL;         LensHub.sol#L32
address internal immutable COLLECT_NFT_IMPL;         LensHub.sol#L33

For

address internal FOLLOW_NFT_IMPL;                         
address internal COLLECT_NFT_IMPL;

Remove

constructor(address followNFTImpl, address collectNFTImpl) {        LensHub.sol#L57
    FOLLOW_NFT_IMPL = followNFTImpl;                                           LensHub.sol#L58
    COLLECT_NFT_IMPL = collectNFTImpl;                                          LensHub.sol#L59
}                                                                                                          LensHub.sol#L60

Change.

function initialize(                          LensHub.sol#L63
    string calldata name,                                                                    
    string calldata symbol,
    address newGovernance
) external override initializer {
    super._initialize(name, symbol);
    _setState(DataTypes.ProtocolState.Paused);
    _setGovernance(newGovernance);
}

For

function initialize(
    string calldata name,
    string calldata symbol,
    address newGovernance
) external override initializer {
    super._initialize(name, symbol);
    FOLLOW_NFT_IMPL = followNFTImpl;
    COLLECT_NFT_IMPL = collectNFTImpl;
    _setState(DataTypes.ProtocolState.Paused);
    _setGovernance(newGovernance);
}

Solution 2: You can use constants for those addresses.

This solution is "better" to think variables as immutable if you use a constant instead but I think you need to make many changes in other constructors as well as in your deployment logic

address constant FOLLOW_NFT_IMPL;         LensHub.sol#L32
address constant COLLECT_NFT_IMPL;          

[miguelmtzinf]

This is not an issue because the variables that are getting a value inside the constructor are immutables, so they are not part of the storage layout of the proxy contract nor implementation. There is no need to change anything and the impact here described is wrong: parameters FOLLOW_NFT_IMPL and COLLECT_NFT_IMPL will have the values passed to LensHub contructor.

[Zer0dot]

Yeah agreed with Miguel, this is invalid.

[0xleastwood]

As the sponsor has stated, immutables are stored in the contract's bytecode and not its storage layout.