Conversation
Conflicts: cadence/contracts/FlowCreditMarket.cdc
run flow deps install FlowTransactionScheduler per https://flow-foundation.slack.com/archives/C08QF29F7TK/p1765918094418619?thread_ts=1765904362.928639&cid=C08QF29F7TK to fix imports
at this commit, failing on
assert(initialHealth >= 1.0, message: "Cannot liquidate healthy position: \(initialHealth)>1")
now we are failing on missing dex handle, as expected
| } | ||
|
|
||
| /// Computes health = totalEffectiveCollateral / totalEffectiveDebt (∞ when debt == 0) | ||
| // TODO: return BalanceSheet, this seems like a dupe of _getUpdatedBalanceSheet |
There was a problem hiding this comment.
There is a slight difference between BalanceSheet and healthFactor. Where one is going to be taking into consideration the current composition of the positions (Collateral & Debit) and the other would be also adding in Queued deposits. We would be using the calculation with queued deposits for when looking if a position is being checked for liquidation. So we are not penalizing users that have capital to top up their position but is currently in queue.
While the other of current composition would be used for every other aspect of the protocol - rebalancing / owning a position / etc.
There was a problem hiding this comment.
Created this issue to address this point: #117
| /// Time this pool most recently had liquidations paused | ||
| access(self) var lastUnpausedAt: UInt64? | ||
|
|
||
| /// TODO: unused! To remove, must re-deploy existing contracts |
There was a problem hiding this comment.
Same as above: Contract updates are not permitted to remove functions. Are we planning to redeploy? If not, maybe just deprecate it
| /// TODO: unused! To remove, must re-deploy existing contracts | |
| // Deprecated: Unused field, but cannot be removed anymore |
| } | ||
| position.balances[seizeType]!.recordWithdrawal(amount: UFix128(seizeAmount), tokenState: seizeState) | ||
| let seizeReserveRef = (&self.reserves[seizeType] as auth(FungibleToken.Withdraw) &{FungibleToken.Vault}?)! | ||
| let seizedCollateral <- seizeReserveRef.withdraw(amount: seizeAmount) |
There was a problem hiding this comment.
It seems that manualLiquidation lets the liquidator choose both seizeAmount and repayAmount (implicitly via repayment.balance) but does not enforce any pricing relationship between them, nor does it require repayAmount > 0.
Concretely:
repayAmount is just repayment.balance and the only bound is repayAmount <= Nd. A zero‑balance vault passes this.
_doLiquidation will still withdraw and return seizeAmount collateral even if the repayment vault is empty.
Impact: a permissionless caller can seize collateral from any unhealthy position while repaying 0.0
Suggested fix: enforce liquidation economics in the contract:
require repayAmount > 0 and seizeAmount > 0
We should enforce a deterministic relationship: either compute seizeAmount from repayAmount (prices + liquidation incentive + factors), or compute required repayAmount from seizeAmount and revert if not met
Tests: add a regression test that repayAmount == 0.0 fails
There was a problem hiding this comment.
The liquidation price is constrained by the DEX price. That logic is present but commented out here, and added and tested in the follow-up PR: #112.
This PR excludes logic related to DEX pricing (and associated tests) to keep PR size under control. The DEX portion of the implementation is commented out, and will be revised in #94
The critical price enforcement piece happens in this line
assert(seizeAmount < quote.inAmount, message: "Liquidation offer must be better than that offered by DEX")
| let Fd = positionView.snapshots[debtType]!.risk.borrowFactor | ||
|
|
||
| let Ce_seize = UFix128(seizeAmount) * UFix128(Pc_oracle) * Fc // effective value of seized collateral ($) | ||
| let De_seize = UFix128(repayAmount) * UFix128(Pd_oracle) * Fd // effective value of repaid debt ($) |
There was a problem hiding this comment.
The protocol’s own effective debt definition divides by borrow factor:
effectiveDebt = (debit * price) / borrowFactor (FlowCreditMarket.cdc (line 1028)–1030)
So the effective debt reduction from repaying should be:
De_seize = (repayAmount * Pd_oracle) / borrowFactor
Suggested fix: change * Fd to / Fd, and add a test with borrowFactor != 1.0 that asserts the post‑liquidation health math matches the effectiveDebt helper.
There was a problem hiding this comment.
Good catch. Fixed the calculation in 0213a18.
| let Ce_post = Ce_pre - Ce_seize // position's total effective collateral after liquidation ($) | ||
| let De_post = De_pre - De_seize // position's total effective debt after liquidation ($) | ||
| let postHealth = FlowCreditMarket.healthComputation(effectiveCollateral: Ce_post, effectiveDebt: De_post) | ||
| assert(postHealth <= self.liquidationTargetHF, message: "Liquidation must not exceed target health: \(postHealth)>\(self.liquidationTargetHF)") |
There was a problem hiding this comment.
This Target-HF rule is unusual and maybe optional. As currently written it mainly prevents “too much repayment for too little seizure” (i.e. a donation that makes the position too healthy), via postHealth <= liquidationTargetHF.
To prevent exploits, we need the opposite kind of enforcement: a rule that makes underpayment fail (e.g. require postHealth >= 1.0 and/or postHealth >= liquidationTargetHF, or compute a minRepayAmount that is mathematically tied to the chosen seizeAmount and revert unless repayAmount >= minRepayAmount).
In Aave v3, the liquidator calls liquidationCall(collateralAsset, debtAsset, user, debtToCover, receiveAToken)and chooses the debt asset + collateral asset + how much debt to repay (debtToCover). The protocol then transfers a proportional amount of collateral, including the liquidation bonus, and enforces a close factor (caps how much debt can be covered per call). (aave.com)
There was a problem hiding this comment.
i.e. a donation that makes the position too healthy
I'm not sure this is an accurate characterization. In general we assume a liquidation occurs when it is economically advantageous for the liquidator and economically disadvantageous for the position owner at the margin, because liquidators are self-interested actors who require incentives. (That isn't necessarily true -- we allow liquidations that benefit the position -- but I think we should expect it to be true in the general case.)
For example, suppose I have a position with 100X of collateral at a price of 1X/$, collateral factor 0.8, and I have withdrawn 80$ of debt (borrow factor 1.0).
C=100X
Pc=1$/X
Ce=(100)(1)(0.8)=80$
D=80$
De=(80)(1)=80$
HF=(80)/(80)=1
Now the price drops to 0.99$/X:
Ce=(100)(1)(0.8)=79.2$
HF=(79.2)/(80)=0.99 < 1.0
I am now eligible for liquidation. A liquidator makes a liquidation offer at a price of 0.98$/X.
- A liquidation at this price will marginally increase my position's health, which makes the protocol happy.
- However, this liquidation price is lower than current oracle price (0.99$/X), which makes me unhappy.
- As the position holder, I want as little of my collateral to be liquidated at this below-market price as possible (this is the purpose of the health factor threshold)
- If the protocol allowed any health-increasing liquidation, a liquidator could repay all my outstanding debt at terms that are unfavourable to me as the position holder.
There was a problem hiding this comment.
To prevent exploits, we need the opposite kind of enforcement: a rule that makes underpayment fail (e.g. require postHealth >= 1.0 and/or postHealth >= liquidationTargetHF
I discussed this with Dete a few weeks ago and we decided that we should accept liquidations that either:
- increase the health, but result in health < 1
- decrease the health
There is some context in this thread, here and here.
Can you elaborate on what kind of exploits you're thinking of that might arise from allowing those kinds of liquidations?
There was a problem hiding this comment.
I’m not opposed to allowing liquidations that (a) increase HF but still leave HF < 1, or (b) even decrease HF. My exploit concern was less about the direction of HF change and more about a price enforcement (repayAmount >= minRepayAmount). Sorry for the confusion.
joshuahannan
left a comment
There was a problem hiding this comment.
No comments on the business logic yet because I don't fully understand the protocol yet, but I left some comments about error messages and such
Also improve documentation for risk parameters, and refactor De/Ce functions so they can be optionally used without constructing a TokenSnapshot.
Co-authored-by: Tim Barry <21149133+tim-barry@users.noreply.github.com> Co-authored-by: Joshua Hannan <joshua.hannan@flowfoundation.org>
Conflicts: cadence/contracts/FlowCreditMarket.cdc cadence/tests/liquidation_phase1_test.cdc
Co-authored-by: Bastian Müller <bastian@turbolent.com>
Closes: #93
Description
Implements a manual liquidation path. Anyone is allowed to perform a liquidation under the following circumstances:
The purpose of liquidation is to remove debt which is at risk of becoming insolvent from FCM. Liquidation is the second line of defense after automated rebalancing. In the future, we intend to add automated liquidation (#97), where the protocol autonomously liquidates unhealthy positions against a DEX, using scheduled transactions.
Notes for reviewers
SwapperProviderfor UniswapV3 #94. [DRAFT] DEX interface for FCM #102 contains a preliminary interface for the DEX.