Skip to content
This repository was archived by the owner on Apr 18, 2025. It is now read-only.

Fix zellic finding 5#584

Merged
kunxian-xia merged 4 commits into
fix/rlp-audit-wave1from
fix/rlp-zellic-finding5
Jul 6, 2023
Merged

Fix zellic finding 5#584
kunxian-xia merged 4 commits into
fix/rlp-audit-wave1from
fix/rlp-zellic-finding5

Conversation

@kunxian-xia
Copy link
Copy Markdown

@kunxian-xia kunxian-xia commented Jul 3, 2023

Description

Fix the finding 5 reported in Zellic/Kalos Audit Report finding 5.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

@kunxian-xia kunxian-xia mentioned this pull request Jul 3, 2023
Merged
12 tasks
@kunxian-xia kunxian-xia force-pushed the fix/rlp-audit-wave1 branch from ba5f76a to 8de5a6d Compare July 3, 2023 12:20
roynalnaruto
roynalnaruto reviewed Jul 4, 2023
View reviewed changes
Comment thread zkevm-circuits/src/rlp_circuit_fsm.rs
let (lt, eq) = tlength_lte_mlength.expr(meta, Some(Rotation::cur()));
cb.require_equal(
"tag_length <= max_length",
sum::expr([
Copy link
Copy Markdown

@roynalnaruto roynalnaruto Jul 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: I would personally prefer or over sum as its more intuitive in this case

Copy link
Copy Markdown
Author

@kunxian-xia kunxian-xia Jul 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But or has higher degree than sum. How about I leave some comments there?

Copy link
Copy Markdown

@roynalnaruto roynalnaruto Jul 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, it works in this case as both lt and eq are boolean, so sum and or will actually give us the same result. But yes, a comment to explain it would be better as sum by itself shouldn't be used (but it works in this case).

Comment thread zkevm-circuits/src/witness/rlp_fsm.rs Outdated
Comment on lines +154 to +155
// The number of bytes of list can not larger than 2^24 = 2^(8*3).
pub(crate) const N_BYTES_LIST: usize = 3;
Copy link
Copy Markdown

@roynalnaruto roynalnaruto Jul 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this change?

Copy link
Copy Markdown
Author

@kunxian-xia kunxian-xia Jul 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because if we want to add tag_length <= max_length for the case of LongList, then the tag_length here means the number of bytes to represent a LongList(e.g. [0xf9, 0x01, 0xff] has tag_length = 2). Maybe we should change the naming of this const?

Copy link
Copy Markdown

@roynalnaruto roynalnaruto Jul 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and the comment as well can be more descriptive..

@kunxian-xia kunxian-xia merged commit 1243ef4 into fix/rlp-audit-wave1 Jul 6, 2023
@kunxian-xia kunxian-xia deleted the fix/rlp-zellic-finding5 branch July 6, 2023 13:14
lispc added a commit that referenced this pull request Aug 16, 2023
@kunxian-xia @roynalnaruto @lispc
Fix the bugs in RLP/Tx/PI circuit which are reported by Zellic & KALO…
2e42287 
…S auditors (#572)

* fix finding 3 (#575)

* Fix zellic finding 4 (#576)

* fix finding 3 (#575)

* fix finding 4

---------

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>

* add range check on diffs (#586)

* Fix finding 10 (#578)

* fix finding 3 (#575)

* fix finding 10

* Fix finding 13 (#579)

* fix finding 3 (#575)

* fix finding 13

* Fix zellic finding 14 (#580)

* fix finding 3 (#575)

* fix finding 14

* Fix zellic finding 5 (#584)

* fix finding 3 (#575)

* fix finding 5

* refine comments

* fmt

* Fix finding 17 (#602)

* add q_last

* fix

* add more diff range check

* fix finding 7 (#625)

* tx_id = 1 when sm starts

* Fix finding 11 : use length for rlc in rlp table (#719)

* fix: use tag_bytes_rlc and tag_length to copy tag's bytes around

* fix lookup input for Len & RLC & GasCost fields in tx circuit

* refactor

* fix

* refactor

* fix col phase issue

* refactor bytes_rlc type

* Fix the bugs in Tx & PI circuits reported by Zellic & KALOS auditors (#612)

* lookup chain_id to RLP table

* fix finding 22 (#614)

* fix finding 21 (#613)

* fix finding 23 (#618)

* fix finding 26 (#622)

* fix finding 28 (#624)

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>

* fix finding 29 (#623)

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>

* enforce is_final is true at the last row and fix RLC related vul (#735)

* Fix finding 30  (#733)

* enforce all txs in a block are included in the tx table

* clippy

---------

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>

* Fix Zellic / Kalos finding25 (#619)

* fix finding 25

* add comment

---------

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>

* fix conflicts

---------

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>
Co-authored-by: Zhang Zhuo <mycinbrin@gmail.com>

* use q_first instead

* fmt

---------

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>
Co-authored-by: Zhang Zhuo <mycinbrin@gmail.com>
darth-cy pushed a commit that referenced this pull request Aug 16, 2023
@kunxian-xia @roynalnaruto
@lispc @darth-cy
Fix the bugs in RLP/Tx/PI circuit which are reported by Zellic & KALO…
fd2be1c 
…S auditors (#572)

* fix finding 3 (#575)

* Fix zellic finding 4 (#576)

* fix finding 3 (#575)

* fix finding 4

---------

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>

* add range check on diffs (#586)

* Fix finding 10 (#578)

* fix finding 3 (#575)

* fix finding 10

* Fix finding 13 (#579)

* fix finding 3 (#575)

* fix finding 13

* Fix zellic finding 14 (#580)

* fix finding 3 (#575)

* fix finding 14

* Fix zellic finding 5 (#584)

* fix finding 3 (#575)

* fix finding 5

* refine comments

* fmt

* Fix finding 17 (#602)

* add q_last

* fix

* add more diff range check

* fix finding 7 (#625)

* tx_id = 1 when sm starts

* Fix finding 11 : use length for rlc in rlp table (#719)

* fix: use tag_bytes_rlc and tag_length to copy tag's bytes around

* fix lookup input for Len & RLC & GasCost fields in tx circuit

* refactor

* fix

* refactor

* fix col phase issue

* refactor bytes_rlc type

* Fix the bugs in Tx & PI circuits reported by Zellic & KALOS auditors (#612)

* lookup chain_id to RLP table

* fix finding 22 (#614)

* fix finding 21 (#613)

* fix finding 23 (#618)

* fix finding 26 (#622)

* fix finding 28 (#624)

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>

* fix finding 29 (#623)

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>

* enforce is_final is true at the last row and fix RLC related vul (#735)

* Fix finding 30  (#733)

* enforce all txs in a block are included in the tx table

* clippy

---------

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>

* Fix Zellic / Kalos finding25 (#619)

* fix finding 25

* add comment

---------

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>

* fix conflicts

---------

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>
Co-authored-by: Zhang Zhuo <mycinbrin@gmail.com>

* use q_first instead

* fmt

---------

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>
Co-authored-by: Zhang Zhuo <mycinbrin@gmail.com>
lispc added a commit that referenced this pull request Aug 17, 2023
@darth-cy @lispc
@kunxian-xia @roynalnaruto
refactor: remove keccak constants from aggregator (#752)
1d190ba 
* add row counting interface for keccak

* add class level capacity calculator for keccak

* remove f capacity from core

* remove capacity calculator in aggregator util

* remove unnecessary imports

* replace max keccak round in core

* replace reference for max keccak

* remove unnecessary keccak imports and constants

* remove max keccak constant

* remove constants in hash cell parsing

* remove constant column sanity check

* add state column usage log

* adjust input bytes column

* add long column padding

* correct fmt

* fix fmt

* minor fixes

* fix

* Fix: allow skipping of L1Msg tx part 2 (calculate num_all_txs in tx circuit) (#778)

* calculate num_l1_msgs and num_l2_txs in tx circuit

* fix

* fmt and clippy

* fix: non-last tx requires next is calldata

* add NumAllTxs in block table and copy it from pi to block table

* add lookup for NumAllTxs in tx circuit

* clippy

* add block num diff check to avoid two real block have same num

* clippy

* address comments

* Fix the bugs in RLP/Tx/PI circuit which are reported by Zellic & KALOS auditors (#572)

* fix finding 3 (#575)

* Fix zellic finding 4 (#576)

* fix finding 3 (#575)

* fix finding 4

---------

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>

* add range check on diffs (#586)

* Fix finding 10 (#578)

* fix finding 3 (#575)

* fix finding 10

* Fix finding 13 (#579)

* fix finding 3 (#575)

* fix finding 13

* Fix zellic finding 14 (#580)

* fix finding 3 (#575)

* fix finding 14

* Fix zellic finding 5 (#584)

* fix finding 3 (#575)

* fix finding 5

* refine comments

* fmt

* Fix finding 17 (#602)

* add q_last

* fix

* add more diff range check

* fix finding 7 (#625)

* tx_id = 1 when sm starts

* Fix finding 11 : use length for rlc in rlp table (#719)

* fix: use tag_bytes_rlc and tag_length to copy tag's bytes around

* fix lookup input for Len & RLC & GasCost fields in tx circuit

* refactor

* fix

* refactor

* fix col phase issue

* refactor bytes_rlc type

* Fix the bugs in Tx & PI circuits reported by Zellic & KALOS auditors (#612)

* lookup chain_id to RLP table

* fix finding 22 (#614)

* fix finding 21 (#613)

* fix finding 23 (#618)

* fix finding 26 (#622)

* fix finding 28 (#624)

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>

* fix finding 29 (#623)

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>

* enforce is_final is true at the last row and fix RLC related vul (#735)

* Fix finding 30  (#733)

* enforce all txs in a block are included in the tx table

* clippy

---------

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>

* Fix Zellic / Kalos finding25 (#619)

* fix finding 25

* add comment

---------

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>

* fix conflicts

---------

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>
Co-authored-by: Zhang Zhuo <mycinbrin@gmail.com>

* use q_first instead

* fmt

---------

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>
Co-authored-by: Zhang Zhuo <mycinbrin@gmail.com>

* add pi comments

* rename preimage col idx

* add keccak rows check

* rename input bytes col finder fn

* modify keccak row env constaint

* modify keccak row env constaint

* add named constant setup vars

* modify keccak row check

* clippy advised

* add comments on chunk hash

* fmt

* avoid constant lookup table

* avoid repetitive computation of input_bytes_col_idx

---------

Co-authored-by: Zhuo Zhang <mycinbrin@gmail.com>
Co-authored-by: xkx <xiakunxian130@gmail.com>
Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@roynalnaruto roynalnaruto roynalnaruto approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

crate-zkevm-circuits

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@kunxian-xia @roynalnaruto