code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/roles/AdminRole.sol#L28

Vulnerability details

Impact

AdminRole mixin exposes critical functions without any restrictions like grantAdmin() revokeAdmin()

Proof of Concept

Criticial functions like grantAdmin() can be externally accessed changing the critical roles like admin.

// for eg: 
  function grantAdmin(address account) external {
    grantRole(DEFAULT_ADMIN_ROLE, account);
  }

Tools Used

Manual Analysis

Recommended Mitigation Steps

Converting the funcitons inside mixins to internal might help.

HardlyDifficult commented 1 year ago

Invalid. These calls are convenience wrappers, exposing an ABI that allows users to interact with roles without having to deal with or understand the role hash. They call grantRole or revokeRole which are public functions in the OpenZeppelin implementation -- inside the OpenZeppelin, these functions will enforce access control. e.g. function grantRole(...) onlyRole(getRoleAdmin(role)) (same for revokeRole).

HickupHH3 commented 1 year ago

Yeap, OZ enforces the caller check. Invalid.

code-423n4 / 2022-08-foundation-findings