Foundry DeFi Stablecoin CodeHawks Audit Contest

Summary

The contract assumes that the amount sent to the getUsdValue() function has 18 decimals, which is not always the case, especially for tokens like USDT and USDC with fewer decimals. This leads to incorrect calculations, resulting in underestimating the user's collateral, which can lead to liquidation or prevent the user from minting DSC tokens.

The impact of this issue is significant as it affects the accuracy of the protocol's liquidation process and can lead to financial losses for users.

To mitigate this vulnerability, two possible steps are recommended: accepting only tokens with exactly 18 decimals for collateral or normalizing the number of decimals for tokens, but careful consideration is needed to avoid the precision loss or overflow issues.

Vulnerability Details

The _revertIfHealthFactor() is the core function of the liquidation process. This function uses _healthFactor() to find the health factor and revert if it is less than MIN_HEALTH_FACTOR. The _healthFactor() function finds the collateral value in USD using the _getAccountInformation() function. The _getAccountInformation() function calls getAccountCollateralValue() to calculate the amount of the collateral. Finally, for all of the collateral tokens, this function calls getUsdValue() to calculate the amount of collateral in USD. The issue is that the getUsdValue() function uses the following function to calculate the price:

This function makes two assumptions:

1. The price that has returned from Chainlink Oracle has 8 decimals.

2. The amount sent to the function has 18 decimals.

Assumption #1 is fine since all of the TOKEN/USD pairs in Chainlink use 8 decimals for the price. However, assumption #2 is not fine. In the documentation of the project, it has been mentioned that:

The system is meant to be such that someone could fork this codebase, swap out WETH & WBTC for any basket of assets they like, and the code would work the same.

In the case that the underlying collateral is a token that does not use 18 decimals, e.g. USDT and USDC, the protocol uses the wrong price for the computation.

Impact

The impact of this issue is that the contract underestimates the collateral of the user and the user can get liquidated, or the user cannot mint any DSC unless they keep a collateral ratio of more than 100 * 1e12%.

Consider another fork of this protocol is deployed which uses USDC and WBTC as the underlying token. We know that the USDC has 6 decimals and DSC supports 18 decimals. Also, WBTC supports 18 decimals. Consider Alice deposits $200 worth of WBTC and borrows $100 worth of DSC. Now, we know that the debt has been 200% over-collateralized, so no liquidator must not be able to liquidate this user.

Now, consider the user to improve the health factor adds an extra $20 of USDC. The collateral is worth $220 and the debt is $100 dollars. The price of WBTC drops and the WBTC amount decreases to $180. Since the collateral is still $180 + $20 = $200, no liquidation should happen now.

However, let's look at the computation of the USD value of this token. $20 worth of USDC is equal to 20 * 1e6 tokens. $100 worth of DSC is equal to 100 * 1e18. Also, consider chainlink returns $1 for the price of USDC, which is 1 * 1e8. The getUsdValue() function will calculate the USD value based on the equation below:
$$
\text{UsdValue} = \frac{1e8 \times 1e10 \times 200 \times 1e6}{1e18} = 20 \times 1e6
$$
Consider the WBTC collateral value in USD returns correct and it is $180 which is 180 * 1e18.

So, the total collateral of the user is 180 * 1e18 + 200 * 1e6. Now, we know that collateralValueInUsd is eqaul to 180 * 1e18 + 200 * 1e6 and the totalDscMinted is equal to 100 * 1e18. The _calculateHealthFactor() function first calculates collateralAdjustedForThreshold to calculate the health factor:
$$
\text{adjustedCollateral} = \frac{(200 \times 1e6 + 180 \times 1e18) \times 50}{100} = 100 \times 1e6 + 90 \times 1e18
$$
Then, it uses this number to calculate the health factor:
$$
\text{healthFactor} = \frac{(100 \times 1e6 + 90 \times 1e18) \times 1e18}{100 \times 1e18} = 1e6 + 0.9 \times 1e18
$$
Then, the liquidate() function compare 1e6 + 0.9 * 1e18 to MIN_HEALTH_FACTOR which is 1e18. Since it is less than MIN_HEALTH_FACTOR, this function will not be reverted, and the user gets liquidated.

Add the test below to the DSCEngineTest.t.sol to observe when a user can get liquidated:

After running the test, we get the following result:

Also, as we mentioned the depositCollateralAndMintDsc() function may lose its functionality. Run the test below:

After running the test, we get the following result:

Tools Used

Manual Review, Foundry

Recommendations

For mitigating this issue we can do two things:

1. Try to accept only tokens that have exactly 18 decimals for the collateral.

2. Try to normalize the number of decimals of the tokens to have 18 decimals; however, if the token has more than 18 decimals, it may lose its precision. Also, when it has less than 18 decimals, overflow may happen. So, consider these two cases as well.