[H1] Some admins functions are unusable because of misuse of variables in upgradeable contracts

[code423n4]

Lines of code

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollectionFactory.sol#L104 https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollectionFactory.sol#L181-L185 https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/ContractFactory.sol#L19 https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/ContractFactory.sol#L30-L33 https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTDropCollection.sol#L101-L103 https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollection.sol#L95-L97

Vulnerability details

Impact

​ Admin functions in NFTCollectionFactor.sol are unusable through a proxy

Proof of Concept

​ Upgradeable contracts cannot use neither constructors nor use immutable variables. The reason for that is they work behind a proxy which calls them using delegatecall so the real variables are stored in the proxy contract.

​ Note that all contracts in OpenZeppelin do not have any constructor.

Issue in NFTCollectionFactory.sol

In NFTCollectionFactory the variable rolesContract which is where admin is consulted is immutable.

address public implementationNFTCollection; 

The result is that it cannot be seen from proxy and all onlyAdmin unable to use or produce unexpected behavior.

modifier onlyAdmin() {
require(rolesContract.isAdmin(msg.sender), "NFTCollectionFactory: Caller does not have the Admin role");
_;
}

Issue in ContractFactory.sol (inherited NFTDropCollection.sol and NFTCollection.sol)

Same problem in this contract

address public immutable contractFactory;

constructor(address _contractFactory) {
    require(_contractFactory.isContract(), "ContractFactory: Factory is not a contract");
    contractFactory = _contractFactory;
}

Recommended Mitigation Steps

1) Remove immutable keyword from variables mentioned 2) Remove constructors . 3) Move the constructor code to initialize() function.
4) Be sure you call initialize() for every parent

A good suggestion to avoid confusions is to add Upgradeable at the end of the name of every contract. For example ContractFactory could be renamed ContractFactoryUpgradeable.

[HardlyDifficult]

Invalid. This is a misunderstanding of immutable variables in upgradeable/proxy contracts.

Both parts of this statement are false, "Upgradeable contracts cannot use neither constructors nor use immutable variables":

• Constructors may be used with upgradeable contracts. Generally they are strongly discouraged because it could be error prone (in fact you'll see in our deploy scripts we specifically suppress warnings about this). They are perfectly fine and safe to use IF they only assign immutable values. Which brings us to the second point..
• Immutable values are determined at the time the contract is deployed and saved in the bytecode. In the end they act just like a constant would have, including the same gas optimization you can expect by using a constant instead of loading from storage.

Since the immutable variables are in the bytecode, they are accessible as expected when called via a proxy.

[HickupHH3]

Yes, I had the same misconception before XD

Immutable variables can be used in constructors to be read by proxies because they're written directly into the bytecode.