Centralized risk in global admins

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenAdmins.sol#L18
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L454-L457
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L320-L325
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/RandomizerNXT.sol#L45-L47
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/RandomizerRNG.sol#L59-L64
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/RandomizerVRF.sol#L92-L97

Vulnerability details

Impact

In the NextGenAdmins contract there are global admins registered by the ownable function registerCollectionAdmin

When a new global administrator registers, it can call multiple functions from multiple contracts, the most critical function who it's present all contracts except in auctionDemo contract it's the updateAdminContract

With this function, the global admin or whoever is given the power to call this function has the ability to change the admin contract, thus taking full control of these contracts(all contracts except auctionDemo) and take the ownership of the contracts

Proof of Concept

This PoC shows how the global admin can be made from the NextGenMinterContract contract by setting the adminsContract variable with a contract controlled by it, the same PoC is applicable to the rest of the contracts

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "forge-std/Test.sol";

import {NextGenAdmins} from "../smart-contracts/NextGenAdmins.sol";
import {NextGenMinterContract} from "../smart-contracts/MinterContract.sol";
import {Ownable} from "openzeppelin-contracts/access/Ownable.sol";

contract CorruptedAdmins is Ownable {
    function retrieveGlobalAdmin(address _address) public view returns(bool) {
        return _address == owner();
    }
    function retrieveFunctionAdmin(address _address, bytes4) public view returns(bool) {
        return _address == owner();
    }

    function retrieveCollectionAdmin(address _address, uint256) public view returns(bool) {
        return _address == owner();
    }

    function isAdminContract() external pure returns (bool) {
        return true;
    }
}

