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

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.

---

The text was updated successfully, but these errors were encountered:

All reactions