nirohgo - PRICE module AddAsset doesn't properly detect faulty configurations

[sherlock-admin]

nirohgo

medium

PRICE module AddAsset doesn't properly detect faulty configurations

Summary

PRICE module AddAsset does not properly check for submodule configuration errors. This may cause assets to be added with faulty feeds that never report a price and compromise price accuracy.

Vulnerability Detail

the AddAsset function calls getCurrentPrice(asset); at the end, to validate the asset configuration. However _getCurrentPrice submodule calls are made using staticcall and do not revert when a single submodule reverts. This means that if a feed configuration error exists (such as using a stable pool with the getWeightedPoolTokenPrice selector in the BalancerPoolTokenPrice module), it will not be detected and the asset will be added with a disfunctional feed.

Impact

Price reporting may be significantly less accurate and less resilient. For example: Assets using getAveragePriceIfDeviation where a "main" more accurate feed is used and a less accurate feed is added for averaging only in case of deviation, if the first feed is misconfigured the asset will be added but will rely solely on the less accurate feed for pricing.

Code Snippet

https://github.com/sherlock-audit/2023-11-olympus/blob/9c8df76dc9820b4c6605d2e1e6d87dcfa9e50070/bophades/src/modules/PRICE/OlympusPrice.v2.sol#L404

https://github.com/sherlock-audit/2023-11-olympus/blob/9c8df76dc9820b4c6605d2e1e6d87dcfa9e50070/bophades/src/modules/PRICE/OlympusPrice.v2.sol#L148

Tool used

Manual Review

Recommendation

Add a flag to _getCurrentPrice that will be used only when testing configuration and will cause the function to revert if any submodule call fails.

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

nirohgo commented:

In the interest of disclosure this is my own finding.

[nevillehuang]

@0xJem addAsset is an admin permissioned function, so this would constitute admin error, suggest to maintain invalid. It is admin responsibility to ensure correct configurations for submodules exists.

[0xJem]

https://github.com/OlympusDAO/bophades/pull/249

[IAm0x52]

Fix looks good. All feeds must succeed when adding asset