UniswapV3Adapter Manual Review Findings

UVA-01M: Improper Integration of Gas Paradigm

Type	Severity	Location
Language Specific		UniswapV3Adapter.sol:L16, L29, L162-L167

Description:

The UniswapV3Adapter codebase attempts to be contextually aware of the gas remaining in a particular transaction and react differently based on this fact, however, it contains a fixed gas cost for the base cost of a transaction as well as improper integration of the gas-based logic.

Impact:

While the code currently executes as expected, future changes to the EVM via EIPs may significantly alter how gas behaves and could cause the _FIXED_GAS_COST_TO_SUPPORT_POOL to invalidate otherwise proper pool additions to the oracle.

Example:

Copy
161if (_currentCardinality < _targetCardinality) {
162  uint32 _gasCostToIncreaseAndAddSupport = uint32(_targetCardinality - _currentCardinality) *
163    _gasCostPerCardinality +
164    _FIXED_GAS_COST_TO_SUPPORT_POOL;
165  if (_gasCostToIncreaseAndAddSupport > gasleft()) {
166    continue;
167  }

Recommendation:

With regards to the fixed gas cost, no gas cost should be assumed as fixed in the EVM ecosystem as EIP proposals that affect gas costs do exist and are routinely integrated to the main chain (such as EIP-2929) which may increase or decrease the gas required for a particular set of instructions thus invalidating the _FIXED_GAS_COST_TO_SUPPORT_POOL value. As far as the gas management code is concerned, we advise it to halt execution instead of continuing if the gasleft does not satisfy the equation. As cardinality for an oracle can only increase / grow, an equation not satisfying the gasleft measurement before the transaction is submitted and after it has been executed cannot change in nature meaning that halting execution at this point will render detection of which _pool needs to be atomically increased in cardinality much easier. To enhance the usability of the contract, we advise the code to instead yield an error with an argument of the _pool that was attempted to be increased in cardinality as well as the delta in cardinality that was detected.

Alleviation:

After extensive discussions with the Mean Finance team and the purpose of the function's "gas awareness" being elaborated, we concluded that the ideal solution to this exhibit is the utilization of a dynamic base cost in addition to a dynamic gas cost per cardinality. This change permits the contract to remain gas-aware whilst being future-proof against any sharp fluctuations in gas costs for the operations performed in the increaseObservationCardinalityNext function. As the solution advised here is implemented in the codebase adequately, we consider this exhibit alleviated.

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

Type	Severity	Location
Code Style		UniswapV3Adapter.sol:L39-L40

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

UVA-03M: Potentially Manipulate-able Liquidity Measurement

Type	Severity	Location
Logical Fault		UniswapV3Adapter.sol:L196

Description:

As indicated in the original Uniswap V3 documentation, the liquidity function yields the current liquidity available in the active tick range and does not yield the liquidity value that is spread across all ticks of the pair thus permitting the order of _getAllPoolsSortedByLiquidity to be relatively cost-effective to manipulate for a malicious party.

Impact:

As the inclusion or adjustment of a pair to the adapter does not currently impose any access control, it is trivial to influence the order in which the pools are presented for a price query to the UNISWAP_V3_ORACLE implementation by influencing the liquidity available in the currently active tick via flash-loans.

Example:

Copy
193// Store liquidity by pool
194uint128[] memory _poolLiquidity = new uint128[](_pools.length);
195for (uint256 i; i < _pools.length; i++) {
196  _poolLiquidity[i] = IUniswapV3Pool(_pools[i]).liquidity();
197}
198
199// Sort both arrays together
200for (uint256 i; i < _pools.length - 1; i++) {
201  uint256 _biggestLiquidityIndex = i;
202  for (uint256 j = i + 1; j < _pools.length; j++) {
203    if (_poolLiquidity[j] > _poolLiquidity[_biggestLiquidityIndex]) {
204      _biggestLiquidityIndex = j;
205    }
206  }
207  if (_biggestLiquidityIndex != i) {
208    // Swap pools
209    (_pools[i], _pools[_biggestLiquidityIndex]) = (_pools[_biggestLiquidityIndex], _pools[i]);
210
211    // Don't need to swap both ways, can just move the liquidity in i to its new place
212    _poolLiquidity[_biggestLiquidityIndex] = _poolLiquidity[i];
213  }
214}

Recommendation:

We advise the rationale behind liquidity-ordered pool sorting to be revised as the IStaticOracle located under @mean-finance/uniswap-v3-oracle does not appear to make use of this trait. Alternatively, if a liquidity based weighting is required the _addOrModifySupportForPair execution flow to be revised potentially by taking advantage of the bytes calldata argument in conjunction with access-controlled addSupportForPairIfNeeded and addOrModifySupportForPair functions.

Alleviation:

The Mean Finance team evaluated this exhibit and elaborated on this particular design choice by stating that whatever pool trait for "ordering" the pools would have been manipulate-able. As a result, they opted to utilize the simplest one (liquidity evaluation) and then to deploy an off-chain bot that constantly monitors the ordering of a particular pair and re-adjusts it should it have changed. As a result of these statements by Mean Finance, we consider this exhibit as an attack vector with minimal-to-none exploitability, however, we will include it within the report as acknowledged to ensure that the Mean Finance team is sufficiently aware of the exhibit's ramifications.