Skip to content

fix: Fixes the problems that result from empty share reserves#415

Merged
jalextowle merged 32 commits intomainfrom
jalextowle/bug/empty-share-reserves-2
Jul 11, 2023
Merged

fix: Fixes the problems that result from empty share reserves#415
jalextowle merged 32 commits intomainfrom
jalextowle/bug/empty-share-reserves-2

Conversation

@jalextowle
Copy link
Copy Markdown
Contributor

@jalextowle jalextowle commented Jul 5, 2023

Fixes: #385.

To add some context to the issue, this PR adds a minimum share reserves that should prevent a host of critical and high severity issues from being possible. During the development of this feature, we noticed another DOS vector that could result from the share reserves being reduced to a small non-zero value. As part of this PR, we found sane defaults for the major yield sources and added tests demonstrating that these default values are sufficient to prevent the numerical issues that have been found with _updateLiquidity.

This minimum share reserves change consists of two parts:

  • Making sure that the share reserves never falls below a minimum threshold.
  • Minting the zero address the minimum share reserves of LP shares during intialization.

With this in mind, we have minimumShareReserves shares set aside in the share reserves that sets a hard lower bound on what the value can be. Similarly, we have minimumShareReserves LP shares set aside to serve as a hard lower bound for the active LP total supply.

@coveralls
Copy link
Copy Markdown
Collaborator

coveralls commented Jul 5, 2023

Coverage Status

coverage: 96.372% (-0.9%) from 97.26% when pulling 190807c on jalextowle/bug/empty-share-reserves-2 into 3ccbe22 on main.

@jalextowle jalextowle force-pushed the jalextowle/bug/empty-share-reserves-2 branch from 2d03d20 to e8df59d Compare July 5, 2023 21:32
@jalextowle jalextowle force-pushed the jalextowle/bug/empty-share-reserves-2 branch from 28ed238 to d3b5c59 Compare July 6, 2023 01:40
@jalextowle jalextowle force-pushed the jalextowle/bug/empty-share-reserves-2 branch from 69ec048 to 1e7f0e6 Compare July 6, 2023 04:37
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jul 6, 2023

Hyperdrive Gas Benchmark

Benchmark suite Current: 190807c Previous: 271650c Deviation Status
addLiquidity: min 709 gas 709 gas 0% 🟰
addLiquidity: avg 45882 gas 45450 gas 0.9505% 🚨
addLiquidity: max 77728 gas 83400 gas -6.8010%
checkpoint: min 514 gas 514 gas 0% 🟰
checkpoint: avg 23019 gas 23022 gas -0.0130%
checkpoint: max 33407 gas 33418 gas -0.0329%
closeLong: min 659 gas 659 gas 0% 🟰
closeLong: avg 47495 gas 46621 gas 1.8747% 🚨
closeLong: max 83240 gas 81764 gas 1.8052% 🚨
closeShort: min 616 gas 616 gas 0% 🟰
closeShort: avg 42212 gas 41352 gas 2.0797% 🚨
closeShort: max 68589 gas 81627 gas -15.9727%
initialize: min 532 gas 529 gas 0.5671% 🚨
initialize: avg 160281 gas 139316 gas 15.0485% 🚨
initialize: max 161781 gas 140816 gas 14.8882% 🚨
openLong: min 661 gas 661 gas 0% 🟰
openLong: avg 116401 gas 116166 gas 0.2023% 🚨
openLong: max 175911 gas 175672 gas 0.1360% 🚨
openShort: min 709 gas 709 gas 0% 🟰
openShort: avg 157459 gas 159418 gas -1.2288%
openShort: max 218810 gas 218571 gas 0.1093% 🚨
removeLiquidity: min 569 gas 569 gas 0% 🟰
removeLiquidity: avg 62904 gas 59523 gas 5.6802% 🚨
removeLiquidity: max 116558 gas 116368 gas 0.1633% 🚨

This comment was automatically generated by workflow using github-action-benchmark.

Copy link
Copy Markdown
Contributor

@jrhea jrhea left a comment

Choose a reason for hiding this comment

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

