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 #743

Open
c4-submissions opened this issue Nov 9, 2023 · 7 comments
Open

QA Report #743

c4-submissions opened this issue Nov 9, 2023 · 7 comments
Labels
bug Something isn't working grade-b Q-19 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sufficient quality report This report is of sufficient quality

Comments

@c4-submissions
Copy link
Contributor

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

@c4-submissions c4-submissions 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 Nov 9, 2023
c4-submissions added a commit that referenced this issue Nov 9, 2023
c4-submissions added a commit that referenced this issue Nov 9, 2023
@141345
Copy link

141345 commented Nov 25, 2023

743 MrPotatoMagic
l r nc
3 0 6

L 1 n [N-01] Variable used only in current contract should be marked with private instead of public visibility
L 2 n [N-02] Typo in struct name collectionAdditionalDataStructure
L 3 n [N-03] Remove redundant check from addMinter() function
L 4 n [N-04] Incorrect collectionPrimaryAndSecondaryAddresses comment should be corrected
L 5 n [N-05] Incorrect comment in function mintAndAuction() should be corrected
L 6 l [L-01] Missing check in function artistSignature() to ensure parameter _signature is not empty
L 7 n [L-02] Avoid copy pasting OZ libraries and inheriting from them
L 8 l [L-03] Missing check in function setCollectionCosts() to ensure salesOption is in range [1-3]
L 9 l [L-04] Missing check in function setCollectionPhases() to ensure allowlist/public end time is greater than start time

@c4-pre-sort
Copy link

141345 marked the issue as sufficient quality report

@c4-pre-sort c4-pre-sort added the sufficient quality report This report is of sufficient quality label Nov 25, 2023
@alex-ppg
Copy link

alex-ppg commented Dec 8, 2023

QA Judgment

The Warden's QA report has been graded B based on a score of 16 combined with a manual review per the relevant QA guideline document located here.

The Warden's submission's score was assessed based on the following accepted findings:

Low-Risk

  • [L-01]

Non-Critical

Informational

  • [N-02]
  • [N-04]
  • [N-05]

@c4-judge
Copy link

c4-judge commented Dec 8, 2023

alex-ppg marked the issue as grade-b

@mcgrathcoutinho
Copy link

mcgrathcoutinho commented Dec 10, 2023

Hi @alex-ppg, thanks for judging this:

Why are L-02, L-03 and N-03 not considered part of the QA report?

Additionally, just keeping #745 in loop here due to the pending decision. if it is considered to be a QA, then I believe it should be added to my QA score as well, which could potentially bump my grade from B to A.

Other than #745, #941 has been marked invalid though it can be considered as a QA issue. This would also help my overall QA score as it is marked as invalid instead of downgraded to QA.

Thank you for your time.

@alex-ppg
Copy link

Hey @mcgrathcoutinho, thanks for requesting clarifications on this QA report! Per the C4 guidelines, regardless of the outcome of issue #745, it cannot be "combined" with this QA report. Any downgraded QA issue is graded independently and the Warden is awarded with the greatest grade of the two. I have proceeded to provide a point-by-point review of your QA report:

  • [N-01]: This is up to preference and these data points may be queried off-chain
  • [N-02]: Awarded I
  • [N-03]: Gas optimization
  • [N-04]: Awarded I
  • [N-05]: Awarded I
  • [L-01]: Awarded QA (L)
  • [L-02]: Preference
  • [L-03]: Inconsequential, an option that is outside the range will "work" in the sense that some models do not require an explicit option. I can see this as QA (NC)
  • [L-04]: Awarded QA (NC)

Combining the above grades will yield us a total of 19 which still does not come close to the lowest A report with a score of 23. Based on the above, I consider this submission to be graded B correctly.

@mcgrathcoutinho
Copy link

Thank you for the clarification

@C4-Staff C4-Staff added the Q-19 label Dec 14, 2023
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-b Q-19 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

7 participants