Fixed documentation nits

[jalextowle]

Resolved Issues

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

Description

This PR updates some documentation nits from the spearbit audit. The audit issue asks for y_max_out to be changed to z_max_out, but I realized that this is also wrong. It should be z_max_in since traders have to pay for longs with shares. Otherwise, all of the suggestions were accepted as specified.

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.

@mcclurejt

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?

@cashd

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?

@sentilesdal

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?

[github-actions[bot]]

Hyperdrive Gas Benchmark

Benchmark suite	Current: c17fe9c00cc8a7fe4e4f8a12e797b51d9758a0a1	Previous: 46c834d403754cb2c6c572563bccd65634c54aff	Deviation	Status
addLiquidity: min	33915 gas	1546 gas	2093.7257%	🚨
addLiquidity: avg	145351 gas	67936 gas	113.9528%	🚨
addLiquidity: max	404331 gas	293159 gas	37.9221%	🚨
checkpoint: min	29232 gas	1182 gas	2373.0964%	🚨
checkpoint: avg	104444 gas	48477 gas	115.4506%	🚨
checkpoint: max	211690 gas	191559 gas	10.5090%	🚨
closeLong: min	31428 gas	1492 gas	2006.4343%	🚨
closeLong: avg	138186 gas	29374 gas	370.4364%	🚨
closeLong: max	2640312 gas	152303 gas	1633.5916%	🚨
closeShort: min	31349 gas	1494 gas	1998.3266%	🚨
closeShort: avg	132792 gas	33444 gas	297.0578%	🚨
closeShort: max	227319 gas	149156 gas	52.4035%	🚨
initialize: min	31283 gas	1451 gas	2055.9614%	🚨
initialize: avg	253484 gas	213962 gas	18.4715%	🚨
initialize: max	322670 gas	253953 gas	27.0589%	🚨
openLong: min	33503 gas	1499 gas	2135.0233%	🚨
openLong: avg	167306 gas	51658 gas	223.8724%	🚨
openLong: max	253022 gas	185562 gas	36.3544%	🚨
openShort: min	33848 gas	1520 gas	2126.8421%	🚨
openShort: avg	169621 gas	51482 gas	229.4763%	🚨
openShort: max	363340 gas	181325 gas	100.3805%	🚨
redeemWithdrawalShares: min	31227 gas	1488 gas	1998.5887%	🚨
redeemWithdrawalShares: avg	63433 gas	22375 gas	183.4994%	🚨
redeemWithdrawalShares: max	167269 gas	109301 gas	53.0352%	🚨
removeLiquidity: min	31323 gas	1530 gas	1947.2549%	🚨
removeLiquidity: avg	218222 gas	151225 gas	44.3029%	🚨
removeLiquidity: max	375251 gas	325781 gas	15.1850%	🚨

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

[MrToph]

The audit issue asks for y_max_out to be changed to z_max_out, but I realized that this is also wrong. It should be z_max_in since traders have to pay for longs with shares. Otherwise, all of the suggestions were accepted as specified.

My understanding was that when we're net long, we are simulating traders closing the net longs and see what effect this has on the present value. Therefore, traders are closing their longs, receiving shares from the contract. Therefore z_out, not z_in. This also matches the notation in https://github.com/delvtech/hyperdrive/pull/688