Closed c4-bot-10 closed 4 months ago
In your mitigation steps you claim that we should change the calculation in a way that we always try to safe a negative number into a unit256?
if (totalBorrow >= bareCollateral) {
unchecked {
uint256 diff = bareCollateral - totalBorrow;
.... }
This will always lead to a revert when this function is called and we have bad debt (totalBorrow == bareCollateral is still no bad debt). By definition bad debt is the difference between bareCollateral and totalBorrow and this needs to be added to the total bad debt amount. Due to symmetry it does not matter if I track bareCollateral - totalBorrow = -1 * (totalBorrow - bareCollateral) because the absolute value of both terms is the same. Only this value is important to track and add it to the total bad debt amount.
GalloDaSballo marked the issue as insufficient quality report
trust1995 marked the issue as duplicate of #74
trust1995 marked the issue as satisfactory
Hi @trust1995,
Thanks for judging the contest, sir.
The detailed explanation of this issue is incorrect and misleading.
Firstly, the totalBadDebtETH
is used to track bad debt only. If this variable is > 0, all incentives and gathered fees will be permanently unclaimable.
However, the author described the following:
- Potential Protocol Insolvency: If the bad debt is consistently overestimated, it could result in the protocol becoming insolvent over time, as the bad debt values are used to account for potential losses and compensate liquidators.
- Unfair Compensation for Liquidators: Since the bad debt values are used to calculate the compensation for liquidators, an overestimation of bad debt could lead to excessive rewards for liquidators, which would be detrimental to the protocol's economics and fairness.
As mentioned above, the totalBadDebtETH
variable will determine whether incentive owners and beneficiaries can claim their incentives and fees only and not be used to compensate liquidators, as the author mentioned. Thus, the impacts described in this issue are wrong.
Secondly, the recommended code is also incorrect, and the sponsor @Foon256 also confirmed this.
As you can see below (see @1
), if totalBorrow
> bareCollateral
, bad debt occurs. If totalBorrow
== bareCollateral
, no bad debt occurs.
In case a bad debt occurs (i.e., totalBorrow
> bareCollateral
), the transaction will be reverted due to an underflow error (see @2
).
@1 if (totalBorrow >= bareCollateral) { //@audit -- If totalBorrow > bareCollateral, bad debt occurs. If totalBorrow == bareCollateral, no bad debt occurs.
unchecked {
@2 uint256 diff = bareCollateral - totalBorrow; //@audit -- If bad debt occurs (i.e., totalBorrow > bareCollateral), the tx will be reverted due to an underflow error
FEE_MANAGER.increaseTotalBadDebtLiquidation(
diff
);
FEE_MANAGER.setBadDebtUserLiquidation(
_nftId,
diff
);
}
}
For this reason, this issue is not accurate enough to be a valid issue.
Thanks for your time, sir!
Hey @trust1995 , this report per se (without considering any other report talking about the problems with bad debt) is totally wrong, the root cause reported in this report is not correct, plus, as the sponsor mentioned, the recommended mitigation is also wrong, it doesn't fix the real issue about reporting bad debt to the FeeManager.
From this report:
The issue is that instead of subtracting diff from bareCollateral to get the actual bad debt amount, diff is added to the total bad debt liquidation (FEE_MANAGER.increaseTotalBadDebtLiquidation(diff)) and set as the bad debt for the user's position (FEE_MANAGER.setBadDebtUserLiquidation(_nftId, diff)).
This is the root cause reported on this report, which is incorrect and has nothing to do with the root cause of #74 . The root cause on #74 is the fact that the bad debt of the same position is reported multiple times to the fee manager, which causes the totalBadDebtETH
on the FeeManager to grow bigger than it should.
For this reason, I think this report doesn't deserve to be marked as a duplicate of #74 .
trust1995 marked the issue as not a duplicate
trust1995 marked the issue as unsatisfactory: Invalid
Lines of code
https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/WiseSecurity/WiseSecurity.sol#L405-L436
Vulnerability details
Impact
The major impact of this issue is:
Proof of Concept
The code is calculating the difference between
totalBorrow
andbareCollateral
correctly, but it is adding the difference to theFEE_MANAGER.increaseTotalBadDebtLiquidation
andFEE_MANAGER.setBadDebtUserLiquidation
instead of subtracting it. Here's the relevant code section:• The function calculates the bare collateral (bareCollateral) and bare total borrow (totalBorrow) for a given _nftId. • If
totalBorrow
is less thanbareCollateral
, the function returns early, as there is no bad debt. • However, iftotalBorrow
is greater thanbareCollateral
, the difference between them (diff = totalBorrow - bareCollateral) is calculated. • The issue is that instead of subtracting diff frombareCollateral
to get the actual bad debt amount, diff is added to the total bad debt liquidation (FEE_MANAGER.increaseTotalBadDebtLiquidation(diff)) and set as the bad debt for the user's position (FEE_MANAGER.setBadDebtUserLiquidation(_nftId, diff)).Tools Used
Manual
Recommended Mitigation Steps
The code should be modified to subtract the difference between
totalBorrow
andbareCollateral
from theFEE_MANAGER.increaseTotalBadDebtLiquidation
andFEE_MANAGER.setBadDebtUserLiquidation
functions, rather than adding it. Here's the corrected code:In the corrected code, the if condition checks if
totalBorrow
is greater than or equal tobareCollateral
. If this condition is true, it calculates the difference betweenbareCollateral
andtotalBorrow
and subtracts this difference from theFEE_MANAGER.increaseTotalBadDebtLiquidation
andFEE_MANAGER.setBadDebtUserLiquidation functions
.Assessed type
Other