fix(wormhole): harden VAA and governance deserialization against malformed input#2051
Merged
troian merged 1 commit intoakash-network:mainfrom Mar 20, 2026
Merged
fix(wormhole): harden VAA and governance deserialization against malformed input#2051troian merged 1 commit intoakash-network:mainfrom
troian merged 1 commit intoakash-network:mainfrom
Conversation
WalkthroughAdded input validation to VAA and governance-packet deserialization to avoid panics, switched governance module decoding to a fallible UTF-8 conversion, enforced non-empty guardian sets, and raised minimum quorum from 0 to 1. Tests were extended to cover these cases across contract entry points. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can disable poems in the walkthrough.Disable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
contracts/wormhole/src/state.rs (2)
297-301:⚠️ Potential issue | 🟡 MinorMissing length check in ContractUpgrade::deserialize.
data.get_u64(24)reads 8 bytes starting at offset 24, requiring at least 32 bytes. Short input could cause a panic.🛡️ Proposed fix
impl ContractUpgrade { pub fn deserialize(data: &[u8]) -> StdResult<Self> { + if data.len() < 32 { + return ContractError::InvalidVAA.std_err(); + } let new_contract = data.get_u64(24); Ok(ContractUpgrade { new_contract }) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/wormhole/src/state.rs` around lines 297 - 301, The ContractUpgrade::deserialize implementation is missing a bounds check before calling data.get_u64(24); add a guard that verifies data.len() >= 32 and return a suitable StdResult::Err (e.g., StdError::generic_err or StdError::invalid_len) with a clear message when the input is too short, then only call data.get_u64(24) to construct and return ContractUpgrade { new_contract } so short slices cannot panic.
305-310:⚠️ Potential issue | 🟡 MinorAdd minimum length validation to prevent panic on short input.
GuardianSetUpgrade::deserializecallsdata.get_u32(0)anddata.get_u8(4)before any bounds check. The underlyingByteUtilsimplementation uses slice operations that panic on out-of-bounds access. Ifdatais shorter than 5 bytes, this will panic. Add a minimum length check upfront.🛡️ Proposed fix
impl GuardianSetUpgrade { pub fn deserialize(data: &[u8]) -> StdResult<Self> { const ADDRESS_LEN: usize = 20; + const MIN_LEN: usize = 5; // 4-byte index + 1-byte guardian count + + if data.len() < MIN_LEN { + return ContractError::InvalidVAA.std_err(); + } let new_guardian_set_index = data.get_u32(0); let n_guardians = data.get_u8(4);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/wormhole/src/state.rs` around lines 305 - 310, The deserialize implementation for GuardianSetUpgrade is calling data.get_u32(0) and data.get_u8(4) with no bounds checks, which can panic on inputs shorter than 5 bytes; update GuardianSetUpgrade::deserialize to first verify data.len() >= 5 (or a computed minimum if you need ADDRESS_LEN and n_guardians later) and return an Err(StdError::generic_err(...)) via StdResult<Self> when the slice is too short before calling get_u32/get_u8; reference the ADDRESS_LEN constant and the get_u32/get_u8 calls so the check is placed immediately before those calls to prevent any out-of-bounds slice access.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@contracts/wormhole/src/state.rs`:
- Around line 297-301: The ContractUpgrade::deserialize implementation is
missing a bounds check before calling data.get_u64(24); add a guard that
verifies data.len() >= 32 and return a suitable StdResult::Err (e.g.,
StdError::generic_err or StdError::invalid_len) with a clear message when the
input is too short, then only call data.get_u64(24) to construct and return
ContractUpgrade { new_contract } so short slices cannot panic.
- Around line 305-310: The deserialize implementation for GuardianSetUpgrade is
calling data.get_u32(0) and data.get_u8(4) with no bounds checks, which can
panic on inputs shorter than 5 bytes; update GuardianSetUpgrade::deserialize to
first verify data.len() >= 5 (or a computed minimum if you need ADDRESS_LEN and
n_guardians later) and return an Err(StdError::generic_err(...)) via
StdResult<Self> when the slice is too short before calling get_u32/get_u8;
reference the ADDRESS_LEN constant and the get_u32/get_u8 calls so the check is
placed immediately before those calls to prevent any out-of-bounds slice access.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f3040419-88d7-4bae-8fcb-ea59fadbaa62
📒 Files selected for processing (3)
contracts/wormhole/src/contract.rscontracts/wormhole/src/state.rscontracts/wormhole/src/testing.rs
troian
approved these changes
Mar 20, 2026
…ormed input ParsedVAA::deserialize panicked on inputs shorter than 6 bytes due to unchecked ByteUtils access before the first bounds check. GovernancePacket had the same class of bug for inputs under 35 bytes. Both are now guarded with early length validation that returns a clean InvalidVAA error. quorum() returned 0 for an empty guardian set, which let unsigned VAAs pass the signature check. Changed to return 1, and added a rejection of empty guardian sets in the upgrade handler. Replaced String::from_utf8().unwrap() in the governance handler with proper error propagation via map_err. All fixes are covered by new unit tests. Full test suite passes (60/60). Made-with: Cursor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
contracts/wormhole/src/state.rs (1)
265-275: UseMIN_LENconsistently to avoid constant drift.Line 275 still slices with a hardcoded
35; tie it toSelf::MIN_LENso future format changes stay safe.♻️ Suggested small refactor
- let payload = data[35..].to_vec(); + let payload = data[Self::MIN_LEN..].to_vec();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/wormhole/src/state.rs` around lines 265 - 275, The code hardcodes the payload start index as 35 instead of using the MIN_LEN constant; in deserialize (and any related offsets) replace the hardcoded slice with Self::MIN_LEN (e.g., payload = data[Self::MIN_LEN..].to_vec()) so the header length is consistently derived from MIN_LEN and won't drift if MIN_LEN changes; keep the existing get_bytes32/get_u8/get_u16 uses as-is or compute offsets from Self::MIN_LEN if you prefer a single derived header_len variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@contracts/wormhole/src/state.rs`:
- Around line 265-275: The code hardcodes the payload start index as 35 instead
of using the MIN_LEN constant; in deserialize (and any related offsets) replace
the hardcoded slice with Self::MIN_LEN (e.g., payload =
data[Self::MIN_LEN..].to_vec()) so the header length is consistently derived
from MIN_LEN and won't drift if MIN_LEN changes; keep the existing
get_bytes32/get_u8/get_u16 uses as-is or compute offsets from Self::MIN_LEN if
you prefer a single derived header_len variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 95e2347c-5c61-438b-952c-3d63f8d1f84d
📒 Files selected for processing (3)
contracts/wormhole/src/contract.rscontracts/wormhole/src/state.rscontracts/wormhole/src/testing.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- contracts/wormhole/src/contract.rs
- contracts/wormhole/src/testing.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
ParsedVAA::deserialize panicked on inputs shorter than 6 bytes due to
unchecked ByteUtils access before the first bounds check. GovernancePacket
had the same class of bug for inputs under 35 bytes. Both are now guarded
with early length validation that returns a clean InvalidVAA error.
quorum() returned 0 for an empty guardian set, which let unsigned VAAs
pass the signature check. Changed to return 1, and added a rejection of
empty guardian sets in the upgrade handler.
Replaced String::from_utf8().unwrap() in the governance handler with
proper error propagation via map_err.
All fixes are covered by new unit tests. Full test suite passes (60/60).
Author Checklist
I have...
!to the type prefix if API or client breaking change - not a breaking changeCHANGELOG.md- minor patch to contract functions