allow sweeping of base tokens

[mcclurejt]

Resolved Issues

• fixes https://github.com/spearbit-audits/delv-review/issues/35

Description

The Hyperdrive pool should only custody vault shares (deposited base tokens are converted to shares, base tokens withdrawn from the vault are paid out immediately). Any lingering base token balance of the contract is by mistake and sweep should therefore be able to transfer out these tokens.

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 and/or Rust, the corresponding checklist can be removed.

@jalextowle

Solidity

• [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?

@jrhea

Solidity

• [ ] 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?
• [ ] 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.
• [ ] 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?
• [ ] 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?
• [ ] Visibility
  • [ ] Are all payable functions restricted to avoid stuck ether?
• [ ] 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?
• [ ] 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?

[mcclurejt]

The recommendation suggests considering the removal of the _totalBase() function from HyperdriveBase which seems fine to me. The only place I see it being used is in the MockHyperdriveBase... any thoughts here?

Also, any idea why the _totalShares() function exists on HyperdriveBase? Couldn't you just use the totalShares storage variable? Having both seems confusing.

[jalextowle]

The recommendation suggests considering the removal of the _totalBase() function from HyperdriveBase which seems fine to me. The only place I see it being used is in the MockHyperdriveBase... any thoughts here?

Also, any idea why the _totalShares() function exists on HyperdriveBase? Couldn't you just use the totalShares storage variable? Having both seems confusing.

We use both _totalBase and _totalShares in the sweep function in HyperdriveAdmin. The totalShares storage variable is only used in MockHyperdrive and isn't available for any of the other instances found in contracts/src/instances. With that in mind, we can't assume that totalShares will be available, and the point of the _totalShares() function is to abstract this out.

[mcclurejt]

The recommendation suggests considering the removal of the _totalBase() function from HyperdriveBase which seems fine to me. The only place I see it being used is in the MockHyperdriveBase... any thoughts here? Also, any idea why the _totalShares() function exists on HyperdriveBase? Couldn't you just use the totalShares storage variable? Having both seems confusing.

We use both _totalBase and _totalShares in the sweep function in HyperdriveAdmin. The totalShares storage variable is only used in MockHyperdrive and isn't available for any of the other instances found in contracts/src/instances. With that in mind, we can't assume that totalShares will be available, and the point of the _totalShares() function is to abstract this out.

ahhh gotcha, not sure what I was thinking with totalShares being a storage variable on HyperdriveBase 🤦 thank you

[github-actions[bot]]

Hyperdrive Gas Benchmark

Benchmark suite	Current: 7ed056be7d2213903ba3c956a63abc178cad93d6	Previous: d52ee9c85f31a11c1c2ce84e0e2139134932c7a6	Deviation	Status
addLiquidity: min	33783 gas	33915 gas	-0.3892%	✅
addLiquidity: avg	143589 gas	143593 gas	-0.0028%	✅
addLiquidity: max	428119 gas	404397 gas	5.8660%	🚨
checkpoint: min	40220 gas	29232 gas	37.5889%	🚨
checkpoint: avg	104045 gas	103642 gas	0.3888%	🚨
checkpoint: max	212154 gas	211837 gas	0.1496%	🚨
closeLong: min	31495 gas	31428 gas	0.2132%	🚨
closeLong: avg	137840 gas	137616 gas	0.1628%	🚨
closeLong: max	2640391 gas	2640321 gas	0.0027%	🚨
closeShort: min	31416 gas	31349 gas	0.2137%	🚨
closeShort: avg	132821 gas	132700 gas	0.0912%	🚨
closeShort: max	227543 gas	227388 gas	0.0682%	🚨
initialize: min	31327 gas	31283 gas	0.1407%	🚨
initialize: avg	253590 gas	253504 gas	0.0339%	🚨
initialize: max	322650 gas	322697 gas	-0.0146%	✅
openLong: min	33547 gas	33503 gas	0.1313%	🚨
openLong: avg	167373 gas	167170 gas	0.1214%	🚨
openLong: max	253206 gas	253061 gas	0.0573%	🚨
openShort: min	33937 gas	33848 gas	0.2629%	🚨
openShort: avg	170257 gas	169941 gas	0.1859%	🚨
openShort: max	385540 gas	363412 gas	6.0890%	🚨
redeemWithdrawalShares: min	31227 gas	31227 gas	0%	🟰
redeemWithdrawalShares: avg	61756 gas	61861 gas	-0.1697%	✅
redeemWithdrawalShares: max	167484 gas	167368 gas	0.0693%	🚨
removeLiquidity: min	31169 gas	31323 gas	-0.4917%	✅
removeLiquidity: avg	223419 gas	222351 gas	0.4803%	🚨
removeLiquidity: max	398777 gas	375299 gas	6.2558%	🚨

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