Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add repaidShares argument to liquidate function #322

Merged
merged 22 commits into from
Aug 23, 2023

Conversation

Jean-Grimal
Copy link
Contributor

@Jean-Grimal Jean-Grimal commented Aug 16, 2023

I think the liquidate function should either take seized or an amount of borrow shares to repay as amount, and one of them must be zero (just like the other entry points function).
Having seized as input is great to help realize bad debt.
But when the whole collateral can’t be seized (when the position just became liquidable), the whole debt can be repaid. And in this case, the max amount of collateral that can be seized is not easy to compute, and that could result in liquidators that doesn’t liquidate the whole debt (and leave a small amount of debt). This could be a problem for liquidators, but also for the protocol (because of borrow dust).
Moreover, this case is the one we want to incentive as this is a scenario where all the debt is liquidated, and therefore bad debt is avoided.
So I think liquidators should not struggle to liquidate the whole debt when it is possible.

Copy link
Contributor

@QGarchery QGarchery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm against the changes in this PR as they seem more dangerous than helpful:

  • borrow dust should not be an issue (by definition, it's dust)
  • it creates another path to liquidation, therefore making the path to realize the bad debt less of an obvious choice
  • liquidators don't want to repay as much as possible, they want to seize as much as possible. They should be incentivized to compute the max amount of collateral that they can seize, and at that point the changes are not helpful

@Jean-Grimal
Copy link
Contributor Author

  • it creates another path to liquidation, therefore making the path to realize the bad debt less of an obvious choice

I slightly agree with this. But if I remember well, we assumed that liquidators always wanted to liquidate as mush as possible, so this shouldn't be a problem? Because in case of bad debt, the whole debt can't be repaid anyway, so we can assume that liquidators will aim for the whole collateral.

  • liquidators don't want to repay as much as possible, they want to seize as much as possible. They should be incentivized to compute the max amount of collateral that they can seize, and at that point the changes are not helpful

Yes but in this case, the tx revert if they put the whole collateral as seized, so having a way of repaying the whole debt without having to compute it is useful (and the computation needs to be done onchain because it is required to be synchronous with the chain). The more debt they repay, the more collateral they seize.

I also don't think this implementation is dangerous, the only logic it adds is of the order of the one we added in the other entry points functions (by adding share input), but it really doesn't change anything in the liquidiation mechanism.

@QGarchery
Copy link
Contributor

But if I remember well, we assumed that liquidators always wanted to liquidate as mush as possible

I don't think we should assume that. The optimal amount to liquidate can very well not be the full amount. Here is the theory, and here is the data backing that claim

Yes but in this case, the tx revert if they put the whole collateral as seized, so having a way of repaying the whole debt without having to compute it is useful (and the computation needs to be done onchain because it is required to be synchronous with the chain). The more debt they repay, the more collateral they seize.

I'm honestly not sure that it's useful, I would assume that they will compute the seized amount anyway

I also don't think this implementation is dangerous, the only logic it adds is of the order of the one we added in the other entry points functions (by adding share input), but it really doesn't change anything in the liquidiation mechanism.

I agree that it's not dangerous in terms of the implementation, but I'm more worried about making the bad debt realization less of a clear choice

src/Morpho.sol Outdated Show resolved Hide resolved
src/Morpho.sol Outdated Show resolved Hide resolved
src/Morpho.sol Outdated Show resolved Hide resolved
src/Morpho.sol Outdated Show resolved Hide resolved
MerlinEgalite
MerlinEgalite previously approved these changes Aug 20, 2023
Copy link
Contributor

@MerlinEgalite MerlinEgalite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except naming it's good to go

test/forge/integration/TestIntegrationLiquidate.t.sol Outdated Show resolved Hide resolved
test/morpho_tests.tree Outdated Show resolved Hide resolved
test/morpho_tests.tree Outdated Show resolved Hide resolved
test/morpho_tests.tree Outdated Show resolved Hide resolved
MathisGD
MathisGD previously approved these changes Aug 21, 2023
Copy link
Contributor

@MathisGD MathisGD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Letsgoo

src/interfaces/IMorpho.sol Outdated Show resolved Hide resolved
src/interfaces/IMorpho.sol Outdated Show resolved Hide resolved
MathisGD
MathisGD previously approved these changes Aug 22, 2023
Rubilmax
Rubilmax previously approved these changes Aug 22, 2023
src/interfaces/IMorpho.sol Show resolved Hide resolved
MerlinEgalite
MerlinEgalite previously approved these changes Aug 22, 2023
@MathisGD
Copy link
Contributor

Holding this one a few more hours, we are debating with @QGarchery

pakim249CAL
pakim249CAL previously approved these changes Aug 22, 2023
QGarchery
QGarchery previously approved these changes Aug 23, 2023
Copy link
Contributor

@QGarchery QGarchery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summarizing my POV on the debate we had on this issue: there are 3 points to it, specification, completeness, and security.

  1. the specification about the risk is not easy to define. Initially, the idea was to ensure that bad debt was always realized. We found that it was not possible, and instead the goal was to make the bad debt as easy to realize as possible, and to incentive it. To me this PR makes the path to bad debt realization less clear, but I agree that this is a vague definition, and that the concrete incentives are still there (lower gas cost).
  2. the completeness of the protocol is to ensure that no operation that would make sense are forbidden. This is why it is possible to withdraw amounts and to withdraw shares, and the same reasoning seems to apply to liquidate. I disagree with this argument, because the liquidate function is not a feature, it is solely there for security, so we shouldn't care about allowing more use cases as long as it is secure.
  3. there is another component to security, which comes from not being able to fully close the debt. The issue would arise when the computation does not allow to repay the full debt when liquidating. This can leave some small debt on that position. If the amount of collateral is low (which can happen because of a withdraw from the user notably), and if the price of the debt token goes up (even over a long period of time), then this creates some bad debt that cannot be easily cleaned. This is because the gas cost of liquidating would not be worth the incentive.

I did not have this last point in mind, and even though I disagree with the first two points, I think it's significantly more important to make the changes of this PR worth it

@Rubilmax
Copy link
Contributor

This is because the gas cost of liquidating would not be worth the incentive.

Liquidators would no longer be incentivized to liquidate in this case, but lenders would in order to realized the bad debt?

@QGarchery
Copy link
Contributor

Liquidators would no longer be incentivized to liquidate in this case, but lenders would in order to realized the bad debt?

It would depend on the value of the debt. We assumed that there are dust amounts of debt in this scenario, but also that the price has enough time to rise up

@MerlinEgalite MerlinEgalite merged commit 389cd0c into main Aug 23, 2023
4 checks passed
@MerlinEgalite MerlinEgalite deleted the feat/add-argument-liquidate branch August 23, 2023 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants