Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ChainlinkInceptionPriceFeed can report stale price #155

Closed
code423n4 opened this issue May 2, 2022 · 1 comment
Closed

ChainlinkInceptionPriceFeed can report stale price #155

code423n4 opened this issue May 2, 2022 · 1 comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate This issue or pull request already exists invalid This doesn't seem right

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/core/contracts/inception/priceFeed/ChainlinkInceptionPriceFeed.sol#L73-L80

Vulnerability details

As stale price is determined by time since last timestamp, the price that is most recent, but wasn't updated for more than _PRICE_ORACLE_STALE_THRESHOLD (say there were no trades on the market) will be rejected, which makes system unavailable in such a case. This can be deemed as a conservative choice.

In the same time real stale price will be accepted as long as it happens to be within _PRICE_ORACLE_STALE_THRESHOLD window. I.e. getAssetPrice do not require the price to be the most recent / fresh, simply accepting any price given that it is not older than _PRICE_ORACLE_STALE_THRESHOLD.

Proof of Concept

getAssetPrice ignores round data, only requiring the timestamp to be new enough:

https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/core/contracts/inception/priceFeed/ChainlinkInceptionPriceFeed.sol#L73-L80

  function getAssetPrice() public view override returns (uint256 price) {
    (, int256 eurAnswer, , uint256 eurUpdatedAt, ) = _eurOracle.latestRoundData();
    require(eurAnswer > 0, "EUR price data not valid");
    require(block.timestamp - eurUpdatedAt < _PRICE_ORACLE_STALE_THRESHOLD, "EUR price data is stale");

    (, int256 answer, , uint256 assetUpdatedAt, ) = _assetOracle.latestRoundData();
    require(answer > 0, "Price data not valid");
    require(block.timestamp - assetUpdatedAt < _PRICE_ORACLE_STALE_THRESHOLD, "Price data is stale");

I.e., recent timestamp doesn't guarantee fresh price, while fresh price can have non-recent timestamp (simply because of slow trading), so this check alone doesn't provide much protection.

Recommended Mitigation Steps

Consider imposing checks for positive price and for the round id, timestamp fields as it's suggested in the docs:

https://docs.chain.link/docs/feed-registry/#latestrounddata

(int256 roundID, int256 priceInUsd, , int256 updatedAt, int256 answeredInRound) = baseAggregator.latestRoundData();

require(priceInUsd > 0 && updatedAt > 0 && answeredInRound >= roundID , "Price invalid");

@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels May 2, 2022
code423n4 added a commit that referenced this issue May 2, 2022
@m19
Copy link
Collaborator

m19 commented May 5, 2022

Duplicate of #1

@m19 m19 marked this as a duplicate of #1 May 5, 2022
@m19 m19 added the duplicate This issue or pull request already exists label May 10, 2022
@m19 m19 closed this as completed May 10, 2022
@gzeoneth gzeoneth added the invalid This doesn't seem right label Jun 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate This issue or pull request already exists invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

3 participants