Olympus Review

Issue H-1: OlympusPrice.v2.sol#storePrice: The moving average prices are used recursively for the calculation of the moving average price.

OlympusPrice.v2.sol#storePrice function:

line 319: call _getCurrentPrice
- _getCurrentPrice function
  - line 138 160: calculate move average price and add to feeds
  - line 170: the returned price is calculated by move average price
line 330:
- update asset.cumulativeObs

The vulnerability arises because storePrice uses both oracle feed prices and moving average price for its calculations.

_getCurrentPrice is used for calculating the current price by mixing oracle and moving average prices. However, storePrice updates the information for calculating the moving average. If _getCurrentPrice includes the moving average price in its calculation, it causes storePrice to retrieve distorted prices. This distortion impacts the accuracy of the moving average due to the recursive use of the key data cumulativeObs.

 function storePrice(address asset_) public override permissioned {
        Asset storage asset = _assetData[asset_];

        // Check if asset is approved
        if (!asset.approved) revert PRICE_AssetNotApproved(asset_);

        // Get the current price for the asset
319:    (uint256 price, uint48 currentTime) = _getCurrentPrice(asset_);

        // Store the data in the obs index
        uint256 oldestPrice = asset.obs[asset.nextObsIndex];
        asset.obs[asset.nextObsIndex] = price;

        // Update the last observation time and increment the next index
        asset.lastObservationTime = currentTime;
        asset.nextObsIndex = (asset.nextObsIndex + 1) % asset.numObservations;

        // Update the cumulative observation, if storing the moving average
330:    if (asset.storeMovingAverage)
            asset.cumulativeObs = asset.cumulativeObs + price - oldestPrice;

        // Emit event
        emit PriceStored(asset_, price, currentTime);
    }

    function _getCurrentPrice(address asset_) internal view returns (uint256, uint48) {
...
138		  uint256[] memory prices = asset.useMovingAverage
            ? new uint256[](numFeeds + 1)
            : new uint256[](numFeeds);
...
        // If moving average is used in strategy, add to end of prices array
160     if (asset.useMovingAverage) prices[numFeeds] = asset.cumulativeObs / asset.numObservations;

...
        // If there is only one price, ensure it is not zero and return
        // Otherwise, send to strategy to aggregate
        if (prices.length == 1) {
            if (prices[0] == 0) revert PRICE_PriceZero(asset_);
            return (prices[0], uint48(block.timestamp));
        } else {
            // Get price from strategy
            Component memory strategy = abi.decode(asset.strategy, (Component));
170         (bool success, bytes memory data) = address(_getSubmoduleIfInstalled(strategy.target))
                .staticcall(abi.encodeWithSelector(strategy.selector, prices, strategy.params));

            // Ensure call was successful
            if (!success) revert PRICE_StrategyFailed(asset_, data);

            // Decode asset price
            uint256 price = abi.decode(data, (uint256));

            // Ensure value is not zero
            if (price == 0) revert PRICE_PriceZero(asset_);

            return (price, uint48(block.timestamp));
        }
}

Issue H-2: Incorrect ProtocolOwnedLiquidityOhm calculation due to inclusion of other user’s reserves

BunniSupply.sol#getProtocolOwnedLiquidityOhm is intended to returns the total of OHM in all of the registered tokens representing Uniswap V3 pools

But anyone can call deposit to add liquidity to a token, the return reserve contains amounts other than the reserves actually belong to the protocol.

    function getProtocolOwnedLiquidityOhm() external view override returns (uint256) {

        uint256 len = bunniTokens.length;
        uint256 total;
        for (uint256 i; i < len; ) {
            TokenData storage tokenData = bunniTokens[i];
            BunniLens lens = tokenData.lens;
            BunniKey memory key = _getBunniKey(tokenData.token);

        .........

            total += _getOhmReserves(key, lens);
            unchecked {
                ++i;
            }
        }


        return total;
    }

    function deposit(
        DepositParams calldata params
    )
        external
        payable
        virtual
        override
        checkDeadline(params.deadline)
        returns (uint256 shares, uint128 addedLiquidity, uint256 amount0, uint256 amount1)
    {
    }

Issue H-3: Incorrect StablePool BPT price calculation

Issue H-4: getBunniTokenPrice wrongly returns the total price of all tokens

The getBunniTokenPrice function returns the total value of Bunni token instead of price, should be divided by total shares.

    function getBunniTokenPrice(
        address bunniToken_,
        uint8 outputDecimals_,
        bytes calldata params_
    ) external view returns (uint256) {
       
        ......

        // Fetch the reserves
        uint256 totalValue = _getTotalValue(token, lens, outputDecimals_);

        return totalValue;
    }