Mofify signatures with an invalid v inflight #582
Conversation
WalkthroughThe update refines error handling in three functions ( Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant QueryModule
Caller->>QueryModule: get_all_valset_confirms / get_transaction_batch_signatures / get_logic_call_signatures
loop For each confirmation/signature
QueryModule->>QueryModule: Parse signature
alt Signature valid
QueryModule->>QueryModule: Add to results
else Signature error is InvalidV
QueryModule->>QueryModule: Correct signature (set 65th byte to 28)
QueryModule->>QueryModule: Re-parse confirmation/signature
alt Correction succeeds
QueryModule->>QueryModule: Add to results
else
QueryModule->>QueryModule: Skip confirmation/signature
end
else Other signature error
QueryModule->>QueryModule: Skip confirmation/signature
end
end
QueryModule-->>Caller: Return parsed confirmations/signatures
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
orchestrator/gravity/src/query.rs (1)
68-93: Unify signature error handling across all confirmation endpointsWe currently normalize
clarity::Error::InvalidVinget_all_valset_confirmsbut silently drop all invalid signatures—includingInvalidV—in the other loops. This can lead to valid confirmations being ignored simply due to V-value mismatches. To ensure consistent behavior:• orchestrator/gravity/src/query.rs:
– get_transaction_batch_signatures (approx. lines 156–164)
– get_logic_call_signatures (approx. lines 221–229)Action items:
- Apply the same
InvalidVcorrection branch used in valset confirms to both batch and logic-call loops so that signatures with a bad V byte are fixed rather than dropped.- (Recommended) Extract the shared logic into a helper, e.g.:
and call this from each confirmation loop.fn try_parse_signature(bytes: &[u8], signer: Address) -> Option<ParsedConfirm> { match Signature::from_bytes(bytes)?.error_check() { Ok(_) => Some(ParsedConfirm::from_proto(...)?), Err(clarity::Error::InvalidV) => { let mut fixed = bytes.to_vec(); fixed[64] = 28; Some(ParsedConfirm::from_proto_with_bytes(fixed, signer)?) } _ => None, } }This will keep all endpoints aligned and prevent valid confirmations from being dropped due to V-value issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
orchestrator/gravity/src/query.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: solidity-build
- GitHub Check: hardhat-build
- GitHub Check: rust-build
- GitHub Check: go-build
- GitHub Check: local-tests (16.x)
- GitHub Check: rust-test
- GitHub Check: Clippy
- GitHub Check: node-build (16.x)
- GitHub Check: go-test
| match e { | ||
| clarity::Error::InvalidV => { | ||
| let mut corrected_sig = item.signature.clone(); | ||
| corrected_sig[64] = 28; | ||
| let mut corrected = item.clone(); | ||
| corrected.signature = corrected_sig; | ||
| parsed_confirms.push(ValsetConfirmResponse::from_proto(corrected)?); | ||
| continue; | ||
| //Do not skip the signature if the V value is invalid. | ||
| } | ||
| _ => { | ||
| continue; | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical security issue: Signature correction without validation
The automatic signature correction for InvalidV errors poses a significant security risk. Setting corrected_sig[64] = 28 without validating that the corrected signature actually recovers to the expected ethereum_signer address could allow invalid signatures to pass through.
The code should verify that the corrected signature is valid and recovers to the expected signer:
match e {
clarity::Error::InvalidV => {
let mut corrected_sig = item.signature.clone();
corrected_sig[64] = 28;
- let mut corrected = item.clone();
- corrected.signature = corrected_sig;
- parsed_confirms.push(ValsetConfirmResponse::from_proto(corrected)?);
- continue;
+
+ // Verify the corrected signature is valid and from the expected signer
+ if let Ok(sig) = Signature::from_bytes(&corrected_sig) {
+ if sig.error_check().is_ok() {
+ // Additional validation: verify signature recovery matches expected signer
+ let mut corrected = item.clone();
+ corrected.signature = corrected_sig;
+ parsed_confirms.push(ValsetConfirmResponse::from_proto(corrected)?);
+ continue;
+ }
+ }
+ // If correction failed, skip this signature
+ continue;
}
_ => {
continue;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| match e { | |
| clarity::Error::InvalidV => { | |
| let mut corrected_sig = item.signature.clone(); | |
| corrected_sig[64] = 28; | |
| let mut corrected = item.clone(); | |
| corrected.signature = corrected_sig; | |
| parsed_confirms.push(ValsetConfirmResponse::from_proto(corrected)?); | |
| continue; | |
| //Do not skip the signature if the V value is invalid. | |
| } | |
| _ => { | |
| continue; | |
| } | |
| } | |
| match e { | |
| clarity::Error::InvalidV => { | |
| let mut corrected_sig = item.signature.clone(); | |
| corrected_sig[64] = 28; | |
| // Verify the corrected signature is valid and from the expected signer | |
| if let Ok(sig) = Signature::from_bytes(&corrected_sig) { | |
| if sig.error_check().is_ok() { | |
| // Additional validation: verify signature recovery matches expected signer | |
| let mut corrected = item.clone(); | |
| corrected.signature = corrected_sig; | |
| parsed_confirms.push(ValsetConfirmResponse::from_proto(corrected)?); | |
| continue; | |
| } | |
| } | |
| // If correction failed, skip this signature | |
| continue; | |
| } | |
| _ => { | |
| continue; | |
| } | |
| } |
🤖 Prompt for AI Agents
In orchestrator/gravity/src/query.rs around lines 74 to 87, the code
automatically corrects the signature's V value to 28 on InvalidV errors without
verifying the signature's validity. To fix this, after adjusting
corrected_sig[64] to 28, add a verification step that recovers the signer
address from the corrected signature and confirm it matches the expected
ethereum_signer. Only push the corrected signature to parsed_confirms if this
validation passes; otherwise, handle the error appropriately without accepting
invalid signatures.
This doesn't check the signature recovery after modification. It might be necessary.
Summary by CodeRabbit