feat: Add Liquidation Price into `top` Calculation (SC-6715) #158

lucas-manuel · 2020-10-30T20:54:47Z

Adding this PR to start a discussion about adding the liquidation price ((art * rate / ink) * mat) into the top calculation.

src/clip.sol

kmbarry1 · 2020-11-02T19:21:01Z

src/dog.sol

@@ -176,6 +180,7 @@ contract Dog is LibNote {
            id = ClipperLike(milk.clip).kick({
                tab: tab,
                lot: dink,
+                lip: div(mul(art, rate), ink),


I think in principle we could save some gas here by doing:

due / dink

dink is required to be non-zero above so we don't need the safe division, and due / dink should have approximately the same value as art * rate / ink. I don't see any additional precision concerns there under normal risk parameter conditions, given the dust checks.

kmbarry1 · 2020-11-02T19:29:21Z

It looks like there's sufficient coverage for both code paths in the tests, although I think it would be nice to add two new tests that explicitly document the intended behaviors--one for a steeply declined or zero OSM price that uses the liquidation price, and one for a case that should use the OSM price instead (e.g. an underwater Vault).

lucas-manuel · 2020-11-03T13:52:50Z

Used due / dink for lip calc, added test for OSM liquidation price case.

src/dog.sol

src/clip.sol

gbalabasquer · 2020-11-03T21:07:28Z

src/dog.sol

@@ -176,6 +180,7 @@ contract Dog is LibNote {
            id = ClipperLike(milk.clip).kick({
                tab: tab,
                lot: dink,
+                lip: due / dink,


I feel something is wrong with this math and the logic. Having a better collateralized CDP means a lower starting price. So let's say the feeds report a lower price than the real, a well collateralized CDP will be much more punished than one that's really underwater.
I feel the starting price should somehow be related to the inverse of the due/dink.

There's no way for the auction contract to know, fundamentally, whether the feeds are wrong. The goal is to have some sort of fallback to a saner price if the feed price is super low. Even without this change, a very low oracle price punishes well-collateralized CDPs more (it's always worse to get liquidated if you have higher collateral/debt). Using liquidation price, corrected for mat, as a floor acts to make the worst-case less bad. Better to be liquidated at a low price than zero. I.e. this PR never makes the starting price any lower than it otherwise would, so it's still a strict improvement.

IMO it may be better to just invest in making the oracles more secure than trying to get extra clever here, although if you have any good ideas let's hear them :).

I agree with @kmbarry1 that in an oracle attack scenario this outcome is strictly better than not having lip. I'm okay with leaving as is, otherwise we might introduce some unnecessary complexity.

Couldn't it happen that a very underwater vault won't be auctioned because the initial price is too high due this function?
I guess in that case it could be reset after the first cycle, which at least for now is not using this liquidation price.
Which btw, shouldn't we consider using it in the reset function somehow as well?

Yes so this is something I wanted to bring up. I wasn't sure if we wanted to use it in the reset function, because then the auction's tops will never decrease during reset which would be bad in a scenario of a steep market decline. However if there was an oracle attack and no one bid on a CDP that was really expensive for example (highly collateralized), then on reset that price would go to zero.

Hmmm you mean highly underwater, right? As if the CDP is highly collateralized, the liquidation will start at a lower price.
But yes, to your point, not really sure what to do with reset, as the same logic than in the originally liquidation would apply here, however the full property of reset wouldn't work if we do this.
I think these are the reasons why I feel this functionality is a bit forced.

Yes sorry that's what I meant. And I do think that governance param solution could work for the reset, definitely open to other ideas though. I'll actually do a quick implementation of what I'm suggesting in the next commit.

It might be interesting to do:

RESET_PRICE = max(buf*OSM_PRICE, PCT*top)

where PCT is the new gov parameter encoding the largest allowed (percentage-wise) decrease in top.

Thats actually what I just did! In the last commit.

sales[id].top = max( rmul(sales[id].top, lop), rmul(rdiv(mul(uint256(val), BLN), spot.par()), buf) );

I made lop directly multiply instead of doing sub(RAY, lop) (E.g. 25% decrease => lop = ray(0.75 ether)). I did this because if lop isn't set for whatever reason it will just be zero, so it won't be considered in the equation. This is IMO better that it working out to be 1 if its not set, which will just set it back to the old top.

Yep, that's what I had in mind.

lucas-manuel · 2020-11-03T21:14:34Z

Made requested changes by @gbalabasquer.

I feel something is wrong with this math and the logic. Having a better collateralized CDP means a lower starting price. So let's say the feeds report a lower price than the real, a well collateralized CDP will be much more punished than one that's really underwater.
I feel the starting price should somehow be related to the inverse of the due/dink.

This is an interesting point...

kmbarry1 · 2020-11-05T14:57:18Z

How do we feel here?

Pros:

get strictly higher prices if oracles are attacked and set too low
more predictable/controlled price decreases

Cons:

might set the starting price too high if a market decline has really occurred (and the error is worse the more undercollateralized a CDP is)
higher gas costs on every liquidation and auction reset
extra code complexity
another governance parameter
price might have even more trouble catching up in a rapidly falling market

Did I miss anything?

IMO it comes down to how likely you think an oracle attack is--we know it's a very severe outcome, but also unlikely, so is it worth the downsides above?

lucas-manuel · 2020-11-09T14:03:49Z

How do we feel here?

Pros:

get strictly higher prices if oracles are attacked and set too low

more predictable/controlled price decreases

Cons:

might set the starting price too high if a market decline has really occurred (and the error is worse the more undercollateralized a CDP is)

higher gas costs on every liquidation and auction reset

extra code complexity

another governance parameter

price might have even more trouble catching up in a rapidly falling market

Did I miss anything?

IMO it comes down to how likely you think an oracle attack is--we know it's a very severe outcome, but also unlikely, so is it worth the downsides above?

This might be over complicating things, but we could have an instant access module that could modify a boolean in the case of an oracle attack to use the liquidation price in that scenario:

sales[id].top = 
    oracleSafe ? 
    rmul(rdiv(mul(uint256(val), BLN), spot.par()), buf) : 
    rmul(max(rmul(lip, mat), rdiv(mul(uint256(val), BLN), spot.par())), buf);

However this still requires us to calculate and pass in lip on every kick(). IMO it might be worth that extra gas to have a safeguard against the system getting attacked, but I do understand that it's very unlikely. @gbalabasquer @kmbarry1 what do you guys think? If we think the cons outweigh the pros here I'm okay with closing this PR.

gbalabasquer · 2020-11-09T17:03:36Z

This might be over complicating things, but we could have an instant access module that could modify a boolean in the case of an oracle attack to use the liquidation price in that scenario:

Hmmm but how would that IAM would be executed and under which rules? I think at that point yes, we would be over complicating this too much.

@gbalabasquer @kmbarry1 what do you guys think? If we think the cons outweigh the pros here I'm okay with closing this PR.

I would feel much better about it if we had a system that protects more to the cdps that are more collateralized than the exact opposite which it is doing now.
I know that a low initial price is better than 0, but I feel the cons list is too long for a functionality that IMO is a bit broken conceptually.
So in my personal opinion I'd vote for desisting on implementing it or would start thinking in a different angle to tackle it.

kmbarry1 · 2020-11-10T04:17:40Z

Definitely agree with Gonzalo that assuming an IAM on top is excessive, complexity-wise.

I'm a little more ambivalent on whether this is worth it. I guess it probably needs a more exhaustive analysis of how an oracle attack would be likely to play out (against one, several, or all collateral types) to figure out exactly how much help this would be. Which, given how close we are to freezing the code, means maybe we should put this aside for now and instead examine it as part of the audit process. If a clear case emerges for needing some mitigation, then we can revisit it.

Btw Gonzalo, I've tried to think about how to address your concern (the fact that it works worse for better-collateralized Vaults) and I think that could end up being very messy, without any trustworthy price oracle...

lucas-manuel · 2020-11-10T04:21:06Z

@kmbarry1 so should we close this PR or leave it open you think (if we are going to reexamine during the audit process, which I agree is a good idea)?

kmbarry1 · 2020-11-10T04:32:50Z

@kmbarry1 so should we close this PR or leave it open you think (if we are going to reexamine during the audit process, which I agree is a good idea)?

Hmmmm, maybe we'll close it for now so no one accidentally spends time reviewing when there are more important tasks? PRs can always be re-opened.

godsflaw · 2021-01-21T16:07:44Z

closing for now

feat: add liquidation price into top calc

9764dc8

lucas-manuel requested a review from a team October 30, 2020 20:55

lucas-manuel self-assigned this Oct 30, 2020

kmbarry1 reviewed Oct 31, 2020

View reviewed changes

src/clip.sol Show resolved Hide resolved

kmbarry1 reviewed Nov 1, 2020

View reviewed changes

src/clip.sol Outdated Show resolved Hide resolved

fix: take out division of lip by par

6d9d3db

kmbarry1 reviewed Nov 2, 2020

View reviewed changes

Lucas added 2 commits November 3, 2020 08:50

feat: add test to cover OSM liquidation price case

726b1a3

fix: remove debug events

ffd9542

lucas-manuel requested review from kmbarry1 and a team November 3, 2020 13:52

gbalabasquer reviewed Nov 3, 2020

View reviewed changes

src/dog.sol Outdated Show resolved Hide resolved

gbalabasquer reviewed Nov 3, 2020

View reviewed changes

src/clip.sol Show resolved Hide resolved

gbalabasquer reviewed Nov 3, 2020

View reviewed changes

fix: remove div, change max to cap

0366d81

feat: add lop gov param to redo functionality

9f1f65f

lucas-manuel added LIQ-2.0 Priority: Medium Status: On Hold Type: Enhancement and removed Type: Enhancement labels Nov 10, 2020

godsflaw closed this Jan 21, 2021

kmbarry1 mentioned this pull request Mar 17, 2021

Consider liquidation price on take #219

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add Liquidation Price into `top` Calculation (SC-6715) #158

feat: Add Liquidation Price into `top` Calculation (SC-6715) #158

lucas-manuel commented Oct 30, 2020 •

edited

Loading

kmbarry1 Nov 2, 2020

lucas-manuel Nov 3, 2020

kmbarry1 commented Nov 2, 2020

lucas-manuel commented Nov 3, 2020

gbalabasquer Nov 3, 2020

kmbarry1 Nov 4, 2020

lucas-manuel Nov 4, 2020 •

edited

Loading

gbalabasquer Nov 4, 2020

lucas-manuel Nov 4, 2020

gbalabasquer Nov 4, 2020

lucas-manuel Nov 4, 2020

kmbarry1 Nov 4, 2020

lucas-manuel Nov 4, 2020 •

edited

Loading

kmbarry1 Nov 5, 2020

lucas-manuel commented Nov 3, 2020

kmbarry1 commented Nov 5, 2020

lucas-manuel commented Nov 9, 2020 •

edited

Loading

gbalabasquer commented Nov 9, 2020

kmbarry1 commented Nov 10, 2020

lucas-manuel commented Nov 10, 2020

kmbarry1 commented Nov 10, 2020

godsflaw commented Jan 21, 2021

feat: Add Liquidation Price into top Calculation (SC-6715) #158

feat: Add Liquidation Price into top Calculation (SC-6715) #158

Conversation

lucas-manuel commented Oct 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmbarry1 commented Nov 2, 2020

lucas-manuel commented Nov 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucas-manuel Nov 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucas-manuel Nov 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucas-manuel commented Nov 3, 2020

kmbarry1 commented Nov 5, 2020

lucas-manuel commented Nov 9, 2020 • edited Loading

gbalabasquer commented Nov 9, 2020

kmbarry1 commented Nov 10, 2020

lucas-manuel commented Nov 10, 2020

kmbarry1 commented Nov 10, 2020

godsflaw commented Jan 21, 2021

feat: Add Liquidation Price into `top` Calculation (SC-6715) #158

feat: Add Liquidation Price into `top` Calculation (SC-6715) #158

lucas-manuel commented Oct 30, 2020 •

edited

Loading

lucas-manuel Nov 4, 2020 •

edited

Loading

lucas-manuel Nov 4, 2020 •

edited

Loading

lucas-manuel commented Nov 9, 2020 •

edited

Loading