Incorrect bad debt accounting can lead to a state where the `claimFeesBeneficial` function is permanently bricked and no new incentives can be distributed, potentially locking pending and future protocol fees in the `FeeManager` contract

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/WiseSecurity/WiseSecurity.sol#L419-L434 https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/FeeManager/FeeManager.sol#L689-L699 https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/FeeManager/FeeManager.sol#L663-L670

Vulnerability details

Bug Description

Protocol fees can be collected from the WiseLending contract and sent to the FeeManager contract via the permissionless FeeManager::claimWiseFees function. During this call, incentives will only be distributed for incentive owners if totalBadDebtETH (global bad debt) is equal to 0:

FeeManager::claimWiseFees

663:        if (totalBadDebtETH == 0) { // @audit: incentives only distributed if there is no global bad debt
664:
665:            tokenAmount = _distributeIncentives( // @audit: distirbutes incentives for `incentive owners` via `gatheredIncentiveToken` mapping
666:                tokenAmount,
667:                _poolToken,
668:                underlyingTokenAddress
669:            );
670:        }

The fees sent to the FeeManager are then able to be claimed by beneficials via the FeeManager::claimFeesBeneficial function or by incentive owners via the FeeManager::claimIncentives function (if incentives have been distributed to the owners):

FeeManager::claimIncentives

