Undefined Roles in initialize Function Could Disrupt Key Protocol Functions

[hats-bug-reporter]

Github username: --
Twitter username: --
Submission hash (on-chain): 0x02870894f121234218b8745ee998d55aaee058c715e425edb834bf1008aace3b
Severity: high

Description:
The InvestToken contract uses the initialize function to set its initial state, including role assignments through OpenZeppelin's AccessControl library. However, during the audit, it was identified that several critical roles are not explicitly assigned in this function:

• PAUSER_ROLE: Allows pausing the contract.
• MINT_ROLE: Allows minting tokens.
• BURN_ROLE: Allows burning tokens.
• RESCUER_ROLE: Allows rescuing tokens from locked accounts.
• UPGRADER_ROLE: Allows upgrading the contract using the UUPS proxy pattern.

Risk

The lack of initial assignment of these roles presents several risks:

1. Insufficient privileges:
   Without initial role assignment, designated accounts cannot perform critical actions, such as pausing operations or minting tokens.

2. Operational errors:
   Normal contract operation could be impacted as these roles will not be available until they are manually assigned post-initialization.

3. Potential abuse:
   Mismanagement of subsequent role assignment could lead to security vulnerabilities, allowing unauthorized users to gain access to sensitive functions.

Recommendation

To mitigate these risks, it is recommended to assign all critical roles within the initialize function. This ensures that authorized accounts have immediate control over specific contract functionalities.

function initialize(
    string calldata _name,
    string calldata _symbol,
    address _initialOwner,
    IYieldOracle _yieldOracle
) public initializer {
    __ERC20_init(_name, _symbol);
    __ERC20Pausable_init();
    __ERC20Permit_init(_name);
    __AccessControl_init();
    __UUPSUpgradeable_init();

    _grantRole(DEFAULT_ADMIN_ROLE, _initialOwner);
    _grantRole(PAUSER_ROLE, _initialOwner);
    _grantRole(MINT_ROLE, _initialOwner);
    _grantRole(BURN_ROLE, _initialOwner);
    _grantRole(RESCUER_ROLE, _initialOwner);
    _grantRole(UPGRADER_ROLE, _initialOwner);

    yieldOracle = _yieldOracle;
}

Proof of Concept: Role Assignment Verification Test

To validate that the roles are correctly assigned after initialization, the following test can be used:

function testCheckTheRoles() public {
    // Check PAUSER_ROLE
    bytes32 PAUSER_ROLE = keccak256("PAUSER_ROLE");
    assertFalse(
        investToken.hasRole(PAUSER_ROLE, admin),
        "PAUSER_ROLE should not be initialized for admin"
    );
    assertFalse(
        investToken.hasRole(PAUSER_ROLE, address(investToken)),
        "PAUSER_ROLE should not be initialized for contract"
    );

    // Check MINT_ROLE
    bytes32 MINT_ROLE = keccak256("MINT_ROLE");
    assertFalse(
        investToken.hasRole(MINT_ROLE, admin),
        "MINT_ROLE should not be initialized for admin"
    );
    assertFalse(
        investToken.hasRole(MINT_ROLE, address(investToken)),
        "MINT_ROLE should not be initialized for contract"
    );

    // Check BURN_ROLE
    bytes32 BURN_ROLE = keccak256("BURN_ROLE");
    assertFalse(
        investToken.hasRole(BURN_ROLE, admin),
        "BURN_ROLE should not be initialized for admin"
    );
    assertFalse(
        investToken.hasRole(BURN_ROLE, address(investToken)),
        "BURN_ROLE should not be initialized for contract"
    );

    // Check RESCUER_ROLE
    bytes32 RESCUER_ROLE = keccak256("RESCUER_ROLE");
    assertFalse(
        investToken.hasRole(RESCUER_ROLE, admin),
        "RESCUER_ROLE should not be initialized for admin"
    );
    assertFalse(
        investToken.hasRole(RESCUER_ROLE, address(investToken)),
        "RESCUER_ROLE should not be initialized for contract"
    );

    // Check UPGRADER_ROLE
    bytes32 UPGRADER_ROLE = keccak256("UPGRADER_ROLE");
    assertFalse(
        investToken.hasRole(UPGRADER_ROLE, admin),
        "UPGRADER_ROLE should not be initialized for admin"
    );
    assertFalse(
        investToken.hasRole(UPGRADER_ROLE, address(investToken)),
        "UPGRADER_ROLE should not be initialized for contract"
    );

    // Verify admin has DEFAULT_ADMIN_ROLE
    bytes32 DEFAULT_ADMIN_ROLE = 0x00;
    assertTrue(
        investToken.hasRole(DEFAULT_ADMIN_ROLE, admin),
        "Admin should have DEFAULT_ADMIN_ROLE"
    );

    // Log current role holders for debugging
    console.log("Role Check Summary:");
    console.log("DEFAULT_ADMIN_ROLE holder:", investToken.hasRole(DEFAULT_ADMIN_ROLE, admin));
    console.log("PAUSER_ROLE holder:", investToken.hasRole(PAUSER_ROLE, admin));
    console.log("MINT_ROLE holder:", investToken.hasRole(MINT_ROLE, admin));
    console.log("BURN_ROLE holder:", investToken.hasRole(BURN_ROLE, admin));
    console.log("RESCUER_ROLE holder:", investToken.hasRole(RESCUER_ROLE, admin));
    console.log("UPGRADER_ROLE holder:", investToken.hasRole(UPGRADER_ROLE, admin));
}

Test Execution

This test can be run using the following Foundry command:

forge test -vv --match-test testCheckTheRoles

Results

The test confirmed the following:

• ✅ DEFAULT_ADMIN_ROLE is correctly assigned to admin.
• ❌ PAUSER_ROLE is not initialized.
• ❌ MINT_ROLE is not initialized.
• ❌ BURN_ROLE is not initialized.
• ❌ RESCUER_ROLE is not initialized.
• ❌ UPGRADER_ROLE is not initialized.

Although the admin has the DEFAULT_ADMIN_ROLE, they cannot perform critical operations without specific roles. It is essential to modify the initialize function as proposed or implement a migration function to assign the missing roles, ensuring the contract operates as intended and minimizing security risks.

[AndreiMVP]

Not an issue, more like a design choice. Either with the initialization or without is fine imo