sherlock-audit / 2023-10-aloe-judging

9 stars 6 forks source link

Chinmay - Borrows are not properly handled in the case of both liabilities0 and liabilities1 unable to be repaid from available assets #104

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

Chinmay

high

Borrows are not properly handled in the case of both liabilities0 and liabilities1 unable to be repaid from available assets

Summary

After withdrawing the uniswap positions, the aggregate amount of assets( fixed assets + withdrawn uni positions + fees) is used to pay back the borrowed assets and if any one of assets has a shortfall, then the other asset is swapped into it to pay back to the lender. But if both liabilities are non-zero after repayment from the balances, nothing is swapped, whatever can be repaid is repaid and the remaining bad debt borrows is left untouched. The problem is that these left borrows will keep accruing more interest in Lender's accounting and will lead to an increased bad debt.

Vulnerability Detail

Borrower.sol Line 252 is meant to swap only if one asset is surplus and one is in shortfall. But the checks above that state that if liabilities0 and liabilities1 are non-zero nothing more can be done. If a a borrower account ever arrives in such a situation, then the remaining unpaid borrow amounts will be left as it is in the Lender's accounting and keep accruing more and more interest forever. Instead, the bad debt should be absorbed into the pool then and there and the accounting updated to make the pending borrows zero(which means shares are going to be diluted a little bit but think about the scenario if many such big positions are hanging in the borrows and accruing interest continuously, eventually the losses to lenders will be higher) and the borrower blacklisted for just this case.

Impact

Lenders continuously accrue losses in the form of bad debt for positions that have no left collateral and the borrower will never pay back that debt because he will lose money. Thus, if many such positions exist, the total borrows will keep growing that have no backing collateral, causing a risk of some of the lenders not being able to withdraw in the future because of this pending borrow amount.

Code Snippet

https://github.com/sherlock-audit/2023-10-aloe/blob/b60c21af24738d517941f18f7caa8c7272f771c5/aloe-ii/core/src/Borrower.sol#L241

Tool used

Manual Review

Recommendation

In the liquidate function, if both liabilities0 and liabilities1 are non-zero, close the position and absorb bad debt into the lender contracts' accounting and blacklist the borrower.

Duplicate of #32

sherlock-admin2 commented 1 year ago

2 comment(s) were left on this issue during the judging contest.

panprog commented:

valid medium, dup of #32

MohammedRizwan commented:

valid

haydenshively commented 1 year ago

I believe #42 and #43 are duplicates of this

panprog commented 1 year ago

Escalate

This issue is dup of #32 and is completely different from #42 and #43 . This issue is also Medium, like #32 and all its dups.

42 - should be a separate Medium issue. It shows how liquidator trying to liquidate account which got into bad debt will be unable to receive it's bonus ETH ante. So bascally ante will be stuck in the contract without ability to get it back by anyone.

43 - should also be a separate Medium issue, a very comprehensive description of why liquidation might not be profitable in certain edge cases, meaning that liquidation will not happen for extended periods of time, which can cause bad debt (but the main topic is how liquidation might not be profitable for liquidators, not bad debt itself).

sherlock-admin2 commented 1 year ago

Escalate

This issue is dup of #32 and is completely different from #42 and #43 . This issue is also Medium, like #32 and all its dups.

42 - should be a separate Medium issue. It shows how liquidator trying to liquidate account which got into bad debt will be unable to receive it's bonus ETH ante. So bascally ante will be stuck in the contract without ability to get it back by anyone.

43 - should also be a separate Medium issue, a very comprehensive description of why liquidation might not be profitable in certain edge cases, meaning that liquidation will not happen for extended periods of time, which can cause bad debt (but the main topic is how liquidation might not be profitable for liquidators, not bad debt itself).

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.

Shogoki commented 1 year ago

Escalate

This issue is dup of #32 and is completely different from #42 and #43 . This issue is also Medium, like #32 and all its dups.

42 - should be a separate Medium issue. It shows how liquidator trying to liquidate account which got into bad debt will be unable to receive it's bonus ETH ante. So bascally ante will be stuck in the contract without ability to get it back by anyone.

43 - should also be a separate Medium issue, a very comprehensive description of why liquidation might not be profitable in certain edge cases, meaning that liquidation will not happen for extended periods of time, which can cause bad debt (but the main topic is how liquidation might not be profitable for liquidators, not bad debt itself).

