distributeExcessIdle fixes

[jalextowle]

Resolved Issues

Spearbit Issue #34

Description

This PR adds two checks to the calculateDistributeExcessIdleShareProceeds function. First, a check was added that verifies that the ending LP share price is within 1 basis point (1e14) of the starting LP share price. If the check fails, idle won't be distributed. Secondly, a check was added that each step of Newton's method is monotonically improving the calculations error. If a step begins to diverge, we break out of the loop to avoid pathological cases caused by rounding issues.

Enforcing additional checks in _distributeExcessIdleSafe can hurt the system's liveness, so in addition to enforcing the checks, a _maxIterations parameter was added to _distributeExcessIdleSafe that configures the amount of iterations that calculateDistributeExcessIdleShareProceeds will use in Newton's method. This argument was also added to checkpoint so that checkpointers can specify the iterations used by _distributeExcessIdleSafe. checkpoint was updated to always distribute excess idle, which will allow LPs or other stakeholders to distribute excess idle with more iterations in the event that idle becomes stuck.

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.

@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?

Rust

• 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?
  • If matching Solidity behavior, are there differential fuzz tests that
    ensure that Rust matches Solidity?

@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?

Rust

• 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?
  • If matching Solidity behavior, are there differential fuzz tests that
    ensure that Rust matches Solidity?

[github-actions]

Hyperdrive Gas Benchmark

Benchmark suite	Current: 196a014	Previous: 46c834d	Deviation	Status
addLiquidity: min	33783 gas	1546 gas	2085.1876%	🚨
addLiquidity: avg	143907 gas	67936 gas	111.8273%	🚨
addLiquidity: max	428143 gas	293159 gas	46.0446%	🚨
checkpoint: min	40220 gas	1182 gas	3302.7073%	🚨
checkpoint: avg	103933 gas	48477 gas	114.3965%	🚨
checkpoint: max	212154 gas	191559 gas	10.7513%	🚨
closeLong: min	31495 gas	1492 gas	2010.9249%	🚨
closeLong: avg	137919 gas	29374 gas	369.5275%	🚨
closeLong: max	2640391 gas	152303 gas	1633.6435%	🚨
closeShort: min	31416 gas	1494 gas	2002.8112%	🚨
closeShort: avg	132620 gas	33444 gas	296.5435%	🚨
closeShort: max	227543 gas	149156 gas	52.5537%	🚨
initialize: min	31327 gas	1451 gas	2058.9938%	🚨
initialize: avg	253591 gas	213962 gas	18.5215%	🚨
initialize: max	322650 gas	253953 gas	27.0511%	🚨
openLong: min	33547 gas	1499 gas	2137.9586%	🚨
openLong: avg	167414 gas	51658 gas	224.0815%	🚨
openLong: max	253206 gas	185562 gas	36.4536%	🚨
openShort: min	33937 gas	1520 gas	2132.6974%	🚨
openShort: avg	170321 gas	51482 gas	230.8360%	🚨
openShort: max	385540 gas	181325 gas	112.6237%	🚨
redeemWithdrawalShares: min	31227 gas	1488 gas	1998.5887%	🚨
redeemWithdrawalShares: avg	61620 gas	22375 gas	175.3966%	🚨
redeemWithdrawalShares: max	167484 gas	109301 gas	53.2319%	🚨
removeLiquidity: min	31169 gas	1530 gas	1937.1895%	🚨
removeLiquidity: avg	223720 gas	151225 gas	47.9385%	🚨
removeLiquidity: max	398777 gas	325781 gas	22.4065%	🚨

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

[mcclurejt]

left a couple comments regarding rounding for the _updateLiquidity checks

[mcclurejt]

LGTM

[jrhea]

approved, but i think i found one nit on a named return type in natspec

[jrhea]

reapproved for codesize