HHK - No deadline and slippage check on takeOverDebt() can lead to unexpected results

[sherlock-admin]

HHK

medium

No deadline and slippage check on takeOverDebt() can lead to unexpected results

Summary

The function takeOverDebt() in the LiquidityBorrowingManager contract doesn't have any deadline check like borrow or repay.

Additionally it also doesn't have any "slippage" check that would make sure the position hasn't changed between the moment a user calls takeOverDebt() and the transaction is confirmed.

Vulnerability Detail

Blockchains are asynchronous by nature, when sending a transaction, the contract targeted can see its state changed affecting the result of our transaction.

When a user wants to call takeOverDebt() he expects that the debt he is gonna take over will be the same (or very close) as when he signed the transaction.

By not providing any deadline check, if the transaction is not confirmed for a long time then the user might end up with a position that is not as interesting as it was.

Take this example:

• A position on Ethereum is in debt of collateral but the borrowed tokens are at profit and the user think the token's price is going to keep increasing, it makes sense to take over.
• The user makes a transaction to take over.
• The gas price rise and the transaction takes longer than he thoughts to be confirmed.
• Eventually the transaction is confirmed but the position is not in profit anymore because price changed during that time.
• User paid the debt of the position for nothing as he won't be making profits.

Additionally when the collateral of a position is negative, lenders can call repay() as part of the "emergency liquidity restoration mode" which will reduce the size of the position. If this happens while another user is taking over the debt then he might end up with a position that is not as interesting as he thoughts.

Take this second example:

• A position on Ethereum with 2 loans is in debt of collateral but the borrowed tokens are at profit and the user think the token's price is going to keep increasing, it makes sense to take over.
• The user makes a transaction to take over.
• While the user's transaction is in the MEMPOOL a lender call 'repay()' and get back his tokens.
• The user's transaction is confirmed and he take over the position but it only has 1 loan now as one of the 2 loans was sent back to the lender. the position might not be at profit anymore or less than it was supposed to be.
• User paid the debt of the position for nothing as he won't be making profits.

Impact

Medium. User might pay collateral debt of a position for nothing.

Code Snippet

https://github.com/sherlock-audit/2023-10-real-wagmi/blob/b33752757fd6a9f404b8577c1eae6c5774b3a0db/wagmi-leverage/contracts/LiquidityBorrowingManager.sol#L395

https://github.com/sherlock-audit/2023-10-real-wagmi/blob/b33752757fd6a9f404b8577c1eae6c5774b3a0db/wagmi-leverage/contracts/LiquidityBorrowingManager.sol#L581

Tool used

Manual Review

Recommendation

Consider adding the modifier checkDeadline() as well as a parameter minBorrowedAmount and compare it to the current borrowedAmount to make sure no lender repaid their position during the take over.

[fann95]

we will add a deadline check.

[fann95]

Fixed: RealWagmi/wagmi-leverage@97f30b9

[IAm0x52]

Escalate

While I agree that having the deadline enforcement is ideal, I don't see how this would cause any negative impact to the user taking over the loan besides wasted gas. Taking over a loan can only be done for a position that is eligible for liquidation. Liquidation is highly incentivized which means that a delayed transaction would already be very unlikely to succeed.

In the event that the position is not changed at all, the new borrower has collateralAmt to protect themselves from excess fees. If the owner of the position repays or if the loan is liquidated by another user ahead of this transaction, the takeOverDebt call will revert. If a lender requests emergency closure ahead of time then the fees owed will be reduced proportionally and any excess underwater fees would be written off here during the emergency repayment.

Due to this I don't see any negative financial impact to the new borrower besides wasted gas. I believe this should be a valid low.

[sherlock-admin2]

Escalate

While I agree that having the deadline enforcement is ideal, I don't see how this would cause any negative impact to the user taking over the loan besides wasted gas. Taking over a loan can only be done for a position that is eligible for liquidation. Liquidation is highly incentivized which means that a delayed transaction would already be very unlikely to succeed.

