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

Divide Before Multiply

ShayanShamsi

Summary

Solidity's integer division truncates. Therefore, multiplication should be performed before division.

Vulnerability Details

If the divisor is greater than the dividend, the division results in zero. This makes any further multiplication operations performed on the quotient also equal zero. This result is unwanted.
The division before multiplication is performed in DSCEngine._calulateHealthfactor(uint256,uint256):

uint256 collateralAdjustedForThreshold = (collateralValueInUsd * LIQUIDATION_THRESHOLD) / LIQUIDATION_PRECISION;
return (collateralAdjustedForThreshold * 1e18) / totalDscMinted;

Here, collateralAdjustedForThreshold is the result of a division that is later multiplied by 1e18.

Impact

Fortunately, there are no dire consequences in our scenario. That is because the collateralValueInUsd will have to equal 1 Wei for the result of the division to equal 0. This is demonstrated in the following calculation. Keep in mind that the denominator (LIQUIDATION_PRECISION) will have to be greater than the numerator (collateralValueInUsd * LIQUIDATION_THRESHOLD) for this to occur.

LIQUIDATION_PRECISION > collateralValueInUsd * LIQUIDATION_THRESHOLD
100 > collateralValueInUsd * 50
collateralValueInUsd < 100 / 50
collateralValueInUsd < 2
collateralValueInUsd = 1

The health factor can not be good if the collateralValueInUsd is equal to 1 Wei. This is because totalDscMinted can not be any less than 1 Wei and totalDscMinted needs to be 50% of collateralValueInUsd in order for the health factor to be considered "good". The only possible case is then of the health factor being bad (less than 1e18). This is automatically achieved since the _calculateHealthFactor returns 0 when collateralValueInUsd equals 1 Wei. The rest of the protocol continues to function without error. However, it should be noted that this does not mean that the health factor is being calculated correctly. Let's take the example where collateralValueInUsd is 1 Wei and the totalDscMinted is 2 Wei. The correct health factor is 25e16 whereas _calculateHealthFactor will just return 0 because the division will result in 0. The following test is currently failing:

function testBreaksHealthFactor() public {
uint256 expectedHealthFactor = 25e16;
uint256 calculatedHealthFactor = dsce.calculateHealthFactor(2, 1);
assertEq(expectedHealthFactor, calculatedHealthFactor);
}

The risky collateralValueInUsd values (in Wei) can be found using the following inequality in the event that LIQUIDATION_THRESHOLD changes:

collateralValueInUsd < 100 / LIQUIDATION_THRESHOLD

It should be noted that the health factor should only be 0 when the collateral value drops to 0. The health factor equalling 0 in all other cases is a bug and should be addressed appropriately.

Tools Used

  • Slither

  • Foundry

Recommendations

Multiply before dividing. _calculateHealthFactor should have the calculation done as follows:

uint256 collateralAdjustedForThreshold = (collateralValueInUsd * LIQUIDATION_THRESHOLD * 1e18) / LIQUIDATION_PRECISION;
return collateralAdjustedForThreshold / totalDscMinted;

This will ensure that the correct value of health factor is returned since the division will now not truncate the answer.
The following test passes after making the change:

function testBreaksHealthFactor() public {
uint256 expectedHealthFactor = 25e16;
uint256 calculatedHealthFactor = dsce.calculateHealthFactor(2, 1);
assertEq(expectedHealthFactor, calculatedHealthFactor);
}

Support

FAQs

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