Skip to content

Conversation

@Rubilmax
Copy link
Collaborator

@Rubilmax Rubilmax commented Dec 5, 2023

@Rubilmax Rubilmax marked this pull request as ready for review December 5, 2023 08:56
@Rubilmax
Copy link
Collaborator Author

Rubilmax commented Dec 5, 2023

@adhusson wrote:

Agree with rounding down seized assets since the liquidate parameters are controlled by the liquidator.

The test testLiquidateSharesInputNoBadDebtRealized is failing because the liquidator should now expect fewer assets seized.

Worth noting that the POC on cantina has a setup with 300 borrowed, 400 collateral. It runs for 399 iterations and consumes 11M gas before the the borrower runs out of collateral. Keeping the LTV constant, both the iterations and the gas necessary to empty the borrower's collateral go up linearly with the borrow1. For instance with 40k collateral at the start it takes 39999 iterations and 1.13B gas to steal all the collateral. You can't steal more than 1.15/price of collateral at a time (edit: added correct stealable amount).

Footnotes

  1. using manual memory management to avoid quadratic memory costs

You can't steal more than 1.15/price of collateral at a time

Can you explain how you get to that number pls?

Seized assets are [ceil|floor](repaidSharesToAssets) * liquidationIncentiveFactor / collateralPrice so you steal at most MAX_LIQUIDATION_INCENTIVE_FACTOR/price.

Jean-Grimal
Jean-Grimal previously approved these changes Dec 5, 2023
@Rubilmax Rubilmax changed the base branch from main to post-cantina December 5, 2023 09:35
Copy link
Contributor

Choose a reason for hiding this comment

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

Given this new issue, I'm putting into question the importance to have the shares as input to the liquidate function. Removing this input is another (albeit radical) solution to this issue

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The only issue I have is that seizedAssets input converted into repaidShares can be greater than borrowShares thus overflow, so a liquidator who wants to liquidate everything would have to input either collateral or collateral - 1 (whereas today they have to either input seizedAssets = collateral or repaidShares = borrowShares)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Besides, it seems weird to remove the granularity of repaid shares to liquidate, because a liquidation is like a repay + withdraw collateral

Copy link
Contributor

@QGarchery QGarchery Dec 5, 2023

Choose a reason for hiding this comment

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

Besides, it seems weird to remove the granularity of repaid shares to liquidate, because a liquidation is like a repay + withdraw collateral

By the same logic it should also expose a liquidate repaidAssets entrypoint. Ideally we would have a use case in mind to enable another entrypoint

The only issue I have is that seizedAssets input converted into repaidShares can be greater than borrowShares thus overflow, so a liquidator who wants to liquidate everything would have to input either collateral or collateral - 1

I think it's even more general, and there are situations where the collateral seized would be less than the full collateral. Typically this is in LTV range [LLTV, 1/LIF]. The choice to have the repaid shares entrypoint was to facilitate full liquidations in those cases, and the choice was made against making bad debt realization more of a natural choice. I'm still not fully convinced by the argument in favor, and I also feel like now the code is less clear because of it

@Rubilmax Rubilmax force-pushed the fix/liquidation-rounding branch from 9fedc64 to 742939d Compare December 5, 2023 14:06
@Rubilmax
Copy link
Collaborator Author

Rubilmax commented Dec 7, 2023

Replaced by #638

@Rubilmax Rubilmax closed this Dec 7, 2023
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.

5 participants