Walls will be regenerated more often than market dictates

[code423n4]

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/policies/Operator.sol#L663-L694

Vulnerability details

Counter accumulation logic favors counter increase over decrease as both are conditioned on the earliest, i.e. the most distant from the current, entry situation instead of the previous one, while initially these most distant entries are all false, being not initialized. This way the counters will be allowed to be increased, but not allowed to be decreased, capturing one side of the market movements only.

This way their rationale of controlling (not in exact way as moves can be drastically different, while here it's a direction only logic) market situation and rebuilding when it's needed only isn't met as counter logic is currently biased to the counter growth side.

The impact is system being regenerated when it's not needed over and over again as each _regenerate() will reinit the observations array, leading to the repeat of the counter increase favoring and the accumulation that doesn't correspond to the cumulative market movement.

Proof of Concept

In _addObservation() _status.x.count-- happens only if regen.observations[regen.nextObservation] == true both for x = low and high cases:

https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/policies/Operator.sol#L663-L694

        /// Update low side regen status with a new observation
        /// Observation is positive if the current price is greater than the MA
        uint32 observe = _config.regenObserve;
        Regen memory regen = _status.low;
        if (currentPrice >= movingAverage) {
            if (!regen.observations[regen.nextObservation]) {
                _status.low.observations[regen.nextObservation] = true;
                _status.low.count++;
            }
        } else {
            if (regen.observations[regen.nextObservation]) {
                _status.low.observations[regen.nextObservation] = false;
                _status.low.count--;
            }
        }
        _status.low.nextObservation = (regen.nextObservation + 1) % observe;

        /// Update high side regen status with a new observation
        /// Observation is positive if the current price is less than the MA
        regen = _status.high;
        if (currentPrice <= movingAverage) {
            if (!regen.observations[regen.nextObservation]) {
                _status.high.observations[regen.nextObservation] = true;
                _status.high.count++;
            }
        } else {
            if (regen.observations[regen.nextObservation]) {
                _status.high.observations[regen.nextObservation] = false;
                _status.high.count--;
            }
        }
        _status.high.nextObservation = (regen.nextObservation + 1) % observe;

But regen.nextObservation is next observation, and all such observations will initially come from the uninitialized _status.x.observations array, which elements will be initially false. This way all down-moves for low case and up-moves for high case will be ignored counter-wise as if (regen.observations[regen.nextObservation]) condition will not be satisfied until the _config.regenObserve of ticks be passed.

This implies that, for example, in a 'up-down-up-down-...' sideways market situation all ups will be counted for the low case and all downs will be counted for the high case, i.e. both counters will be equal to accumulated one side only and most likely exceed the regeneration threshold, while as the market didn't really moved the regeneration will be excessive.

_addObservation() is a part of operate() logic which will be called via heartbeats:

https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/policies/Operator.sol#L194-L201

    /// @inheritdoc IOperator
    function operate() external override onlyWhileActive onlyRole("operator_operate") {
        /// Revert if not initialized
        if (!initialized) revert Operator_NotInitialized();

        /// Update the prices for the range, save new regen observations, and update capacities based on bond market activity
        _updateRangePrices();
        _addObservation();

There regeneration logic is triggered when enough time passed and counter is bigger than config_.regenThreshold:

https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/policies/Operator.sol#L208-L220

        /// Check if walls can regenerate capacity
        if (
            uint48(block.timestamp) >= RANGE.lastActive(true) + uint48(config_.regenWait) &&
            _status.high.count >= config_.regenThreshold
        ) {
            _regenerate(true);
        }
        if (
            uint48(block.timestamp) >= RANGE.lastActive(false) + uint48(config_.regenWait) &&
            _status.low.count >= config_.regenThreshold
        ) {
            _regenerate(false);
        }

Recommended Mitigation Steps

The mechanics isn't exact, but the design can be updated to look at the most recent entry, so sideways market won't generate the regeneration, i.e. for the low case as an example:

        /// Update low side regen status with a new observation
        /// Observation is positive if the current price is greater than the MA
        uint32 observe = _config.regenObserve;
        Regen memory regen = _status.low;
+       uint32 lastObservation = regen.nextObservation == 0 ? observe - 1 : regen.nextObservation - 1;
        if (currentPrice >= movingAverage) {
-           if (!regen.observations[regen.nextObservation]) {
+           if (!lastObservation) {
                _status.low.observations[regen.nextObservation] = true;
                _status.low.count++;
            }
        } else {
-           if (regen.observations[regen.nextObservation]) {
+           if (lastObservation) {
                _status.low.observations[regen.nextObservation] = false;
                _status.low.count--;
            }
        }
        _status.low.nextObservation = (regen.nextObservation + 1) % observe;

[Oighty]

The observations arrays use a circular buffer to track the last regenObserve price observations. Initializing the observation arrays to all false requires the system to making regenThreshold positive observations before regenerating a side. If initialized to some true values, it could happen more quickly.

It is expected that regenerations will occur during "stable" price period (price is between the cushions/walls). They may be superfluous, but it ensures that the sides have the proper capacity in the event of large market movements.

Based on this, I don't see an issue with the design. Can you elaborate on how this negatively impacts the protocol?

[dmitriia]

Well, if you look for the design pitfalls, the main one, as it's mentioned in the description, is that number of the moves is quite irrelevant metric for the market movement. You know, for example, that market decline oftentimes happens in a quick and brutal manner (i.e. number of moves is small, size of moves is big). In the same time market tend to rise slowly. To count the number of moves of a given direction looks to be a pointless activity in this regard: marker can do a quick big drop or climb for a long time, but gaining not so much as a result.

I.e. the relevant metric is the overall size of the total move, not the number of the moves in the direction.

Here, on one hand the count is measured, on the other hand it's measured wrongly as array initialization does mess up the frequency. Overall impact for the project is that walls rebuilding will be performed without real alignment to the market movements as moves are counted, instead of being accumulated, and the rebuilding itself be done too frequently vs the stated logic without any need to do so, just per glitch in the implementation.

[Oighty]

I disagree that the array initialization messes up the count. Counts are kept separately for the high and low sides. We initialize the count to 0 on initialization and the observation arrays are initialized to all false. Therefore, the system must observe a minimum of regenThreshold positive observations on a given side to be regenerated on that side. If the array was initialized with some true values and the count was non-zero, this number would be less and the configuration of regenThreshold would be inaccurate.

As for whether the design could be improved by using a different type of metric, that may be the case, but it would rely on additional external data that is not available via a suitable oracle.

[Oighty]

As for the up-down-up-down scenario not counting one direction of movement, they will be reflected if the previous value in that slot of the circular buffer is the opposite value. E.g. if there is a true value in slot 2 and then the buffer circles around again, if the observation is negative, it will switch the value to false and reduce the count.

[0xean]

this comes down to a design question at this point and cannot be awarded inside of the M criteria as the protocol is function as expected per the sponsor. Downgrading to QA. Closing as dupe of #500 the wardens QA report.