I agree that this should be duped with #32 but as i stated in my other escalation i think #42 should also be duplicated with #32

chinmay-farkya commented 1 year ago

Escalate

This issue is dup of #32 and is completely different from #42 and #43 . This issue is also Medium, like #32 and all its dups.

42 - should be a separate Medium issue. It shows how liquidator trying to liquidate account which got into bad debt will be unable to receive it's bonus ETH ante. So bascally ante will be stuck in the contract without ability to get it back by anyone.

43 - should also be a separate Medium issue, a very comprehensive description of why liquidation might not be profitable in certain edge cases, meaning that liquidation will not happen for extended periods of time, which can cause bad debt (but the main topic is how liquidation might not be profitable for liquidators, not bad debt itself).

I disagree. This is a high severity issue because : Consider the situation that after subtraction from the available token balances in the contract, the remaining liabilities > assets (ie. uniswap position). Since it can be a fairly common occurence that users have more of the assets as uniswap positions and almost none as the raw token balances, naturally to earn more LP rewards from uniswap. This means that most of the assets might exist as uniswap positions for almost all users.

Now if such a price change occurs (as quoted in the issue, and fairly possible because crypto can be very volatile) this means that there are fairly high chances of bad debt => when almost all positions get liquidated in case of abrupt price change. And almost all users may have it as the primary source of asset, withdrawing of liquidity might yield suboptimal liability coverage.

This means that many users (possibly with large debt) can get liquidated at the same time and their debt will not be covered and their position will not be closed (as explained in the issue above) and will keep accruing more interest with time, rendering the aloe market for that pool unusable forever, as well as lenders losing their existing deposits.

So, high impact and fair possibility => high severity.

Also, this bad debt can also get accumulated due to another issue that I reported here

cvetanovv commented 1 year ago

Escalate

This issue is dup of #32 and is completely different from #42 and #43 . This issue is also Medium, like #32 and all its dups.

42 - should be a separate Medium issue. It shows how liquidator trying to liquidate account which got into bad debt will be unable to receive it's bonus ETH ante. So bascally ante will be stuck in the contract without ability to get it back by anyone.

43 - should also be a separate Medium issue, a very comprehensive description of why liquidation might not be profitable in certain edge cases, meaning that liquidation will not happen for extended periods of time, which can cause bad debt (but the main topic is how liquidation might not be profitable for liquidators, not bad debt itself).

I agree with the escalation. At first, I had put 42 and 43 on their own. But I saw the person's comments from the protocol and decided to duplicate them. But now that I read it again it seems to me what you wrote in the escalation for correct

haydenshively commented 1 year ago

My bad on the original comment, I agree with escalation as well.

Trumpero commented 1 year ago

Planning to accept the escalation from @panprog, mark this issue as a dup of #32, and label #42 and #43 as separate unique medium issues.

Czar102 commented 1 year ago

From my understanding this issue presents a scenario where bad debt is created and it cannot be paid back.

The existence of bad debt and the way it is handled seem to be a part of the protocol design, suggested, for example, here.

Planning to (de-)duplicate as suggested, but this issue and #32 should be invalid.

roguereddwarf commented 1 year ago

@Czar102 It is indeed a design decision of the protocol to not socialize bad debt based on the fact that dynamic LTV should prevent any such bad debt in the first place.

If the fact that not socializing bad debt is a design decision is sufficient to mark this as invalid, I agree that this should be invalid. Even though in internal discussions with @Trumpero I said lack of bad debt socialization is a Medium.

chinmay-farkya commented 1 year ago

It is indeed a design decision of the protocol to not socialize bad debt based on the fact that dynamic LTV should prevent any such bad debt in the first place.

That doesn't mean there will never be a scenario with both liabilities exceeding the assets. Specifically, my issue says that in cases of flash price movements, it is possible that some big positions get affected, impacting the aloe market forever. Also, in normal cases too, bad interest accruing on existing bad debt can not be called a design choice imo, especially when we are talking about bad debt of both tokens.

Even if the protocol decides to not socialize, it should at least close those bad positions so that they do not keep accruing more interest overtime.

roguereddwarf commented 1 year ago
That doesn't mean there will never be a scenario with both liabilities exceeding the assets. Specifically, my issue says that in cases of flash price movements, it is possible that some big positions get affected, impacting the aloe market forever.
Also, in normal cases too, bad interest accruing on existing bad debt can not be called a design choice imo, especially when we are talking about bad debt of both tokens.

