Summary
When a liquidator liquidates a user with bad debt, values for liquidator's s_collateralDeposited and s_DSCMinted are not reduced. Liquidator gains more than expected collateral and also continues holding on to the minted DSC if one looks at the protocol's accounting variable.
Vulnerability Details
Add the following test to DSCEngine.t.sol and run via forge test --mt testLiquidationAccounting -vv:
function testLiquidationAccounting() public {
int256 startPrice = 2e8;
MockV3Aggregator(ethUsdPriceFeed).updateAnswer(startPrice);
address borrower = makeAddr("borrower");
address _liquidator = makeAddr("_liquidator");
ERC20Mock(weth).mint(_liquidator, 5000 ether);
ERC20Mock(weth).mint(borrower, 3000 ether);
vm.startPrank(borrower);
ERC20Mock(weth).approve(address(dsce), type(uint256).max);
dsc.approve(address(dsce), type(uint256).max);
dsce.depositCollateralAndMintDsc(weth, 3000 ether, 3000e18);
vm.stopPrank();
dsce.getHealthFactor(borrower);
int256 dippedPrice = 12e7;
MockV3Aggregator(ethUsdPriceFeed).updateAnswer(dippedPrice);
dsce.getHealthFactor(borrower);
console.log(
"START: total USD value of _liquidator",
((ERC20Mock(weth).balanceOf(_liquidator) * uint256(dippedPrice)) / 1e8)
+ dsce.getAccountCollateralValue(_liquidator)
);
vm.startPrank(_liquidator);
ERC20Mock(weth).approve(address(dsce), type(uint256).max);
dsc.approve(address(dsce), type(uint256).max);
dsce.depositCollateralAndMintDsc(weth, 5000 ether, 3000e18);
dsce.getHealthFactor(_liquidator);
dsce.liquidate(weth, borrower, 3000e18);
vm.stopPrank();
console.log(
"END: total USD value of _liquidator",
((ERC20Mock(weth).balanceOf(_liquidator) * uint256(dippedPrice)) / 1e8)
+ dsce.getAccountCollateralValue(_liquidator)
);
The liquidator starts with USD 6000 (5000 wETH at USD 1.2/wETH) and liquidates borrower's 3000 DSC tokens of bad debt. For this, he should receive USD 300 (10% of USD 3000) as bonus and hence his final balance (account wETH balance + collateral value) should be USD 6300. Also, he should not be holding the minted DSCs after liquidation as they are burned by the protocol after transferring from the liquidator to the protocol here.
However, you get an incorrect value in the above test. Recommended fix is mentioned below.
Impact
More rewards are given to the liquidator; basically free money which can be used for future minting and liquidations. Breaks the system.
Tools Used
Recommendations
There are 2 functions which need to be fixed-
In _burnDsc(), add the lines marked below (L274-275):
272 function _burnDsc(uint256 amountDscToBurn, address onBehalfOf, address dscFrom) private {
273 s_DSCMinted[onBehalfOf] -= amountDscToBurn;
274 @>
275 @> s_DSCMinted[dscFrom] -= amountDscToBurn;
276 bool success = i_dsc.transferFrom(dscFrom, address(this), amountDscToBurn);
277
278 if (!success) {
279 revert DSCEngine__TransferFailed();
280 }
281 i_dsc.burn(amountDscToBurn);
282 }
In liquidate(), add the lines marked below (L256-257):
229 function liquidate(address collateral, address user, uint256 debtToCover)
230 external
231 moreThanZero(debtToCover)
232 nonReentrant
233 {
234
235 uint256 startingUserHealthFactor = _healthFactor(user);
236 if (startingUserHealthFactor >= MIN_HEALTH_FACTOR) {
237 revert DSCEngine__HealthFactorOk();
238 }
239
240
241
242
243
244
245 uint256 tokenAmountFromDebtCovered = getTokenAmountFromUsd(collateral, debtToCover);
246
247
248
249
250
251 uint256 bonusCollateral = (tokenAmountFromDebtCovered * LIQUIDATION_BONUS) / LIQUIDATION_PRECISION;
252 uint256 totalCollateralToRedeem = tokenAmountFromDebtCovered + bonusCollateral;
253 _redeemCollateral(user, msg.sender, collateral, totalCollateralToRedeem);
254
255 _burnDsc(debtToCover, user, msg.sender);
256 @>
257 @> s_collateralDeposited[msg.sender][collateral] -= tokenAmountFromDebtCovered;
258
259 uint256 endingUserHealthFactor = _healthFactor(user);
260 if (endingUserHealthFactor <= startingUserHealthFactor) {
261 revert DSCEngine__HealthFactorNotImproved();
262 }
263 _revertIfHealthFactorIsBroken(msg.sender);
264 }