In the event that the position is not changed at all, the new borrower has collateralAmt to protect themselves from excess fees. If the owner of the position repays or if the loan is liquidated by another user ahead of this transaction, the takeOverDebt call will revert. If a lender requests emergency closure ahead of time then the fees owed will be reduced proportionally and any excess underwater fees would be written off here during the emergency repayment.

Due to this I don't see any negative financial impact to the new borrower besides wasted gas. I believe this should be a valid low.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[Czar102]

A user calling takeOverDebt() proposes to liquidate a position. Enforcing timely execution is callers' responsibility. Adding a deadline argument seems like a good add-on, but its lack does not seem to cause any loss of funds.
As far as I understand, this function will be called by MEV extractors, so revert after repaying is not an issue. It's the risk they are taking by design.
Realizing losses by liquidators because of price movement is out of scope. If there are other possible kinds of losses, please let me know. Otherwise will consider the issue a low and accept the escalation.

[HHK-ETH]

If a lender requests emergency closure ahead of time then the fees owed will be reduced proportionally and any excess underwater fees would be written off here during the emergency repayment.

I believe this is wrong, the line tagged add the collateral that was available to use to the fees owned to the lenders but it doesn't write off the underwater fees. These underwater fees are saved here. By updating the accLoanRatePerSeconds the contract will be able to recalculate the missing collateral on future calls.

Thus if a lender use emergency repay the user taking over the debt will be repaying the full missing collateral for a position that might not be worth it anymore.

[HHK-ETH]

You can check this comment from another issue to illustrate my point: #119 (comment)

As confirmed by the sponsor, the debt is maintained as no one is paying it off and to do that we play with the accLoanRatePerSeconds.

This means the user taking over pays the debt the position owned to all lenders including the ones that emergency withdrew. So we need to make sure no loan was emergency repaid during that time otherwise the user may end up at a loss by repaying the whole collateral missing for a position that is not worth it anymore.

My proposition was first to add a deadline but this is only half a fix, thus I proposed to also introduce a minBorrowedAmount parameter.

[Czar102]

@IAm0x52 could you verify the above comment?

Thus if a lender use emergency repay the user taking over the debt will be repaying collateral for a position that might not be worth it.

@HHK-ETH that would mean that the emergency withdraw could make underwater positions more underwater? Could you define "might not be worth it"?

[HHK-ETH]

@HHK-ETH that would mean that the emergency withdraw could make underwater positions more underwater? Could you define "might not be worth it"?

What I mean is that if you want to take over then you estimate that keeping the current position can make you money and thus you're okay to pay back the missing collateral.
Let's take an example:

• A position has 5 ETH long over 2 different loans and 0.2 ETH of missing collateral
• User is bullish on ETH and wants to take over instead of liquidate the position. Take note that when liquidating you don't have to pay the missing collateral.
• During the take over, one of the loans is reimbursed taking away 3 ETH from the position.
• Once take over is confirmed, the position now holds only 2 ETH and not the initial 5 ETH. the user repaid 0.2 ETH but has a smaller position than expected and the long might not generate enough profits to reimburse the extra collateral paid (0.2 ETH).

If he knew it would have happened he maybe would have called liquidate() instead or borrow() to borrow from different positions that might not generate as much upside but at least wouldn't require to repay missing collateral.

[HHK-ETH]

Take this second example:

This example used in the initial finding can be a little confusing as I talk about positions in profits. But after going through the contracts again I think the profits are not necessarily taken away when a lender emergency repays and so would stay on the position.

Still the example I gave above still stands, even in profits, if they're not superior than the underwater collateral paid (minus the liquidationBonus you got from the old position) you end up at a loss that you might not be able to reimburse longing a smaller position than expected.

[IAm0x52]

@HHK-ETH that would mean that the emergency withdraw could make underwater positions more underwater? Could you define "might not be worth it"?

What I mean is that if you want to take over then you estimate that keeping the current position can make you money and thus you're okay to pay back the missing collateral. Let's take an example:

