Skip to content

Fix Fees not being charged on Close {Short, Long}#538

Merged
ControlCplusControlV merged 45 commits intomainfrom
controlc/high_vuln_fixes
Aug 11, 2023
Merged

Fix Fees not being charged on Close {Short, Long}#538
ControlCplusControlV merged 45 commits intomainfrom
controlc/high_vuln_fixes

Conversation

@ControlCplusControlV
Copy link
Copy Markdown
Contributor

Added a fix which takes fees from the shareProceeds when closing either a long or a short, along with accompanying shareReserves tests to ensure they are updating properly

This should address #422 and the issue described there, but I am unsure on my fix so please let me know if there are any issues in my solution/testing

Comment thread contracts/src/Hyperdrive.sol Outdated
Comment thread contracts/src/Hyperdrive.sol Outdated
Comment thread contracts/src/Hyperdrive.sol Outdated
Comment thread contracts/src/Hyperdrive.sol Outdated
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jul 28, 2023

Hyperdrive Gas Benchmark

Benchmark suite Current: 340cf0d Previous: 4827596 Deviation Status
addLiquidity: min 785 gas 785 gas 0% 🟰
addLiquidity: avg 43314 gas 43692 gas -0.8651%
addLiquidity: max 71032 gas 80573 gas -11.8414%
checkpoint: min 514 gas 514 gas 0% 🟰
checkpoint: avg 23181 gas 22776 gas 1.7782% 🚨
checkpoint: max 34040 gas 32767 gas 3.8850% 🚨
closeLong: min 852 gas 852 gas 0% 🟰
closeLong: avg 45986 gas 45526 gas 1.0104% 🚨
closeLong: max 85951 gas 85364 gas 0.6876% 🚨
closeShort: min 809 gas 809 gas 0% 🟰
closeShort: avg 41718 gas 40697 gas 2.5088% 🚨
closeShort: max 87386 gas 86800 gas 0.6751% 🚨
initialize: min 714 gas 714 gas 0% 🟰
initialize: avg 159446 gas 159389 gas 0.0358% 🚨
initialize: max 233486 gas 233486 gas 0% 🟰
openLong: min 740 gas 740 gas 0% 🟰
openLong: avg 112024 gas 113199 gas -1.0380%
openLong: max 179317 gas 179317 gas 0% 🟰
openShort: min 782 gas 782 gas 0% 🟰
openShort: avg 152859 gas 150245 gas 1.7398% 🚨
openShort: max 219062 gas 218825 gas 0.1083% 🚨
removeLiquidity: min 762 gas 762 gas 0% 🟰
removeLiquidity: avg 57174 gas 57544 gas -0.6430%
removeLiquidity: max 119610 gas 118973 gas 0.5354% 🚨

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

Comment thread contracts/src/Hyperdrive.sol
Comment thread contracts/src/Hyperdrive.sol Outdated
Comment thread test/integrations/hyperdrive/NegativeInterestLongFeeTest.t.sol Outdated
@coveralls
Copy link
Copy Markdown
Collaborator

coveralls commented Jul 28, 2023

Coverage Status

coverage: 98.278% (+0.02%) from 98.254% when pulling 340cf0d on controlc/high_vuln_fixes into 1ba3542 on main.

Comment thread test/units/hyperdrive/CloseLongTest.t.sol Outdated
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.

couple of questions/nits

Comment thread test/units/hyperdrive/CloseShortTest.t.sol Outdated
Comment thread test/units/hyperdrive/CloseShortTest.t.sol Outdated
Comment thread contracts/src/Hyperdrive.sol
Co-authored-by: Jonny Rhea <5555162+jrhea@users.noreply.github.com>
Comment thread test/units/hyperdrive/CloseLongTest.t.sol Outdated
Comment thread test/units/hyperdrive/CloseLongTest.t.sol Outdated
Comment thread test/units/hyperdrive/CloseLongTest.t.sol Outdated
Comment thread test/integrations/hyperdrive/NegativeInterestShortFeeTest.t.sol
Comment thread test/integrations/hyperdrive/ReentrancyTest.t.sol Outdated
Comment thread test/integrations/hyperdrive/ReentrancyTest.t.sol
Copy link
Copy Markdown
Contributor

@jalextowle jalextowle left a comment

Choose a reason for hiding this comment

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

The logic is all there. We need to do a bit more polishing and I'd like to see full logical coverage of the changes made to calculateShortProceeds in the HyperdriveMath unit tests. Once that is done, I'll approve this PR.

@jalextowle jalextowle self-requested a review August 8, 2023 14:56
Comment thread contracts/src/HyperdriveShort.sol Outdated
Copy link
Copy Markdown
Contributor

@jalextowle jalextowle left a comment

Choose a reason for hiding this comment

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

LGTM once all of the comments are resolved

@ControlCplusControlV ControlCplusControlV enabled auto-merge (squash) August 10, 2023 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants