Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Locked down the sweep function #473

Merged
merged 7 commits into from
Jul 21, 2023

Conversation

jalextowle
Copy link
Contributor

@jalextowle jalextowle commented Jul 20, 2023

Fixes: #507.

Along with fixing the Spearbit issue, this PR addresses a medium Certora issue by locking down the targets of sweep in ERC4626Hyperdrive. Previously, the sweep function could be called on any target that is not equal to baseToken or pool. Some tokens are accessible through more than one address, which means that it was theoretically possible for all of the Hyperdrive funds to be swept into governance if such a token was used as the base or pool token. To solve this problem, this PR adds a mapping called isSweepable that keeps track of which addresses can be swept. This list can't be updated after initialization, so users can verify that the pool is safe from being swept.

@github-actions
Copy link

github-actions bot commented Jul 20, 2023

Hyperdrive Gas Benchmark

Benchmark suite Current: a3f5262 Previous: dc10e7a Deviation Status
addLiquidity: min 785 gas 785 gas 0% 🟰
addLiquidity: avg 43835 gas 43445 gas 0.8977% 🚨
addLiquidity: max 80528 gas 70800 gas 13.7401% 🚨
checkpoint: min 514 gas 514 gas 0% 🟰
checkpoint: avg 22776 gas 23044 gas -1.1630%
checkpoint: max 32767 gas 33438 gas -2.0067%
closeLong: min 852 gas 852 gas 0% 🟰
closeLong: avg 45539 gas 45984 gas -0.9677%
closeLong: max 85364 gas 86074 gas -0.8249%
closeShort: min 809 gas 809 gas 0% 🟰
closeShort: avg 40709 gas 40869 gas -0.3915%
closeShort: max 86896 gas 87306 gas -0.4696%
initialize: min 714 gas 714 gas 0% 🟰
initialize: avg 159368 gas 159493 gas -0.0784%
initialize: max 233486 gas 233625 gas -0.0595%
openLong: min 740 gas 740 gas 0% 🟰
openLong: avg 113317 gas 113674 gas -0.3141%
openLong: max 179472 gas 180018 gas -0.3033%
openShort: min 782 gas 782 gas 0% 🟰
openShort: avg 150017 gas 150644 gas -0.4162%
openShort: max 218899 gas 219306 gas -0.1856%
removeLiquidity: min 762 gas 762 gas 0% 🟰
removeLiquidity: avg 57230 gas 57462 gas -0.4037%
removeLiquidity: max 118973 gas 119378 gas -0.3393%

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

@coveralls
Copy link
Collaborator

coveralls commented Jul 20, 2023

Coverage Status

coverage: 98.243% (+0.01%) from 98.23% when pulling a3f5262 on jalextowle/audit/lock-down-sweep into 014108a on main.

@ControlCplusControlV
Copy link
Contributor

Codesize issues are becoming very annoying, can't wait to fix it

Copy link
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.

No issue with the changes themselves, good work, but could you add negative and positive scenario tests?

Copy link
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.

looks good just some comments/questions

contracts/src/factory/HyperdriveFactory.sol Show resolved Hide resolved
foundry.toml Outdated Show resolved Hide resolved
@jalextowle jalextowle enabled auto-merge (squash) July 21, 2023 15:17
@jalextowle jalextowle merged commit 71c6688 into main Jul 21, 2023
@jalextowle jalextowle deleted the jalextowle/audit/lock-down-sweep branch July 21, 2023 15:41
gtowle03 pushed a commit that referenced this pull request Jul 21, 2023
* Locked down the `sweep` function

* Added a check on `isSweepable` in `test_erc4626_sweep`

* Appeased the linter

* Turned optimizer down to avoid code size issues

* Addressed review feedback from @jrhea
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.

Rewards of underlying protocols are stuck in pools
4 participants