delvtech / hyperdrive

An automated market maker for fixed and variable yield with on-demand terms.
Apache License 2.0
33 stars 4 forks source link

Negative Liquidity #525

Closed ryangoree closed 1 year ago

ryangoree commented 1 year ago

Crash Report

Description

While using the LP address (0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266) on the MockHyperdrive deployment, I open a large short , then a large long, and the result is negative liquidity.

Expected Behavior

I wouldn't be able to open positions that result in negative liquidity.

Actual Behavior

I am able to.

Steps to Reproduce

  1. Start with 100,000 shareReserves and a sharePrice around 1.03

    image
  2. Using the LPer, open a short of 110,000 bonds

    image
  3. Using the LPer, open a long of 300,000 bonds

    image

(shareReserves * sharePrice - longsOutstanding) < 0 = true ...

Possible Solution

Screenshots or Error Messages

Included in the steps

Logs or Stack Traces

Environment

Related Issues

Any Other Information

Working on fixes to UI to help it run in the compose app

jalextowle commented 1 year ago

I was able to reproduce this on v0.0.9. My next step is to write a unit test that can reproduce the issue. This breaks a core invariant, so I think it's critical or high severity. Once we understand the problem better, we can assess whether this is the correct severity.

jalextowle commented 1 year ago

I haven't been able to reproduce this with a unit test unfortunately, so I've reverted to adding log statements to v0.0.9 and building a new Dockerfile locally. I've produced the following logs with this approach:

_pricePerShare() 2.160899008325319744
sharePrice 3.737862552602893784
shareReserves 81157.242006211437458785
longsOutstanding 318403.289312306663979316
minimumShareReserves 1.000000000000000000
_pricePerShare() 2.160899008325319744

The bookmarking _pricePerShare() values are logged at the beginning and the end of the function. The sharePrice value is what is actually used to evaluate our invariant. This could definitely cause the issue that we're seeing. The question now is why exactly this happens. This may be an issue specific to MockHyperdriveTestnet, which could explain why replicating with v0.0.10 is challenging.

jalextowle commented 1 year ago

Circling back on this, we identified a problem in the base buffer invariant in both openLong and closeShort, which explains the issue that we're seeing here. This was fixed in: #532. Since this issue was reported on v0.0.9, I made a repro branch off of v0.0.9 to verify that applying that fix addresses this problem: https://github.com/delvtech/hyperdrive/tree/jalextowle/repro/competition-bug-1. I tested this locally by building the Dockerfile with and without the patch. Without the patch, the smoke test (exactly the same as Ryan's steps) reproduces the issue and the system has negative liquidity. With the patch, the smoke test fails on openLong with 0x18846de9. This error code maps to BaseBufferExceedsShareReserves which is the error that we'd expect to see when our solvency checks are violated.

With all of this in mind, what remains is for @ryangoree to verify that the issue was fixed in the repro branch. I'm building a docker image right now that will be able to be used to verify this with the compose app and the frontend.

jalextowle commented 1 year ago

Here is the docker tag that should be used for testing the repro: 36fe8ea5d6e8be852eb5a8f46ae48fc7edc9a903.

ryangoree commented 1 year ago

With all of this in mind, what remains is for @ryangoree to verify that the issue was fixed in the repro branch. I'm building a docker image right now that will be able to be used to verify this with the compose app and the frontend.

I was still able to reproduce with the 36fe8ea5d6e8be852eb5a8f46ae48fc7edc9a903 tag.

Starting pool info:

(index) Value
shareReserves '100000'
bondReserves '295205.4510130795248'
lpTotalSupply '99999'
sharePrice '1.001809043632673765'
longsOutstanding '0'
longAverageMaturityTime '0'
shortsOutstanding '0'
shortAverageMaturityTime '0'
shortBaseVolume '0'
withdrawalSharesReadyToWithdraw '0'
withdrawalSharesProceeds '0'

Step 1: Open the largest short I'm able to (the highest I can go before the call to openShort in view mode reverts.)

image

Post short pool info

(index) Value
shareReserves '530.954959798999723678'
bondReserves '406196.4510130795248'
lpTotalSupply '99999'
sharePrice '1.012668958731524118'
longsOutstanding '0'
longAverageMaturityTime '0'
shortsOutstanding '110991'
shortAverageMaturityTime '1721865600'
shortBaseVolume '99469.045040201000276322'
withdrawalSharesReadyToWithdraw '0'
withdrawalSharesProceeds '0'

Step 2: Open the largest long I'm able to (the highest I can go before the call to openLong in view mode reverts.)

image

Post long pool info

(index) Value
shareReserves '2582.914787021925392491'
bondReserves '403524.67120005567840337'
lpTotalSupply '99999'
sharePrice '1.013054901205587337'
longsOutstanding '2664.373160528077277119'
longAverageMaturityTime '1721865600'
shortsOutstanding '110991'
shortAverageMaturityTime '1721865600'
shortBaseVolume '99469.045040201000276322'
withdrawalSharesReadyToWithdraw '0'
withdrawalSharesProceeds '0'

shareReserves * sharePrice - longsOutstanding = -48.665405615251366

ryangoree commented 1 year ago

I've confirmed I'm using the right tag in the container's files:

image

image

jalextowle commented 1 year ago

