Minter / Staker / Spender roles can never be revoked`..,

mediumCode4rena

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/Neuron.sol#L93-L96 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/access/AccessControl.sol#L40-L47 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/access/AccessControl.sol#L194-L207

Vulnerability details

Impact

From Neuron::addMinter and AccessControl::setupRole

solidity📋
    function addMinter(address newMinterAddress) external {
        require(msg.sender == _ownerAddress);
        _setupRole(MINTER_ROLE, newMinterAddress);
    }

    function _setupRole(bytes32 role, address account) internal virtual {
        _grantRole(role, account);
    }

    function _grantRole(bytes32 role, address account) internal virtual {
        if (!hasRole(role, account)) {
            _roles[role].members[account] = true;
            emit RoleGranted(role, account, _msgSender());
        }
    }

• Roles of minter, staker, spender can never be revoked due to missing default admin implementation. The DEFAULT_ADMIN_ROLE = 0x00 which is default role which is admin to all the roles, and the real contract owner should own this role, since it is not granted, the owner cannot govern the roles.

• Another wrong implemnatation is using _setupRole to grant roles intead of using _grantRole, because of the warnings in the library contract.

From Openzeppelin::AccessControl and AccessControl::setupRole

solidity📋

* [WARNING]
    * ====
    * This function should only be called from the constructor when setting
    * up the initial roles for the system.
    *
    * Using this function in any other way is effectively circumventing the admin
    * system imposed by {AccessControl}.
    * ====
    *
    * NOTE: This function is deprecated in favor of {_grantRole}.
    */
    function _setupRole(bytes32 role, address account) internal virtual {
        _grantRole(role, account);
    }

    * Roles can be granted and revoked dynamically via the {grantRole} and
    * {revokeRole} functions. Each role has an associated admin role, and only
    * accounts that have a role's admin role can call {grantRole} and {revokeRole}.
    *
    * By default, the admin role for all roles is `DEFAULT_ADMIN_ROLE`, which means
    * that only accounts with this role will be able to grant or revoke other
    * roles. More complex role relationships can be created by using
    * {_setRoleAdmin}.
    *
    * WARNING: The `DEFAULT_ADMIN_ROLE` is also its own admin: it has permission to
    * grant and revoke this role. Extra precautions should be taken to secure
    * accounts that have been granted it.
    */

Proof of Concept

• As you can see the owner cannot revoke becasue there is no admin for that role, owner should be a default admin for all the roles granted.

sh📋
[PASS] test_POC_Revoke() (gas: 72392)
Traces:
  [72392] NeuronTest::test_POC_Revoke() 
    ├─ [27181] Neuron::addMinter(NeuronTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496]) 
    │   ├─ emit RoleGranted(role: 0x9f2df0fed2c77648de5860a4cc508cd0818c85b8b8a1ab4ceeef8d981c8956a6, account: NeuronTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], sender: NeuronTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
    │   └─ ← ()
    ├─ [0] VM::expectRevert() 
    │   └─ ← ()
    ├─ [34420] Neuron::revokeRole(0x9f2df0fed2c77648de5860a4cc508cd0818c85b8b8a1ab4ceeef8d981c8956a6, NeuronTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496]) 
    │   └─ ← "AccessControl: account 0x7fa9385be102ac3eac297483dd6233d62b3e1496 is missing role 0x0000000000000000000000000000000000000000000000000000000000000000"
    └─ ← ()

-Now paste the below POC into test/Neuron.t.sol and run forge t --mt test_POC_Revoke -vvvv

solidity📋

    function test_POC_Revoke() external {
        _neuronContract.addMinter(_ownerAddress);

        vm.expectRevert();
        _neuronContract.revokeRole(keccak256("MINTER_ROLE"), _ownerAddress);
    }

Tools Used

Manual + Foundry testing

Recommended Mitigation Steps

Modify the constructor on Neuron.sol to grant default admin role to owner.

diff📋
    constructor(address ownerAddress, address treasuryAddress_, address contributorAddress)
        ERC20("Neuron", "NRN")
    {
        _ownerAddress = ownerAddress;
        treasuryAddress = treasuryAddress_;
        isAdmin[_ownerAddress] = true;
        _mint(treasuryAddress, INITIAL_TREASURY_MINT);
        _mint(contributorAddress, INITIAL_CONTRIBUTOR_MINT);

+       _grantRole(DEFAULT_ADMIN_ROLE, _ownerAddress);
    }

Assessed type

Access Control