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

Audit of the calculateMaxLong flow #774

Closed dpaiton closed 8 months ago

dpaiton commented 8 months ago

I did a deep dive into the math and algorithms underlying calculateMaxLong, which computes an input amount for the maximum possible long trade in a given pool. The calculateMaxLong function is auxiliary to the core Hyperdrive smart contracts, but is heavily used by the front-end, simulation, and bot frameworks. Additionally, calculating the maximum and targeted long requires an understanding of the openLong accounting, which at its core is a fundamental component of the Hyperdrive system.

The full writeup is available via this overleaf document, and a summary follows in the form of a pseudo "audit report".

I ranked the issues into "low", "medium", and "high" based on my perceived impact. The issues all stem from my own confusions or misunderstandings, and hopefully can be resolved via explanations and not actual code change requirements. I identified 2 low level issues, 3 medium level issues, and 2 high level issues. You can refer to the full writeup for more detailed descriptions that are in-context and with links.


Low

  1. overloading “delta” variable

    • We use ∆x, ∆y, ∆z to mean “amount a user supplied” as well as “amount the pool reserves have changed” in various points throughout the docs. We should be more explicit about what we mean with different symbols to avoid confusion when reading the equations.
  2. unclear how to derive initial long guess

    • We were unable to show all of the steps to derive ∆x from s(∆x) (eq 29).

Medium

  1. “yieldspace price” vs “hyperdrive price”

    • Our “price” variable is the inverse of the yieldspace price. This is generally confusing without explicit mention in the docstrings. More importantly, however, we identified in max.rs, l154 an error: the exponent should be negative time stretch to invert the price formula.
  2. curve fee doc equation error

    • The equation for curve fee in fees.rs incorrectly uses the variable dz. This equation can be removed in favor of the one in the docstring above the function.
  3. error in share reserves after opening a long

    • The max.rs docs have an erroneous - zmin in the equation for the share reserves after opening a long.

High

  1. fees might cause curve drift

    • Our AMM curve function (equation 1) does not include fees. Because of this (which can also be thought of as a consequence of bonds being a “virtual” reserve), when we open a long & exchange shares for bonds on the curve, the extraction of fees changes the constant k. This might cause the 'curve to drift' over a long period since we don't also adjust the other side of the reserves after the fee is taken out. How are we recalculating reserve levels to guarantee that k is held constant after fees are taken out? Or does that not matter?
  2. extra checkpoint exposure term in initial max long guess

    • the max.rs code has an extra checkpoint exposure term in the numerator that we could not account for

Since this issue contains a multitude of sub-issues, I will set tasks here that are resolved with an explanation here or a new issue.

jalextowle commented 8 months ago

High 1: There is a proposal from the Spearbit audit to re-work the fees, which I may address next week, but the drift we'd be most worried about is a drift in price. If you look at other AMM protocols (Uni V2 is the simplest to look into for this), you'll see that they also experience drifts in k.

High 2: I don't think it's an extra term. It could definitely be documented better, but what it is doing is basically accounting for the fact that the checkpoint may have negative margin that can eaten through because of netting. For example, let's say the checkpoint exposure is -100 and the exposure is 1000. If we open 100 longs, the checkpoint exposure will be 0 and the exposure will still be 1000 since exposure is calculated as:

$$ e = \sum_c{\max(e_c, 0)} $$

On first glance, the code looks weird because it's negating $\min(e_c, 0)$, but it does this because exposure is subtracted in $s_0$, so it actually has an additive effect.

jalextowle commented 8 months ago

Low 2: There was definitely some mad science going on in this estimate. The exponent and scaling factor seemed like they would work, and after testing, this hypothesis was shown to work most of the time. After working on the LP optimization problems, I'm skeptical that picking a guess in this way is really needed. It feels like it's doing what Newton's method should be able to do better. That said, it dramatically improved the convergence in testing, so something weird is going on here.

jalextowle commented 8 months ago

Low 1: You're welcome to take a crack at this. It's not always easy to see where the documentation you wrote falls short.

dpaiton commented 8 months ago

https://github.com/delvtech/hyperdrive/pull/830 addresses comment issues low 2; medium 1; medium 2; high 2.

Alex's response resolves high 1 in the sense that we know it's a problem and do not see a need to address it at this time.

I have resolved discrepancies in the doc, aside from a lingering one with the base update equation. I will leave that unresolved unless it is marked as a priority by the protocol team.

dpaiton commented 8 months ago

Closing this issue until unresolved items are deemed worth addressing.