TransformerOracle Manual Review Findings

TOE-01M: Ill-Advised Self-Ownership of Role

Type	Severity	Location
Code Style		TransformerOracle.sol:L35-L36

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

TOE-02M: Potential Race Condition of Price Quote

Type	Severity	Location
Language Specific		TransformerOracle.sol:L51-L57, L59-L65

Description:

The shouldMapToUnderlying and avoidMappingToUnderlying functions can be invoked at any point in time by the ADMIN_ROLE thus permitting a malicious ADMIN_ROLE to attempt a slippage attack by detecting a high-value evaluation transaction that depends on the quote function and "disabling" the transformer of both (or either of) the two assets and skewing the transaction's result to their benefit.

Impact:

Generally, EIP-4626 shares use a "smaller" denomination or amount than the original deposit. As a result, a user may have set a slippage check on the "end" value of the deposit in EIP-4626 shares that is much smaller than what a trade to the actual asset should guarantee which could be exploited to perform an unfavourable trade and thus incur a high slippage for the original transaction submitter at the benefit of the malicious ADMIN_ROLE party. This type of issue can also happen inadvertently if the shouldMapToUnderlying or avoidMappingToUnderlying is mis-used.

Example:

Copy
51/// @inheritdoc ITransformerOracle
52function shouldMapToUnderlying(address[] calldata _dependents) external onlyRole(ADMIN_ROLE) {
53  for (uint256 i; i < _dependents.length; i++) {
54    willAvoidMappingToUnderlying[_dependents[i]] = false;
55  }
56  emit DependentsWillMapToUnderlying(_dependents);
57}
58
59/// @inheritdoc ITransformerOracle
60function avoidMappingToUnderlying(address[] calldata _dependents) external onlyRole(ADMIN_ROLE) {
61  for (uint256 i; i < _dependents.length; i++) {
62    willAvoidMappingToUnderlying[_dependents[i]] = true;
63  }
64  emit DependentsWillAvoidMappingToUnderlying(_dependents);
65}

Recommendation:

We advise some protection mechanism to be put in place against this type of timing attack (or race-condition) that can currently occur on-chain. Solutions include introducing an additional parameter to the _data payload of quote that validates the "transformer" status of each token or enforcing a time-delay in the state change of the willAvoidMappingToUnderlying mapping either of which is an adequate remediation to this exhibit.

Alleviation:

The Mean Finance team acknowledged that taking advantage of the _data payload in the quote function would have been an ideal solution, however, they have opted to retain the current practice in place as they believe the gas benefit from off-chain workflows outweighs the risk of centralization. As a result, we consider this exhibit acknowledged.

TOE-03M: Potentially Restrictive Underlying Disassociation

Type	Severity	Location
Logical Fault		TransformerOracle.sol:L52-L57, L60-L65

Description:

The shouldMapToUnderlying and avoidMappingToUnderlying functions are meant to be utilized to allow a particular token to not query its transformer even if it exists which is useful in cases such as wrapper tokens (WETH / ETH) whereby oracles may utilize either of the two assets for a particular trade pair as evidenced in the documentation of ITransformerOracle. While the intention is sound, the current implementation is rather restrictive as some pairs may use WETH and others may use ETH whilst toggling the value of the willAvoidMappingToUnderlying to either false or true will cause either all ETH or all WETH pairs to be used rather than both in conjunction.

Impact:

The current system will "lock-out" an asset from being queried using its wrapped / identical-value counterpart due to the current design of the system leading to a decrease in the total available oracles for the underlying asset.

Example:

Copy
51/// @inheritdoc ITransformerOracle
52function shouldMapToUnderlying(address[] calldata _dependents) external onlyRole(ADMIN_ROLE) {
53  for (uint256 i; i < _dependents.length; i++) {
54    willAvoidMappingToUnderlying[_dependents[i]] = false;
55  }
56  emit DependentsWillMapToUnderlying(_dependents);
57}
58
59/// @inheritdoc ITransformerOracle
60function avoidMappingToUnderlying(address[] calldata _dependents) external onlyRole(ADMIN_ROLE) {
61  for (uint256 i; i < _dependents.length; i++) {
62    willAvoidMappingToUnderlying[_dependents[i]] = true;
63  }
64  emit DependentsWillAvoidMappingToUnderlying(_dependents);
65}

Recommendation:

We advise a double mapping to be utilized instead either along or without the "global" flag that allows the transformer to be queried on a case-by-case basis thus exposing a greater degree of granularity to the transformer oracle system.

Alleviation:

A specialized pairSpecificMappingConfig has been introduced to the codebase that makes use of a key calculated from both token addresses of a particular pair to assess whether the transformer should be utilized or not. The system was introduced alongside the existing willAvoidMappingToUnderlying system ensuring backwards compatibility.