Skip to content

Feat: liquidation cap and grace period timelock in recovery mode#6

Open
jasm1n33 wants to merge 7 commits intomainfrom
feat/liquidation-cap-and-timelock
Open

Feat: liquidation cap and grace period timelock in recovery mode#6
jasm1n33 wants to merge 7 commits intomainfrom
feat/liquidation-cap-and-timelock

Conversation

@jasm1n33
Copy link
Contributor

  • Liquidation collateral amount would cap with 1.1 * debt in recovery mode.
  • After getting into the recovery mode, a grace period is applied.
  • Any position with MCR < ICR < CCR cannot be liquidated in grace period.

@jasm1n33 jasm1n33 requested a review from RayHuang880301 March 14, 2025 05:30
Copy link
Collaborator

@StillFantastic StillFantastic left a comment

Choose a reason for hiding this comment

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

Also need to update the grace period timestamp if necessary after the following operations:

  • Liquidation
  • Redemption
  • Closing trove

Comment on lines +89 to +91
singleLiquidation = _tryLiquidateWithCap(
troveManager, account, debtInStabPool, troveManagerValues.MCR, troveManagerValues.price
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. If the intention is to cap the liquidation reward to 10%, then I think 110% should be used here instead of MCR.
  2. _tryLiquidateWithCap only liquidates a trove when SP can cover the debt (no redistribution). Consider if it is the expected behavior.


uint256 TCR = SatoshiMath._computeCR(entireSystemColl, entireSystemDebt);
if (TCR >= Config.CCR || ICR >= TCR) break;
_checkRecoveryModeGracePeriod();
Copy link
Collaborator

Choose a reason for hiding this comment

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

_checkRecoveryModeGracePeriod should not throw an error here.

debtInStabPool -= singleLiquidation.debtToOffset;
} else if (ICR < troveManagerValues.MCR) {
singleLiquidation = _tryLiquidateWithCap(
troveManager, account, debtInStabPool, troveManagerValues.MCR, troveManagerValues.price
Copy link
Collaborator

Choose a reason for hiding this comment

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

MCR -> 110%

uint256 TCR = SatoshiMath._computeCR(entireSystemColl, entireSystemDebt);
if (TCR >= Config.CCR || ICR >= TCR) continue;
// check the recovery mode grace period
_checkRecoveryModeGracePeriod();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should not throw.

Comment on lines +115 to +116
s.lastGracePeriodStartTimestamp = Config.UNSET_TIMESTAMP;
s.recoveryModeGracePeriodDuration = Config.MINIMUM_GRACE_PERIOD;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Besides this, I suggest adding another initialization function dedicated to these two variables. Otherwise, the state could be problematic after upgrading, e.g., unable to start a grace period countdown if already in recovery mode.

I'm thinking about adding an initv2 with a reinitializer modifier. Also have to make sure to to call initv2 inside the original init as well so that it becomes compatible for new chain deployment.

_checkRecoveryModeGracePeriod();

singleLiquidation = _tryLiquidateWithCap(
_troveManager, account, debtInStabPool, troveManagerValues.MCR, troveManagerValues.price
Copy link
Collaborator

Choose a reason for hiding this comment

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

MCR -> 110%

_liquidateNormalMode(s, troveManager, account, debtInStabPool, troveManagerValues.sunsetting);
} else if (ICR < troveManagerValues.MCR) {
singleLiquidation = _tryLiquidateWithCap(
troveManager, account, debtInStabPool, troveManagerValues.MCR, troveManagerValues.price
Copy link
Collaborator

Choose a reason for hiding this comment

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

MCR -> 110%

// check the recovery mode grace period
_checkRecoveryModeGracePeriod();
singleLiquidation = _tryLiquidateWithCap(
troveManager, account, debtInStabPool, troveManagerValues.MCR, troveManagerValues.price
Copy link
Collaborator

Choose a reason for hiding this comment

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

MCR -> 110%

Copy link
Collaborator

@StillFantastic StillFantastic left a comment

Choose a reason for hiding this comment

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

  • LiquidationFacet.sol Line 104, 127, 222, 252
    • Should include collateral surplus
    • totalCollGasCompensation should be multiplied by price

I found two additional problems during the review. Currently, when a Trove gets liquidated, the order of some operations is as follows:

  1. Update surplusBalances for borrower
  2. Send collateral to SP and decrease totalActiveCollateral
  3. In finalizeLiquidation, totalActiveCollateral would be reduced for the amount in (1.)
  4. Send collateral compensation to liquidator

First, in step 2, _exitCollFromStrategy could be triggered when the value of totalActiveCollateral hasn't been fully updated yet. Second, in step 3, _exitColl should be called (similar to _redeemCloseTrove) to ensure there is enough collateral in the contract for users to claim.

These 2 problems should have no effect as long as the retain percentage is 0%. We can consider to fix in another PR.

{
_requireCallerIsSatoshiXapp();

IBorrowerOperationsFacet(satoshiXApp).syncGracePeriod();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be called at the end of the function, because here the total system collateral has not been updated yet.

);
}

syncGracePeriod();
Copy link
Collaborator

Choose a reason for hiding this comment

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

syncGracePeriod will be called in the finalizeLiquidation


totals.collateralToSendToRedeemer = totals.totalCollateralDrawn - totals.collateralFee;

IBorrowerOperationsFacet(satoshiXApp).syncGracePeriod();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be call at the end of redeemCollateral

Copy link
Collaborator

@StillFantastic StillFantastic left a comment

Choose a reason for hiding this comment

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

  • TroveManager.sol also needs to be upgraded.
  • Does DeploySetup.s.sol need to be updated as well?

function run() public {
vm.startBroadcast(OWNER_PRIVATE_KEY);

_upgradeBOFaucet();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Facet

Comment on lines +34 to +36
_upgradeBOFaucet();
_upgradeLiquidationFacet();
_upgradeInitializer();
Copy link
Collaborator

Choose a reason for hiding this comment

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

We better bundle these 3 txs into 1 to avoid the case of other txs from users getting in the middle.


// initV2
Initializer initializer = Initializer(SATOSHI_X_APP_ADDRESS);
initializer.initV2();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Already called in _upgradeInitializer?

});

ISatoshiXApp XAPP = ISatoshiXApp(SATOSHI_X_APP_ADDRESS);
XAPP.diamondCut(facetCuts, address(0), data);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does not work for initialize data

selectors[18] = IBorrowerOperationsFacet.forceResetTM.selector;

bytes4[] memory newSelectors = new bytes4[](1);
newSelectors[0] = IBorrowerOperationsFacet.syncGracePeriod.selector;
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is also a syncGracePeriod in the LiquidationFacet, it causes selector collision.

Copy link
Collaborator

@StillFantastic StillFantastic left a comment

Choose a reason for hiding this comment

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

I think it's good to upgrade now.
For the new deployment, DeploySetup.s.sol has not been updated yet.

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.

3 participants