Closed c4-bot-8 closed 5 months ago
GalloDaSballo marked the issue as sufficient quality report
A coded POC would have been better, worth checking
GalloDaSballo marked the issue as duplicate of #243
This explanation is exceptionally hard to follow. I have the right to close it because it: 1) Is submitted as High 2) Lacks coded / step by step POC
trust1995 marked the issue as unsatisfactory: Insufficient quality
hello @trust1995, To better express the problems found, sometimes I write POC, sometimes I post the code directly. I found that some other wardens have added links directly to their methods, which are concise and selected. I'll try to do the same, hopefully I can express the problem I found better. After this issue was raised, I checked a few times to check if my expression was clear, wrote out the flow of the problem, pasted in the core method, gave a simple and easy to understand example, and hopefully expressed my meaning clearly. I searched for all possible impacts, worrying about missing impacts. I spent a long time reading the code, auditing the project, I want to express better, I want to submit the issue can be "selected for report". When I see comments that POC is better, I tell myself to write POC in the future, I try to find the problem, I try to improve my expression, I hope the person who sees the issue can read it with minimum effort, I know that a bad issue will make the judge feel bad, I try to refine the issue, I try to become a better warden, I'm always trying to improve my expression. I've been trying to improve my presentation skills. I wrote out the problem function with a link, I think it is more clearly expressed and conducive to reading, if posting the code, is more conducive to reading, then I will definitely post all the relevant code in the future. As a warden, no one tells you how to express better, only explore by yourself little by little. I would like to add issue, I hope it can meet the requirements.
We can look at the flow of the liquidation, which will first call the liquidatePartiallyFromTokens method with the following code: https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/WiseLending.sol#L1250-L1309
function liquidatePartiallyFromTokens(
uint256 _nftId,
uint256 _nftIdLiquidator,
address _paybackToken,
address _receiveToken,
uint256 _shareAmountToPay
)
external
syncPool(_paybackToken)
syncPool(_receiveToken)
returns (uint256)
{
CoreLiquidationStruct memory data;
data.nftId = _nftId;
data.nftIdLiquidator = _nftIdLiquidator;
data.caller = msg.sender;
data.tokenToPayback = _paybackToken;
data.tokenToRecieve = _receiveToken;
data.shareAmountToPay = _shareAmountToPay;
data.maxFeeETH = WISE_SECURITY.maxFeeETH();
data.baseRewardLiquidation = WISE_SECURITY.baseRewardLiquidation();
(
data.lendTokens,
data.borrowTokens
) = _prepareAssociatedTokens(
_nftId,
_receiveToken,
_paybackToken
);
data.paybackAmount = paybackAmount(
_paybackToken,
_shareAmountToPay
); // @audit Convert the shares to be liquidated into quantities to be paid out
_checkPositionLocked(
_nftId,
msg.sender
); // @audit check whether the position is locked
_checkLiquidatorNft(
_nftId,
_nftIdLiquidator
); // @audit check whether __nftIdLiquidator can liquidate _nftId
WISE_SECURITY.checksLiquidation(
_nftId,
_paybackToken,
_shareAmountToPay
); // @audit check whether the liquidation share exceeds the maximum liquidation share
return _coreLiquidation(
data
);
We can see that this method mainly carries out the following operations:
The fourth step calls the checksLiquidation method to check that the maximum share is not exceeded.
function checksLiquidation(
uint256 _nftIdLiquidate,
address _tokenToPayback,
uint256 _shareAmountToPay
)
external
view
{
...
// @audit Checks that the maximum liquidation share is not exceeded
checkMaxShares(
_nftIdLiquidate,
_tokenToPayback,
borrowETHTotal,
unweightedCollateralETH,
_shareAmountToPay
);
}
The checkMaxShares function code is as follows: https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/WiseSecurity/WiseSecurityHelper.sol#L876-L900
function checkMaxShares(
uint256 _nftId,
address _tokenToPayback,
uint256 _borrowETHTotal,
uint256 _unweightedCollateralETH,
uint256 _shareAmountToPay
)
public
view
{
...
if (_shareAmountToPay <= maxShares) { // @audit Not greater than the maximum liquidation amount is fine, otherwise it will REVERT
return;
}
revert TooManyShares();
}
The fifth step is the core of liquidation,_coreLiquidation method code is as follows. https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/WiseCore.sol#L625-L686
function _coreLiquidation(
CoreLiquidationStruct memory _data
)
internal
returns (uint256 receiveAmount)
{
_validateNonZero(
_data.paybackAmount
);
// @audit Calculate the amount liquidated as a percentage of the assets of the liquidation position
uint256 collateralPercentage = WISE_SECURITY.calculateWishPercentage(
_data.nftId,
_data.tokenToRecieve,
WISE_ORACLE.getTokensInETH(
_data.tokenToPayback,
_data.paybackAmount
),
_data.maxFeeETH,
_data.baseRewardLiquidation
);
_validateParameter(
collateralPercentage,
PRECISION_FACTOR_E18
);
// @audit Update the number of pools after liquidation
_corePayback(
_data.nftId,
_data.tokenToPayback,
_data.paybackAmount,
_data.shareAmountToPay
);
// @audit Calculate the amount of assets that the liquidator will get
receiveAmount = _calculateReceiveAmount(
_data.nftId,
_data.nftIdLiquidator,
_data.tokenToRecieve,
collateralPercentage
);
// @audit Check for bad debts
WISE_SECURITY.checkBadDebtLiquidation(
_data.nftId
);
// @audit Security check on lendTokens and borrowTokens
_curveSecurityChecks(
_data.lendTokens,
_data.borrowTokens
);
// @audit Pull the tokens that need to be returned from the liquidator
_safeTransferFrom(
_data.tokenToPayback,
_data.caller,
address(this),
_data.paybackAmount
);
// @audit Send the number of assets that the liquidator can obtain
_safeTransfer(
_data.tokenToRecieve,
_data.caller,
receiveAmount
);
}
_coreLiquidation method mainly for the following operations: 1.Calculate the amount liquidated as a percentage of the assets of the liquidation position 2.Update the number of pools after liquidation 3.Calculate the amount of assets that the liquidator will get 4.Check for bad debts 5.Security check on lendTokens and borrowTokens 6.Pull the tokens that need to be returned from the liquidator 7.Send the number of assets that the liquidator can obtain
The problem arises in step 4, calling checkBadDebtLiquidation to check for bad debt https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/WiseSecurity/WiseSecurity.sol#L405-L436
function checkBadDebtLiquidation(
uint256 _nftId
)
external
onlyWiseLending
{
...
unchecked {
uint256 diff = totalBorrow
- bareCollateral; // @audit Calculate the total bad debt amount
FEE_MANAGER.increaseTotalBadDebtLiquidation(
diff
); // @audit Record the total amount of bad debt incurred
FEE_MANAGER.setBadDebtUserLiquidation(
_nftId,
diff
); // @audit Record the amount of bad debt generated by the position
}
}
A bad debt arises when the amount borrowed is greater than the amount of the asset. Then record the bad debt separately 1.Record the total amount of bad debts by increasingTotalBadDebtLiquidation Then call _increaseTotalBadDebt to record the total bad debt in the FeeManager.totalBadDebtETH variable https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/FeeManager/FeeManagerHelper.sol#L89-L100
function _increaseTotalBadDebt(
uint256 _amount
)
internal
{
totalBadDebtETH += _amount; // Record the total bad debt
...
}
2.Record the position amount through setBadDebtUserLiquidation Then call _setBadDebtPosition to record the bad debt position in FeeManager.badDebtPosition variable. https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/FeeManager/FeeManagerHelper.sol#L77-L84
function _setBadDebtPosition(
uint256 _nftId,
uint256 _amount
)
internal
{
badDebtPosition[_nftId] = _amount;
}
The total bad debt amount recorded at this point is the amount of bad debt incurred. If a user generates 5ETH bad debt, it will be recorded as 5ETH, if this position is liquidated in 2 times, the bad debt in the first time is 5ETH, the bad debt in the second liquidation is 5ETH, the total bad debt will be recorded as 10ETH, while the actual bad debt is only 5ETH.
Then the 2 methods paybackBadDebtForToken, paybackBadDebtNoReward can be called to clear the bad debt. We use the paybackBadDebtForToken method as an example, which is implemented as follows
function paybackBadDebtForToken(
uint256 _nftId,
address _paybackToken,
address _receivingToken,
uint256 _shares
)
external
returns (
uint256 paybackAmount,
uint256 receivingAmount
)
{
updatePositionCurrentBadDebt(
_nftId
); // @audit Update the position price information and call _updateUserBadDebt to update the bad debt situation.
...
WISE_LENDING.corePaybackFeeManager(
_paybackToken,
_nftId,
paybackAmount,
_shares
); // @audit Update the pool information after the position pays off the bad debt.
_updateUserBadDebt(
_nftId
); // @audit Update bad debt
...
}
The paybackBadDebtForToken method mainly implements the following functions: 1.Update the position price information and call _updateUserBadDebt to update the bad debt situation. 2.Update the pool information after the position pays off the bad debt. 3.Update bad debt
The implementation of _updateUserBadDebt is as follows:
function _updateUserBadDebt(
uint256 _nftId
)
internal
{
...
uint256 currentBadDebt = badDebtPosition[
_nftId
]; // @audit The current bad debt in the position
if (currentBorrowETH < currentCollateralBareETH) {
_eraseBadDebtUser(
_nftId
); // @audit If there are no more bad debts, the position bad debt is set to
_decreaseTotalBadDebt(
currentBadDebt
); // @audit The total bad debt can be reduced by up to the current bad debt in the position
emit UpdateBadDebtPosition(
_nftId,
0,
block.timestamp
);
return;
}
unchecked {
uint256 newBadDebt = currentBorrowETH
- currentCollateralBareETH; // @audit New bad debt
_setBadDebtPosition(
_nftId,
newBadDebt
); // @audit Sets the position's new bad debt
newBadDebt > currentBadDebt
? _increaseTotalBadDebt(newBadDebt - currentBadDebt)
: _decreaseTotalBadDebt(currentBadDebt - newBadDebt); // @audit Increase or decrease the corresponding total bad debt
...
}
}
This method mainly updates the bad debt, we can see that the total bad debt can only be reduced by the bad debt in the position
If a position is liquidated more than once, the total bad debt is greater than the bad debt of the position. And there is no way to clear the total bad debt to 0 again.
The inability to clear the total bad debt to zero will cause the following functions to be affected claimFeesBeneficial will result in fees not being captured
function claimFeesBeneficial(
address _feeToken,
uint256 _amount
)
external
{
address caller = msg.sender;
if (totalBadDebtETH > 0) { // The total bad debt must be equal to 0 in order to execute
revert ExistingBadDebt();
}
...
}
claimWiseFees Rewards cannot be distributed
function claimWiseFees(
address _poolToken
)
public
{
...
if (totalBadDebtETH == 0) { // @audit Rewards cannot be distributed
tokenAmount = _distributeIncentives(
tokenAmount,
_poolToken,
underlyingTokenAddress
);
}
...
}
claimWiseFeesBulk is a batch execution of claimWiseFees, which will result in rewards not being distributed
function claimWiseFeesBulk()
external
{
uint256 i;
uint256 l = getPoolTokenAddressesLength();
while (i < l) {
claimWiseFees(
poolTokenAddresses[i]
); // Bulk execution of claimWiseFees
unchecked {
++i;
}
}
emit ClaimedFeesWiseBulk(
block.timestamp
);
}
I appreciate the resubmission, however I must maintain that the original section was not sufficient for High severity. I must leave it unsatisfactory, but I'm sure you will improve your submission writing in future contests!
trust1995 marked the issue as duplicate of #74
@trust1995 Marking this report as a duplicate of #74 wouldn't cause it to be included in the rewards distribution?
No, because it is marked as unsatisfactory.
@trust1995 Thank you for your reply, I can understand not meeting High's criteria, I guess there is no compromise to give me a partial reward? It took me almost 2 weeks to find out about this issue and this is the only issue I have had a chance with this program. can this issue be handled as medium or QA? Since being unemployed as a warden and earning very little, it makes me happy to earn 100$ at a time. Thanks again.
Lines of code
https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/WiseLending.sol#L1306 https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/WiseCore.sol#L665-L667 https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/WiseSecurity/WiseSecurity.sol#L423-L435 https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/FeeManager/FeeManager.sol#L494-L508 https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/FeeManager/FeeManagerHelper.sol#L89-L100
Vulnerability details
Impact
The total debt is overstated, which can result in fees earned under the protocol not being reutilized
Proof of Concept
When a position has more than 100% debt, it can be liquidated via the method liquidatePartiallyFromTokens to liquidate the position. The amount to be liquidated is fine as long as it does not exceed the maximum liquidatable shares. When liquidating, the total bad debt incurred and the bad debt of the position will be recorded separately.The code is as follows: https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/WiseSecurity/WiseSecurity.sol#L405-L436
The problem occurs when the total bad debt is recorded. We can see the logic of the bad debt, which is calculated in full. For example, the bad debt value of a position bad debt is 5ETH. The first liquidation is not liquidated, the repayment of 1ETH, then the total bad debt record is 4ETH, the bad debt of the position record is 4ETH. the second user made another repayment of 1ETH, then the bad debt of the position record is 3ETH, the total bad debt continues to increase, the record is 7ETH.In fact, the total bad debt should only be 3ETH.the remaining bad debt even if the call to the Even if the updatePositionCurrentBadDebt method is called to clear the bad debt, it can only clear 3ETH of the position's record. the total bad debt is reduced by 3ETH, and the total bad debt is still left at 4ETH. the total bad debt can't be completely cleared, and the affected methods are: claimFeesBeneficial claimWiseFees claimWiseFeesBulk will result in the agreement fees not being able to be withdrawn and will only be in locked in the agreement.
Also this makes it easy to launch a DDos attack, as long as a position is liquidated 2 times this problem will occur. Bad users get liquidation rewards normally. Costs are very low.
Tools Used
Manual Reveiw
Recommended Mitigation Steps
There are 2 ways to solve this problem:
Assessed type
Math