284:    function claimIncentives(
285:        address _feeToken
286:    )
287:        public
288:    {
289:        uint256 amount = gatheredIncentiveToken[msg.sender][_feeToken]; // @audit: mapping incremented in _distributeIncentives function
290:
291:        if (amount == 0) {
292:            revert NoIncentive();
293:        }

However, beneficials are only able to claim fees if there is currently no global bad debt in the system (totalBadDebtETH == 0).

FeeManager::claimFeesBeneficial

689:    function claimFeesBeneficial(
690:        address _feeToken,
691:        uint256 _amount
692:    )
693:        external
694:    {
695:        address caller = msg.sender;
696:
697:        if (totalBadDebtETH > 0) { // @audit: can't claim fees when there is bad debt
698:            revert ExistingBadDebt();
699:        }

Below I will explain how the bad debt accounting logic used during partial liquidations can result in a state where totalBadDebtETH is permanently greater than 0. When this occurs, beneficials will no longer be able to claim fees via the FeeManager::claimFeesBeneficial function and new incentives will no longer be distributed when fees are permissionlessly collected via the FeeManager::claimWiseFees function.

When a position is partially liquidated, the WiseSecurity::checkBadDebtLiquidation function is executed to check if the position has created bad debt, i.e. if the position's overall borrow value is greater than the overall (unweighted) collateral value. If the post liquidation state of the position created bad debt, then the bad debt is recorded in a global and position-specific state:

WiseSecurity::checkBadDebtLiquidation

405:    function checkBadDebtLiquidation(
406:        uint256 _nftId
407:    )
408:        external
409:        onlyWiseLending
410:    {
411:        uint256 bareCollateral = overallETHCollateralsBare(
412:            _nftId
413:        );
414:
415:        uint256 totalBorrow = overallETHBorrowBare(
416:            _nftId
417:        );
418:
419:        if (totalBorrow < bareCollateral) { // @audit: LTV < 100%
420:            return;
421:        }
422:
423:        unchecked {
424:            uint256 diff = totalBorrow
425:                - bareCollateral;
426:
427:            FEE_MANAGER.increaseTotalBadDebtLiquidation( // @audit: global state, totalBadDebtETH += diff
428:                diff
429:            );
430:
431:            FEE_MANAGER.setBadDebtUserLiquidation( // @audit: position state, badDebtPosition[_nftId] = diff
432:                _nftId,
433:                diff
434:            );
435:        }
436:    }

FeeManagerHeper.sol

77:    function _setBadDebtPosition(
78:        uint256 _nftId,
79:        uint256 _amount
80:    )
81:        internal
82:    {
83:        badDebtPosition[_nftId] = _amount; // @audit: position bad debt set
84:    }
85:
86:    /**
87:     * @dev Internal increase function for global bad debt amount.
88:     */
89:    function _increaseTotalBadDebt(
90:        uint256 _amount
91:    )
92:        internal
93:    {
94:        totalBadDebtETH += _amount; // @audit: total bad debt incremented

As we can see above, the method by which the global and position's state is updated is not consistent (total debt increases, but position's debt is set to recent debt). Since liquidations can be partial, a position with bad debt can undergo multiple partial liquidations and each time the totalBadDebtETH will be incremented. However, the badDebtPosition for the position will only be updated with the most recent bad debt that was recorded during the last partial liquidation. Note that due to the condition on line 419 of WiseSecurity::checkBadDebtLiquidation, the badDebtPosition will be reset to 0 when totalBorrow == bareCollateral (LTV == 100%). However, in this case, any previously recorded bad debt for the position will not be deducted from the totalBadDebtETH. Lets consider two examples:

Scenario 1: Due to a market crash, a position's LTV goes above 100%. The position gets partially liquidated, incrementing totalBadDebtETH by x (bad debt from 1st liquidation) and setting badDebtPosition[_nftId] to x. The position gets partially liquidated again, this time incrementing totalBadDebtETH by y (bad debt from 2nd liquidation) and setting badDebtPosition[_nftId] to y. The resulting state:

totalBadDebtETH == x + y
badDebtPosition[_nftId] == y

Scenario 2: Due to a market crash, a position's LTV goes above 100%. The position gets partially liquidated, incrementing totalBadDebtETH by x and setting badDebtPosition[_nftId] to x. The position gets partially liquidated again, but this time the totalBorrow is equal to bareCollateral (LTV == 100%) and thus no bad debt is created. Due to the condition on line 419, totalBadDebtETH will be incremented by 0, but badDebtPosition[_nftId] will be reset to 0. The resulting state:

totalBadDebtETH == x
badDebtPosition[_nftId] == 0

Note that scenario 1 is more likely to occur since scenario 2 requires the additional partial liquidation to result in an LTV of exactly 100% for the position.

As we can see, partial liquidations can lead to totalBadDebtETH being artificially inflated with respect to the actual bad debt created by a position.

When bad debt is created, it is able to be paid back via the FeeManager::paybackBadDebtForToken or FeeManager::paybackBadDebtNoReward functions. However, the maximum amount of bad debt that can be deducted during these calls is capped at the bad debt recorded for the position specified (badDebtPosition[_nftId]). Therefore, the excess "fake" bad debt can not be deducted from totalBadDebtETH, resulting in totalBadDebtETH being permanently greater than 0.

Below is the logic that deducts the bad debt created by a position when it is paid off via one of the payback functions mentioned above:

FeeManagerHelper::_updateUserBadDebt

170:        unchecked {
171:            uint256 newBadDebt = currentBorrowETH
172:                - currentCollateralBareETH;
173:
174:            _setBadDebtPosition( // @audit: badDebtPosition[_nftId] = newBadDebt
175:                _nftId,
176:                newBadDebt
177:            );
178:
179:            newBadDebt > currentBadDebt // @audit: totalBadDebtETH updated with respect to change in badDebtPosition
180:                ? _increaseTotalBadDebt(newBadDebt - currentBadDebt)
181:                : _decreaseTotalBadDebt(currentBadDebt - newBadDebt);

The above code is invoked in the FeeManagerHelper::updatePositionCurrentBadDebt function, which is in turn invoked during both of the payback functions previously mentioned. You will notice that the above code properly takes into account the change in the bad debt of the position in question. I.e. if the badDebtPosition[_nftId] decreased (after being paid back), then the totalBadDebtETH will decrease as well. Therefore, the totalBadDebtETH can only be deducted by at most the current bad debt of a position. Returning to the previous example in scenario 1, this means that totalBadDebtETH would remain equal to x, since only y amount of bad debt can be paid back.

Impact

In the event a position creates bad debt, partial liquidations of that position can lead to the global totalBadDebtETH state variable being artificially inflated. This additional "fake debt" can not be deducted from the global state when the actual bad debt of the position is paid back. Thus, the FeeManager::claimFeesBeneficial function will be permanently DOS-ed, preventing any beneficials from claiming fees in the FeeManager contract. Additionally, no new incentives are able to be distributed to incentive owners in this state. However, protocol fees can still be collected in this state via the permissionless FeeManager::claimWiseFees function, and since incentive owners and beneficials are the only entities able to claim these fees, this can lead to fees being permanently locked in the FeeManager contract.

Justification for Medium Severity:

Although not directly affecting end users, the function of claiming beneficial fees and distributing new incentives will be permanently bricked. To make matters worse, anyone can continue to collect fees via the permissionless FeeManager::claimWiseFees function, which will essentially "burn" any pending or future fees by locking them in the FeeManager (assuming all previously gathered incentives have been claimed). This value is therefore leaked from the protocol every time additional fees are collected in this state.

Once this state is reached, any pending or future fees should ideally be left in the WiseLending contract, providing value back to the users instead of allowing that value to be unnecessarily "burned". However, the permissionless nature of the FeeManager::claimWiseFees function allows bad actors to further grief the protocol during this state by continuing to collect fees.

Note that once this state is reached, and Wise Lending is made aware of the implications, all fees (for all pools) can be set to 0 by the master address. This would ensure that no future fees are sent to the FeeManager. However, this does not stop pending fees from being collected. Additionally, a true decentralized system (such as a DAO) would likely have some latency between proposing such a change (decreasing fee value) and executing that change. Therefore, any fees distributed during that period can be collected.

Proof of Concept

Place the following test in the contracts/ directory and run with forge test --match-path contracts/BadDebtTest.t.sol:

// SPDX-License-Identifier: -- WISE --

pragma solidity =0.8.24;

import "./WiseLendingBaseDeployment.t.sol";

contract BadDebtTest is BaseDeploymentTest {
    address borrower = address(0x01010101);
    address lender = address(0x02020202);

    uint256 depositAmountETH = 10e18; // 10 ether
    uint256 depositAmountToken = 10; // 10 ether
    uint256 borrowAmount = 5e18; // 5 ether

    uint256 nftIdLiquidator; // nftId of lender
    uint256 nftIdLiquidatee; // nftId of borrower

    uint256 debtShares;

    function _setupIndividualTest() internal override {
        _deployNewWiseLending(false);

        // set token value for simple calculations
        MOCK_CHAINLINK_2.setValue(1 ether); // 1 token == 1 ETH
        assertEq(MOCK_CHAINLINK_2.latestAnswer(), MOCK_CHAINLINK_ETH_ETH.latestAnswer());
        vm.stopPrank();

        // fund lender and borrower
        vm.deal(lender, depositAmountETH);
        deal(address(MOCK_WETH), lender, depositAmountETH);
        deal(address(MOCK_ERC20_2), borrower, depositAmountToken * 2);
    }

    function testScenario1() public {
        // --- scenario is set up --- //
        _setUpScenario();

        // --- shortfall event/crash creates bad debt, position partially liquidated logging bad debt --- //
        _marketCrashCreatesBadDebt();

        // --- borrower gets partially liquidated again --- //
        vm.prank(lender);

        LENDING_INSTANCE.liquidatePartiallyFromTokens(
            nftIdLiquidatee,
            nftIdLiquidator, 
            address(MOCK_WETH),
            address(MOCK_ERC20_2),
            debtShares * 2e16 / 1e18
        );

        // --- global bad det increases again, but user bad debt is set to current bad debt created --- // 
        uint256 newTotalBadDebt = FEE_MANAGER_INSTANCE.totalBadDebtETH();
        uint256 newUserBadDebt = FEE_MANAGER_INSTANCE.badDebtPosition(nftIdLiquidatee);

        assertGt(newUserBadDebt, 0); // userBadDebt reset to new bad debt, newUserBadDebt == current_bad_debt_created
        assertGt(newTotalBadDebt, newUserBadDebt); // global bad debt incremented again
        // newTotalBadDebt = old_global_bad_debt + current_bad_debt_created

        // --- user bad debt is paid off, but global bad is only partially paid off (remainder is fake debt) --- // 
        _tryToPayBackGlobalDebt();

        // --- protocol fees can no longer be claimed since totalBadDebtETH will remain > 0 --- // 
        vm.expectRevert(bytes4(keccak256("ExistingBadDebt()")));
        FEE_MANAGER_INSTANCE.claimFeesBeneficial(address(0), 0);
    }

    function testScenario2() public {
        // --- scenario is set up --- // 
        _setUpScenario();

        // --- shortfall event/crash creates bad debt, position partially liquidated logging bad debt --- //
        _marketCrashCreatesBadDebt();

        // --- Position manipulated so second partial liquidation results in totalBorrow == bareCollateral --- //
        // borrower adds collateral
        vm.prank(borrower);

        LENDING_INSTANCE.solelyDeposit(
            nftIdLiquidatee, 
            address(MOCK_ERC20_2), 
            6
        );

        // borrower gets partially liquidated again
        vm.prank(lender);

        LENDING_INSTANCE.liquidatePartiallyFromTokens(
            nftIdLiquidatee,
            nftIdLiquidator, 
            address(MOCK_WETH),
            address(MOCK_ERC20_2),
            debtShares * 2e16 / 1e18
        );

        uint256 collateral = SECURITY_INSTANCE.overallETHCollateralsBare(nftIdLiquidatee);
        uint256 debt = SECURITY_INSTANCE.overallETHBorrowBare(nftIdLiquidatee);
        assertEq(collateral, debt); // LTV == 100% exactly

        // --- global bad debt is unchanged, while user bad debt is reset to 0 --- // 
        uint256 newTotalBadDebt = FEE_MANAGER_INSTANCE.totalBadDebtETH();
        uint256 newUserBadDebt = FEE_MANAGER_INSTANCE.badDebtPosition(nftIdLiquidatee);

        assertEq(newUserBadDebt, 0); // user bad debt reset to 0
        assertGt(newTotalBadDebt, 0); // global bad debt stays the same (fake debt)

        // --- attempts to pay back fake global debt result in a noop, totalBadDebtETH still > 0 --- // 
        uint256 paybackShares = _tryToPayBackGlobalDebt();

        assertEq(LENDING_INSTANCE.userBorrowShares(nftIdLiquidatee, address(MOCK_WETH)), paybackShares); // no shares were paid back

        // --- protocol fees can no longer be claimed since totalBadDebtETH will remain > 0 --- //
        vm.expectRevert(bytes4(keccak256("ExistingBadDebt()")));
        FEE_MANAGER_INSTANCE.claimFeesBeneficial(address(0), 0);
    }

    function _setUpScenario() internal {
        // lender supplies ETH
        vm.startPrank(lender);

        nftIdLiquidator = POSITION_NFTS_INSTANCE.mintPosition();

        LENDING_INSTANCE.depositExactAmountETH{value: depositAmountETH}(nftIdLiquidator);

        vm.stopPrank();

        // borrower supplies collateral token and borrows ETH
        vm.startPrank(borrower);

        MOCK_ERC20_2.approve(address(LENDING_INSTANCE), depositAmountToken * 2);

        nftIdLiquidatee = POSITION_NFTS_INSTANCE.mintPosition();

        LENDING_INSTANCE.solelyDeposit( // supply collateral
            nftIdLiquidatee, 
            address(MOCK_ERC20_2), 
            depositAmountToken
        );

        debtShares = LENDING_INSTANCE.borrowExactAmountETH(nftIdLiquidatee, borrowAmount); // borrow ETH

        vm.stopPrank();
    }

    function _marketCrashCreatesBadDebt() internal {
        // shortfall event/crash occurs
        vm.prank(MOCK_DEPLOYER);

        MOCK_CHAINLINK_2.setValue(0.3 ether);

        // borrower gets partially liquidated
        vm.startPrank(lender);

        MOCK_WETH.approve(address(LENDING_INSTANCE), depositAmountETH);

        LENDING_INSTANCE.liquidatePartiallyFromTokens(
            nftIdLiquidatee,
            nftIdLiquidator, 
            address(MOCK_WETH),
            address(MOCK_ERC20_2),
            debtShares * 2e16 / 1e18 + 1 
        );

        vm.stopPrank();

        // global and user bad debt is increased
        uint256 totalBadDebt = FEE_MANAGER_INSTANCE.totalBadDebtETH();
        uint256 userBadDebt = FEE_MANAGER_INSTANCE.badDebtPosition(nftIdLiquidatee);

        assertGt(totalBadDebt, 0); 
        assertGt(userBadDebt, 0);
        assertEq(totalBadDebt, userBadDebt); // user bad debt and global bad debt are the same
    }

    function _tryToPayBackGlobalDebt() internal returns (uint256 paybackShares) {
        // lender attempts to pay back global debt
        paybackShares = LENDING_INSTANCE.userBorrowShares(nftIdLiquidatee, address(MOCK_WETH));
        uint256 paybackAmount = LENDING_INSTANCE.paybackAmount(address(MOCK_WETH), paybackShares);

        vm.startPrank(lender);

        MOCK_WETH.approve(address(FEE_MANAGER_INSTANCE), paybackAmount);

        FEE_MANAGER_INSTANCE.paybackBadDebtNoReward(
            nftIdLiquidatee, 
            address(MOCK_WETH), 
            paybackShares
        );

        vm.stopPrank();

        // global bad debt and user bad debt updated
        uint256 finalTotalBadDebt = FEE_MANAGER_INSTANCE.totalBadDebtETH();
        uint256 finalUserBadDebt = FEE_MANAGER_INSTANCE.badDebtPosition(nftIdLiquidatee);

        assertEq(finalUserBadDebt, 0); // user has no more bad debt, all paid off
        assertGt(finalTotalBadDebt, 0); // protocol still thinks there is bad debt
    }
}

Tools Used

Manual

Recommended Mitigation Steps

I would recommend updating totalBadDebtETH with the difference of the previous and new bad debt of a position in the WiseSecurity::checkBadDebtLiquidation function, similar to how it is done in the FeeManagerHelper::_updateUserBadDebt internal function.

Example implementation:

diff --git a/./WiseSecurity/WiseSecurity.sol b/./WiseSecurity/WiseSecurity.sol
index d2cfb24..75a34e8 100644
--- a/./WiseSecurity/WiseSecurity.sol
+++ b/./WiseSecurity/WiseSecurity.sol
@@ -424,14 +424,22 @@ contract WiseSecurity is WiseSecurityHelper, ApprovalHelper {
             uint256 diff = totalBorrow
                 - bareCollateral;

-            FEE_MANAGER.increaseTotalBadDebtLiquidation(
-                diff
-            );
+            uint256 currentBadDebt = FEE_MANAGER.badDebtPosition(_nftId);

             FEE_MANAGER.setBadDebtUserLiquidation(
                 _nftId,
                 diff
             );
+
+            if (diff > currentBadDebt) {
+                FEE_MANAGER.increaseTotalBadDebtLiquidation(
+                    diff - currentBadDebt
+                );
+            } else {
+                FEE_MANAGER.decreaseTotalBadDebtLiquidation(
+                    currentBadDebt - diff
+                );
+            }
         }
     }

Assessed type

Other

[c4-pre-sort]

GalloDaSballo marked the issue as primary issue

[c4-pre-sort]

GalloDaSballo marked the issue as sufficient quality report

[c4-pre-sort]

GalloDaSballo marked the issue as high quality report

[vm06007]

maybe @Foon256 can comment on this.

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-judge]

trust1995 marked the issue as selected for report

[c4-judge]

trust1995 changed the severity to 3 (High Risk)

[GalloDaSballo]

Summary of the issue

When a market accrues bad debt, which can be inflated due to an accounting error, fees and incentives will no longer be distributed Note The discussion had quite a bit of back and forth, for this reason the whole conversation is pasted below

Discussion

Alex: This seems to be tied to a specific interpretation of this discussion we've had around loss of yield as high

Hickup: fees would be considered as matured yield? given that it extends beyond the protocol to beneficials and incentive owners, i'm leaning towards a high more than a med

Alex: Yes it would be considered matured

I don't have an opinion on this report yet and will follow up later today with my notes

Not fully made up my mind but here's a couple of points:

For Med: Loss of Yield -> There is no loss of principal so Med seems fine

For High: The contract is not losing yield in some case, the contract is losing 100% of all yield The contract is no longer serving it's purpose

External Conditions: Bad debt must be formed

Bad debt handling is part of the system design, so assuming this can happen is fair, and starting from a scenario in which this can happen is also fair

That said, in reality, this may never happen

My main point for downgrade is that while the contract is losing all of the yield, nothing beside that is impacted, not fully sure on this one

LSDan: I'm aligned with high on this one. Even though the conditions that lead to it are rare and there are arguably external conditions in some scenarios, there is a direct loss of funds and the functional loss of a contract's purpose. Once this situation occurs, there is no clean way back from it.

Alex: I think this is the issue where we will have some contention, I think the Sponsor interpretation is important to keep in mind as it's pretty rational

I would like to think about it a bit more

I'm leaning towards Med on this report, I think the Sponsors POV is valid

There is an accounting error, it would not cause permanent loss of funds It would be mitigated by deprecating the market and creating a new one

My main argument is that if this was live, this would trigger a re-deploy but it would not trigger any white hat rescue operation, as funds would be safe

Hickup:

74: When we stick to the c4a rules to which we agreed all the loss of fees are no user funds and therrfore should be threated differently.

The core argument for M severity is that fees are a secondary concern.

This goes against the supreme court decision where fees shouldn't be treated as 2nd class citizens: https://docs.code4rena.com/awarding/judging-criteria/severity-categorization#loss-of-fees-as-low Loss of fees should be regarded as an impact similar to any other loss of capital: Loss of real amounts depends on specific conditions and likelihood considerations.

Likelihood: Requirement of bad debt formation. Once there is, funds (fees) are permanently bricked.

There is an accounting error, it would not cause permanent loss of funds It would be mitigated by deprecating the market and creating a new one

The funds you are referring to are user funds? Separately, I don't see how it would mitigate the bricking once it happens.

Alex: I don't think that the ruling means that loss of fees should be treated as high at all times

The main argument is that the broken accounting doesn't create a state that is not recoverable: Some fees are lost User deprecates market (raises interests or pauses) Deployes new Market System resumes functioning as intended

My main argument is that this would not cause a War Room, it would cause a deprecation that the system can handle

Hickup: In what cases / scenarios would loss of fees be high then? Most, if not all, won't have a war room for protocol fees

The reason I would consider to justify downgrading is the low likelihood of the external requirement of bad debt formation + >= 2 partial liquidations

I would dissent and argue for high severity. permanent loss of unclaimed fees blast radius: affects not just the protocol, but incentive owners and beneficiaries. Had the fees gone only to the protocol, I'd lean a bit more towards M.

Is WiseLending immutable in a poolToken instance?

What contracts would have to be re-deployed?

Alex: Liquidation premium being denied could be a valid High loss of yield, Loss of gas for refunds when the system entire goal is that (e.g. keepers, voting on Nouns)

Alex’s Input

The finding shows how in the specific case of liquidations with bad debt, a market will stop accounting for fees

2 aggravating circumnstances seem to be: Inability to pause and replace each market The Math for bad debt is also wrong, leading to the inability to fix the bug

This would still cause a loss of fees for a certain period of time, as the admin would eventually be able to set the market fees to either a state that would cause users to stop using it or 0 as a means to stop the loss

I think that the accounting mistake is notable, and I understand the reasoning for raising severity

That said, because we have to judge by impact of the finding, I believe Medium Severity to be most appropriate

Hickuphh3’s Input

I maintain my stance for H severity for the reasons I stated above: permanent loss of unclaimed fees impact on protocol ecosystem: beneficiaries and incentive owners

LsDan’s Input

I'm still of the opinion that High is most appropriate here. The impact is significant enough that raising the severity beyond medium makes sense.

Deliberation

The severity is kept at High Severity, with a non unanimous verdict

Additional Context by the Lead Judge

I recommend monitoring how this decision influences future decisions on severities, especially when it comes to a percentage loss of yield, an attacker having the button to cause a loss of yield, against this instance which is the permanent inability for the contract to record a gain of yield.

[thebrittfactor]

For transparency and per conversation with the sponsors, see here for the Wise Lending team's mitigation.