Mento A-1

The MedianDeltaBreaker contract implements an Exponential Moving Average (EMA) to calculate the percentage difference between the current reported median and the previous median rate EMA value, calculated from the following formula:

currentMedian * smoothingFactor + previousMedianEMA * (1 - smoothingFactor)

Where

• smoothingFactor is set for each specific rateFeedID with setSmoothingFactor() and can’t be higher than 1.
• currentMedian is the externally reported value from the SortedOracles.medianRate().

This allows the MedianDeltaBreaker to weigh the latest median provided by oracles (reported through SortedOracles.report() and read through SortedOracles.medianRate()) and consider the previously stored value.

function shouldTrigger(address rateFeedID) public returns (bool triggerBreaker) {
  (uint256 currentMedian, ) = sortedOracles.medianRate(rateFeedID);

  uint256 previousRatesEMA = medianRatesEMA[rateFeedID];
  ...
  FixidityLib.Fraction memory smoothingFactor = FixidityLib.wrap(getSmoothingFactor(rateFeedID));
  medianRatesEMA[rateFeedID] = FixidityLib
    .wrap(currentMedian)
    .multiply(smoothingFactor)
    .add(FixidityLib.wrap(previousRatesEMA).multiply(FixidityLib.fixed1().subtract(smoothingFactor)))
    .unwrap(); 

  return exceedsThreshold(previousRatesEMA, currentMedian, rateFeedID);
}

However, this implementation will only remain true if shouldTrigger() function is called once per value update in the SortedOracle median rate; and since the shouldTrigger() function is public (as per the IBreaker.sol interface) and doesn’t have any access control, any external actor can perform any arbitrary shouldTrigger() calls incorrectly updating the medianRatesEMA values with the same reported medianRate.

Remediations to Consider

• Updating the medianRatesEMA only if a new report was provided for that specific rateFeedID or
• Adding access control to this function and allowing only to call it by SortedOracles or BreakerBox contracts.

Note: For the second suggestion, it's worth adding an additional view function to allow users or external points to check this condition.

When the BiPoolManager creates an exchange, after performing several validations on the inputs, the token precision multipliers are set in order to deal with assets with different token decimals while calculating swap amounts.

For example, USDC has 6 decimal places, having a token precision multiplier of 1e12 (10 ** (18 - 6)), as we can see in the function’s logic:

tokenPrecisionMultipliers[exchange.asset0] = 10**(18 - uint256(IERC20Metadata(exchange.asset0).decimals()));
tokenPrecisionMultipliers[exchange.asset1] = 10**(18 - uint256(IERC20Metadata(exchange.asset1).decimals()));

However, the function setTokenPrecisionMultipliers allows to arbitrarily change the token precision multipliers, in this case, say for USDC, from 1e12 to an arbitrarily large or small value. Which would then affect the calculations in the swap in/out and in the get amounts in/out functions.

Additionally, the function doesn’t check whether the tokens are valid and registered with the Reserve contract.

Remediations to Consider

• Perform the same proper input validations implemented in the createExchange function or
• Remove the setTokenPrecisionMultipliers from the contract.

When creating an exchange in the BiPoolManager contract, the pricing module is specified as an input parameter in the PoolExchange _exchange struct. However, there are no validations that the address of the pricing module corresponds to a canonical version of this module. This can cause, for example, the previous version of the constant sum module to be used in the exchange creation and, therefore, to calculate swap amounts incorrectly. The only check performed during swaps is in the pricingModuleIdentifier:

function pricingModuleIdentifier(PoolExchange memory exchange) internal view returns (bytes32) {
  return keccak256(abi.encodePacked(exchange.pricingModule.name()));
} 

This also opens up other scenarios where invalid pricing module addresses can be used.

Remediations to Consider

Keeping in the BiPoolManager contract’s storage the address of the latest or allowed pricing modules and asserting it’s used in createExchange.

The MedianDeltaBreaker contract implements an EMA (Exponential Moving Average) that weights the current median rate and adds as a consideration the previously stored median EMA rate; this mitigates immediate price spikes in the median but also allows constant trends in the price to move without triggering the breaker.

As we can see in the shouldTrigger() logic, there is a special consideration when the previousRatesEMA == 0. This condition will only be true in the initial call after enabling this breaker for a specific rateFeedID.

function shouldTrigger(address rateFeedID) public returns (bool triggerBreaker) {
    ...
  uint256 previousRatesEMA = medianRatesEMA[rateFeedID];
  if (previousRatesEMA == 0) {
    // Previous recorded EMA will be 0 the first time this rate feed is checked.
    medianRatesEMA[rateFeedID] = currentMedian;
    return false;
  }
    ...
}

However, since this function will store the current median without any constraints, the reported median, at the execution time of shouldTrigger() call, could be outside of the expected values or manipulated. This will render an invalid median that will be used as a reference to check every subsequent oracle report for that rateFeedID.

Remediations to Consider

• Adding an expected range of median values where the initial medianRatesEMA value will be stored and used as reference, or
• Setting an initial reference median value, similar to the ValueDeltaBreaker contract.

The trading limits act as a safeguard and are checked during the swap operation if the trading config was created for the token. A valid configuration should not permit setting these limits to 0 for enabled directional trading flags.

The function Broker.configureTradingLimit() doesn’t validate the above criteria when setting the trading config for a token.

