Use of NextGenAdmins in all contracts that define an adminsContract is flawed and any Admin can become the owner of adminsContract

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/NextGenCore.sol#L322-L325
https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/MinterContract.sol#L454-L457
https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/RandomizerNXT.sol#L45-L47
https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/RandomizerVRF.sol#L94-L97
https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/RandomizerRNG.sol#L61-L64

Vulnerability details

Let’s take the example of RandomizerNXT for the sake of simplicity.

Impact

In RandomizerNXT.sol and Admin of adminsContract can design himself as a function admin for the updateAdminContract function and update the admin contract to one he is the owner of.

This defeats the whole purpose of the onlyOwner modifier in the registerAdmin function in NextGenAdmins.sol

This is marked as MEDIUM as the probability of a malicious Admin is moderate.

Proof of Concept

When the RandomizerNXT.sol contract is initially deployed, the adminsContract is set in the constructor.

constructor(address _randoms, address _admin, address _gencore) {
        randoms = IXRandoms(_randoms);
        adminsContract = INextGenAdmins(_admin);
        gencore = _gencore;
        gencoreContract = INextGenCore(_gencore);
}

In this NextGenAdmins, the owner() and only the owner() is able to designate global Admins via the registerAdmin function as defined below

function registerAdmin(address _admin, bool _status) public onlyOwner {
        adminPermissions[_admin] = _status;
}

Let’s put ourselves in the situation where a global Admin has been chosen by the owner().

This Admin should be able to define FunctionAdmins with the registerFunctionAdmin function as defined below

function registerFunctionAdmin(address _address, bytes4 _selector, bool _status) public AdminRequired {
        functionAdmin[_address][_selector] = _status;
}

but should NOT be able to register other global Admins as the registerAdmin function uses the onlyOwner modifier.

However, Admin can declare themselves as a Function Admin for the updateAdminsContract function, meaning he is able to update the adminsContract to a contract from which he is the owner() and where he can nominate global Admins.

Tools Used

Manual review / Visual Studio

Recommended Mitigation Steps

The updateAdminsContract function should use an OwnerRequired modifier instead of the FunctionAdminRequired(this.updateAdminsContract.selector) modifier it is currently using.
This OwnerRequired modifier could look like the below

modifier OwnerRequired() {
      require(adminsContract.owner() == msg.sender , "Not allowed");
      _;
}

This would ensure that admins are not able to replace the owner and result in stronger access control.

Assessed type

Governance

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-sponsor]

a2rocket (sponsor) disputed

[a2rocket]

this is the intended design, owner of the NextGenAdmins contract can set Admins which are fully trusted.

[alex-ppg]

The Warden specifies a way whereby a function administrator can escalate themselves to be a global administrator. This falls under suitable findings per the relevant Centralization Risks Supreme Court verdict, however, I am inclined to mark this as a non-issue.

The reasoning behind this judgment is that a function administrator is expected to have complete access to the relevant function, and the updateAdminsContract function can only achieve one outcome; the upgrade of the administrative contract that handles overall ACL for the contract.

As such, a function administrator of that particular function is considered to be identical to a global administrator in my viewpoint given the responsibilities associated with the function. I believe this finding to be better suited for an Analysis review / QA (NC) and thus of overinflated severity.

To note, all duplicated issues of this exhibit do not refer to privilege escalation, they instead pertain to various centralization abuses that fall under the same SC ruling and thus are considered invalid. If necessary, this finding and its duplicates will be split into separate issues but for now, the whole group falls under centralization risks and is invalid.

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Overinflated severity