Hmm, it seems like that reduced the size of the issue but that there is still a problem. I’ll take another look.

jalextowle commented 1 year ago

I was able to repro the issue by playing around at the boundaries following Ryan's report. Since the share price has changed in the past few hours, the values I used for the short and long were different (110_755e18 and 2_123e18). Here are the logs from running the smoke test:

  Starting pool info:
  shareReserves: 100000.000000000000000000
  bondReserves: 295205.451013079524800000
  sharePrice: 1.000117643328259765
  longsOutstanding: 0.000000000000000000
  shortsOutstanding: 0.000000000000000000
  idle 100010.764215182648240235

  Opening short
  _deposit: balance: 100000.000000000000000000
  _deposit: totalShares: 100000.000000000000000000
  _deposit: balance: 100011.764332825976500000
  _deposit: totalShares: 100000.000000000000000000
  _deposit: sharePrice: 1.113251122452523296
  Opening long
  _deposit: balance: 111325.112245252329602183
  _deposit: totalShares: 110162.440157709189523380
  _deposit: balance: 111325.112245252329602183
  _deposit: totalShares: 110162.440157709189523380
  _deposit: sharePrice: 1.029825701780383158
  openLong: sharePrice 1.029825701780383158
  _pricePerShare=1.010908171937456794
  _marketState.shareReserves=2613.232526460647109036
  longsOutstanding_=2662.312900567631311189
  _sharePrice=1.029825701780383158
  _minimumShareReserves=1.000000000000000000
  Ending pool info:
  shareReserves: 2613.232526460647109036
  bondReserves: 403290.793990659789278038
  sharePrice: 1.010908171937456794
  longsOutstanding: 2662.312900567631311189
  shortsOutstanding: 110755.000000000000000000
  idle -21.585692567734309279

The logs collected on the ethereum node are:

_deposit: balance: 111325.257613221925999094
_deposit: totalShares: 110162.486257361522239628
_deposit: balance: 111325.328215237155052059
_deposit: totalShares: 110162.486257361522239628
_deposit: sharePrice: 1.029827231297224457
openLong: sharePrice 1.029827231297224457
_pricePerShare=1.010909708709163050
_marketState.shareReserves=2613.286662504484334533
longsOutstanding_=2662.308993970799702656
_sharePrice=1.029827231297224457
_minimumShareReserves=1.000000000000000000
jalextowle commented 1 year ago

Here are some more logs:

_deposit: balance: 111307.504918191078349037
_deposit: totalShares: 110156.875329782483701117
_deposit: balance: 111307.575508947622070420
_deposit: totalShares: 110156.875329782483701117
_pricePerShare: balance=113430.575508947622070420
_pricePerShare: getAccruedInterest()=0.000000000000000000
_pricePerShare: _totalShares=110156.875329782483701117
_deposit: sharePrice: 1.029718527957192761
openLong: sharePrice 1.029718527957192761
_pricePerShare: balance=113430.575508947622070420
_pricePerShare: getAccruedInterest()=0.000000000000000000
_pricePerShare: _totalShares=112218.603794756027328714
_pricePerShare=1.010800096180204214
_marketState.shareReserves=2606.543344145900627339
longsOutstanding_=2662.790889894605847882
_sharePrice=1.029718527957192761
_minimumShareReserves=1.000000000000000000
jalextowle commented 1 year ago

Ok, I found an issue related to this. From the above logs, we can see that the balance used in the first _pricePerShare call of 113430.575508947622070420 is notably larger than the balance after accruing interesting of 111307.575508947622070420. Despite the balance change, the totalShares haven't changed. This results in a temporary fluctuation in the _pricePerShare, because the share price used during execution is the updated assets divided by the old total shares. Before and after the transaction, the share price is the same. Within the transaction, there is a discrepancy caused by the ordering of accounting updates: https://github.com/delvtech/hyperdrive/blob/v0.0.9/contracts/test/MockHyperdriveTestnet.sol#L85.

I'll fix the issue in MockHyperdriveTestnet and push a commit to the repro branch, and then we can do another round of testing. I'll start by attempting to break it by manually finding the max short and long. Another TODO is to verify that this error is not present in any of the contracts on v0.0.10. It doesn't seem that the problem occurs in ERC4626Hyperdrive: https://github.com/delvtech/hyperdrive/blob/v0.0.10/contracts/src/instances/ERC4626Hyperdrive.sol#L91. In both branches of the if-statement, the share price is calculated by the underlying vault. In the first branch, we use the pool's deposit function which updates both the pool's assets and the pool's total shares. In the second, we're transferring shares directly to the pool, which doesn't change the underlying vault's share price.

jalextowle commented 1 year ago

I've been doing a bisection on the latest commit of repro, and I haven't been able to reproduce the issue. It either reverts or ends with a positive amount of idle. The smallest idle I've gotten so far is 0.072931700969063997, but I will do several more rounds.

jalextowle commented 1 year ago

The smallest so far is: idle = 0.000000000103154207. All of the results have either had positive idle (with increasing tiny idles) or the correct error code (0x18846de9).

ryangoree commented 1 year ago

I was unable to reproduce with the patches 🎉

jalextowle commented 1 year ago

This isn't a problem on the latest branch. We can re-open if there is a reversion.