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

DSCEngine.sol hardcodes `ADDITIONAL_FEED_PRECISION` which would lead to overvaluation of user's collateral value for some tokens

bauchibred

Summary

ADDITIONAL_FEED_PRECISION is being hardcoded to 10 in DSCEngine.sol which would lead to an overvaluation in some cases, see Vulnerability Details.

Vulnerability Details

As stated in Summary, ADDITIONAL_FEED_PRECISION is being hardcoded to `10, this is probably implemented since initially 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

Further discussions in the public chat suggests that the stablecoin is now intended to work with any token that has a USD feed.
Now the DSCEngine.sol contract assumes that all chainlink USD feeds return price in 8 decimals which is why the ADDITIONAL_FEED_PRECISION is being hardcoded to `10 so as to normalise the price, but this is not always the case.

For example the AMPL/USD feed is denoted with 18 decimals.
As can be seen here

Now do note that the ADDITIONAL_FEED_PRECISION is being used in 2 instances in to normalise price, i.e in the getTokenAmountFromUsd() and getUsdValue() functions, as can be seen below:

function getTokenAmountFromUsd(address token, uint256 usdAmountInWei) public view returns (uint256) {
// price of ETH (token)
// $/ETH ETH ??
// $2000 / ETH. $1000 = 0.5 ETH
AggregatorV3Interface priceFeed = AggregatorV3Interface(s_priceFeeds[token]);
(, int256 price,,,) = priceFeed.staleCheckLatestRoundData();
// ($10e18 * 1e18) / ($2000e8 * 1e10)
return (usdAmountInWei * PRECISION) / (uint256(price) * ADDITIONAL_FEED_PRECISION);
}
...
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 where as the getTokenAmountFromUsd() function is not heavily used within the engine, aside being called externally it's still being used in the liquidate() function, whereas the getUsdValue() is massively implemented, for example 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 means that all the calculation of user's current state or their collateral value would be massively mis-valued, since using this report's case study the AMPL/USD returned price would be unnecasirly normalized by 10 decimals, in short the collateral would be misvalued with 10 decimals.

Impact

Massive misvaluation of collateral's dollar value, difference of a whooping 10 decimals.

Tools Used

Manual Audit

Recommend Mitigation

ADDITIONAL_FEED_PRECISION should not be hardcoded, since hardcoding most of the time leads to lack of flexibility of a protocol, in this instance too I'd suggest that the decimals of each feed should be queried and then ADDITIONAL_FEED_PRECISION be set to 18-decimals

Support

FAQs

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