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 from
Jul 6, 2023
Merged

Fix zellic finding 5 #584

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ba5f76a
fix finding 3 (#575)
kunxian-xia Jun 29, 2023
c524076
fix finding 5
kunxian-xia Jun 30, 2023
bfa56ff
refine comments
kunxian-xia Jul 6, 2023
0a9189d
fmt
kunxian-xia Jul 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 46 additions & 1 deletion zkevm-circuits/src/rlp_circuit_fsm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,8 @@ pub struct RlpCircuitConfig<F> {
tidx_lte_tlength: ComparatorConfig<F, 3>,
/// Check for max_length <= 32
mlength_lte_0x20: ComparatorConfig<F, 3>,
/// Check for tag_length <= max_length
tlength_lte_mlength: ComparatorConfig<F, 3>,
/// Check for depth == 0
depth_check: IsEqualConfig<F>,
/// Check for depth == 1
Expand Down Expand Up @@ -536,7 +538,9 @@ impl<F: Field> RlpCircuitConfig<F> {
},
);

// if (tx_id' == tx_id and format' != format) or (tx_id' != tx_id and tx_id' != 0)
// These two cases are not the very last non-padding RLP instance.
// 1. (tx_id' == tx_id and format' != format)
// 2. (tx_id' != tx_id and tx_id' != 0)
cb.condition(
sum::expr([
// case 1
Expand Down Expand Up @@ -572,6 +576,23 @@ impl<F: Field> RlpCircuitConfig<F> {
},
);

// For the very last non-padding RLP instance, we have
// tx_id' != tx_id && tx_id' == 0
cb.condition(
and::expr([
is_padding_in_dt.expr(Rotation::next())(meta),
not::expr(tx_id_check_in_dt.is_equal_expression.expr()),
]),
|cb| {
// byte_rev_idx == 1
cb.require_equal(
"byte_rev_idx is 1 at the last index",
meta.query_advice(data_table.byte_rev_idx, Rotation::cur()),
1.expr(),
);
},
);

cb.gate(meta.query_fixed(q_enabled, Rotation::cur()))
});

Expand Down Expand Up @@ -756,6 +777,12 @@ impl<F: Field> RlpCircuitConfig<F> {
|meta| meta.query_advice(max_length, Rotation::cur()),
|_meta| 0x20.expr(),
);
let tlength_lte_mlength = ComparatorChip::configure(
meta,
cmp_enabled,
|meta| meta.query_advice(tag_length, Rotation::cur()),
|meta| meta.query_advice(max_length, Rotation::cur()),
);
let depth_check = IsEqualChip::configure(
meta,
cmp_enabled,
Expand Down Expand Up @@ -1121,6 +1148,14 @@ impl<F: Field> RlpCircuitConfig<F> {
// Bytes => DecodeTagStart
cb.condition(tidx_eq_tlen, |cb| {
// assertions
let (lt, eq) = tlength_lte_mlength.expr(meta, Some(Rotation::cur()));
cb.require_equal(
"tag_length <= max_length",
sum::expr([

@roynalnaruto roynalnaruto Jul 4, 2023

Copy link
Copy Markdown

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

@kunxian-xia kunxian-xia Jul 5, 2023

Copy link
Copy Markdown
Author

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?

@roynalnaruto roynalnaruto Jul 5, 2023

Copy link
Copy Markdown

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).

lt, eq,
]),
true.expr(),
);
emit_rlp_tag!(meta, cb, tag_expr(meta), false);

// state transitions.
Expand Down Expand Up @@ -1270,6 +1305,8 @@ impl<F: Field> RlpCircuitConfig<F> {
// LongList => DecodeTagStart
cb.condition(tidx_eq_tlen.expr(), |cb| {
// assertions
let (lt, eq) = tlength_lte_mlength.expr(meta, Some(Rotation::cur()));
cb.require_equal("tag_length <= max_length", sum::expr([lt, eq]), true.expr());

// state transitions
update_state!(meta, cb, tag, tag_next_expr(meta));
Expand Down Expand Up @@ -1392,6 +1429,7 @@ impl<F: Field> RlpCircuitConfig<F> {
byte_value_gte_0xf8,
tidx_lte_tlength,
mlength_lte_0x20,
tlength_lte_mlength,
depth_check,
depth_eq_one,

Expand Down Expand Up @@ -1613,6 +1651,13 @@ impl<F: Field> RlpCircuitConfig<F> {
F::from(witness.state_machine.tag_idx as u64),
F::from(witness.state_machine.tag_length as u64),
)?;
let tlength_lte_mlength_chip = ComparatorChip::construct(self.tlength_lte_mlength.clone());
tlength_lte_mlength_chip.assign(
region,
row,
F::from(witness.state_machine.tag_length as u64),
F::from(witness.state_machine.max_length as u64),
)?;

let depth_check_chip = IsEqualChip::construct(self.depth_check.clone());
depth_check_chip.assign(
Expand Down
4 changes: 2 additions & 2 deletions zkevm-circuits/src/witness/rlp_fsm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,8 +151,8 @@ use crate::{
},
};

// The number of bytes of list can not larger than 2^24.
pub(crate) const N_BYTES_LIST: usize = 1 << 24;
// The number of bytes of list can not larger than 2^24 = 2^(8*3).
pub(crate) const N_BYTES_LIST: usize = 3;

@roynalnaruto roynalnaruto Jul 4, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

why this change?

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

Copy link
Copy Markdown
Author

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?

@roynalnaruto roynalnaruto Jul 5, 2023

Copy link
Copy Markdown

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..

pub(crate) const N_BYTES_CALLDATA: usize = 1 << 24;

fn eip155_tx_sign_rom_table_rows() -> Vec<RomTableRow> {
Expand Down