feat(EMP contracts): Subsequent liquidations reset withdrawal timer #1859

chrismaree · 2020-08-17T16:25:58Z

Motivation

Summary

Very large positions are difficult to liquidate in the current EMP contracts as liquidators are required to pay off all sponsor debt when liquidating. This problem is particularly acute in the request withdrawal case.

This PR implements a change to the EMP contracts that re-sets the requested withdrawal liveness timer on every subsequent liquidation against a position.

Details

Each time a position has a liquidation submitted against the contracts check if there is a pending withdrawal request. If there is one, and it is not expired, then the sponsor's withdrawal request timestamp is re-set to the current time + the liquidation liveness.

Discussion

I don't check pending transfer requests. I think this is fine. Any thoughts on this point?

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

nicholaspai · 2020-08-17T16:37:03Z

core/contracts/financial-templates/expiring-multiparty/Liquidatable.sol

+        if (
+            positionToLiquidate.withdrawalRequestPassTimestamp > 0 && // this is a subsequent liquidation.
+            positionToLiquidate.withdrawalRequestPassTimestamp < getCurrentTime() && // liveness has not passed yet.
+            maxTokensToLiquidate.isGreaterThan(minSponsorTokens) // above the minimum threshold.


nit: I think we treat the threshold as a >= threshold rather than a > threshold (look at create() so I think this should be isGreaterThanOrEqualTo. But it doesn't really matter if it really makes the testing more difficult

I agree that we should be consistent here.

I have implemented this change. PTAL.

nicholaspai · 2020-08-17T16:37:54Z

core/contracts/financial-templates/expiring-multiparty/Liquidatable.sol

+        // then re-set the liveness. This enables a liquidation against a withdraw request to be "dragged out" if the position is very large.
+        if (
+            positionToLiquidate.withdrawalRequestPassTimestamp > 0 && // this is a subsequent liquidation.
+            positionToLiquidate.withdrawalRequestPassTimestamp < getCurrentTime() && // liveness has not passed yet.


This should be <= instead of < based on how withdraw() handles comparising the request timestamp to getCurrentTime()

Hmmmm, why would we only do this positions that have not yet passed the liveness? Do we see this being meaningfully helpful to UX? I don't think it necessarily matters that much -- it just seems like it makes the casework more complex and harder to explain, so I think we'd need to have a good reason to add this complexity over just doing it in all withdrawal-based liquidations.

My main logic with this was if you had passed the liquidation liveness you should not be able to re-set this again. If this restriction was not added then a passed withdrawal request could have it's liveness re-set. This might actually be desired, in the case that the sponsor is slow to withdraw.

This should be <= instead of < based on how withdraw() handles comparising the request timestamp to getCurrentTime()

Any response to this?

This should be <= instead of < based on how withdraw() handles comparising the request timestamp to getCurrentTime()

Any response to this?

Yup. To be strictly consistent I agree with you.

nicholaspai · 2020-08-17T16:39:32Z

core/test/financial-templates/Liquidatable.js

+      );
+
+      // Create a subsequent liquidation partial and check that it also advances the withdrawal request timer
+      await liquidationContract.setCurrentTime(liquidationTime.add(liquidationLiveness.divn(2)).toString());


Can you add in a couple tests here for:

liquidations below minSponsorTokens should not advance the timer

once the withdraw request timetsamp has passed liveness, a liquidation above minSponsorTokens should not advance the timer

sure. done 1. Have not added one as we might remove this logic. will add a test for it, depending on how @mrice32 's comment is resolved

nicholaspai

This is so much cleaner than the withdraw & GCR defense, realyl curious what @daywiss @kendricktan @mrice32 can think of in terms of adverse/unintended consequences

coveralls · 2020-08-17T16:49:59Z

Coverage increased (+0.02%) to 92.704% when pulling fecf0b3 on chrismaree/liquidation-liveness-reset into 8aae10f on master.

mrice32

Looks great! I agree with @nicholaspai that I think this could be a really clean solution to the whale issue. As discussed IRL, I think we probably want team feedback around this feature, especially from non-eng wrt how this might affect usability and ease of explanation for the withdraw request process.

mrice32 · 2020-08-17T17:09:28Z

core/contracts/financial-templates/expiring-multiparty/Liquidatable.sol

+        if (
+            positionToLiquidate.withdrawalRequestPassTimestamp > 0 && // this is a subsequent liquidation.
+            positionToLiquidate.withdrawalRequestPassTimestamp < getCurrentTime() && // liveness has not passed yet.
+            maxTokensToLiquidate.isGreaterThan(minSponsorTokens) // above the minimum threshold.


I agree that we should be consistent here.

mrice32 · 2020-08-17T17:09:40Z

core/contracts/financial-templates/expiring-multiparty/Liquidatable.sol

+        // then re-set the liveness. This enables a liquidation against a withdraw request to be "dragged out" if the position is very large.
+        if (
+            positionToLiquidate.withdrawalRequestPassTimestamp > 0 && // this is a subsequent liquidation.
+            positionToLiquidate.withdrawalRequestPassTimestamp < getCurrentTime() && // liveness has not passed yet.


Hmmmm, why would we only do this positions that have not yet passed the liveness? Do we see this being meaningfully helpful to UX? I don't think it necessarily matters that much -- it just seems like it makes the casework more complex and harder to explain, so I think we'd need to have a good reason to add this complexity over just doing it in all withdrawal-based liquidations.

daywiss · 2020-08-17T18:10:35Z

so if i am understanding the intention behind this change, its that i can initiate a liquidation at a small price, essentially buying myself some more time to get more cash and send another liquidation, and so on, until the entire position can be liquidated?

just as some general thoughts an not with the intention of changing implementation:

i feel like this feature should be capped at a max time limit or time scaled with respect to either the total position size or the new liquidation size. for instance larger positions have longer time limit, or smaller liquidations have shorter time extension. its probably ok to be unbounded because of costs but it does seem strange.
im worried about side effects outside of the griefing one we identified. For instance could it be advantageous for someone to delay a large withdraw in order to get their own through quicker or to eat the capital with some unknown attack? Or maybe to get a better rate for minting? You guys probably have a better idea than me so I will go with your assessment. What helps is the delay is already pretty long without the reset, so most likely any attack could also happen within 2xliveness could happen in the first liveness period.
agree with matt that we need eyes from outside eng to 👍 or 👎

i will defer to others in eng on the technical implementation of this

nicholaspai · 2020-08-17T20:19:48Z

im worried about side effects outside of the griefing one we identified. For instance could it be advantageous for someone to delay a large withdraw in order to get their own through quicker or to eat the capital with some unknown attack

This would certainly be a factor if the contract were undercollateralized (i.e. every token is backed by < 100% of its value in collateral), but the contract being > 100% collateralized is a core assumption we make. In this weird state, sponsors would grief each other in order to "race" to get their collateral out of the contract.

i feel like this feature should be capped at a max time limit or time scaled with respect to either the total position size or the new liquidation size. for instance larger positions have longer time limit, or smaller liquidations have shorter time extension. its probably ok to be unbounded because of costs but it does seem strange.

Is the argument for a max time limit to protect sponsors from being griefed? The counter against a max time limit is that it sets a limit on the amount of time you can delay a malicious whale sponsor from withdrawing their collateral. However, we'd need to make sure to prevent the possibility that whales would split their positions amongst many smaller positions to take advantage of shortened extension times.

Independent of that, scaling the time extensions by position size is really interesting and makes sense intuitively (larger positions should get longer time extensions). Finally, should we consider increasing the withdrawal liveness by the existing withdrawal liveness plus some time? This way, malicious whales get penalized more for invalid withdrawals. A simple implementation would set withdrawRequestPassTimestamp = (currentWithdrawRequestPassTimestamp - currentTime) + withdrawLiveness

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

mrice32

This is awesome! Very nice and minimal change :).

Just wanted to note some things we discussed IRL:

We decided that the liveness inequality makes sense. Timer should not reset if the liveness has already passed.
We decided that the min sponsor threshold is slightly inelegant, but makes sense.

I think this is good to go once we add a test for the post-liveness withdraw that we decided we would keep.

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

pyggie · 2020-08-30T16:36:45Z

Each time a position has a liquidation submitted against the contracts check if there is a pending withdrawal request. If there is one, and it is not expired, then the sponsor's withdrawal request timestamp is re-set to the current time + the liquidation liveness.

What happens if the newly extended withdrawal pass timestamp is after expiration? Could repeated liquidation nibbles extend it indefinitely?

implemented contract changes

e75dfde

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

chrismaree requested review from mrice32, daywiss and nicholaspai August 17, 2020 16:26

nicholaspai reviewed Aug 17, 2020

View reviewed changes

nicholaspai approved these changes Aug 17, 2020

View reviewed changes

mrice32 reviewed Aug 17, 2020

View reviewed changes

chrismaree added 3 commits August 18, 2020 11:50

Updated implementation

612585e

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

removed exclusive test

ec7f88e

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

updated inequality

6555a6e

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

chrismaree mentioned this pull request Aug 18, 2020

feat(EMP contracts): Bound withdraw by GCR #1850

Closed

mrice32 approved these changes Aug 18, 2020

View reviewed changes

mrice32 changed the title ~~feat(EMP contracts): Subsequent liquidations reset withdrawl timer~~ feat(EMP contracts): Subsequent liquidations reset withdrawal timer Aug 18, 2020

daywiss approved these changes Aug 19, 2020

View reviewed changes

chrismaree added 2 commits August 19, 2020 15:54

small clean in test comments

360ce7e

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

Merge branch 'master' into chrismaree/liquidation-liveness-reset

fecf0b3

chrismaree merged commit 9ac5269 into master Aug 20, 2020

chrismaree deleted the chrismaree/liquidation-liveness-reset branch August 20, 2020 11:43

nicholaspai mentioned this pull request Sep 9, 2020

(Short Audit Review by OpenZeppelin): Revised EMP contracts #1970

Closed

chrismaree mentioned this pull request Sep 16, 2020

EMP Feature: Reset position liveness timer for each liquidation submitted #1858

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(EMP contracts): Subsequent liquidations reset withdrawal timer #1859

feat(EMP contracts): Subsequent liquidations reset withdrawal timer #1859

chrismaree commented Aug 17, 2020 •

edited by nicholaspai

Loading

nicholaspai Aug 17, 2020

mrice32 Aug 17, 2020

chrismaree Aug 18, 2020

nicholaspai Aug 17, 2020

mrice32 Aug 17, 2020

chrismaree Aug 18, 2020

nicholaspai Aug 18, 2020

chrismaree Aug 18, 2020

nicholaspai Aug 17, 2020

chrismaree Aug 18, 2020

nicholaspai left a comment

coveralls commented Aug 17, 2020 •

edited

Loading

mrice32 left a comment

mrice32 Aug 17, 2020

mrice32 Aug 17, 2020

daywiss commented Aug 17, 2020

nicholaspai commented Aug 17, 2020 •

edited

Loading

mrice32 left a comment

pyggie commented Aug 30, 2020

feat(EMP contracts): Subsequent liquidations reset withdrawal timer #1859

feat(EMP contracts): Subsequent liquidations reset withdrawal timer #1859

Conversation

chrismaree commented Aug 17, 2020 • edited by nicholaspai 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

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

nicholaspai left a comment

Choose a reason for hiding this comment

coveralls commented Aug 17, 2020 • edited Loading

mrice32 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daywiss commented Aug 17, 2020

nicholaspai commented Aug 17, 2020 • edited Loading

mrice32 left a comment

Choose a reason for hiding this comment

pyggie commented Aug 30, 2020

chrismaree commented Aug 17, 2020 •

edited by nicholaspai

Loading

coveralls commented Aug 17, 2020 •

edited

Loading

nicholaspai commented Aug 17, 2020 •

edited

Loading