Some function-level admins can escalate their own roles to drain all funds from NextGenMinterContract

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L322
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L461

Vulnerability details

Impact

At the outset, it must be acknowledged that the NextGen team considers all admin roles to be trusted, and findings related to an admin abusing their defined role are not valid. However, an admin having the ability to escalate their own role unilaterally and execute functionality they were not approved to execute — even if only a malicious admin would exercise this ability — should be considered a serious vulnerability. This is even more true if this vulnerability is exploitable for major monetary gain, as is the case here.

Because the owner of the NextGenAdmins contract is treated as a global admin throughout the system, any FunctionAdmin with the authority to updateAdminContract (L454, NextGenMinterContract) can deploy their own admin contract, replace the old one, and drain all the funds from the minter contract by calling emergencyWithdraw (L461, NextGenMinterContract).

Even considering the team's intent to only use multisigs as admins, the balance of NextGenMinter could very easily get high enough to tempt more than one possible attacker. And regardless, functionality with as much risk as emergencyWithdraw must be handled with the absolute highest level of care — and that means there should not be any implicitly defined access to it. Such caveats are bound to be forgotten or overlooked, especially by future developers who join the NextGen team and external maintainers of forks (such as UNIC at launch).

Proof of Concept

Full test can be viewed here: https://gist.github.com/ethanbennett/2f4850e761317d8f559474bf07dfd15d

function test_AdminExploit() external {
    address admin = 0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045;
    vm.deal(address(_minter), 100 ether);

    // set function admin
    _admins.registerFunctionAdmin(admin, _minter.updateAdminContract.selector, true);

    // malicious function admin replaces admins contract
    vm.prank(admin);
    NextGenAdmins newAdmins = new NextGenAdmins();
    _minter.updateAdminContract(address(newAdmins));

    // function admin now has power of global admin, can drain all funds
    vm.prank(admin);
    _minter.emergencyWithdraw();

    assertEq(address(admin).balance, 100 ether);
}

Recommended mitigation

Consider treating updateAdminContract and emergencyWithdraw both as especially high-security functions: explicitly define who can access these functions, and under what circumstances. It would also be worthwhile to implement a multi-party approval system for these functions, as seen in NextGenMinterContract when proposing addresses and percentages.

Assessed type

Access Control

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-pre-sort]

141345 marked the issue as remove high or low quality report

[c4-pre-sort]

141345 marked the issue as duplicate of #303

[c4-pre-sort]

141345 marked the issue as not a duplicate

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-sponsor]

a2rocket (sponsor) disputed

[a2rocket]

all admins are trusted roles.

[c4-judge]

alex-ppg marked the issue as duplicate of #1922

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Overinflated severity