Skip to content

Certora Audit Fixes#304

Merged
jalextowle merged 21 commits intomainfrom
jalextowle/audit/certora-fixes
Jun 13, 2023
Merged

Certora Audit Fixes#304
jalextowle merged 21 commits intomainfrom
jalextowle/audit/certora-fixes

Conversation

@jalextowle
Copy link
Copy Markdown
Contributor

@jalextowle jalextowle commented Jun 11, 2023

This PR addresses the remaining Certora issues reported before June 12th, 2023. Specifically this PR addresses the following questions:

Week 1 Question E

Question calculateSharesInGivenBondsOut and calculateBondsInGivenSharesOut in YieldSpaceMath.sol : for non-zero amount to receive, the functions can return a zero amount to pay. Can this be problematic?

Mitigation The mitigation of question E from Week 1 was to write a test that tests trades at various orders of magnitude to verify that the YieldSpaceMath function in question (calculateSharesInGivenBondsOut) doesn't return zero when reasonable values are provided. (The only pool in which this would be conceivable is a USDC pool, and even then, the input amount would have to be on the order of 1 wei which has a tiny value relative to gas costs).

Week 4 Question A.1

Question The inputs calculated in HyperdriveLP.sol for the average times have a great loss of accuracy since there is a transition between 18 decimals to seconds and then back to 18 decimals. An error of 1e18 seconds in longAverageMaturityTime is then multiplied by a factor of 1e18/ _positionDuration ~ 1e10.

Mitigation The mitigation of question A.1 from Week 4 was to accept the suggestion by adding _calculateTimeRemainingScaled to avoid descaling and rescaling the average time remaining values in HyperdriveLP.

Week 4 Question A.2

Question The netCurveTrade has units of [bonds] but maxCurveTrade has units of [shares] or [bonds/price]. So the ‘else’ branch where the net curve trade is negative, there seems to be a mixture of units, and the value that is passed to the calculateSharesInGivenBondsOut should be in bonds, not shares.

Mitigation The mitigation of question A.2 from Week 4 was to update calculatePresentValue to make use of the new YieldSpaceMath.calculateMaxBuy. After thinking about the problem, we realized that we didn't need to use HyperdriveMath.calculateMaxLong because when we apply the net trade we can assume that the amount of longs outstanding is zero (we close them all and still have outstanding shorts). With this in mind, all we need to do is calculate the amount of shorts that need to be closed to get to a spot price approximating one. We use the change in bonds from calculateMaxBuy, which addresses the units mismatch. The logic itself was also updated based on the recent work on calculateMaxLong.

@jalextowle jalextowle changed the base branch from main to jalextowle/fix/negative-interest-checks June 11, 2023 21:52
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 11, 2023

Hyperdrive Gas Benchmark

Benchmark suite Current: 39968bc Previous: 51af3a2 Deviation Status
addLiquidity: min 711 gas 711 gas 0% 🟰
addLiquidity: avg 48293 gas 46022 gas 4.9346% 🚨
addLiquidity: max 82677 gas 82666 gas 0.0133% 🚨
checkpoint: min 514 gas 492 gas 4.4715% 🚨
checkpoint: avg 23022 gas 23006 gas 0.0695% 🚨
checkpoint: max 33418 gas 33405 gas 0.0389% 🚨
closeLong: min 659 gas 637 gas 3.4537% 🚨
closeLong: avg 45964 gas 45760 gas 0.4458% 🚨
closeLong: max 81328 gas 80396 gas 1.1593% 🚨
closeShort: min 616 gas 616 gas 0% 🟰
closeShort: avg 40762 gas 40932 gas -0.4153%
closeShort: max 81627 gas 80864 gas 0.9436% 🚨
initialize: min 645 gas 667 gas -3.2984%
initialize: avg 139425 gas 139985 gas -0.4000%
initialize: max 140665 gas 141275 gas -0.4318%
openLong: min 685 gas 685 gas 0% 🟰
openLong: avg 104937 gas 106144 gas -1.1371%
openLong: max 147691 gas 147242 gas 0.3049% 🚨
openShort: min 711 gas 711 gas 0% 🟰
openShort: avg 139284 gas 144037 gas -3.2998%
openShort: max 206577 gas 206572 gas 0.0024% 🚨
removeLiquidity: min 547 gas 547 gas 0% 🟰
removeLiquidity: avg 60959 gas 61634 gas -1.0952%
removeLiquidity: max 116479 gas 113267 gas 2.8358% 🚨

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

Base automatically changed from jalextowle/fix/negative-interest-checks to main June 12, 2023 21:02
@jalextowle jalextowle marked this pull request as ready for review June 13, 2023 01:47
Copy link
Copy Markdown
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.

just some nits to fix

Comment thread contracts/src/libraries/HyperdriveMath.sol
Comment thread contracts/src/libraries/YieldSpaceMath.sol Outdated
Comment thread test/integrations/hyperdrive/NonstandardDecimals.sol Outdated
Comment thread test/integrations/hyperdrive/NonstandardDecimals.sol Outdated
Comment thread test/units/libraries/YieldSpaceMath.t.sol Outdated
@jalextowle jalextowle enabled auto-merge (squash) June 13, 2023 02:36
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.

2 participants