security: close Critical and High findings in fix-gas#129
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements critical security hardening patches, including transactional state changes, atomic checkpoint finalization, strict mutable object ownership verification, and checkpoint gas limits. The review feedback identifies several key issues: the use of the unstable let_chains feature in both the Move runtime and the Python helper script which breaks stable Rust compilation; a severe performance bottleneck and potential race condition caused by cloning the entire Blockchain history and dropping/re-acquiring locks during checkpoint finalization; a second redundant blockchain clone in the atomic updates builder; and head-of-line blocking in the DAG vertex production when a transaction exceeds the remaining checkpoint gas budget.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if let Some(s_addr) = sender | ||
| && stored_obj.owner != s_addr | ||
| { |
There was a problem hiding this comment.
This uses the unstable let_chains feature (if let ... && ...), which will fail to compile on stable Rust. Since sender.is_some_and is already used elsewhere in the codebase, we should use it here as well to maintain compatibility with stable Rust. Note that scripts/apply_critical_high_fixes.py must also be updated to avoid regenerating this unstable syntax.
if sender.is_some_and(|s_addr| stored_obj.owner != s_addr) {| ''' if let Some(s_addr) = sender | ||
| && stored_obj.owner != s_addr | ||
| { | ||
| return Err(anyhow::anyhow!( | ||
| "Ownership verification failed: mutable object arguments must be owned by the authenticated sender" | ||
| )); | ||
| }''', |
There was a problem hiding this comment.
This replacement introduces the unstable let_chains feature into mod.rs, which will break compilation on stable Rust. Update the replacement string to use sender.is_some_and(...) instead.
''' if sender.is_some_and(|s_addr| stored_obj.owner != s_addr) {
return Err(anyhow::anyhow!(
"Ownership verification failed: mutable object arguments must be owned by the authenticated sender"
));
}''',| let mut next_chain = self | ||
| .blockchain | ||
| .read() | ||
| .unwrap_or_else(|e| e.into_inner()) | ||
| .clone(); |
There was a problem hiding this comment.
Cloning the entire Blockchain struct (including dag_checkpoints) on every checkpoint finalization introduces a significant performance bottleneck that scales quadratically self.blockchain introduces a race condition where concurrent updates to the blockchain could be silently overwritten (lost updates). Consider refactoring this to acquire the write lock upfront or using an incremental update pattern that avoids cloning the entire history.
| for tx in transactions { | ||
| let next = declared_gas | ||
| .checked_add(tx.transaction.gas_limit()) | ||
| .ok_or_else(|| anyhow::anyhow!("Checkpoint gas budget overflow"))?; | ||
| if next > max_checkpoint_gas { | ||
| break; | ||
| } | ||
| declared_gas = next; | ||
| selected.push(tx); | ||
| } |
There was a problem hiding this comment.
Using break when a transaction's gas limit exceeds the remaining checkpoint budget causes head-of-line blocking. It prevents any subsequent transactions from being packed, even if they have smaller gas limits and belong to different senders (which wouldn't violate sequence number ordering). Consider using continue instead of break for transactions from different senders, while keeping track of skipped senders to ensure we don't execute subsequent transactions from a skipped sender out of order.
| let mut slim_chain = chain.clone(); | ||
| for existing in &mut slim_chain.dag_checkpoints { | ||
| if !existing.transactions.is_empty() { | ||
| *existing = Self::checkpoint_without_transactions(existing); | ||
| } | ||
| } |
There was a problem hiding this comment.
Cloning the chain here results in a second full clone of the blockchain history during checkpoint finalization (since it was already cloned in apply_checkpoint.rs). This is highly inefficient. Since slim_chain is only used to serialize a version without transactions, consider passing chain by value or optimizing the serialization to avoid cloning the entire history.
jamesatomc
force-pushed
the
audit/fix-critical-high
branch
from
c543cb2 to
b6d1531
Compare
1 participant
Scope
Remediates all Critical/High findings from the
fix-gasaudit:Deliberate fail-closed behavior
Distributed multi-validator finality remains disabled in this patch. It must not be re-enabled until Mysticeti committed output is connected to real validator vote aggregation / quorum certificates. This patch does not reconstruct committee private keys or fabricate quorum signatures.
Validation
The branch includes a deterministic patch workflow that formats, checks, and tests
kanari-core,kanari-move-runtime-v1, andkanari-node. Keep this PR in draft until all checks pass and the generated source commit is reviewed.