radev_sw - The mutability of roles in `Unitas.sol` renders their utility ineffective. Therefore roles become useless.

[sherlock-admin]

radev_sw

medium

The mutability of roles in Unitas.sol renders their utility ineffective. Therefore roles become useless.

Summary

The mutability of roles in Unitas.sol renders their utility ineffective. Therefore roles become useless. Similarly, the role definitions in ERC20Token.sol, InsurancePool.sol, TimelockController.sol, TokenManager.sol, and XOracle.sol are equally mutable, thus their intended use is compromised.

Vulnerability Detail

Currently creating Unitas.sol contract, the contract necessitates the allocation of specific roles, such as GOVERNOR_ROLE, GUARDIAN_ROLE, TIMELOCK_ROLE, and PORTFOLIO_ROLE. The design implies that only users holding these particular roles have the ability to execute certain functions. For instance, only a user bearing the GUARDIAN_ROLE can trigger pause() and unpause() functions in the Unitas.sol contract. Similarly, in the ERC20Token.sol contract, only a user holding the MINTER_ROLE can activate mint() and burn() functions. There are various such instances.

This kind of attack/abuse is currently hard to track. There is no centralized database of created editions and their admins at the time of creations (i.e. a mapping). This makes it hard to track down malicious creators who create editions for other people. Looping through the emitted events and comparing current admins to the emitted admins is a hassle especially if this protocol gains a lot of traction in the future which I assume is the end goal here.

Impact

Take, for example, the ERC20Token.sol contract and the MINTER_ROLE. A potential bypass to the system's design can be a malicious minter creating an edition for an individual who doesn’t possess the MINTER_ROLE, by creating the edition and granting the MINTER_ROLE to the non-minter using the AccessControl.sol grantRole() function. This way the non-minter can revoke the original minter role in this edition and gain full ownership. Now this non-minter user can execute mint() and burn() functions.

Code Snippet

• Unitas.sol contract code: https://github.com/sherlock-audit/2023-04-unitasprotocol/blob/main/Unitas-Protocol/src/Unitas.sol#L37-L40

• OpenZeppelin AccessControlUpgradeable.sol contract: https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/access/AccessControlUpgradeable.sol#L151-L153

• ERC20Token.sol contract code: https://github.com/sherlock-audit/2023-04-unitasprotocol/blob/main/Unitas-Protocol/src/ERC20Token.sol#L13-L15

• InsurancePool.sol contract code: https://github.com/sherlock-audit/2023-04-unitasprotocol/blob/main/Unitas-Protocol/src/InsurancePool.sol#L14-L18

• TimelockController.sol contract code: https://github.com/sherlock-audit/2023-04-unitasprotocol/blob/main/Unitas-Protocol/src/TimelockController.sol#L24-L28

• TokenManager.sol contract code: https://github.com/sherlock-audit/2023-04-unitasprotocol/blob/main/Unitas-Protocol/src/TokenManager.sol#L21-L22

• XOracle.sol contract code: https://github.com/sherlock-audit/2023-04-unitasprotocol/blob/main/Unitas-Protocol/src/XOracle.sol#L8-L9

• OpenZeppelin AccessControl.sol contract: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/AccessControl.sol#L145-L147

Tool used

Manual Review

Recommendation

In ERC20Token.sol, InsurancePool.sol, TimelockController.sol, TokenManager.sol and XOracle.sol implementations contracts, it is recommended to override the grantRole() function of AccessControl.sol with something like:

Do the same thing for Unitas.sol implementation contract with grantRole() function of AccessControlUpgradeable.sol.

[radeveth]

I think this finding should be valid one.

Links to similar findings: https://github.com/code-423n4/2022-12-escher-findings/issues/521 https://github.com/sherlock-audit/2022-10-nftport-judging/issues/81