Function Admin of addMinterContract has too much power

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

In NextGenCore.sol, the addMinterContract function allows the Function Admin of addMinterContract to set the address of the minterContract, meaning he could add any contract instead of an actual minterContract, as long as it defines the same function names (because of the check require(IMinterContract(_minterContract).isMinterContract() == true, "Contract is not Minter");)

This is dangerous as it means this Function Admin’s malicious contract could the execute all the functions checking

require(msg.sender == minterContract, "Caller is not the Minter Contract");

in the NextGenCore.sol contract.

This includes the airDropTokens, mint and burnToMint functions, which are key to the functioning of the NextGenCore contract.

We mark this vulnerability as MEDIUM because the probability of the Function Admin to be malicious is relatively low, as he is himself set by a Global Admin.

Proof of Concept

Let’s have a look at the addMinterContract function below

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

This allows the Function Admin for addMinterContract to set a new address for the minterContract variable of the NextGenCore contract.

The line

require(IMinterContract(_minterContract).isMinterContract() == true, "Contract is not Minter");

means that this admin will be able to set any value for this address as long as it points to a contract defining the same function names as an actual minterContract (including a isMinterContract public function returning true).

Through this malicious contract, he could then bypass all the require(msg.sender == minterContract, "Caller is not the Minter Contract"); checks in the various functions of the NextGenCore, which is a clear shortfall of governance for this contract, as these functions govern airdrops, burning and minting mechanisms.

Tools Used

Manual Review / Visual Studio

Recommended Mitigation Steps

Set the value of minterContract address at contract creation, in the constructor. That will prevent any Function Admin from manipulating the NextGenCore contract.

Assessed type

Governance

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-pre-sort]

141345 marked the issue as duplicate of #584

[c4-pre-sort]

141345 marked the issue as duplicate of #522

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as duplicate of #303

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Overinflated severity