Fenwick Rounding Improvements (Invariant fix F3 F4) #719
Conversation
| // If clearing out the bucket deposit, ensure it's zeroed out | ||
| if (redeemedLPs_ == params_.bucketLPs) { | ||
| removedAmount_ = scaledDepositAvailable; | ||
| removedAmount_ = scaledDepositAvailable; // TODO: shouldn't this be moved below the below, and use unscaled amount? |
There was a problem hiding this comment.
we need the scaled removed amount for further calculations in moveQuoteToken and removeQuoteToken
There was a problem hiding this comment.
What worries me is that, if we do this by setting scaledDeposit, we might mess up the subsequent line that tries to ensure that we remove everything. scaledDepositAvailable/depositScale could be less than unscaledDeposit, perhaps, especially because / rounds down.
My gut is that it would be better to move this below where we set unscaledRemovedAmount, and if redeemedLPs==bucketLPs, THEN we set unscaledRemovedAmount to unscaledDepositAvailable, and removedAmount_ to scaledDepositAvailable.
There was a problem hiding this comment.
kk, makes sense, not sure though if we should add it in this PR or if in different but should be OK to make
grandizzy
changed the title
grandizzy
assigned prateek105, MikeHathaway, ith-harvey, mattcushman and EdNoepel and unassigned prateek105 and MikeHathaway
grandizzy
requested review from
prateek105,
MikeHathaway,
ith-harvey and
EdNoepel
foundry.toml
Outdated
| runs = 1000 # Number of times that a sequence of function calls is generated and run | ||
| depth = 30 # Number of function calls made in a given run. | ||
| runs = 100 # Number of times that a sequence of function calls is generated and run | ||
| depth = 200 # Number of function calls made in a given run. |
There was a problem hiding this comment.
is there value in decreasing runs and increasing depth?
There was a problem hiding this comment.
restored to 1000 with 5e8bcff
grandizzy
changed the title
Rounding Improvements - Changes to address invariants F3 and F4
High level
(x * y * x) / 1e36is generally more accurate thanMaths.wmul(Maths.wmul(x,y),z), as the latter rounds offx*ywhich is then multiplied byz, amplifying the roundoff errorprice * collateralcan result in large number in high priced and heavily used pool, so to reduce the risk of overflow we partially round along the way and rescale:(price * collateral / 1e12) * z / 1e6retains enough of the accuracy gain, while allowing for an additional 1e12 quantity in the product.Description of bug or vulnerability and solution
Contract size
Pre Change
<PASTE_OUTPUT_HERE>
Post Change
<PASTE_OUTPUT_HERE>
Gas usage
Pre Change
<PASTE_OUTPUT_HERE>
Post Change
<PASTE_OUTPUT_HERE>