CoreCollection.initialize doesn't use the onlyUninitialized modifier, allowing arbitrary re-initialized by owner

[code423n4]

Lines of code

https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreCollection.sol#L87

Vulnerability details

Impact

Scarcity and uniqueness arguably the core philosophy of NFTs. This bug allows NFT creators(CoreProxy owners) to re-initialize the CoreProxy contract anytime, effectively breaking all promises on scarcity and uniqueness granted upon contract creation. It also allows changing of other fields such as fees for minting, uri for minted NFTs, thus putting buyers at the mercy of contract owner.

Proof of Concept

For proxy contracts, the backing implementation often provides an initialize function to help proxy populate initial state. Since the function is intended for initialization, it is expected to be called just once(just like constructors should only be called once for normal contracts).

However, in the case of CoreCollection.initialize, no modifiers that check whether the contract has already been initialized is used, thus allowing contract owners to re-initialize the contract states whenever they want.

    function initialize(
        string memory _collectionName,
        string memory _collectionSymbol,
        string memory _collectionURI,
        uint256 _maxSupply,
        uint256 _mintFee,
        address _payableToken,
        bool _isForSale,
        address _splitFactory
    ) external onlyOwner onlyValidSupply(_maxSupply) {
        _name = _collectionName;
        _symbol = _collectionSymbol;
        _baseUri = _collectionURI;
        maxSupply = _maxSupply;
        mintFee = _mintFee;
        payableToken = IERC20(_payableToken);
        isForSale = _isForSale;
        splitFactory = _splitFactory;
        initialized = true;
    }

Re-initialize allows re-setting several critical fields, including _baseUri, mineFee and maxSupply, just to name a few. This violates the common guideline of NFTs, and should be mitigated before actual contract deployment.

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

Add the onlyUnInitialized modifier to initialize.

    function initialize(
        ...
    ) external onlyOwner onlyUninitialized onlyValidSupply(_maxSupply) {
        ...
    }

[sofianeOuafir]

duplicate of #4