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

The ```testMustImproveHealthFactorOnLiquidation()``` function is not fulfilling its intended purpose as described.

0xanmol

Summary

The testMustImproveHealthFactorOnLiquidation() is meant to check if the user health factor improves on liquidation or not but it is checking for a revert statement that will hit if the health factor is not improved.

Vulnerability Details

The testMustImproveHealthFactorOnLiquidation() function is currently utilizing the mockMoreDebtDSC.sol contract for testing purposes. However, an issue exists with the burn function within mockMoreDebtDSC.sol, which erroneously sets the priceFeed to 0. Consequently, when the liquidator attempts to liquidate a user, the price feed is set to zero. As a result, the user's health factor fails to improve and immediately crashes to 0, triggering the condition that leads to a revert.

if (endingUserHealthFactor <= startingUserHealthFactor) {
revert DSCEngine__HealthFactorNotImproved();
}

This is not exactly checking if the user health factor improves upon liquidation or not this is simply crashing the price which doesn’t make any sense with the test name.

Impact

This creates confusion and not exactly does what it is meant to do. If the protocol undergoes some changes in liquidation logic this test will always pass without reflecting the actual scenario.

Tools Used

manual review

Recommendations

Instead of using mockMoreDebtDSC.sol token we can simply use DSC token and check if the new health factor is greater than old health factor after liquidation.

​
function testMustImproveHealthFactorOnLiquidation() public {
// Arrange - Setup
DecentralizedStableCoin Dsc = new DecentralizedStableCoin();
tokenAddresses = [weth];
feedAddresses = [ethUsdPriceFeed];
address owner = msg.sender;
vm.prank(owner);
DSCEngine mockDsce = new DSCEngine(
tokenAddresses,
feedAddresses,
address(Dsc)
);
Dsc.transferOwnership(address(mockDsce));
// Arrange - User
vm.startPrank(user);
ERC20Mock(weth).approve(address(mockDsce), amountCollateral);
mockDsce.depositCollateralAndMintDsc(weth, amountCollateral, amountToMint);
vm.stopPrank();
​
// Arrange - Liquidator
collateralToCover = 12 ether;
ERC20Mock(weth).mint(liquidator, collateralToCover);
​
vm.startPrank(liquidator);
ERC20Mock(weth).approve(address(mockDsce), collateralToCover);
uint256 debtToCover = 10 ether;
mockDsce.depositCollateralAndMintDsc(weth, collateralToCover, amountToMint);
Dsc.approve(address(mockDsce), debtToCover);
// Act
int256 ethUsdUpdatedPrice = 18e8; // 1 ETH = $18
MockV3Aggregator(ethUsdPriceFeed).updateAnswer(ethUsdUpdatedPrice);
​
// Act/Assert
uint256 userHealthFactorBeforeLiquidation = mockDsce.getHealthFactor(user);
mockDsce.liquidate(weth, user, debtToCover);
uint256 userHealthFactorAfterLiquidation = mockDsce.getHealthFactor(user);
assert(userHealthFactorAfterLiquidation > userHealthFactorBeforeLiquidation);
vm.stopPrank();
}

Support

FAQs

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