Okay i made an initial pass, but i still haven't reviewed the tests

Comment thread contracts/src/HyperdriveLP.sol
Comment thread contracts/src/HyperdriveLP.sol Outdated
Comment thread contracts/src/HyperdriveLong.sol Outdated
Comment thread contracts/src/HyperdriveLP.sol
Comment thread contracts/src/HyperdriveShort.sol Outdated
Comment thread contracts/src/HyperdriveStorage.sol Outdated
Comment thread contracts/src/HyperdriveStorage.sol Outdated
Comment thread contracts/src/factory/HyperdriveFactory.sol Outdated
Comment thread contracts/src/instances/DsrHyperdrive.sol Outdated
Comment thread contracts/src/instances/StethHyperdrive.sol
Copy link
Copy Markdown
Contributor

@ControlCplusControlV ControlCplusControlV left a comment

Choose a reason for hiding this comment

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

Minor nits from me + Jonny's suggestions, can re-review once those are patched

Comment thread test/integrations/DsrHyperdrive.t.sol Outdated
Comment thread test/integrations/DsrHyperdrive.t.sol Outdated
Comment thread test/integrations/DsrHyperdrive.t.sol Outdated
Comment thread test/integrations/hyperdrive/LpWithdrawalTest.t.sol
Comment thread contracts/src/instances/DsrHyperdrive.sol Outdated
@ControlCplusControlV
Copy link
Copy Markdown
Contributor

Have you looked at fixing CI? Would prefer if CI failing was not the norm

Copy link
Copy Markdown
Contributor

@ControlCplusControlV ControlCplusControlV left a comment

Choose a reason for hiding this comment

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

Some minor nits, looks really good but I still have some outstanding questions and still a lot of commented out code

Comment thread contracts/src/HyperdriveLP.sol Outdated
Comment thread contracts/src/HyperdriveLP.sol Outdated
Comment thread contracts/src/HyperdriveLP.sol
Comment thread contracts/src/instances/DsrHyperdrive.sol Outdated
Comment thread contracts/src/interfaces/IHyperdriveWrite.sol Outdated
Comment thread test/integrations/hyperdrive/LpWithdrawalTest.t.sol Outdated
Comment thread test/integrations/hyperdrive/NonstandardDecimals.sol
Comment thread test/units/hyperdrive/ExtremeInputs.t.sol Outdated
Comment thread test/units/hyperdrive/ExtremeInputs.t.sol Outdated
Comment thread test/units/hyperdrive/ExtremeInputs.t.sol Outdated
@jalextowle
Copy link
Copy Markdown
Contributor Author

Have you looked at fixing CI? Would prefer if CI failing was not the norm

It's just a spelling issue. I fixed most of the spelling mistakes before I left, but I missed this one. My style is generally to review when the linter is broken or a unit tests or two is broken, but to make sure that everything is fixed prior to the PR being merged. I'm happy to do something different, but in this particular case, it would have slowed down our development cycle over a typo.

@jalextowle jalextowle force-pushed the jalextowle/bug/empty-share-reserves-2 branch 2 times, most recently from 2a8cab0 to 5da6a5e Compare July 11, 2023 05:01
@jalextowle jalextowle force-pushed the jalextowle/bug/empty-share-reserves-2 branch from 5da6a5e to 68c401e Compare July 11, 2023 05:03
@ControlCplusControlV
Copy link
Copy Markdown
Contributor

Hmm, looks like ERC4626HyperdriveDeployer has broken the codesize limit by 105.0

Copy link
Copy Markdown
Contributor

@ControlCplusControlV ControlCplusControlV left a comment

Choose a reason for hiding this comment

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

:shipit:

@jalextowle jalextowle merged commit 5007450 into main Jul 11, 2023
@jalextowle jalextowle deleted the jalextowle/bug/empty-share-reserves-2 branch July 11, 2023 19:41
Copy link
Copy Markdown
Contributor

@gtowle03 gtowle03 left a comment

Choose a reason for hiding this comment

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

Excellent work. I am very proud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: Issues with empty share reserves

5 participants