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

Implement UndercollateralizedPosition error #79

Closed
wants to merge 3 commits into from

Conversation

peyha
Copy link
Collaborator

@peyha peyha commented Oct 4, 2024

2. preLCF1 <= preLCF2;
3. WAD <= preLIF1 <= preLIF2.

Note that `preLCF1 <= WAD` and `preLCF1 <= WAD` is not mandatory.
Copy link

@adhusson adhusson Oct 4, 2024

Choose a reason for hiding this comment

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

Suggested change
Note that `preLCF1 <= WAD` and `preLCF1 <= WAD` is not mandatory.
Note that `preLCF1 <= WAD` and `preLCF2 <= WAD` is not mandatory.

The PreLiquidation smart-contract enforces:
1. preLltv < LLTV;
2. preLCF1 <= preLCF2;
3. WAD <= preLIF1 <= preLIF2.
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that this will create a silent merge conflict with #77

3. WAD <= preLIF1 <= preLIF2.

Note that `preLCF1 <= WAD` and `preLCF1 <= WAD` is not mandatory.
Indeed without this, the close factor can reach 100% when the position LTV is less than LLTV allowing additionnal pre-liquidation close factor configurations.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure that this sentence is really understandable as it is

Copy link
Contributor

@colin-morpho colin-morpho Oct 6, 2024

Choose a reason for hiding this comment

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

Agreed, what do you mean in this very context by "allowing additional pre-liquidation close factor configuration"? Do you want to say close factors that make no sense? that are not covered by this contract? that are not safe?

Comment on lines +299 to +302
oraclePrice = bound(oraclePrice, 1, ORACLE_PRICE_SCALE / 2);
OracleMock(preLiquidationParams.preLiquidationOracle).setPrice(oraclePrice);

uint256 collateralPrice = IOracle(preLiquidationParams.preLiquidationOracle).price();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
oraclePrice = bound(oraclePrice, 1, ORACLE_PRICE_SCALE / 2);
OracleMock(preLiquidationParams.preLiquidationOracle).setPrice(oraclePrice);
uint256 collateralPrice = IOracle(preLiquidationParams.preLiquidationOracle).price();
uint256 collateralPrice = bound(oraclePrice, 1, ORACLE_PRICE_SCALE / 2);
oracle.setPrice(collateralPrice);

@@ -139,6 +139,9 @@ contract PreLiquidation is IPreLiquidation, IMorphoRepayCallback {

uint256 collateralPrice = IOracle(PRE_LIQUIDATION_ORACLE).price();
uint256 collateralQuoted = uint256(position.collateral).mulDivDown(collateralPrice, ORACLE_PRICE_SCALE);

require(collateralQuoted > 0, ErrorsLib.UncollateralizedPosition());
Copy link
Contributor

Choose a reason for hiding this comment

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

Remark that this corresponds to ltv = +infinity and that #77 already prevents ltv > LLTV.
I think that it's possible to do the check of #77 in a way that also prevents the division by zero that this PR prevents: check that borrowed <= LLTV.wMulDown(collateralQuoted).
This is because borrowed is rounded up so it is >= 1

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I really like your solution, wdyt @colin-morpho does it help for FV ?

Copy link
Contributor

Choose a reason for hiding this comment

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

See de168dd. So now there shouldn't be any division by zero

Copy link
Contributor

Choose a reason for hiding this comment

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

I think Quentin's suggestions is the right way to go!

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree too, closing it then

Copy link
Contributor

@colin-morpho colin-morpho left a comment

Choose a reason for hiding this comment

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

Few comments to imporve the README, perhaps we should consider @QGarchery's porposition.


### Pre-liquidation parameters restrictions

The PreLiquidation smart-contract enforces:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The PreLiquidation smart-contract enforces:
The PreLiquidation smart-contract enforces the following properties.

2. preLCF1 <= preLCF2;
3. WAD <= preLIF1 <= preLIF2.

Note that `preLCF1 <= WAD` and `preLCF1 <= WAD` is not mandatory.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Note that `preLCF1 <= WAD` and `preLCF1 <= WAD` is not mandatory.
Note: it is not mandatory that `preLCF1 <= WAD` and `preLCF1 <= WAD` hold.

@MathisGD
Copy link
Contributor

MathisGD commented Oct 6, 2024

Replaced by #77

@MathisGD MathisGD closed this Oct 6, 2024
@MathisGD MathisGD reopened this Oct 6, 2024
@MathisGD
Copy link
Contributor

MathisGD commented Oct 6, 2024

Reopened for the changes on the readme, but we should remove the new require

@peyha
Copy link
Collaborator Author

peyha commented Oct 7, 2024

I'll just create another PR for the readme

@colin-morpho
Copy link
Contributor

Should we close this PR then?

@MathisGD
Copy link
Contributor

MathisGD commented Oct 7, 2024

replaced by #82

@MathisGD MathisGD closed this Oct 7, 2024
@MathisGD MathisGD deleted the feat/undercollateralization branch October 7, 2024 09:15
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