contract PoCTest is Test {
    function test() public {
        address corruptedAdmin = address(6);

        // Deploy contracts
        NextGenAdmins genAdmins = new NextGenAdmins();
        NextGenMinterContract minter = new NextGenMinterContract(
            address(0), // _gencore
            address(0),// _del
            address(genAdmins)// _adminsContract
        );

        // Hypothetically the minter has 100 ether
        vm.deal(address(minter), 100 ether);

        // The owner register a new global admin
        genAdmins.registerAdmin(corruptedAdmin, true);

        // The global admin is corrupted, set the adminsContract of the minter
        vm.startPrank(corruptedAdmin);
        CorruptedAdmins corruptedAdmins = new CorruptedAdmins();
        minter.updateAdminContract(address(corruptedAdmins));

        console.log("Previous Balance of CorruptedAdmin:", corruptedAdmin.balance);
        console.log("Previous Balance of Minter:", address(minter).balance);

        // The corrupted global admin take all the ether from the minter contract
        minter.emergencyWithdraw();

        // Finally the corrupted global admin have the 100 ETH and the control of the minter
        console.log("Final Balance of CorruptedAdmin:", corruptedAdmin.balance);
        console.log("Final Balance of Minter:", address(minter).balance);
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

Remove the global admins(adminPermissions and all his behavior), this would make the admin have permission over the functions that the owner assigns to them through functions registerFunctionAdmin, registerBatchFunctionAdmin and registerCollectionAdmin:

File: smart-contracts/NextGenAdmins.sol

@@ -12,10 +12,7 @@ pragma solidity ^0.8.19;

 import "./Ownable.sol";

-contract NextGenAdmins is Ownable{
-
-    // sets global admins
-    mapping(address => bool) public adminPermissions;
+contract NextGenAdmins is Ownable {

     // sets collection admins
     mapping (address => mapping (uint256 => bool)) private collectionAdmin;
@@ -23,31 +20,15 @@ contract NextGenAdmins is Ownable{
     // sets permission on specific function
     mapping (address => mapping (bytes4 => bool)) private functionAdmin;

-    constructor() {
-        adminPermissions[msg.sender] = true;
-    }
-
-    // certain functions can only be called by an admin
-    modifier AdminRequired {
-      require((adminPermissions[msg.sender] == true) || (_msgSender()== owner()), "Not allowed");
-      _;
-    }
-
-    // function to register a global admin
-
-    function registerAdmin(address _admin, bool _status) public onlyOwner {
-        adminPermissions[_admin] = _status;
-    }
-
     // function to register function admin

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

     // function to register batch functions admin

-    function registerBatchFunctionAdmin(address _address, bytes4[] memory _selector, bool _status) public AdminRequired {
+    function registerBatchFunctionAdmin(address _address, bytes4[] memory _selector, bool _status) public onlyOwner {
         for (uint256 i=0; i<_selector.length; i++) {
             functionAdmin[_address][_selector[i]] = _status;
         }
@@ -55,17 +36,11 @@ contract NextGenAdmins is Ownable{

     // function to register a collection admin

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

-    // function to retrieve global admin
-
-    function retrieveGlobalAdmin(address _address) public view returns(bool) {
-        return adminPermissions[_address];
-    }
-
     // function to retrieve collection admin

File: smart-contracts/INextGenAdmins.sol

@@ -4,9 +4,6 @@ pragma solidity ^0.8.19;

 interface INextGenAdmins {

-    // retrieve global admin
-    function retrieveGlobalAdmin(address _address) external view returns(bool);
-
     // retrieve function admin
     function retrieveFunctionAdmin(address _address, bytes4 _selector) external view returns(bool);

File: smart-contracts/AuctionDemo.sol

@@ -29,7 +29,7 @@ contract auctionDemo is Ownable {

     // certain functions can only be called by auction winner or admin
     modifier WinnerOrAdminRequired(uint256 _tokenId, bytes4 _selector) {
-      require(msg.sender == returnHighestBidder(_tokenId) || adminsContract.retrieveFunctionAdmin(msg.sender, _selector) == true || adminsContract.retrieveGlobalAdmin(msg.sender) == true, "Not allowed");
+      require(msg.sender == returnHighestBidder(_tokenId) || adminsContract.retrieveFunctionAdmin(msg.sender, _selector) == true, "Not allowed");
       _;
     }

File: smart-contracts/MinterContract.sol

@@ -134,21 +134,21 @@ contract NextGenMinterContract is Ownable {

     // certain functions can only be called by an admin or the artist
     modifier ArtistOrAdminRequired(uint256 _collectionID, bytes4 _selector) {
-      require(msg.sender == gencore.retrieveArtistAddress(_collectionID) || adminsContract.retrieveFunctionAdmin(msg.sender, _selector) == true || adminsContract.retrieveGlobalAdmin(msg.sender) == true, "Not allowed");
+      require(msg.sender == gencore.retrieveArtistAddress(_collectionID) || adminsContract.retrieveFunctionAdmin(msg.sender, _selector) == true, "Not allowed");
       _;
     }

     // certain functions can only be called by a global or function admin

     modifier FunctionAdminRequired(bytes4 _selector) {
-      require(adminsContract.retrieveFunctionAdmin(msg.sender, _selector) == true || adminsContract.retrieveGlobalAdmin(msg.sender) == true , "Not allowed");
+      require(adminsContract.retrieveFunctionAdmin(msg.sender, _selector) == true , "Not allowed");
       _;
     }

     // certain functions can only be called by a collection, global or function admin

     modifier CollectionAdminRequired(uint256 _collectionID, bytes4 _selector) {
-      require(adminsContract.retrieveCollectionAdmin(msg.sender,_collectionID) == true || adminsContract.retrieveFunctionAdmin(msg.sender, _selector) == true || adminsContract.retrieveGlobalAdmin(msg.sender) == true, "Not allowed");
+      require(adminsContract.retrieveCollectionAdmin(msg.sender,_collectionID) == true || adminsContract.retrieveFunctionAdmin(msg.sender, _selector) == true, "Not allowed");
       _;
     }

File: smart-contracts/NextGenCore.sol

@@ -114,14 +114,14 @@ contract NextGenCore is ERC721Enumerable, Ownable, ERC2981 {
     // certain functions can only be called by a global or function admin

     modifier FunctionAdminRequired(bytes4 _selector) {
-      require(adminsContract.retrieveFunctionAdmin(msg.sender, _selector) == true || adminsContract.retrieveGlobalAdmin(msg.sender) == true , "Not allowed");
+      require(adminsContract.retrieveFunctionAdmin(msg.sender, _selector) == true, "Not allowed");
       _;
     }

     // certain functions can only be called by a collection, global or function admin

     modifier CollectionAdminRequired(uint256 _collectionID, bytes4 _selector) {
-      require(adminsContract.retrieveCollectionAdmin(msg.sender,_collectionID) == true || adminsContract.retrieveFunctionAdmin(msg.sender, _selector) == true || adminsContract.retrieveGlobalAdmin(msg.sender) == true, "Not allowed");
+      require(adminsContract.retrieveCollectionAdmin(msg.sender,_collectionID) == true || adminsContract.retrieveFunctionAdmin(msg.sender, _selector) == true, "Not allowed");
       _;
     }

File: smart-contracts/RandomizerNXT.sol

@@ -32,7 +32,7 @@ contract NextGenRandomizerNXT {
     // certain functions can only be called by a global or function admin

     modifier FunctionAdminRequired(bytes4 _selector) {
-      require(adminsContract.retrieveFunctionAdmin(msg.sender, _selector) == true || adminsContract.retrieveGlobalAdmin(msg.sender) == true , "Not allowed");
+      require(adminsContract.retrieveFunctionAdmin(msg.sender, _selector) == true, "Not allowed");
       _;
     }

File: smart-contracts/RandomizerRNG.sol

@@ -33,7 +33,7 @@ contract NextGenRandomizerRNG is ArrngConsumer, Ownable {
     }

     modifier FunctionAdminRequired(bytes4 _selector) {
-        require(adminsContract.retrieveFunctionAdmin(msg.sender, _selector) == true || adminsContract.retrieveGlobalAdmin(msg.sender) == true, "Not allowed");
+        require(adminsContract.retrieveFunctionAdmin(msg.sender, _selector) == true, "Not allowed");
         _;
     }

File: smart-contracts/RandomizerVRF.sol

@@ -45,7 +45,7 @@ contract NextGenRandomizerVRF is VRFConsumerBaseV2, Ownable {
     }

     modifier FunctionAdminRequired(bytes4 _selector) {
-      require(adminsContract.retrieveFunctionAdmin(msg.sender, _selector) == true || adminsContract.retrieveGlobalAdmin(msg.sender) == true , "Not allowed");
+      require(adminsContract.retrieveFunctionAdmin(msg.sender, _selector) == true, "Not allowed");
       _;
     }

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 #584

[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 duplicate of #877

[c4-judge]

alex-ppg marked the issue as duplicate of #303

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Overinflated severity