Foundry DeFi Stablecoin CodeHawks Audit Contest
Cyfrin
DeFiFoundry
15,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Valid

Current staleness check would cause regular lengthy denials of service

bauchibred

Summary

The OracleLib.sol has a hardcoded TIMEOUT value, 3 hours, which is used in the staleCheckLatestRoundData() function to know if the returned price is stale or not.
This is probably implemented since the primary tokens would be BTC, ETH, but from the public discord chat Patrick has stated the below as reply to a question on what type of tokens would be used:

weth & wbtc are the intended tokens, but any token with a USD chainlink price feed can work

Do note that since the heartbeats of both BTC and ETH feeds are 1 hr, this does not affect them but for different tokens this would be a big issue see Vulnerability Details

Vulnerability Details

As previously stated in Summary, we can see from this line that TIMEOUT has been hardcoded to three hours.
Additionally it's been used here:

function staleCheckLatestRoundData(AggregatorV3Interface priceFeed)
public
view
returns (uint80, int256, uint256, uint256, uint80)
{
(uint80 roundId, int256 answer, uint256 startedAt, uint256 updatedAt, uint80 answeredInRound) =
priceFeed.latestRoundData();
​
​
uint256 secondsSince = block.timestamp - updatedAt;
//@audit
if (secondsSince > TIMEOUT) revert OracleLib__StalePrice();
​
​
return (roundId, answer, startedAt, updatedAt, answeredInRound);
}

Now the issue is that 3 hours is actually too short for some feeds, take a very popular token as an example, USDC, note that USDC/USD has a 24 hour heartbeat, i.e 86400s .

Since timeout has been set to 10800 seconds, querying USDC/USD price will be constantly failing after 10800 seconds have passed since the USDC/USD feed has been updated and until it's updated again (in this case, the DoS will continue for 75600 seconds (21 hours) after every 10800 seconds (3 hours)).
Which means that all instances of querying prices would be affected, use the getUsdValue() as an example

function getUsdValue(address token, uint256 amount) public view returns (uint256) {
AggregatorV3Interface priceFeed = AggregatorV3Interface(s_priceFeeds[token]);
(, int256 price,,,) = priceFeed.staleCheckLatestRoundData();
// 1 ETH = $1000
// The returned value from CL will be 1000 * 1e8
return ((uint256(price) * ADDITIONAL_FEED_PRECISION) * amount) / PRECISION;
}

Would be important to note that this function is also used while checking the health factor of an account, or while gettiing it's information, and multiple other instances within DSCEngine.sol:

function getHealthFactor(address user) external view returns (uint256) {
return _healthFactor(user);
}
​
​
/*
* Returns how close to liquidation a user is
* If a user goes below 1, then they can get liquidated
*/
function _healthFactor(address user) private view returns (uint256) {
(uint256 totalDscMinted, uint256 collateralValueInUsd) = _getAccountInformation(user);
return _calculateHealthFactor(totalDscMinted, collateralValueInUsd);
}
​
​
function _getAccountInformation(address user)
private
view
returns (uint256 totalDscMinted, uint256 collateralValueInUsd)
{
totalDscMinted = s_DSCMinted[user];
collateralValueInUsd = getAccountCollateralValue(user);
}
function getAccountCollateralValue(address user) public view returns (uint256 totalCollateralValueInUsd) {
// loop through each collateral token, get the amount they have deposited, and map it to
// the price, to get the USD value
for (uint256 i = 0; i < s_collateralTokens.length; i++) {
address token = s_collateralTokens[i];
uint256 amount = s_collateralDeposited[user][token];
totalCollateralValueInUsd += getUsdValue(token, amount);
}
return totalCollateralValueInUsd;
}
​

This easily means that all the calculation of user's current state and their collateral value can't be accessed for those 21 hours, putting this in percentage, this means that critical functionalities for some users can't be processed 87.5% of the whole lifetime of DSC.

Impact

Criticial functionalities that use querying of prices could be blocked for 21 out of 24 hours everyday for the DSCEngine, to even worsen the case imagine user has two tokens with pricefeeds of 24 hours that get updated at different times, there is a high chance that the loop through all collateral tokens in the getAccountCollateralValue() function would almost always fail, as there is a chance that the 10800 seconds window of one of these tokens would not coincide with the 10800 seconds of the other, essentially meaning that while one of the tokens is available the other would DOS, and when the latter is available the former causes a DOS.

Now imagine if the tokens with lengthy heartbeats are more than two in the array.

Tools Used

Manual Audit

Recommend Mitigation

Consider the heartbeat of feeds that are going to be integrated before setting TIMEOUT, if they have a very lengthy heartbeat, their stale timeout check should also be lengthy so as to curb the DOS.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.