SAAJ - No check for empty array can result in unexpected outcome

[sherlock-admin]

SAAJ

medium

No check for empty array can result in unexpected outcome

Summary

Functions that have array parameters does not have check to avoid passing empty array arguments which can lead to failure of operations or others unexpected outcome.

Vulnerability Detail

No check for empty array can result in unexpected outcome

Impact

Transactions can be processed without any data inside when empty array will be passed inside functions _updateAssetPriceFeeds, updateAssetMovingAverage and _updateAssetMovingAverage that will result in certain unexpected behaviour like operation failure.

Code Snippet

https://github.com/sherlock-audit/2023-11-olympus/blob/main/bophades/src/modules/PRICE/OlympusPrice.v2.sol#L455 https://github.com/sherlock-audit/2023-11-olympus/blob/main/bophades/src/modules/PRICE/OlympusPrice.v2.sol#L611 https://github.com/sherlock-audit/2023-11-olympus/blob/main/bophades/src/modules/PRICE/OlympusPrice.v2.sol#L651

Tool used

Manual Review

Recommendation

The recommendation is made for having require condition that checks empty array is not passed as input to the functions of _updateAssetPriceFeeds, updateAssetMovingAverage and _updateAssetMovingAverage.

    function updateAssetMovingAverage(
        address asset_,
        bool storeMovingAverage_,
        uint32 movingAverageDuration_,
        uint48 lastObservationTime_,
        uint256[] memory observations_
    ) external override permissioned {
        // Ensure asset is already added
        if (!_assetData[asset_].approved) revert PRICE_AssetNotApproved(asset_);
+   (observations_.length > 0, “observations array cannot be empty”);

        // If not storing the moving average, validate that it's not being used by the strategy.
        // If it is, then you are moving from storing a moving average to not storing a moving average.
        // First, change the strategy to not use the moving average, then update the moving average data.
        if (_assetData[asset_].useMovingAverage && !storeMovingAverage_)
            revert PRICE_ParamsStoreMovingAverageRequired(asset_);

        _updateAssetMovingAverage(
            asset_,
            storeMovingAverage_,
            movingAverageDuration_,
            lastObservationTime_,
            observations_
        );

        // Emit event
        emit AssetMovingAverageUpdated(asset_);
    }

function updateAssetPriceFeeds(
        address asset_,
        Component[] memory feeds_
    ) external override permissioned {
        // Ensure asset is already added
        if (!_assetData[asset_].approved) revert PRICE_AssetNotApproved(asset_);
+   (feeds_.length > 0, “feeds array cannot be empty”);

        _updateAssetPriceFeeds(asset_, feeds_);

        // Validate the configuration
        _getCurrentPrice(asset_);

        // Emit event
        emit AssetPriceFeedsUpdated(asset_);
    }

    function _updateAssetMovingAverage(
        address asset_,
        bool storeMovingAverage_,
        uint32 movingAverageDuration_,
        uint48 lastObservationTime_,
        uint256[] memory observations_
    ) internal {
        Asset storage asset = _assetData[asset_];
+   (observations_.length > 0, “observations array cannot be empty”);

        // Remove existing cached or moving average data, if any
        if (asset.obs.length > 0) delete asset.obs;

        // Ensure last observation time is not in the future
        if (lastObservationTime_ > block.timestamp)
            revert PRICE_ParamsLastObservationTimeInvalid(
                asset_,
                lastObservationTime_,
                0,
                uint48(block.timestamp)
            );

        if (storeMovingAverage_) {
            // If storing a moving average, validate params
            if (movingAverageDuration_ == 0 || movingAverageDuration_ % observationFrequency != 0)
                revert PRICE_ParamsMovingAverageDurationInvalid(
                    asset_,
                    movingAverageDuration_,
                    observationFrequency
                );

            uint16 numObservations = uint16(movingAverageDuration_ / observationFrequency);
            if (observations_.length != numObservations)
                revert PRICE_ParamsInvalidObservationCount(
                    asset_,
                    observations_.length,
                    numObservations,
                    numObservations
                );

            asset.storeMovingAverage = true;

            asset.movingAverageDuration = movingAverageDuration_;
            asset.nextObsIndex = 0;
            asset.numObservations = numObservations;
            asset.lastObservationTime = lastObservationTime_;
            asset.cumulativeObs = 0; // reset to zero before adding new observations
            for (uint256 i; i < numObservations; ) {
                if (observations_[i] == 0) revert PRICE_ParamsObservationZero(asset_, i);

                asset.cumulativeObs += observations_[i];
                asset.obs.push(observations_[i]);
                unchecked {
                    ++i;
                }
            }

            // Emit Price Stored event for new cached value
            emit PriceStored(asset_, observations_[numObservations - 1], lastObservationTime_);
        } else {
            // If not storing the moving average, validate that the array has at most one value (for caching)
            if (observations_.length > 1)
                revert PRICE_ParamsInvalidObservationCount(asset_, observations_.length, 0, 1);

            asset.storeMovingAverage = false;
            asset.movingAverageDuration = 0;
            asset.nextObsIndex = 0;
            asset.numObservations = 1;
            if (observations_.length == 0) {
                // If no observation provided, get the current price and store it
                // We can do this here because we know the moving average isn't being stored
                // and therefore, it is not being used in the strategy to calculate the price
                (uint256 currentPrice, uint48 timestamp) = _getCurrentPrice(asset_);
                asset.obs.push(currentPrice);
                asset.lastObservationTime = timestamp;

                // Emit Price Stored event for new cached value
                emit PriceStored(asset_, currentPrice, timestamp);
            } else {
                // If an observation is provided, validate it and store it
                if (observations_[0] == 0) revert PRICE_ParamsObservationZero(asset_, 0);

                asset.obs.push(observations_[0]);
                asset.lastObservationTime = lastObservationTime_;

                // Emit Price Stored event for new cached value
                emit PriceStored(asset_, observations_[0], lastObservationTime_);
            }

            // We don't track cumulativeObs when not storing the moving average, even though there is one data point in the array for caching
            asset.cumulativeObs = 0;
        }
    }
}

[nevillehuang]

Invalid, all the above mentioned functions are permissioned admin functions, so this would constitute admin input error not accepted based on sherlock rules since admin are trusted to input appropriate inputs.