FunctionAdminRequired modifier does not specify any granular security around function permissions per contract or per collection

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/NextGenAdmins.sol#L44

Vulnerability details

Impact

FunctionAdminRequired modifier, widely used throughout the NextGen codebase, does not specify any granular security around function permissions per contract/collection. This opens up potentially significant security issues in terms of actors intentionally given FunctionAdmin access for a given function selector unintentionally gaining FunctionAdmin access to functions with the same selector on different contracts. Additionally, with no specification on which collection(s) the FunctionAdmins are granted access for for a given function selector, a bad actor that may deserve permissions to a given function for, say, Collection #1 but not Collection #2 will thus have access to calling the function by selector on both Collections #1 and #2. This vulnerability deals with access control of trusted (and potentially untrusted) actors, and puts the protocol at risk.

Proof of Concept

    function retrieveFunctionAdmin(address _address, bytes4 _selector) public view returns(bool) {
        return functionAdmin[_address][_selector];
    }

NextGenAdmins.retrieveFunctionAdmin is used widely in many of NextGen's contracts. As you can see, it only establishes access for a given address's permission for a function selector, with no additional security control.

Exploit Case 1: Same function name, different contracts

• whether referring to currently launched contracts or future, yet-to-be-deployed contracts, contracts that may have functions declared with the same name will clearly have security issues, as any actors permissioned with FunctionAdmin rights for that function selector in the past intentionally, will in the present or future unintentionally also have permissions for that function on other contracts that may not have been accounted for by the NextGen team.

Exploit Case 2: Different functions with different names that hash to the same bytes4 selector value

• similarly, though rare but not rare enough to discount how big of a security loophole this is, functions with different signatures can hash to the same bytes4 selector. Obvious how this could lead to unintentional, and even more difficult to debug than Case 1, selector clashes.

Tools Used

Solidity

Recommended Mitigation Steps

More granular control via the following:

// sets permission on specific function per contract
mapping(address => mapping(address => mapping(bytes4 => bool))) private functionAdminByContract;

// sets permission on specific function per collection
mapping(address => mapping(address => mapping(bytes4 => bool))) private functionAdminByCollection;
  
function registerFunctionAdminByContract(address _address, address _contract_address, bytes4 _selector, bool _status) public AdminRequired { 
      functionAdminByContract[_address][_contract_address][_selector] = _status;
}

function registerFunctionAdminByCollection(address _address, address _collection_address, bytes4 _selector, bool _status) public AdminRequired { 
      functionAdminByCollection[_address][_collection_address][_selector] = _status;
}

Assessed type

Access Control

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-sponsor]

a2rocket (sponsor) disputed

[a2rocket]

this is the intended design, function admins are trusted users.

[c4-judge]

alex-ppg marked the issue as duplicate of #303

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Overinflated severity