Owners can set function and collection admins

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenAdmins.sol#L44
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenAdmins.sol#L50
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenAdmins.sol#L58
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenAdmins.sol#L31

Vulnerability details

https://seize-io.gitbook.io/nextgen/nextgen-smart-contracts/minter
https://code4rena.com/contests/2023-10-nextgen#top

Impact

From the provided readme and documentation, the functions to register a function and a collection admin are only to be called by a global admin who are registered by the owners.

The registerFunctionAdmin, registerBatchFunctionAdmin and registerCollectionAdmin functions are protected using the AdminRequired modifier.
However, the modifier is allows both the global admin and the owner. This gives the owner the ability to register function and collection admins which breaks the protocol's main invariant.

Proof of Concept

From the provided readme

Main invariants
Properties that should NEVER be broken under any circumstance:

Admin roles can only be registered on the Admin Contract.
Global Admins can only be registered by the Admin Contract owner. 
Function and Collection admins can only be registered by global admins.  

and the provided documentation,

2. Register a Function Admin
The registerFunctionAdmin(..) function is used to register function admins who can call specific functions on the Core and Minter contracts.
...
Notes:
This function can be called by a global admin.

3. Register a Collection Admin
The registerCollectionAdmin(..) function is used to register collection admins who can call certain functions for a specific collection on the Core and Minter contracts.
...
Notes:
This function can be called by a global admin.

The registerFunction/Batch/CollectionAdmin functions uses the AdminRequired modifier.

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

    // function to register batch functions admin
function registerBatchFunctionAdmin(address _address, bytes4[] memory _selector, bool _status) public AdminRequired {
        for (uint256 i=0; i<_selector.length; i++) {
            functionAdmin[_address][_selector[i]] = _status;
        }
    }

    // function to register a collection admin

function registerCollectionAdmin(uint256 _collectionID, address _address, bool _status) public AdminRequired {
        require(_collectionID > 0, "Collection Id must be larger than 0");
        collectionAdmin[_address][_collectionID] = _status;
    }

The AdminRequired modifier is defined - it allows the msg.sender to be a global admin or the owner.

      modifier AdminRequired {
      require((adminPermissions[msg.sender] == true) || (_msgSender()== owner()), "Not allowed");
      _;
    }

This allows the owner to be able to set these admins, but breaks the protocol's invariants.

Tools Used

Manual code review

Recommended Mitigation Steps

Updating the modifier, e.g

    modifier AdminRequired {
      require((adminPermissions[msg.sender] == true);
      _;
    }

Assessed type

Access Control

[a2rocket]

owner or global admins should register functions or collection admins, this is how it should work.

[c4-sponsor]

a2rocket (sponsor) disputed

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-judge]

alex-ppg marked the issue as duplicate of #303

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Overinflated severity