RFC: Bot submission standardization #118
Replies: 19 comments 37 replies
-
|
The category example should be |
Beta Was this translation helpful? Give feedback.
-
|
you've cut out the |
Beta Was this translation helpful? Give feedback.
-
|
@IllIllI000 That was an oversight on my part with the |
Beta Was this translation helpful? Give feedback.
-
|
Sorry, somehow I missed your response in other response emails. |
Beta Was this translation helpful? Give feedback.
-
|
@hannahtuttle tagging because I substantively edited |
Beta Was this translation helpful? Give feedback.
-
|
@IllIllI000 Thank you for the feedback! I did end up leave the naming the same for the schema but I am working on some docs that will have more details of what each field is to make this clearer. |
Beta Was this translation helpful? Give feedback.
-
|
Great proposal, I like it. It's not entirely clear to me what is the expected value of "content" for each instance. Could that be a code snippet, comments, any text content, or is it something more specific? |
Beta Was this translation helpful? Give feedback.
-
|
@DadeKuma just making sure you saw the comment thread immediately above this, where @IllIllI000 asked the same question and @hannahtuttle responded. |
Beta Was this translation helpful? Give feedback.
-
|
@CloudEllie yes I've read it, thanks for your reply! |
Beta Was this translation helpful? Give feedback.
-
|
Can add below items to the standard:
Judging side: |
Beta Was this translation helpful? Give feedback.
-
|
This particular RFC is intended to move C4 towards a submission standard, and does not include judging standards. Judging questions expected to be discussed during the upcoming Supreme Court session. |
Beta Was this translation helpful? Give feedback.
-
|
I would also like some sort of free-form data field for the report as a whole, for things like my rubric section |
Beta Was this translation helpful? Give feedback.
-
|
Makes sense. Maybe an "overview" or "comments" section? |
Beta Was this translation helpful? Give feedback.
-
|
That would work for my purposes, but consider the fact that the information I'm providing is not meant to be "read" per-se (it's free-form data), so if, for example, you were to display the comments at the top of the report, it may look strange. Maybe have "comments" for the top, and "footnotes" for the bottom? |
Beta Was this translation helpful? Give feedback.
-
|
Great idea, 2 notes:
|
Beta Was this translation helpful? Give feedback.
-
|
If you're trying to market your bot as your own, it may be better to make up your own titles, rather than copying someone else's. If you don't care, and want to copy, that's also fine. In either case some judges already regularly use this tool, which addresses this problem: https://illilli000.github.io/races/2023-07-lens/scorer.html I agree that having a schema would be great |
Beta Was this translation helpful? Give feedback.
-
|
I would suggest using a unique ID instead of trying to use the same title (what is the "correct" title for existing issues?) The judge should do the following:
This takes an initial effort, however, it will make judging a LOT easier in the long run |
Beta Was this translation helpful? Give feedback.
-
|
I think this is a good idea. |
Beta Was this translation helpful? Give feedback.
-
|
I'd like to bump this suggestion of publishing an actual JSON schema. Using a schema would mean that bots would always have a source of truth of whether a submission will be accepted or not. Essentially you (C4) could have zero validation-specific code, and instead always accept any JSON that passes schema validation. This would prevent cases where backend validation code is tweaked, but bots don't know about/conform to this change, and thus we get a bunch of surprise rejections during a race. It would also help to get new bots up to speed much faster, and you'd get fewer questions over all. |
Beta Was this translation helpful? Give feedback.
-
|
I want to bump this up, @DadeKuma offers a great solution. This problem has always happened, and it depends more or less on the judge. Inconsistent scoring of the same finds I'd like to remind that Alex (don't know the github handle) has a great tool that already does this, but in title only. https://discord.com/channels/810916927919620096/1095870155118088283/1126076968127975454 Perhaps it could be combined with @DadeKuma's solution? |
Beta Was this translation helpful? Give feedback.
-
|
Good idea! I proposed also that "scope file list" should have a standard form, and we could take this change in order to standardize input and output. |
Beta Was this translation helpful? Give feedback.
-
|
I also would add the possibility to insert "scoping details" section inside well-formed input, in order to help bot to know, for example, code coverage, foundry/hardhat using, using of openzeppelin libraries, erc20/erc721 token and so on |
Beta Was this translation helpful? Give feedback.
-
|
@Simon-Busch this may be relevant to the work you're doing - wanted to make sure you see the suggestion. |
Beta Was this translation helpful? Give feedback.
-
|
Great idea, |
Beta Was this translation helpful? Give feedback.
-
|
We absolutely need one reference for scope of contest and do not change it when is running. |
Beta Was this translation helpful? Give feedback.
-
|
Some comments/questions:
|
Beta Was this translation helpful? Give feedback.
-
|
Beta Was this translation helpful? Give feedback.
-
|
The severity levels for bot crew are what is currently being used in the bot submissions with the exception of
Yes the JSON Schema is what is being used to validate the submission. So if the report does not pass/match the JSON format then the report/submission will not be accepted.
This is being computed only for the
This is a great idea as with the proposed schema above we are not capturing that data. I will add this as an optional field. Is this something that should be added to each instance or the whole issue?
We could add this as an optional field or would it be better to add this as part of the description for each issue?
|
Beta Was this translation helpful? Give feedback.
-
|
@hannahtuttle,
I've looked at all the recent winning bot reports and don't see any with a Refactoring severity. Can you please link to a report that has this? Maybe it is appearing in bot submissions that have yet to win which would explain why they are unknown to all except the submitter and those that have access to non-winner reports. |
Beta Was this translation helpful? Give feedback.
-
|
My recommendation: {
"summary": [
{
"severity": "H",
"issues": [
{
"title": "Issue 1",
"instances": "10"
},
{
"title": "Issue 2",
"instances": "20"
}
],
"extra": "Total: 30 instances over 2 issues"
},
{
"severity": "M",
"description": "{{description}}",
"issues": [
{
"title": "{{issue title}}",
"instances": "{{number of instances}}"
},
{
"title": "{{issue title}}",
"instances": "{{number of instances}}"
}
],
"extra": "{{extra information}}"
},
{
"severity": "G",
"description": "Gas totals use lower bounds of ranges and count two iterations of each for-loop. All values above are runtime, not deployment, values; deployment values are listed in the individual issue descriptions. The table above as well as its gas numbers do not include any of the excluded findings.",
"issues": [
{
"title": "Issue 1",
"instances": "10",
"saved": "100"
},
{
"title": "Issue 2",
"instances": "20",
"saved": "200"
},
{
"title": "Issue 3",
"instances": "20",
"saved": "-"
}
],
"extra": "Total: 50 instances over 3 issues with **300 gas** saved"
}
],
"findings": [
{
"severity": "H",
"issues": [
{
"title": "Issue 1",
"description": "Description of issue 1",
"instances": [
{
"link": "https://github.com/code-423n4/2023-10-zksync/blob/main/code/contracts/ethereum/contracts/zksync/facets/Mailbox.sol#L37-L37",
"content": "```solidity\nFile: code/contracts/ethereum/contracts/zksync/facets/Mailbox.sol\n\n// @audit requestL2Transaction\n37 contract MailboxFacet is Base, IMailbox {\n\n```\n*GitHub*: [[37](https://github.com/code-423n4/2023-10-zksync/blob/main/code/contracts/ethereum/contracts/zksync/facets/Mailbox.sol#L37-L37)]"
}
]
},
{
"title": "{{issue title}}",
"description": "{{issue description}}",
"instances": [
{
"link": "{{link to lines of code for judges to review}}",
"content": "{{content of issue, it can be combination of codes, text, diff and etc}}"
}
]
}
]
},
{
"severity": "M",
"issues": [...]
},
{
"severity": "G",
"issues": [...]
}
]
}
this can be used to produce:SummaryHigh Risk Issues
Total: 30 instances over 2 issues Medium Risk Issues{{description}}
{{extra information}} Gas OptimizationsGas totals use lower bounds of ranges and count two iterations of each
Total: 50 instances over 3 issues with 300 gas saved High Risk Issues[H‑01] Issue 1Description of issue 1 File: code/contracts/ethereum/contracts/zksync/facets/Mailbox.sol
// @audit requestL2Transaction
37 contract MailboxFacet is Base, IMailbox {
GitHub: [37] [H‑02] {{issue title}}{{issue description}} {{content of issue, it can be combination of codes, text, diff and etc}} Medium Risk Issues[...] Gas Issues[...] Notes:
|
Beta Was this translation helpful? Give feedback.
-
|
I propose to avoiding summary information. Once that we have "findings" field, we can compute summary. |
Beta Was this translation helpful? Give feedback.
-
|
Yes, that is also possible. I wanted to avoid placing any computation on the C4 side and provide more flexibility for bots to include custom information in the summary. |
Beta Was this translation helpful? Give feedback.
-
|
@0kn0t The current change will only create a Markdown for the winning report which is what will be used on the audit repo and the gist that is created from the issue. There is already a process in place to create the summary for each severity type in the report when the markdown is generated. |
Beta Was this translation helpful? Give feedback.
-
|
Very good move! Please, let's not overcomplicate this too much; we can always expand the structure. I have something similar as an intermediate step in generating Markdown. {
"issue": "MissingCheckAddressZeroInInitializer",
"severity": "Low",
"gas": null,
"title": "Missing checks for address(0) in initializer",
"description": [
"Check for zero-address to avoid the risk of setting address(0) for state variables when contract is initialized."
],
"instances": [
{
"contract": "contracts\/Tokens\/Prime\/Prime.sol",
"startLine": 130,
"endLine": 167,
"diff": null,
"comment": null
},
{
"contract": "contracts\/Tokens\/Prime\/PrimeLiquidityProvider.sol",
"startLine": 90,
"endLine": 111,
"diff": null,
"comment": null
}
]
}In the |
Beta Was this translation helpful? Give feedback.
-
|
Just a note re: rollout timeline - given the active, ongoing discussion here (which has been very fruitful, btw!), I think we'll need to revise the proposed dates. Oct 23 might still be feasible, but I appreciate that all bot crews will need sufficient time to update their bots. It would be helpful to hear how much notice would be reasonable, i.e. 1 week, 2 weeks, etc. |
Beta Was this translation helpful? Give feedback.
-
|
Based on "Disputed is also acceptable as a severity level, for validation purposes." |
Beta Was this translation helpful? Give feedback.
-
Just for the format of output. |
Beta Was this translation helpful? Give feedback.
-
I'd suggest a minimum of 2 weeks notice. We need some finalized guidance on the JSON structure and then time to implement it. |
Beta Was this translation helpful? Give feedback.
-
JSON SchemaAfter reviewing these suggestions above and implementing changes to the new process, the new expected JSON is here: Once the winning report has been selected, a Markdown file will be generated only for the winning report. Testing environmentTo give bot crews sufficient time to test the new submission requirements, a testing environment will be made available starting next week. Caveat: I will be OOO until Oct 28th, so please log bugs or change requests and I will address them when I'm back in the office. New ship date: Nov 6In order to give everyone sufficient time to change their bots to match this expected output -- and to allow me time to address any bugs or change requests -- the new date for these changes going live is Monday November 6th. All audits that start on or after the 6th will require bot reports to be submitted using the above JSON format. |
Beta Was this translation helpful? Give feedback.
-
|
Will "instance count" be defined as For example, using your earlier example, is the instance count here |
Beta Was this translation helpful? Give feedback.
-
|
I think in this case you should probably have 3 seperate instances :) |
Beta Was this translation helpful? Give feedback.
-
|
Yeah I get that, but the problem is then you have duplicate blocks and/or it's difficult to show context -- hence the discussion at #118 (reply in thread) which changed You'll note that @IllIllI000 suggested renaming
|
Beta Was this translation helpful? Give feedback.
-
|
@ryanjshaw Based on my understanding of how these are currently being calculated the instances are currently being counted based on the |
Beta Was this translation helpful? Give feedback.
-
|
I was going to write the same thing, I don't think loc should be an array. Each instance should have its own json object. In the example given, there should be three separate instances. With separate instances, you can freely include whatever you want in the content (such as comments, etc.). The current one suggests that we should do one instance for each contract, and it will be more reasonable to do multiple instances per contract. When I say contract, I mean the .sol file. To sum up, we should have a main node for issues and then individual instances, either with a key enabling grouping by contract, or further nesting per contract. {
"comment": null,
"footnote": null,
"findings": [
{
"severity": "Medium",
"title": "Title of issue/finding",
"description": "description for the issue found",
"gasSavings": null,
"category": null,
"instances": [
{
"contract": "contracts\/ERC20MultiDelegate.sol",
"content": "",
"loc": 0
},
{
"contract": "contracts\/ERC20MultiDelegate.sol",
"content": "",
"loc": 0
}
]
}
]
}OR: {
"comment": null,
"footnote": null,
"findings": [
{
"severity": "Medium",
"title": "Title of issue/finding",
"description": "description for the issue found",
"gasSavings": null,
"category": null,
"instances": [
{
"contract": "contracts\/ERC20.sol",
"items": [
{
"content": "",
"loc": 0
}
]
},
{
"contract": "contracts\/ERC20MultiDelegate.sol",
"items": [
{
"content": "",
"loc": 0
}
]
}
]
}
]
} |
Beta Was this translation helpful? Give feedback.
-
|
Agree with the gist of what you're saying -- this is how my bot works internally, except since a file can contain more than one contract, I used the literal term |
Beta Was this translation helpful? Give feedback.
-
|
can you link to a gist of the end report markdown where it wouldn't be possible in the proposed format? If it's just a matter of how you're storing it in your JSON, you can just have a translation layer to the c4 format, and dump that separately |
Beta Was this translation helpful? Give feedback.
-
|
I believe you can do it in the proposed format but I think it will count as one instance, unless it's agreed that |
Beta Was this translation helpful? Give feedback.
-
|
"contract": "contracts/ERC20MultiDelegate.sol" should be change to Not necessarily every file contains contract or library or interface. |
Beta Was this translation helpful? Give feedback.
-
|
Based on the feedback received so far on the submission testing environment, we have compiled a list of issues to fix / improve and expect to have most of them addressed by Nov 1, at which point further testing can be done. We'll assess readiness and the proposed ship date (Nov 6) later this week and post an update here and in Discord to either confirm Nov 6 or postpone by a week. |
Beta Was this translation helpful? Give feedback.
-
|
I would really, really, really, really like a file-based submission method. As it is, a lot of people's browsers/VMs crash when trying to paste large blobs, and having to copy and paste seems sub-optimal |
Beta Was this translation helpful? Give feedback.
-
|
@IllIllI000 This is on my list for changes to make this week. I'm going to fix the bugs first just so everyone can test with the current set up and then add a file upload option as well. |
Beta Was this translation helpful? Give feedback.
-
|
Hello Everyone, Based on further feedback and updates, we have decided to postpone ship date by a week to allow time to make these fixes/updates. The new release date will be Nov 13th at which time all bot races and qualifiers will expect the new json format to be submitted. Thank you all for your feedback and patience. |
Beta Was this translation helpful? Give feedback.
-
Currently, bot reports are submitted as Markdown files, which are then parsed by the bot judges through a somewhat labor-intensive process (albeit with the assistance of some tools).
In the interest of reducing manual effort from judges, as well as furthering standardization to support better overall tooling, Code4rena bot races will soon require reports to be submitted in a standardized format.
New bot report format
Bot crews will submit a single, compilation-style bot report (as they currently do), but in .json format (i.e. no longer in Markdown).
Bot reports should be formatted like this:
Accepted severity levels are:
HighMediumLowRefactoringNonCriticalGasDisputedis also acceptable as a severity level, for validation purposes.The bot race submission form will be updated to validate that the correct format is used.
Proposed timeline
With these things in mind, we're looking for input as to whether October 16 or October 23, 2023 might be reasonable dates to implement this change -- meaning that bot races on or after the implementation date would require .json submissions.
Process notes
Beta Was this translation helpful? Give feedback.
All reactions