jalextowle commented 2 weeks ago

Resolved Issues

Fixes: https://github.com/spearbit-audits/delv-week-review/issues/13

Description

This PR adds some tests to make sure that the weighted spot price behaves as expected with instantaneous trades and fixes an oversight where the current weighted spot price is used for the addLiquidity check instead of the previous weighted spot price.

The previous weighted spot price will never be 0 in addLiquidity because the current checkpoint is always minted with _applyCheckpoint. In _applyCheckpoint, we always update the previous weighted spot price. This uses the current spot price for the update, so unless the current spot price is 0, the weighted spot price will be non-zero. Some tests were added to clarify this.

Review Checklists

Please check each item before approving the pull request. While going through the checklist, it is recommended to leave comments on items that are referenced in the checklist to make sure that they are reviewed. If there are multiple reviewers, copy the checklists into sections titled ## [Reviewer Name]. If the PR doesn't touch Solidity, the corresponding checklist can be removed.

@mcclurejt

[x] Tokens
- [ ] Do all approve calls use forceApprove?
- [ ] Do all transfer calls use safeTransfer?
- [ ] Do all transferFrom calls use msg.sender as the from address?
  - [ ] If not, is the function access restricted to prevent unauthorized token spend?
[x] Low-level calls (call, delegatecall, staticcall, transfer, send)
- [ ] Is the returned success boolean checked to handle failed calls?
- [ ] If using delegatecall, are there strict access controls on the addresses that can be called? It shouldn't be possible to delegatecall arbitrary addresses, so the list of possible targets should either be immutable or tightly controlled by an admin.
[x] Reentrancy
- [ ] Are functions that make external calls or transfer ether marked as nonReentrant?
  - [ ] If not, is there documentation that explains why reentrancy is not a concern or how it's mitigated?
[x] Gas Optimizations
- [ ] Is the logic as simple as possible?
- [ ] Are the storage values that are used repeatedly cached in stack or memory variables?
- [ ] If loops are used, are there guards in place to avoid out-of-gas issues?
[x] Visibility
- [ ] Are all payable functions restricted to avoid stuck ether?
[x] Math
- [ ] Is all of the arithmetic checked or guarded by if-statements that will catch underflows?
- [ ] If Safe functions are altered, are potential underflows and overflows caught so that a failure flag can be thrown?
- [ ] Are all of the rounding directions clearly documented?
[x] Testing
- [ ] Are there new or updated unit or integration tests?
- [ ] Do the tests cover the happy paths?
- [ ] Do the tests cover the unhappy paths?
- [ ] Are there an adequate number of fuzz tests to ensure that we are covering the full input space?

coveralls commented 2 weeks ago

Pull Request Test Coverage Report for Build 9554965756

Details

2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 92.124%

Totals
Change from base Build 9523138246:	0.0%
Covered Lines:	1965
Relevant Lines:	2133

💛 - Coveralls

coveralls commented 2 weeks ago

Pull Request Test Coverage Report for Build 9554961305

Details

2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 92.124%

Totals
Change from base Build 9523138246:	0.0%
Covered Lines:	1965
Relevant Lines:	2133

💛 - Coveralls

github-actions[bot] commented 2 weeks ago

Hyperdrive Gas Benchmark

Benchmark suite	Current: 1bccaa4770ef8b93f7768ff5f9fc048bf1f264af	Previous: e5525e96fea5bc61e0eec7fe694049aa6722aa1c	Deviation	Status
`addLiquidity: min`	`33937` gas	`33937` gas	`0%`	🟰
`addLiquidity: avg`	`155520` gas	`155877` gas	`-0.2290%`	✅
`addLiquidity: max`	`429244` gas	`429437` gas	`-0.0449%`	✅
`checkpoint: min`	`40316` gas	`40316` gas	`0%`	🟰
`checkpoint: avg`	`144722` gas	`144636` gas	`0.0595%`	🚨
`checkpoint: max`	`255931` gas	`255830` gas	`0.0395%`	🚨
`closeLong: min`	`31361` gas	`31361` gas	`0%`	🟰
`closeLong: avg`	`135883` gas	`136439` gas	`-0.4075%`	✅
`closeLong: max`	`2539376` gas	`2621435` gas	`-3.1303%`	✅
`closeShort: min`	`31327` gas	`31327` gas	`0%`	🟰
`closeShort: avg`	`131252` gas	`132305` gas	`-0.7959%`	✅
`closeShort: max`	`400884` gas	`272530` gas	`47.0972%`	🚨
`initialize: min`	`31349` gas	`31349` gas	`0%`	🟰
`initialize: avg`	`333397` gas	`333352` gas	`0.0135%`	🚨
`initialize: max`	`400023` gas	`399922` gas	`0.0253%`	🚨
`openLong: min`	`33370` gas	`33370` gas	`0%`	🟰
`openLong: avg`	`173247` gas	`174248` gas	`-0.5745%`	✅
`openLong: max`	`334107` gas	`335241` gas	`-0.3383%`	✅
`openShort: min`	`33936` gas	`33936` gas	`0%`	🟰
`openShort: avg`	`168353` gas	`168925` gas	`-0.3386%`	✅
`openShort: max`	`415244` gas	`415870` gas	`-0.1505%`	✅
`redeemWithdrawalShares: min`	`31251` gas	`31251` gas	`0%`	🟰
`redeemWithdrawalShares: avg`	`75026` gas	`75830` gas	`-1.0603%`	✅
`redeemWithdrawalShares: max`	`305734` gas	`305633` gas	`0.0330%`	🚨
`removeLiquidity: min`	`31301` gas	`31301` gas	`0%`	🟰
`removeLiquidity: avg`	`214867` gas	`214933` gas	`-0.0307%`	✅
`removeLiquidity: max`	`403825` gas	`404194` gas	`-0.0913%`	✅

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

