StatefulChainlinkOracle Manual Review Findings

SCO-01M: Centralization of Chainlink Price Evaluation

Type	Severity	Location
Centralization Concern		StatefulChainlinkOracle.sol:L109-L115

Description:

The StatefulChainlinkOracle contains a significant centralization point whereby the "mapping" between tokens can be arbitrarily assigned thus allowing complete control over what quote is yielded by the StatefulChainlinkOracle to callers from the ADMIN_ROLE.

Example:

Copy
108/// @inheritdoc IStatefulChainlinkOracle
109function addMappings(address[] calldata _addresses, address[] calldata _mappings) external onlyRole(ADMIN_ROLE) {
110  if (_addresses.length != _mappings.length) revert InvalidMappingsInput();
111  for (uint256 i = 0; i < _addresses.length; i++) {
112    _tokenMappings[_addresses[i]] = _mappings[i];
113  }
114  emit MappingsAdded(_addresses, _mappings);
115}

Recommendation:

While mapping functionality is desired, we advise the way it is setup to be revised either by ensuring that the mappings are set up once during the contract's constructor (less flexibility, more security) or by establishing more rigorous workflows such as a DAO in place of an arbitrarily assigned role or a timelock-based approach to setting new entries.

Alleviation:

The Mean Finance team evaluated this exhibit and considered to retain the current centralization in place as it satisfies their purposes. As a result, we consider this exhibit acknowledged.

SCO-02M: Potentially Incorrect Notion of Oracle Pairs

Type	Severity	Location
Language Specific		StatefulChainlinkOracle.sol:L215-L216

Description:

The linked comments make reference of "stablecoin" oracles when mentioning validation of the ETH_USD_PAIR which is incorrect as all stablecoins regardless of type (reserve-backed, algorithmic etc.) do possess a natural deviation from the actual dollar and thus should not be regarded as "USD" oracles.

Example:

Copy
214if ((_isTokenAETH && _isTokenBUSD) || (_isTokenAUSD && _isTokenBETH)) {
215  // Note: there are stablecoins/ETH pairs on Chainlink, but they are updated less often than the USD/ETH pair.
216  // That's why we prefer to use the USD/ETH pair instead
217  return PricingPlan.ETH_USD_PAIR;
218} else if (_isTokenBUSD) {

Recommendation:

We advise this to be properly assimilated by the Mean Finance team as otherwise the contract may be misconfigured to utilize a stablecoin as a "USD" feed in future iterations of the code as well as other system modules.

Alleviation:

The incorrect comments have been removed from the codebase as the Mean Finance team stated this is leftover code from previous notions whereby USD stablecoins were treated as a special case which is no longer true. As a result, we consider this exhibit dealt with.

SCO-03M: Ill-Advised Self-Ownership of Role

Type	Severity	Location
Code Style		StatefulChainlinkOracle.sol:L36-L37

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
36// We are setting the super admin role as its own admin so we can transfer it
37_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.