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

No precision scaling when calculating the total collateral

mahyar

Summary

If the collateral tokens that user own, have different decimals (not 18) user's health factor will be calculated incorrectly.

Vulnerability Details

In DSCEngine contract in order to user be able to redeem collateral, mint DSC, burn DSC and liquidation, thier health factor should be greater than the minimum and this check is done with help of _healthFactor function.

_healthFactor function use two values related to user for calculate the health factor, first one is totalDscMinted and second one is collateralValueInUsd.

But collateralValueInUsd which is user's total collateral in USD, is calculated incorrectly. Lets see the problem:

this value is getting calculated in getAccountCollateralValue function

function getAccountCollateralValue(address user) public view returns (uint256 totalCollateralValueInUsd) {
​
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;
}

As you can see it loop through all collateral tokens allowed in contract and for getting the price of each token in USD it call getUsdValue function, But the problem is this function return the token price with the token's decimals and the returned value gets added to totalCollateralValueInUsd variable, here is the getUsdValue calculataion:

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;
}

lets say price is 10$ with 18 decimals 10000000000000000000 and the amount of token passed is 4 with 8 decimals 400000000. (10000000000000000000 * 400000000) / 1e18 returned value from this calculation is 4000000000 which is based of the token decimals not 18 decimals.

So by knowing this, getAccountCollateralValue function adds the returned value from getUsdValue to totalCollateralValueInUsd variable which is wrong, adding different token values with different decimals will end up with incorrect value.

Impact

Imagine user has two collateral tokens: WETH (18 decimals) and WBTC (8 decimals)

getAccountCollateralValue adds the balance of user in both assets in USD together, (we know that returned USD value of WETH has 18 decimals and returned USD value of WBTC has 8) if amount WETH is 5 tokens and WBTC 2 sum of them will be 5000000000200000000

Here are the problems that this vulnerability can make (all of this happens because collateralValueInUsd will be lower than expected and health factor will be reduced too much):

  1. Redeeming : user should have alots of collateral in order to redeem small value othewise it reverts.

  2. Minting : user should have alots of collateral in order to mint small amount of tokens othewise it reverts.

  3. Burning : Again user should have alots of collateral in order to burn small amount of tokens othewise it reverts.

  4. Liquidation : user will get liquidated and they loose their collateral.

  5. View function : if any important offchain operations rely on health factor or total collateral of user, this will break alots of things

Tools Used

Manual Analysis

Recommendations

Since you compare the returned value from _calculateHealthFactor with the MIN_HEALTH_FACTOR = 1e18, the collateralValueInUsd should be based of 18 decimals.
So in getAccountCollateralValue before passing the amount to getUsdValue and add it with total amount, you need to scale amount's precision to 18 if token's decimals is not 18, it should like this:

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];
​
+ uint8 decimals = IERC20(token).decimals()
+ if(decimals != 18){
+ amount = (amount * 1e18) / (10 ** decimals);
+ }
totalCollateralValueInUsd += getUsdValue(token, amount);
}
return totalCollateralValueInUsd;
}

Support

FAQs

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