coveralls commented 2 weeks ago

Pull Request Test Coverage Report for Build 9556661001

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

22 of 22 (100.0%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.07%) to 92.189%

Totals
Change from base Build 9523138246:	0.07%
Covered Lines:	1971
Relevant Lines:	2138

💛 - Coveralls

coveralls commented 2 weeks ago

Pull Request Test Coverage Report for Build 9556686918

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

22 of 22 (100.0%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.07%) to 92.189%

Totals
Change from base Build 9523138246:	0.07%
Covered Lines:	1971
Relevant Lines:	2138

💛 - Coveralls

coveralls commented 2 weeks ago

Pull Request Test Coverage Report for Build 9556687808

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

22 of 22 (100.0%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.07%) to 92.189%

Totals
Change from base Build 9523138246:	0.07%
Covered Lines:	1971
Relevant Lines:	2138

💛 - Coveralls

coveralls commented 2 weeks ago

Pull Request Test Coverage Report for Build 9556689438

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

22 of 22 (100.0%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.07%) to 92.189%

Totals
Change from base Build 9523138246:	0.07%
Covered Lines:	1971
Relevant Lines:	2138

💛 - Coveralls

coveralls commented 2 weeks ago

Pull Request Test Coverage Report for Build 9556718283

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

22 of 22 (100.0%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.07%) to 92.189%

Totals
Change from base Build 9523138246:	0.07%
Covered Lines:	1971
Relevant Lines:	2138

💛 - Coveralls

coveralls commented 2 weeks ago

Pull Request Test Coverage Report for Build 9556729015

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

22 of 22 (100.0%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.07%) to 92.189%

Totals
Change from base Build 9523138246:	0.07%
Covered Lines:	1971
Relevant Lines:	2138

💛 - Coveralls

coveralls commented 2 weeks ago

Pull Request Test Coverage Report for Build 9556728706

Details

22 of 22 (100.0%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.07%) to 92.189%

Totals
Change from base Build 9523138246:	0.07%
Covered Lines:	1971
Relevant Lines:	2138

💛 - Coveralls

coveralls commented 2 weeks ago

Pull Request Test Coverage Report for Build 9557401375

Details

22 of 22 (100.0%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.07%) to 92.189%

Totals
Change from base Build 9556880128:	0.07%
Covered Lines:	1971
Relevant Lines:	2138

💛 - Coveralls

coveralls commented 2 weeks ago

Pull Request Test Coverage Report for Build 9570423024

Details

22 of 22 (100.0%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.02%) to 92.139%

Totals
Change from base Build 9556880128:	0.02%
Covered Lines:	1969
Relevant Lines:	2137

delvtech / hyperdrive

Use the previous weighted spot price instead of the current weighted spot price for `addLiquidity` #1058

Resolved Issues

Description

Review Checklists

@mcclurejt

Pull Request Test Coverage Report for Build 9554965756

Details

💛 - Coveralls

Pull Request Test Coverage Report for Build 9554961305

Details

💛 - Coveralls

Hyperdrive Gas Benchmark

Pull Request Test Coverage Report for Build 9556661001

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Pull Request Test Coverage Report for Build 9556686918

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Pull Request Test Coverage Report for Build 9556687808

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Pull Request Test Coverage Report for Build 9556689438

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Pull Request Test Coverage Report for Build 9556718283

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Pull Request Test Coverage Report for Build 9556729015

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Pull Request Test Coverage Report for Build 9556728706

Details

💛 - Coveralls

Pull Request Test Coverage Report for Build 9557401375

Details

💛 - Coveralls

Pull Request Test Coverage Report for Build 9570423024

Details

💛 - Coveralls