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

Prices: offchain oracle response liveliness not checked #21

Open
sambacha opened this issue Dec 7, 2022 · 0 comments
Open

Prices: offchain oracle response liveliness not checked #21

sambacha opened this issue Dec 7, 2022 · 0 comments
Assignees
Labels
Document Attack Pattern Document an Attack Pattern not listed documentation Improvements or additions to documentation
Milestone

Comments

@sambacha
Copy link
Contributor

sambacha commented Dec 7, 2022

Offchain oracle response liveliness not checked.

No liveness checks are performed while retrieving oracle data. As a result, prices could
be outdated yet used anyways affecting deposits, borrows, repayments, and any other
source that relies on Chainlink’s prices.

The data retrieval from the rate conversion wrapper does not check the retrieved price
and the success condition. As a result, the PriceFeedWrapper.latestAnswer() could
return negative or invalid data yet used anyways across the market.
The mentioned function has the following implementation:

function latestAnswer() external view returns (int256) {
int256 mainPrice = mainPriceFeed.latestAnswer();
(, bytes memory data) =
address(wrapper).staticcall(abi.encodeWithSelector(conversionSelector, baseUnit));
uint256 rate = abi.decode(data, (uint256));
return int256(rate.mulDivDown(uint256(mainPrice), baseUnit));
}

On the other hand, Auditor.assetPrice() is implemented as follows:

function assetPrice(IPriceFeed priceFeed) public view returns (uint256) {
if (address(priceFeed) == BASE_FEED) return basePrice;
int256 price = priceFeed.latestAnswer();
if (price <= 0) revert InvalidPrice();
return uint256(price) * baseFactor;
}

The low level staticcall function has two returns, a boolean success and bytes
data. Currently, the decoded rate has no rules as the price has in assetPrice(). Also,
there are no checks that ensure that the boolean return is true.

Recommendation

Check both the boolean return and the retrieved rate if possible.

@sambacha sambacha added the Document Attack Pattern Document an Attack Pattern not listed label Dec 7, 2022
@sambacha sambacha self-assigned this Dec 7, 2022
@sambacha sambacha added this to the v4 milestone Dec 7, 2022
@sambacha sambacha moved this to 🔀 Backlog in Manifold Public Dec 7, 2022
@sambacha sambacha added the documentation Improvements or additions to documentation label Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Document Attack Pattern Document an Attack Pattern not listed documentation Improvements or additions to documentation
Projects
Status: 🔀 Backlog
Development

No branches or pull requests

1 participant