This is true. We can never be sure about anything. But dynamic LTV is supposed to ensure that even when there is a "flash price movement" there can be no bad debt. Not having a bad debt socialization mechanism therefore is a design decision and if this is how the judging rules are to be interpreted, then I think this should be invalid. Your issue is the same as the other proposed duplicates, just providing a different point of view.

chinmay-farkya commented 1 year ago

But dynamic LTV is supposed to ensure that even when there is a "flash price movement" there can be no bad debt.

If I'm correct LTVs depend on the IV, which can only be updated once per hour, and upto a certain limit, which means it is a lagging indicator. So this does not conclude that bad debt cannot happen. The IV and LTV will take time to adjust to such a price movement, during this duration a borrower can be underwater and get liquidated leaving behind remaining debt.

roguereddwarf commented 1 year ago

If I'm correct LTVs depend on the IV, which can only be updated once per hour, and upto a certain limit, which means it is a lagging indicator. This is true. But the IV model that Aloe uses for it's calculations, including the number of updates that can be made to IV, is based on historic data. Based on historic data it can therefore be concluded that the chance for bad debt to be caused is tiny. I.e., it won't occur even once within our lifetimes.

The model is not perfect but it's a design decision to use that model.

roguereddwarf commented 1 year ago

See here that LTV is based on a financial model: https://docs.aloe.capital/aloe-ii/mathematics/adaptive-ltvs

If the update frequency of IV was a problem or IV was calculated incorrectly, that would be a separate issue (see some of the other valid issues). The fact that there is no debt socialization mechanism is a design decision though.

chinmay-farkya commented 1 year ago

From my understanding this issue presents a scenario where bad debt is created and it cannot be paid back.

The existence of bad debt and the way it is handled seem to be a part of the protocol design, suggested, for example, here.

Planning to (de-)duplicate as suggested, but this issue and #32 should be invalid.

I still think this to be a valid medium. As the lead Judge and the sponsor agreed above. At least the bad debt position needs to be closed if losses not being socialized is a design choice.

Also, I think brushing it off as a design choice is vague because the following issues : https://github.com/sherlock-audit/2023-10-aloe-judging/issues/34 https://github.com/sherlock-audit/2023-10-aloe-judging/issues/145 https://github.com/sherlock-audit/2023-10-aloe-judging/issues/55

can be brushed off as design choices.

I think the current finding does cause loss of funds due to accumulation of more interest(even if bad debt is considered unlikely) which is never gonna be paid, justifying medium severity.

roguereddwarf commented 1 year ago

Some of the issues you mentioned cannot be brushed off as design choices. In this case it can be "brushed off". Even though that's not the right word for it. The reason is as stated above:

Based on historic data it can therefore be concluded that the chance for bad debt to be caused is tiny. I.e., it won't occur even once within our lifetimes.
chinmay-farkya commented 1 year ago

I believe that the notion "bad debt cannot happen" is incorrect. There are tons of different reasons bad debt is possible even if IV is based on historic data, one of those being that the future price action is not guaranteed to follow the historic data.

The purpose of a medium severity issue is to flag an event of loss of funds, even if it is unlikely as per our judgement. The occurence of variable and abrupt price action is quite regular in crypto (and IV is a lagging indicator in such a situation) which will cause bad debt. So, the protocol needs to protect against bad debt in all possible ways.

Not accumulating more debt on already existing bad debt is one of those ways. Rather if the protocol believes bad debt is never possible, all protections against it could be done away with. That should never be the case imo.

Czar102 commented 1 year ago

Bad debt can happen and this is considered by the protocol team. If we make no assumptions about markets, this scenario can be thought of as very probable. But nevertheless, this is how the protocol was designed.

And yes, some of the mentioned issues are commenting design choices. The escalated ones will be treated as they should.

Czar102 commented 1 year ago

Result: Invalid Duplicate of #32

sherlock-admin2 commented 1 year ago

Escalations have been resolved successfully!

Escalation status:

haydenshively commented 12 months ago

Fixed in https://github.com/aloelabs/aloe-ii/pull/223

roguereddwarf commented 12 months ago

Mitigation Review:

See parent issue #32.