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

[c4-bot-2]

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

    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

* [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.

[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

    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.

    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

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #20

[c4-judge]

HickupHH3 marked the issue as satisfactory

[c4-judge]

HickupHH3 marked the issue as selected for report

[HickupHH3]

selecting as best report because it also mentions that _grantRole should be used instead of _setupRole

[iceBearRobot]

Dear Judge: This issue have been covered by 4naly3er: https://github.com/code-423n4/2024-02-ai-arena/blob/main/4naly3er-report.md#l-1-do-not-use-deprecated-library-functions Automated Findings / Publicly Known Issues section is considered a publicly known issue and is ineligible for awards.

Also provide references to the same issue in 2023-08-dopex for comparison. https://github.com/code-423n4/2023-08-dopex-findings/issues/871

Thanks for your reply!!

[Haxatron]

QA (Centralization Risk). For this to be an actual issue, you would need to compromise either the admin or the minter / staker / spender contracts.

[0xbtk]

Hey @iceBearRobot and @Haxatron,

The issue isn't about deprecated functions or compromising roles.

It's about the devs not initializing the DEFAULT_ADMIN_ROLE correctly, which makes all roles irrevocable. This doesn't pose a risk of centralization. Take another look at the issue please.

[Haxatron]

For that to be an actual issue, the admin must either be compromised, accidentally assign the roles to wrong address or compromise the contracts assigned this role.

[0xbtk]

@Haxatron Haven't you ever encountered protocols where an owner assigns a role to an address(Not by mistake), and then revoked it later on when the role is no longer needed?

This falls under:

Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

[Haxatron]

The address will be immutable contracts, if they are EOAs then maybe it will be a different story.

I will defer to the judge for this one.

[McCoady]

Given some of the reports mention being unable to revoke in the case of malicious actors or leaked private keys, some added context from the sponsor during the contest:

This meaning both the above are impossible barring significant error on the part of the protocol team (assigning roles to a wrong address (an EOA)). Whether or not being unable to revoke previously trusted contracts warrants a med is up to the judge's decision, but thought the added context would be useful.

[HickupHH3]

Yes, I'm excluding admin error in this. Simply about functionality: not being able to revoke roles might be problematic for deprecation / migration purposes