Fix/bugbot security fixes #2
Conversation
- SorobanHelper: validate max 255 bytes to prevent silent data truncation - SorobanRpcClient: fix XDR boolean decode using proper SCVal format - Rust contract: use numeric comparison instead of byte length for balance - Rust tests: use HMAC-SHA256 matching production algorithm Fixes identified by Cursor Bugbot code review: - EncodeBytesAsScVal/EncodeStringAsScVal silent truncation for >255 bytes - DecodeBooleanFromXdr false positives from xdrBytes.Any(b => b == 0x01) - verify_balance_proof comparing balance_data.len() instead of values - Tests using SHA256 while contract uses HMAC-SHA256
- Added has_digits flag to track if at least one digit was parsed - Return None for empty input, '-' alone, '.' alone, etc. - Added test_verify_balance_proof_malformed_input test case - Prevents malformed balance data from being treated as zero
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on January 20
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| // Too many decimal places, stop processing | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Decimal parsing fails with more than 8 decimal places
The parse_decimal_to_scaled function adds the digit to result before checking if decimal_places > 8 and breaking. When input has more than 8 decimal places (e.g., "1.123456789"), the 9th digit is included in the result, but decimal_places becomes 9. Since scale_needed uses saturating_sub(9) which equals 0, no scaling is applied. This causes incorrect balance comparisons - for example, "1.123456789" (parsed as 1123456789) would incorrectly compare as greater than "2.0" (parsed as 200000000), potentially allowing insufficient balances to pass verification.
| { | ||
| // Basic heuristic: look for boolean indicators | ||
| return xdrBytes.Any(b => b == 0x01); | ||
| return true; |
There was a problem hiding this comment.
Bug: XDR decoder contradicts comment about type discriminant handling
The comment at line 246 explicitly states "Or Type 1 (SCV_VOID which we treat as false)" but the code at lines 261-264 returns true for typeDiscriminant == 1. This is a direct contradiction between the documented behavior and the implementation. If type 1 is actually SCV_VOID (as the comment states and per Stellar's XDR spec), returning true for it could cause void/empty contract responses to be incorrectly interpreted as successful verification results.
1 participant
Note
Harden SCVal encoding/decoding and Rust balance verification, add decimal parsing, and update tests to HMAC-SHA256 with new negative cases.
ZkpSharp/Integration/Stellar/SorobanHelper.cs: addMaxBytesLength(255) and enforce length checks inEncodeBytesAsScVal/EncodeStringAsScValto prevent truncation.ZkpSharp/Integration/Stellar/SorobanRpcClient.cs: replace heuristic XDR bool parsing with SCVal discriminant validation and value extraction.contracts/stellar/contracts/proof-balance)src/lib.rs:verify_balance_proofnow parses decimals and compares numerically instead of byte-length.parse_decimal_to_scaled()returningNoneon malformed inputs and scaling to 1e8.src/test.rs:compute_test_hmac()and use it to generate proofs (HMAC-SHA256 with ipad/opad).test_verify_balance_proof_insufficientandtest_verify_balance_proof_malformed_input.1.3.2; updateCHANGELOG.mdand release notes to reflect security fixes.Written by Cursor Bugbot for commit a933484. This will update automatically on new commits. Configure here.