manifoldfinance / defi-threat

a globally-accessible knowledge base of adversary tactics and techniques based on real-world observations on decentralized finance
Mozilla Public License 2.0
485 stars 53 forks source link

Prices: offchain oracle response liveliness not checked #21

Open sambacha opened 1 year ago

sambacha commented 1 year ago

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, thePriceFeedWrapper.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.