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

Hardcoding timeout to 3 hours doesn't protect DSC from using stale data

bauchibred

Summary

OracleLib.sol implements a hardcoded TIMEOUT value, which is used in the staleCheckLatestRoundData() function to know if the returned price is stale or not, however in some instances this could be side stepped, see Vulnerability Details

Vulnerability Details

As previously stated in Summary, we can see in 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 long for some feeds, take ETH/USD or BTC/USD as an example (using this as an example since DSC is primarily supposed to be backed by BTC/ETH), here is the feed for ETH?USD as seen it has a deviation threshold of 0.5% and also a heartbeat of 1 hour, which means that in the case the price does not make a 0.5% difference in any direction it's supposed to get updated within 1 hour, now would be key to note that stale price checks are done in order to make sure that the right price is integrated, in a case where an update does not get relayed from chainlink within the past hour for any of these feeds the price is stale.
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 getting 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 in the case where a stale price is integrated, the calculation of user's current state in the internal _healthFactor() could be wrongly returned as above 1 and they can't get liquidated where as they are below the healthy level, leading to DSC allowing users hold over-collaterized tokens, alternatively the current state could be returned as below 1 and users get unfairly liquidated whereas their account is above the healthy level.

Impact

DSC would work with outdated price data for ETH/USD and BTC/USD.

Tools Used

Manual Audit

Recommend Mitigation

Set a different TIMEOUT value for different feeds, it should be shorter than 3 hours for the aforementioned feeds, i.e BTC, ETH.
Additionally since it's being relayed in the public discord chat that any erc20 token that has a USD chainlink feed can be used for protocol then respective checks should be made for each and the timeout value should be set based on their heartbeats.

Support

FAQs

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