OracleAggregator Manual Review Findings

OAR-01M: Centralization of Oracle Aggregator Operation

Type	Severity	Location
Centralization Concern		OracleAggregator.sol:L100-L108, L111-L135

Description:

The OracleAggregator module of the Mean Finance ecosystem is significantly centralized as the ADMIN_ROLE members can overrule any previously defined oracle on-chain and the same members are responsible for providing the list of "available" oracles for selection for particular pairs.

Example:

Copy
100function forceOracle(
101  address _tokenA,
102  address _tokenB,
103  address _oracle,
104  bytes calldata _data
105) external onlyRole(ADMIN_ROLE) {
106  _revertIfNotOracle(_oracle);
107  _setOracle(_tokenA, _tokenB, ITokenPriceOracle(_oracle), _data, true);
108}

Recommendation:

We advise this trait of the system to be revised and a more decentralized approach to be utilized such as the usage of DAOs to invoke these functions instead of a permissible role and timelocks to be put in place for highly sensitive functions such as the forceOracle one as its misuse can significantly impact the protocol.

Alleviation:

The Mean Finance team elaborated that the current ADMIN_ROLE owner in the system will be a multi-signature wallet and potentially a timelock contract that enforces a delay in the transactions executed. In the future, the role will be transferred to a DAO rendering the forceOracle workflow more decentralized. As a result of these statements, we consider this exhibit acknowledged and to-be alleviated in a future iteration of the protocol.

OAR-02M: Ill-Advised Self-Ownership of Role

Type	Severity	Location
Code Style		OracleAggregator.sol:L24-L25

Description:

The SUPER_ADMIN_ROLE is owned by itself permitting the role to be arbitrarily assigned and a single assignee to be able to overtake the whole system by removing the role from other parties in the system.

Example:

Copy
24// We are setting the super admin role as its own admin so we can transfer it
25_setRoleAdmin(SUPER_ADMIN_ROLE, SUPER_ADMIN_ROLE);

Recommendation:

To prevent mis-use and generally increase the resilience of the system, we advise dedicated "transfer of ownership" workflows to be coded in the contract that make use of the internal functions exposed by the AccessControl implementation and to avoid assigning self-ownership of the role via the _setRoleAdmin function.

Alleviation:

The Mean Finance evaluated this exhibit and opted to retain the current behaviour in place citing a relevant issue of the original OpenZeppelin dependency as rationale that a manual transfer of a role would still be flawed in regards to the system being compromised. We would like to clarify that what we advised is that special "transfer of ownership" workflows are introduced to the contract which can in turn be time-delayed or guarded by other such security features that permit an incorrect / malicious transfer to be cancelled rather than be exploited immediately. In any case, we consider this exhibit acknowledged as the Mean Finance team has opted to retain the current paradigm across all contracts.