In addition, there are no checks to ensure that the trading limit for L0 is smaller than the trading limit for L1, and they’re both smaller than the global trading limit LG.

Remediations to Consider

Consider validating the trading config inputs to ensure that invalid trading limits cannot be set.

In BreakerBox.sol, when turning on the breakers for certain rateFeedIds, there will be an execution gap between the toggleBreaker() call and the comparison of the corresponding breaker trigger conditions in checkAndSetBreakers().

Since the BiPoolManager contract uses the view function getRateFeedTradingMode() before executing swaps, which reads the current breaker status but doesn’t update each dependency:

function getRateFeedTradingMode(address rateFeedID) external view returns (uint8) {
require(rateFeedStatus[rateFeedID], "Rate feed ID has not been added");
uint8 tradingMode = rateFeedTradingMode[rateFeedID];
for (uint256 i = 0; i < rateFeedDependencies[rateFeedID].length; i++) {
  tradingMode = tradingMode | rateFeedTradingMode[rateFeedDependencies[rateFeedID][i]];
}
return tradingMode;
}

It is possible that the conditions that trigger a breaker may already be present. However, until the next oracle feed SortedOracles::report(), or external call to checkAndSetBreakers(), the trading mode won’t consider this breaker. This is especially visible with the MedianDeltaBreaker, where the first update will only store the median rate.

Remediations to Consider

• Calling checkAndSetBreakers when adding breakers to a specific rateFeedID in the toggleBreaker logic or,
• Make sure to execute a checkAndSetBreakers atomically call after enabling a breaker.

In MedianDeltaBreaker, the initial value and start point, where medianRatesEMA == 0, only exist the first time a specific rateFeedID is set for this breaker. When a rateFeedID needs to be removed and later added again into the MedianDeltaBreaker, there is no way to reset the initial reference value, which means it cannot easily adjust to newly established rates. It will require a custom higher configuration of the breaker threshold to temporarily bypass the reported values.

Remediations to Consider

Implementing a reset function if the breaker is disabled from the BreakerBox contract to allow a new initialization of the median rates.

The following require statement is used in the internal functions_getAmountIn and _getAmountOut contained in the BiPoolManager.sol. This is repeated code that could be refactored into a function modifier.

//The following require statement is used in multiple functions 
require(
      (tokenIn == exchange.asset0 && tokenOut == exchange.asset1) ||
        (tokenIn == exchange.asset1 && tokenOut == exchange.asset0),
      "tokenIn and tokenOut must match exchange"
    );

Remediations to Consider

Consider converting this validation code to a function modifier and using it for the internal functions _getAmountOut and _getAmountIn.

In the BreakerBox contract, a rateFeedId could have a set of rate feed dependencies mapped to it via the setRateFeedDependencies function.

However, when a rate feed is removed, its dependencies are not deleted.

Remediations to Consider

Consider adding the following line to the removeRateFeed function

delete rateFeedDependencies[rateFeedID];

Consider adding the corresponding natspec comments to these structs.

1. In the BiPoolManager docs page, the Executing Swaps section mentions the following:

   This function executes, in that it prices the swap and updates the virtual bucket sizes of the PoolExchange.

   However, it doesn’t consider the case where an exchange uses the ConstantSumPricingModule, which doesn’t update the bucket sizes on swaps.

2. On the Pricing Modules page, both pricing module links to GitHub are broken. Also, the constant sum pricing module formula should be updated to reflect the newly implemented value constant sum formula.

3. Some of the links included in the documentation pages are currently broken:

   • The config contract link is broken on the deployment parameters page: https://docs.mento.org/mento/developers/deployments/parameters
   • Github links in the BreakerBox section: https://docs.mento.org/mento/developers/smart-contracts/breakerbox

4. Consider adding documentation pages for the specific breaker contract implementations (MedianDeltaBreaker and ValueDeltaBreaker).

In BiPoolManager.sol, when asserting the asset0 and asset1 decimals and setting the tokenPrecisionMultipliers, createExchange() function performs the same external call twice for each operation.

if (IERC20Metadata(exchange.asset0).decimals() > 18) {
  revert("asset0 decimals must be <= 18");
}
if (IERC20Metadata(exchange.asset1).decimals() > 18) {
  revert("asset1 decimals must be <= 18");
}

tokenPrecisionMultipliers[exchange.asset0] = 10**(18 - uint256(IERC20Metadata(exchange.asset0).decimals()));
tokenPrecisionMultipliers[exchange.asset1] = 10**(18 - uint256(IERC20Metadata(exchange.asset1).decimals()));

Consider saving these values to memory to save gas and avoid unnecessary external calls.

Consider using the return values in the function declaration to improve readability and consistency with other functions.

function getOracleExchangeRate(address target) internal view returns (uint256 rateNumerator, uint256 rateDenominator) {
    (rateNumerator, rateDenominator) = sortedOracles.medianRate(target);
    require(rateDenominator > 0, "exchange rate denominator must be greater than 0");
  }

In StableTokenV2, the storage layout implemented deprecated variables for unused slots from the previous StableToken implementation. However, for slots 8 and 12, inflationState and exchangeRegistryId respectively, there aren’t any deprecated variables declarations. These slots are included in the __gap reserve variable.

Consider adding deprecated variables for these slots to keep track of these unused but non-empty variables in case of any future implementation migration.