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

Open
c4-bot-6 opened this issue Oct 10, 2024 · 5 comments
Open

QA Report #19

c4-bot-6 opened this issue Oct 10, 2024 · 5 comments
Labels
3rd place bug Something isn't working grade-b Q-02 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons

Comments

@c4-bot-6
Copy link
Contributor

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

@c4-bot-6 c4-bot-6 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 Oct 10, 2024
c4-bot-4 added a commit that referenced this issue Oct 10, 2024
c4-bot-3 added a commit that referenced this issue Oct 10, 2024
@blockchainstar12 blockchainstar12 added the sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons label Oct 14, 2024
@c4-judge
Copy link
Contributor

OpenCoreCH marked the issue as grade-b

@CrystallineButterfly
Copy link

CrystallineButterfly commented Oct 16, 2024

Hello @OpenCoreCH Thank you for judging, this was my first rust based audit and so I am completely accountable for my assumptions and lack of concrete assertions. So I feel your judging is fair and accurate, I am leaving a comment as I would move to suggest given #16 #17 and #18 have been placed as QA also, would it be fair to suggest, that they can be included as part of my QA report and move this to a grade A? If the value for all my issues is deemed beneficial for codedEstate, that is. I completely respect whatever your conclusion comes to be.

All the best- K42

@OpenCoreCH
Copy link

Hi @CrystallineButterfly, thanks for your comment. I have looked at the downgraded issues when judging the report. However, #16 and #17 describe issues where the recommendation would not change anything about the behaviour of the system (as well as 03 in the QA report). It is debatable whether handling these cases gracefully or just accepting the panic is preferable, so I treat this as more or less neutral when judging. For #18, the impact of the current design decision is unfortunately not really explained: "it could open up other security risks if combined with other attack vectors and can be better handled." -> Which security risks if combined with which attack vectors? If these questions were answered, it would be more valuable for a project. Similarly, if there were more issues like L-01 and L-02 which describe things that could really improve the behaviour of the system.

@CrystallineButterfly
Copy link

Hey @OpenCoreCH thank you for the reply, my assumptions would more so be of an account compromised giving the user who used approve_all less protection if there account is compromised; so this would be more so a social engineering vector, which wouldn't be in scope as it's the users who would be accountable. So i respect your judgement, and understand these don't provide enough value for the project, it helps me do better in next rust audits 🫡

All the best- K42

@thebrittfactor
Copy link

For awarding purposes, C4 staff have marked as 3rd place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3rd place bug Something isn't working grade-b Q-02 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons
Projects
None yet
Development

No branches or pull requests

7 participants