• A position has 5 ETH long over 2 different loans and 0.2 ETH of missing collateral
• User is bullish on ETH and wants to take over instead of liquidate the position. Take note that when liquidating you don't have to pay the missing collateral.
• During the take over, one of the loans is reimbursed taking away 3 ETH from the position.
• Once take over is confirmed, the position now holds only 2 ETH and not the initial 5 ETH. the user repaid 0.2 ETH but has a smaller position than expected and the long might not generate enough profits to reimburse the extra collateral paid (0.2 ETH).

If he knew it would have happened he maybe would have called liquidate() instead or borrow() to borrow from different positions that might not generate as much upside but at least wouldn't require to repay missing collateral.

The problem with this is that the user taking over would not owe 0.2 ETH still because here fees owed by the position is reduced. So if the position owed 0.2 ETH and closed 3/5 (60%) then it would also remove 3/5 of the fees as we can see here since it uses a proportional calculation. This would leave the position with 0.08 ETH to repay when taken over.

[Czar102]

@HHK-ETH can you confirm?

[HHK-ETH]

The problem with this is that the user taking over would not owe 0.2 ETH still because here fees owed by the position is reduced. So if the position owed 0.2 ETH and closed 3/5 (60%) then it would also remove 3/5 of the fees as we can see here since it uses a proportional calculation. This would leave the position with 0.08 ETH to repay when taken over.

But if the total collateral to be paid is below 0 the fees accounted here are only the dailyCollateral available not the missing collateral. I believe feesOwed is accounting only fees that have been paid and not fees that couldn't be paid because of missing collateral.

[HHK-ETH]

Take this poc you can copy paste it in the main test file just keep the setup and nft creation test and delete the rest:

it("Updated accRate", async () => {
        const amountWBTC = ethers.utils.parseUnits("0.05", 8); //token0
        let deadline = (await time.latest()) + 60;
        const minLeverageDesired = 50;
        const maxCollateralWBTC = amountWBTC.div(minLeverageDesired);

        const loans = [
            {
                liquidity: nftpos[3].liquidity,
                tokenId: nftpos[3].tokenId,
            },
            {
                liquidity: nftpos[5].liquidity,
                tokenId: nftpos[5].tokenId,
            },
        ];

        const swapParams: ApproveSwapAndPay.SwapParamsStruct = {
            swapTarget: constants.AddressZero,
            swapAmountInDataIndex: 0,
            maxGasForCall: 0,
            swapData: swapData,
        };

        const borrowParams = {
            internalSwapPoolfee: 500,
            saleToken: WETH_ADDRESS,
            holdToken: WBTC_ADDRESS,
            minHoldTokenOut: amountWBTC,
            maxCollateral: maxCollateralWBTC,
            externalSwap: swapParams,
            loans: loans,
        };

        //borrow tokens
        await borrowingManager.connect(bob).borrow(borrowParams, deadline);

        await time.increase(3600 * 25); //1h of missing collateral since when we borrow we add collateral for a day
        deadline = (await time.latest()) + 60;

        const borrowingKey = await borrowingManager.userBorrowingKeys(bob.address, 0);

        let repayParams = {
            isEmergency: true,
            internalSwapPoolfee: 0,
            externalSwap: swapParams,
            borrowingKey: borrowingKey,
            swapSlippageBP1000: 0,
        };

        const oldCollateralMissing = await borrowingManager.checkDailyRateCollateral(borrowingKey);
        console.log(oldCollateralMissing.balance);

        //Alice emergency repay her loan
        await borrowingManager.connect(alice).repay(repayParams, deadline);

        const newCollateralMissing = await borrowingManager.checkDailyRateCollateral(borrowingKey);
        console.log(newCollateralMissing.balance);

        //missing collateral is still the same
        expect(oldCollateralMissing.balance).to.eq(newCollateralMissing.balance);
    });

It shows that the missing collateral is still the same after emergency repay.

[IAm0x52]

