Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QA Report #122

Open
c4-bot-8 opened this issue Mar 28, 2024 · 7 comments
Open

QA Report #122

c4-bot-8 opened this issue Mar 28, 2024 · 7 comments
Labels
bug Something isn't working grade-a Q-01 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax selected for report This submission will be included/highlighted in the audit report

Comments

@c4-bot-8
Copy link
Contributor

See the markdown file with the details of this report here.

@c4-bot-8 c4-bot-8 added bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Mar 28, 2024
c4-bot-4 added a commit that referenced this issue Mar 28, 2024
c4-bot-8 added a commit that referenced this issue Mar 28, 2024
@razzorsec
Copy link

The first “QA” was considered medium by us!

@c4-judge
Copy link
Contributor

alex-ppg marked the issue as grade-a

@c4-judge c4-judge added the selected for report This submission will be included/highlighted in the audit report label Apr 29, 2024
@c4-judge
Copy link
Contributor

alex-ppg marked the issue as selected for report

@alex-ppg
Copy link

This submission was graded as the best due to exceeding the 50% accuracy threshold whilst containing valid and thoroughly elaborated findings in an easily digestible format. To note, the first QA finding will be upgraded accordingly once judging concludes and this exhibit is split into a second one as a duplicate of #97.

@Bauchibred
Copy link

Hi @alex-ppg, thanks for judging the contest, I'd like to ask if it's possible you have any comments in regards to the upgradability of some of the listed borderline low/medium issues, a couple were attached here in the QA report as an attempt on not spamming the judging repo with reports that could end up being finalised as QA, I'd appreciate a quick glance on this borderline issues to see if any could be upgraded.

To ease the re-review, I believe grepping the markdown file with the word medium would pinpoint most of these issues, however I'd appreciate re-review as not all have been linked with the medium word, thanks once again for your time.

@alex-ppg alex-ppg mentioned this issue May 2, 2024
@alex-ppg
Copy link

alex-ppg commented May 2, 2024

Hey @Bauchibred, appreciate the in-depth analysis of the QA report and your contribution to the PJQA process! I have evaluated all findings that infer a medium upgrade as follows:

  • QA-02: While this is indeed a mistake, I believe a QA (L) severity is appropriate as the off-chain software of zkSync Era relies on events rather than return variables (as a return variable is accessible to the caller whilst events are accessible to all listeners). I will raise this concern with the zkSync Era team for a revisit but will retain a QA (L) ruling for now.
  • QA-06: This is invalid as the cumulative balance values of the contract are at least equal to the totalSupply due to the token being a closed-circuit system (i.e. balances cannot increase from 0 without totalSupply being increased as well)
  • QA-09: The Additional Note item is indeed correct in the sense that regardless of whether this finding is valid, the exhibit cannot be considered as a valid HM due to being out-of-scope. One of the duplicates from that contest is actually mine, and I understand the implications fully, however, I believe they have been adequately covered in the previous contest and this item is out-of-scope.
  • QA-13: Likewise, this is a valid QA finding from the perspective of additional insight into a past finding but cannot be considered in scope as an HM vulnerability.
  • QA-28: I believe a QA (L) severity is fair, and it is impossible to mimic the extcodecopy instruction's operation due to zkSync Era's internal structure.

I believe that the present ruling is fair, but will make sure to notify the sponsor for a re-evaluation of QA-02 in case it merits an upgrade. It definitely is a mistake in the code, but I do not believe its ramifications to be impactful as nodes utilize events as highlighted in other exhibits such as #112.

@DecentralDisco
Copy link

Regarding validity of items in this QA report, per conversation with the judge @alex-ppg:

The only clear mistake is QA-06, and the rest are passable NC / L / I recommendations. As such, I confirm that all QA items except for QA-06 are valid.

As such, QA-06 will be excluded from the final audit report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working grade-a Q-01 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax selected for report This submission will be included/highlighted in the audit report
Projects
None yet
Development

No branches or pull requests

7 participants