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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 6 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9764dc8
feat: add liquidation price into top calc
Oct 30, 2020
6d9d3db
fix: take out division of lip by par
Nov 2, 2020
726b1a3
feat: add test to cover OSM liquidation price case
Nov 3, 2020
ffd9542
fix: remove debug events
Nov 3, 2020
0366d81
fix: remove div, change max to cap
Nov 3, 2020
9f1f65f
feat: add lop gov param to redo functionality
Nov 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 19 additions & 13 deletions src/clip.sol
View file
Open in desktop
Expand Up @@ -66,8 +66,9 @@ contract Clipper is LibNote {
AbacusLike public calc; // Current price calculator

uint256 public buf; // Multiplicative factor to increase starting price [ray]
uint256 public tail; // Time elapsed before auction reset [seconds]
uint256 public cusp; // Percentage drop before auction reset [ray]
uint256 public tail; // Required time elapsed before auction reset [seconds]
uint256 public cusp; // Required percentage drop before auction reset [ray]
uint256 public lop; // Percentage drop of previous top on auction reset [ray]

uint256 public kicks; // Total auctions
uint256[] public active; // Array of active auction ids
Expand Down Expand Up @@ -101,7 +102,7 @@ contract Clipper is LibNote {

event Take(
uint256 indexed id,
uint256 max,
uint256 cap,
uint256 price,
uint256 owe,
uint256 tab,
Expand Down Expand Up @@ -144,8 +145,9 @@ contract Clipper is LibNote {
// --- Administration ---
function file(bytes32 what, uint256 data) external note auth {
if (what == "buf") buf = data;
else if (what == "tail") tail = data; // Time elapsed before auction reset
else if (what == "cusp") cusp = data; // Percentage drop before auction reset
else if (what == "tail") tail = data; // Required time elapsed before auction reset
else if (what == "cusp") cusp = data; // Required percentage drop before auction reset
else if (what == "lop") lop = data; // Percentage drop of previous top on auction reset
else revert("Clipper/file-unrecognized-param");
}
function file(bytes32 what, address data) external note auth {
Expand All @@ -162,6 +164,9 @@ contract Clipper is LibNote {
function min(uint256 x, uint256 y) internal pure returns (uint256 z) {
return x <= y ? x : y;
}
function max(uint256 x, uint256 y) internal pure returns (uint256 z) {
gbalabasquer marked this conversation as resolved.
Show resolved Hide resolved
return x >= y ? x : y;
}
function sub(uint256 x, uint256 y) internal pure returns (uint256 z) {
require((z = x - y) <= x);
}
Expand All @@ -178,8 +183,9 @@ contract Clipper is LibNote {
// --- Auction ---

// start an auction
function kick(uint256 tab, // Debt [rad]
uint256 lot, // Collateral [wad]
function kick(uint256 tab, // Debt [rad]
uint256 lot, // Collateral [wad]
uint256 lip, // Liquidation price [ray]
address usr // Liquidated CDP
) external auth isStopped(1) returns (uint256 id) {
require(kicks < uint256(-1), "Clipper/overflow");
Expand All @@ -196,10 +202,10 @@ contract Clipper is LibNote {
// Could get this from rmul(Vat.ilks(ilk).spot, Spotter.mat()) instead,
// but if mat has changed since the last poke, the resulting value will
// be incorrect.
(PipLike pip, ) = spot.ilks(ilk);
(PipLike pip, uint256 mat) = spot.ilks(ilk);
kmbarry1 marked this conversation as resolved.
Show resolved Hide resolved
(bytes32 val, bool has) = pip.peek();
require(has, "Clipper/invalid-price");
sales[id].top = rmul(rdiv(mul(uint256(val), BLN), spot.par()), buf);
sales[id].top = rmul(max(rmul(lip, mat), rdiv(mul(uint256(val), BLN), spot.par())), buf);

emit Kick(id, sales[id].top, tab, lot, usr);
}
Expand All @@ -223,15 +229,15 @@ contract Clipper is LibNote {
(PipLike pip, ) = spot.ilks(ilk);
(bytes32 val, bool has) = pip.peek();
require(has, "Clipper/invalid-price");
sales[id].top = rmul(rdiv(mul(uint256(val), 10 ** 9), spot.par()), buf);
sales[id].top = max(rmul(sales[id].top, lop), rmul(rdiv(mul(uint256(val), BLN), spot.par()), buf));

emit Redo(id, sales[id].top, sales[id].tab, sales[id].lot, sales[id].usr);
}

// Buy amt of collateral from auction indexed by id
function take(uint256 id, // Auction id
uint256 amt, // Upper limit on amount of collateral to buy [wad]
uint256 max, // Maximum acceptable price (DAI / collateral) [ray]
uint256 cap, // Maximum acceptable price (DAI / collateral) [ray]
address who, // Receiver of collateral, payer of DAI, and external call address
bytes calldata data // Data to pass in external call; if length 0, no call is done
) external lock isStopped(2) {
Expand All @@ -246,7 +252,7 @@ contract Clipper is LibNote {
require(sub(now, sale.tic) <= tail && rdiv(price, sale.top) >= cusp, "Clipper/needs-reset");

// Ensure price is acceptable to buyer
require(max >= price, "Clipper/too-expensive");
require(cap >= price, "Clipper/too-expensive");

// Purchase as much as possible, up to amt
uint256 slice = min(sale.lot, amt); // slice <= lot
Expand Down Expand Up @@ -293,7 +299,7 @@ contract Clipper is LibNote {
sales[id].lot = sale.lot;
}

emit Take(id, max, price, owe, sale.tab, sale.lot, sale.usr);
emit Take(id, cap, price, owe, sale.tab, sale.lot, sale.usr);
}

function _remove(uint256 id) internal {
Expand Down
3 changes: 2 additions & 1 deletion src/dog.sol
View file
Open in desktop
Expand Up @@ -22,7 +22,7 @@ pragma solidity >=0.5.12;
import "./lib.sol";

interface ClipperLike {
function kick(uint256 tab, uint256 lot, address usr) external returns (uint256);
function kick(uint256 tab, uint256 lot, uint256 lip, address usr) external returns (uint256);
}

interface VatLike {
Expand Down Expand Up @@ -176,6 +176,7 @@ contract Dog is LibNote {
id = ClipperLike(milk.clip).kick({
tab: tab,
lot: dink,
lip: due / dink,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 :).

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, that's what I had in mind.

usr: urn
});
}
Expand Down