I believe this is wrong, the line tagged add the collateral that was available to use to the fees owned to the lenders but it doesn't write off the underwater fees. These underwater fees are saved here. By updating the accLoanRatePerSeconds the contract will be able to recalculate the missing collateral on future calls.

Yes this comment is incorrect. Underwater fees are saved. My initial comment regarding emergency closure was made based on the misconception that underwater fees are written off. Since they are not written off and fees continue to accumulate as normal minBorrowedAmount would still effectively function as a deadline. If the position is currently accumulating 1e16 interest per second then using a minBorrowedAmount that is 1e17 above the current amount owed, then if the takeOverDebt is delayed by more than 10 seconds then the call will fail.

[HHK-ETH]

See the finding recommendation:

minBorrowedAmount and compare it to the current borrowedAmount to make sure no lender repaid their position during the take over

My proposition doesn't act as a deadline but as a check on the position's size, by checking borrowingsInfo[key].borrowedAmount. If it's below the passed parameter minBorrowedAmount then revert.

This effectively protects you from having lender repaying their position while your transaction is being confirmed.

[IAm0x52]

See the finding recommendation:

minBorrowedAmount and compare it to the current borrowedAmount to make sure no lender repaid their position during the take over

My proposition doesn't act as a deadline but as a check on the position's size, by checking borrowingsInfo[key].borrowedAmount. If it's below the passed parameter minBorrowedAmount then revert.

So then what exact loss scenario are you preventing? As shown above, there is already effectively a deadline. The only one I see would be for a lender to frontrun their call with an emergency closure. But what would be the benefit of that? The lender calling it would end up losing fees that it would have otherwise received. They could potentially cause a loss to the user taking over the loan but would guarantee a loss for themselves.

[HHK-ETH]

So then what exact loss scenario are you preventing? As shown above, there is already effectively a deadline. The only one I see would be for a lender to frontrun their call with an emergency closure. But what would be the benefit of that? The lender calling it would end up losing fees that it would have otherwise received. They could potentially cause a loss to the user taking over the loan but would guarantee a loss for themselves.

The lender doesn't have to be malicious, he just need to be submitting a repay around the same time that the user is taking over. He doesn't know that someone is planning on taking over and might want his tokens back. Yes this is not likely to happen but it can happen if both are unlucky and will result in loss for the user and less fees for the lender.

I submitted this finding with slippage in the title because you could see it as a swap, let's imagine a nice world with no MEV. If you submit a swap on a pool but someone else swap on it before you (this user is not malicious but just happened to have called swap around the same time as you), you end up with a different amount than expected, Thus minOut act as a slippage protection.
Here it's kind of the same but way less likely because a position can have only 7 loans maximum so there is only 7 other actors, still the chance is not 0.

Additionally like you said lender can go rogue and loose his fees to make user taking over pay collateral for a smaller position.

Since there is money at stake I submitted this as a medium (and it seemed to be fitting criteria 1 or 3 https://docs.sherlock.xyz/audits/judging/judging#v.-how-to-identify-a-medium-issue) but ultimately if you think it's not likely enough to happen and cost is too high for a malicious lender then I could understand that severity is lowered.

[fann95]

#119 I have made changes to the emergency repayment. Now, in case of an emergency repayment, the debts for this liquidity will also be written off. Therefore, if someone takes over a debt, he will not pay in the event of a frontrunning emergency repayment.

[Czar102]

I think this issue is correctly classified as a medium. Planning to reject the escalation.

[HHK-ETH]

If only the slippage part of this finding is accepted as a medium and the deadline is considered low I would suggest reviewing the duplicate #134 as it only talks about deadline check.

[Czar102]

Planning to invalidate #134, too.

[Evert0x]

Based on the conversation this issue should be Medium without #134 as a duplicate.

[Evert0x]

Result:
Medium
Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• IAm0x52: rejected

[IAm0x52]

Fix looks good. The checkDeadline modifier has been added. In conjuction with changes made for #119 this is no longer an issue.