Post-judge QA

[JeeberC4]

🧑‍⚖️ Post-judging QA; deadline: 10/13/2022 01:00PM PST

1. Please post feedback about specific findings on the relevant issue in this repo.
2. More general feedback for the judge can be shared here.

The judge for this contest is @gititGoro

Reminders

• Please keep your comments as fact-based as possible.
• C4 judges have final authority in determining validity and severity.
• You are welcome to start new discussions here!
• Please link to specific issues wherever relevant.

Thank you!

[0xA5DF]

Hi

2 notes about the rulings:

• I've reported M-06 (FraxlendPair#setTimeLock: Allows the owner to reset TIME_LOCK_ADDRESS #129) in my QA report
  • QA report by brgltd also mentions something similar, not sure if it qualifies (he talks only about limiting the replacement to whenNotPaused)
• About M-01 (Penalty rate is used for pre-maturity date as well #111), IIUC the main justification to downgrade it to med was that the addInterest() function is intended to mitigate this. However, nowhere in the docs or the code it seems like this is the intention of that function. I think we can all agree that having a way to mitigate an issue once you're aware of it is worthless if you're not aware of it.
  • On top of that, it should be noted (as mentioned in the issue itself too) that the bug can cause a penalty rate to be applied retroactively for days or even weeks for being late even for just a few minutes, while the penalty rate has no limit and can easily be set to 10% a day. I think that having a case where you can easily be charged a fine of 50% (having 5 days since it was last called) instead of less than 1% (for being late in an hour) should be considered high severity.

[gititGoro]

Regarding the QA issues with timelock, you're correct. That was an error on my part. I will be upgrading those sub items to medium risk.

On the penalty rate, code itself is a form of documentation. The existence of a public and unpermissioned addInterest function means firstly that this isn't a situation where a hapless user watches on in horror as their funds are lost.

It also means that keepers can be incentivized, even by third parties, to ensure that the wrong interest is not applied which means that a real world, profit generating solution exists to eliminate the issue entirely which means value isn't even being leaked. Indeed, with the correct incentives, MEV activity can be leveraged to keep the interest rates up to date. Eg. whoever triggers addInterest gets a small share of that interest.

Sidenode: a entrepreneurial warden may want to get their code ready in time for the Fraxlend deployment.

The only instances then where funds are lost is where the numbers are so low that it isn't worth a keeper's gas expenditure. In which case, by definition, it's not even worth the depositor/lender or borrower's interest for the interest rate to be accurately calculated because the gas of claiming would overwhelm gains or losses made. In this scenario, it's just leakage that looks like an increased transaction cost.

[0xA5DF]

I will be upgrading those sub items to medium risk.

Thanks 🙂

On the penalty rate, code itself is a form of documentation.

But it doesn't seem that that's the intention of the function, for the simple reason that the project wasn't aware of the issue (because it'd be easier to fix the issue than to patch it by add the public addInterest() function). The sponsor only came up with it as a hotfix once he was aware of the issue (i.e. when he came across our reports).
If any, it seems that the intention of the function is to update the interest so that the public view function (e.g. assetsOf()) would get more accurate values.

this isn't a situation where a hapless user watches on in horror as their funds are lost.

That's true once the issue is raised, but till then the only reason a user wouldn't be watching as their funds are lost is simply because they wouldn't be aware of the issue till the aftermath. But assets will definitely be lost.

[El-Ku]

How was the gas reports given score on this contest. My report, #208, received a score of 4. The optimizations suggested in the report were tested with the sponsor provided test, and the gas savings reported by forge was tabulated to easily see the effect of suggested optimization (The numbers weren't randomly made up).

I see several other reports(#178 , #313 , #131 to list a few ) scored higher than mine and couldn't understand what is my report lacking to get such a low score?

[0xA5DF]

Yeah, I'm also a bit confused on the scoring, my report includes an optimization of 10.5K gas units for the _addInterest() which is called by almost every function (~15 public functions). I doubt there's any report that can top that (esp. since those function aren't deployment function but 'user' function which are called at least x100 times than the deployment functions, and most reports barely scratch a few Ks in total).
And I got a score of 20.
The only other report that mentions it also gave the wrong solution (you can't make a bytes variable immutable) while my submission includes a detailed and tested code diff.

Maybe I should've been more clear and explicit of how significant the saving is, as it's easy to miss it with so many gas reports going on.

[gititGoro]

There are two ways to score high with Gas and QA:

1. Provide a short list of very high quality findings
2. Produce an enormous list of low quality findings.

(Documenting quality doesn't confer direct scoring but instead weights the above numbers)

In this contest, most of the Gas savings found were low and of similar orders of magnitude. Even where they vary, the comparisons are disputable because real world usage vs unit test demonstrations would offer two competing views. Benchmarking is an art in data analysis.

Nonetheless, because there weren't any home runs in gas savings or QA suggestions (due to the high quality of the sponsor's code), the wardens who scored higher were simply those who produced more findings. Option 1 simply didn't yield a good strategy to high scoring.

There's no obvious solution to this conundrum of quality over quantity. On the one hand, it may simply be that a contest has code where i's weren't dotted and t's weren't crossed but where algorithms were efficient. Then you'd expect such a skewed scoring.

It may be worth requiring sponsors establish an official benchmark test that behaves like an integration test. A warden must then test all suggested changes in 1 run. Then all wardens have just one number to compare. Issues where the numbers aren't provided are marked invalid. Then you rank relative to the warden whose tests cost the most gas and get relative percentages where 100% is the least efficient suggestion. The final score is then

100 - R

QA is obviously more subjective so I doubt you'd be able to get as formulaic.

[0xA5DF]

because there weren't any home runs in gas savings

I beg to differ, I think that 10.5K saving on most of the most frequently used functions in the contract is very significant, I've included fully functional code diff.
I also tried to include gas-report diff, but due to a bug in Forge the savings don't show up.

Even where they vary, the comparisons are disputable because real world usage vs unit test demonstrations would offer two competing views.

You're right that in many cases the savings can depend on the scenario, but at least for the case of my report the savings are pretty straight forward - 10.5K for each time those functions are called (the only exception would be when paused, past maturity date or when _addInterest() has been called already in the same block. Not very common conditions).

[El-Ku]

It may be worth requiring sponsors establish an official benchmark test that behaves like an integration test. A warden must then test all suggested changes in 1 run. Then all wardens have just one number to compare. Issues where the numbers aren't provided are marked invalid. Then you rank relative to the warden whose tests cost the most gas and get relative percentages where 100% is the least efficient suggestion. The final score is then

100 - R

I am assuming this is how it would ideally need to have happened but in this contest gas reports weren't judged based on this description (obviously as there was no official benchmark test).

So, coming back to the scenario in this contest:

Most of the reports(including the 100 scored report) doesn't provide any numbers. Or didnt do their mitigations and actually see if their finding was really reducing the gas cost or not. More like copy paste from old reports and pasting links.

I urge you to re-check how you judge gas reports. It takes effort and time to do the mitigations and present the results in a tabular form so that the sponsors and the judges can easily see and verify. As they say, there is no way to refute a POC. But if the argument is that "real world usage vs unit test demonstrations would offer two competing views", then I guess I dont have anything to refute that.

[IllIllI000]

@gititGoro please comment on the following:

• Pair creation can be griefed #248 (comment)
• Gas Optimizations #328 (comment)
• Gas Optimizations #7 (comment)
• Gas Optimizations #198 (comment)

The gas ones are the for the reports you ranked the highest