Some critical functions must be called only by the owner of the contract

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/MinterContract.sol#L448
https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/NextGenCore.sol#L315
https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/RandomizerNXT.sol#L41
https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/RandomizerRNG.sol#L61
https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/RandomizerVRF.sol#L94

Vulnerability details

In the 'MinterContract', 'NextGenCore', 'RandomizerNXT', 'RandomizerRNG', 'RandomizerVRF' contracts which have updateCoreContract, updateAdminContract, addMinterContract, updateRandomizerContract functions are using FunctionAdminRequired() modifier meaning that only function or global admin can call them. These functions can be considered as critical and sensetive for the protocol. Also MinterContract and RandomizerRNG contracts have emergencyWithdraw() function with FunctionAdminRequired modifier.

    function updateAdminContract(address _newadminsContract) public FunctionAdminRequired(this.updateAdminContract.selector) {
        require(INextGenAdmins(_newadminsContract).isAdminContract() == true, "Contract is not Admin");
        adminsContract = INextGenAdmins(_newadminsContract);
    }

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

    function emergencyWithdraw() public FunctionAdminRequired(this.emergencyWithdraw.selector) {
        uint balance = address(this).balance;
        address admin = adminsContract.owner();
        (bool success, ) = payable(admin).call{value: balance}("");
        emit Withdraw(msg.sender, success, balance);
    }

By function admins and global admins having access to these functions they are able to take full control over the contracts and the whole protocol which is not safe.

Impact

Function and global admins having accees to critical and sensetive functions is not safe. Malicious admin can change the state of the contract or the whole protocol and gain full control.

Proof of Concept

Scenario 1:

Lets imagine that Bob becomes malicous function admin. Time has passed and MinterContract ETH balance is 100 ETH.

1. Bob creates his own adminContract
2. In MinterContract Bob calls updateAdminContract(address _newadminsContract) setting his own adminContract
3. Bob calls emergencyWithdraw()

       uint balance = address(this).balance;
        address admin = adminsContract.owner();
        (bool success, ) = payable(admin).call{value: balance}("");
        emit Withdraw(msg.sender, success, balance);

4. adminsContract.owner() will be Bob and he will be able to steal all the funds in the contract.

NOTE: RandomizerRNG contract have emergencyWithdraw() function as well.

Scenario 2:

1. Bob is function admin in NextGenCore and MinterContract contracts.
2. Bob creates his own adminContract.
3. Calls updateAdminContract(address _newadminsContract) in both NextGenCore and MinterContract contracts.
4. Bob is the owner of the adminContrac and only he can add other admins
5. Bob now has full control over these 2 contracts.

Recommended Mitigation Steps

Limit the access to all the critical and sensetive function to be called only by the owner of the contract.

Assessed type

Access Control

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[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 primary issue

[c4-judge]

alex-ppg marked the issue as duplicate of #303

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Overinflated severity