--- name: multiversx-code-analysis description: Comprehensive code analysis toolkit for MultiversX smart contracts. Covers differential review (version comparison, upgrade safety), fix verification (validate patches, regression testing), and variant analysis (find similar bugs across codebase). Use when reviewing PRs, verifying security patches, or hunting for bug variants. --- # MultiversX Code Analysis Toolkit for analyzing MultiversX smart contract code changes, verifying fixes, and finding bug variants across codebases. ## When to Use - Reviewing pull requests with contract changes - Auditing upgrade proposals before deployment - Verifying security patches and bug fixes - Finding similar vulnerabilities across the codebase - Creating comprehensive vulnerability reports --- ## Section 1: Differential Review Analyze differences between two versions of a MultiversX codebase, focusing on security implications of changes, storage layout compatibility, and upgrade safety. ### 1.1 Upgradeability Checks (MultiversX-Specific) #### Storage Layout Compatibility **CRITICAL**: Storage layout changes can corrupt existing data. ##### Struct Field Ordering ```rust // v1 - Original struct #[derive(TopEncode, TopDecode, TypeAbi)] pub struct UserData { pub balance: BigUint, // Offset 0 pub last_claim: u64, // Offset 1 } // v2 - DANGEROUS: Reordered fields pub struct UserData { pub last_claim: u64, // Now at Offset 0 - BREAKS EXISTING DATA pub balance: BigUint, // Now at Offset 1 - CORRUPTED } // v2 - SAFE: Append new fields only pub struct UserData { pub balance: BigUint, // Offset 0 - unchanged pub last_claim: u64, // Offset 1 - unchanged pub new_field: bool, // Offset 2 - NEW (safe) } ``` ##### Storage Mapper Key Changes ```rust // v1 #[storage_mapper("user_balance")] fn user_balance(&self, user: &ManagedAddress) -> SingleValueMapper; // v2 - DANGEROUS: Changed storage key #[storage_mapper("userBalance")] // Different key = new empty storage! fn user_balance(&self, user: &ManagedAddress) -> SingleValueMapper; ``` #### Initialization on Upgrade **CRITICAL**: `#[init]` is NOT called on upgrade. Only `#[upgrade]` runs. ```rust // v2 - Added new storage mapper #[storage_mapper("newFeatureEnabled")] fn new_feature_enabled(&self) -> SingleValueMapper; // WRONG: Assuming init runs #[init] fn init(&self) { self.config().set(DefaultConfig::new()); self.new_feature_enabled().set(true); // Never runs on upgrade! } // CORRECT: Initialize in upgrade #[upgrade] fn upgrade(&self) { self.new_feature_enabled().set(true); // Properly initializes } ``` #### Breaking Changes Checklist | Change Type | Risk | Mitigation | |------------|------|------------| | Struct field reorder | Critical | Never reorder, only append | | Storage key rename | Critical | Keep old key, migrate data | | New required storage | High | Initialize in `#[upgrade]` | | Removed endpoint | Medium | Ensure no external dependencies | | Changed endpoint signature | Medium | Version API or maintain compatibility | | New validation rules | Medium | Consider existing state validity | ### 1.2 Regression Analysis #### New Features Impact - Do new features break existing invariants? - Are there new attack vectors introduced? - Do gas costs change significantly? #### Deleted Code Analysis When code is removed, verify: - Was this an intentional security fix? - Was a validation check removed (potential vulnerability)? - Are there other code paths that depended on this? ```rust // v1 - Had balance check fn withdraw(&self, amount: BigUint) { require!(amount <= self.balance().get(), "Insufficient balance"); // ... withdrawal logic } // v2 - Check removed - WHY? fn withdraw(&self, amount: BigUint) { // Missing balance check! Was this intentional? // ... withdrawal logic } ``` #### Modified Logic Analysis For changed code, verify: - Edge cases still handled correctly - Error messages updated appropriately - Related code paths updated consistently ### 1.3 Review Workflow #### Step 1: Generate Clean Diff ```bash # Between git tags/commits git diff v1.0.0..v2.0.0 -- src/ # Ignore formatting changes git diff -w v1.0.0..v2.0.0 -- src/ # Focus on specific file git diff v1.0.0..v2.0.0 -- src/lib.rs ``` #### Step 2: Categorize Changes ```markdown ## Change Summary ### Storage Changes - [ ] user_data struct: Added `reward_multiplier` field (SAFE - appended) - [ ] New mapper: `feature_flags` (VERIFY: initialized in upgrade) ### Endpoint Changes - [ ] deposit(): Added token validation (SECURITY FIX) - [ ] withdraw(): Changed gas calculation (VERIFY: no DoS vector) ### Removed Code - [ ] legacy_claim(): Removed entire endpoint (VERIFY: no external callers) ### New Code - [ ] batch_transfer(): New endpoint (FULL REVIEW NEEDED) ``` #### Step 3: Trace Data Flow For each changed data structure: 1. Find all read locations 2. Find all write locations 3. Verify consistency across changes #### Step 4: Verify Test Coverage ```bash # Check if new code paths are tested sc-meta test # Generate test coverage report cargo tarpaulin --out Html ``` ### 1.4 Security-Specific Diff Checks #### Access Control Changes ```rust // v1 - Owner only #[only_owner] #[endpoint] fn sensitive_action(&self) { } // v2 - DANGEROUS: Removed access control #[endpoint] // Now public! Was this intentional? fn sensitive_action(&self) { } ``` #### Payment Handling Changes ```rust // v1 - Validated token #[payable] fn deposit(&self) { let payment = self.call_value().single(); require!(payment.token_identifier == self.accepted_token().get(), "Wrong token"); } // v2 - DANGEROUS: Removed validation #[payable] fn deposit(&self) { let payment = self.call_value().single(); // Missing token validation! Accepts any token now } ``` #### Arithmetic Changes ```rust // v1 - Safe arithmetic let result = a.checked_add(&b).unwrap_or_else(|| sc_panic!("Overflow")); // v2 - DANGEROUS: Removed overflow protection let result = a + b; // Can overflow! ``` ### 1.5 Differential Review Output Template ```markdown # Differential Review Report **Versions Compared**: v1.0.0 → v2.0.0 **Reviewer**: [Name] **Date**: [Date] ## Summary [One paragraph overview of changes] ## Critical Findings 1. [Finding with severity and recommendation] ## Storage Compatibility - [ ] No struct field reordering - [ ] New mappers initialized in #[upgrade] - [ ] Storage keys unchanged ## Breaking Changes | Change | Impact | Migration Required | |--------|--------|-------------------| | ... | ... | ... | ## Recommendations 1. [Specific actionable recommendation] ``` ### Common Pitfalls - **Assuming init runs on upgrade**: Always check `#[upgrade]` function - **Missing storage migration**: Renamed keys lose existing data - **Removed validations**: Could be intentional security fix or accidental vulnerability - **Changed math precision**: Can affect existing calculations - **Modified access control**: Could expose sensitive functions --- ## Section 2: Fix Verification Rigorously verify that a reported vulnerability has been eliminated without introducing regressions or new issues. ### 2.1 The Verification Loop #### Step 1: Reproduce the Bug Create a test scenario that demonstrates the vulnerability: ```json // scenarios/exploit_before_fix.scen.json { "name": "Demonstrate vulnerability - should fail before fix", "steps": [ { "step": "scCall", "comment": "Attacker exploits the vulnerability", "tx": { "from": "address:attacker", "to": "sc:vulnerable_contract", "function": "vulnerable_endpoint", "arguments": ["...exploit_payload..."], "gasLimit": "5,000,000" }, "expect": { "status": "0", "message": "*" } } ] } ``` #### Step 2: Apply the Fix Review the code modification that addresses the vulnerability. #### Step 3: Verify Fix Effectiveness Run the exploit scenario — it MUST now fail (or behave correctly): ```bash # The exploit scenario should now pass (exploit blocked) sc-meta test --scenario scenarios/exploit_before_fix.scen.json ``` #### Step 4: Run Regression Suite ALL existing tests must still pass: ```bash # Full test suite sc-meta test # Or with cargo cargo test ``` ### 2.2 Common Fix Failures #### Partial Fix The fix addresses one path but misses variants: ```rust // VULNERABILITY: Missing amount validation #[endpoint] fn deposit(&self) { let amount = self.call_value().egld(); // No check for amount > 0 } // PARTIAL FIX: Only fixed deposit, not transfer #[endpoint] fn deposit(&self) { let amount = self.call_value().egld(); require!(amount > 0, "Amount must be positive"); // Fixed! } #[endpoint] fn transfer(&self, amount: BigUint) { // Still missing amount > 0 check! <- VARIANT NOT FIXED } ``` **Verification**: Use variant analysis (Section 3) to find all similar code paths. #### Moved Bug (Fix Creates New Issue) ```rust // VULNERABILITY: Reentrancy #[endpoint] fn withdraw(&self) { let balance = self.balance().get(); self.tx().to(&caller).egld(&balance).transfer(); // External call before state update self.balance().clear(); } // BAD FIX: Prevents reentrancy but creates DoS #[endpoint] fn withdraw(&self) { self.locked().set(true); // Lock added let balance = self.balance().get(); self.tx().to(&caller).egld(&balance).transfer(); self.balance().clear(); // Missing: self.locked().set(false); <- LOCK NEVER RELEASED! } // CORRECT FIX: Checks-Effects-Interactions pattern #[endpoint] fn withdraw(&self) { let balance = self.balance().get(); self.balance().clear(); // State update BEFORE external call self.tx().to(&caller).egld(&balance).transfer(); } ``` #### Incomplete Validation ```rust // VULNERABILITY: Integer overflow let total = amount1 + amount2; // Can overflow // INCOMPLETE FIX: Checks one but not both require!(amount1 < MAX_AMOUNT, "Amount1 too large"); let total = amount1 + amount2; // Still overflows if amount2 is large! // CORRECT FIX: BigUint is arbitrary precision (no overflow), but validate bounds let total = &amount1 + &amount2; require!(total <= BigUint::from(MAX_ALLOWED), "Amount exceeds maximum"); ``` ### 2.3 Verification Checklist #### Code Review - [ ] Fix addresses the root cause, not just symptoms - [ ] All code paths with similar patterns are fixed (variant analysis) - [ ] No new vulnerabilities introduced by the fix - [ ] Fix follows MultiversX best practices #### Testing - [ ] Exploit scenario created that fails on vulnerable code - [ ] Exploit scenario passes (blocked) on fixed code - [ ] All existing tests pass (no regressions) - [ ] Edge cases tested (boundary values, empty inputs, max values) #### Documentation - [ ] Fix commit clearly describes the vulnerability - [ ] Test scenario documents the attack vector - [ ] Any behavioral changes documented ### 2.4 Test Scenario Template ```json { "name": "Verify fix for [VULNERABILITY_ID]", "comment": "This scenario verifies that [DESCRIPTION] is properly fixed", "steps": [ { "step": "setState", "comment": "Setup vulnerable state", "accounts": { "address:attacker": { "nonce": "0", "balance": "1000" }, "sc:contract": { "code": "file:output/contract.wasm" } } }, { "step": "scCall", "comment": "Attempt exploit - should fail after fix", "tx": { "from": "address:attacker", "to": "sc:contract", "function": "vulnerable_function", "arguments": ["exploit_input"] }, "expect": { "status": "4", "message": "str:Expected error message" } }, { "step": "checkState", "comment": "Verify state unchanged (exploit blocked)", "accounts": { "sc:contract": { "storage": { "str:sensitive_value": "original_value" } } } } ] } ``` ### 2.5 Verification Report Template ```markdown # Fix Verification Report ## Vulnerability Reference - **ID**: [CVE/Internal ID] - **Severity**: [Critical/High/Medium/Low] - **Description**: [Brief description] ## Fix Details - **Commit**: [git commit hash] - **Files Changed**: [list of files] - **Approach**: [Description of fix approach] ## Verification Results ### Exploit Reproduction - [ ] Exploit scenario created: `scenarios/[name].scen.json` - [ ] Scenario fails on vulnerable code (commit: [hash]) - [ ] Scenario passes on fixed code (commit: [hash]) ### Regression Testing - [ ] All existing tests pass - [ ] No new warnings from `cargo clippy` - [ ] Gas costs within acceptable range ### Variant Analysis - [ ] Searched for similar patterns using variant analysis - [ ] All variants addressed: [list or "none found"] ## Conclusion **Status**: [VERIFIED / NEEDS WORK / REJECTED] **Notes**: [Any additional observations] **Signed**: [Reviewer name, date] ``` ### 2.6 Red Flags During Verification - Fix is overly complex for the issue - Fix changes unrelated code - No test added for the specific vulnerability - Fix relies on external assumptions - Gas cost increased significantly - Access control modified without clear justification --- ## Section 3: Variant Analysis Multiply the value of a single vulnerability finding by systematically locating similar issues elsewhere in the codebase. ### 3.1 The Variant Analysis Process ``` 1. Find Initial Bug → Specific vulnerability instance 2. Abstract Pattern → What makes this a bug? 3. Create Search → Grep/Semgrep queries 4. Find Variants → All similar occurrences 5. Verify Each → Confirm true positives 6. Report All → Document the bug class ``` ### 3.2 Common MultiversX Variant Patterns #### Pattern: Missing Payment Validation **Variant Search:** ```bash # Find all payable endpoints grep -rn "#\[payable" src/ # Check for missing token validation grep -A 30 "#\[payable" src/*.rs > payable_endpoints.txt # Manually review each for token_identifier validation ``` **Semgrep Rule:** ```yaml rules: - id: mvx-payable-no-token-check patterns: - pattern: | #[payable] $ANNOTATIONS fn $FUNC(&self, $...PARAMS) { $...BODY } - pattern-not: | #[payable] $ANNOTATIONS fn $FUNC(&self, $...PARAMS) { <... token_identifier ...> } ``` #### Pattern: Unbounded Iteration ```bash # Find all .iter() calls on storage mappers grep -rn "\.iter()" src/ # Find all for loops over storage grep -rn "for.*in.*self\." src/ ``` **Checklist for Each:** - [ ] Is iteration bounded? - [ ] Can a user grow the collection? - [ ] Is there pagination? #### Pattern: Callback State Assumptions ```bash # Find all callbacks grep -rn "#\[callback\]" src/ # Check for proper result handling grep -A 20 "#\[callback\]" src/*.rs | grep -c "ManagedAsyncCallResult" ``` **All Callbacks Need:** ```rust #[callback] fn any_callback(&self, #[call_result] result: ManagedAsyncCallResult) { match result { ManagedAsyncCallResult::Ok(_) => { /* success */ }, ManagedAsyncCallResult::Err(_) => { /* handle failure! */ } } } ``` #### Pattern: Missing Access Control ```bash # Find functions that modify admin-like storage grep -rn "admin\|owner\|config\|fee" src/ | grep "\.set(" # Cross-reference with access control grep -B 10 "admin.*\.set\|config.*\.set" src/*.rs | grep -v "only_owner" ``` #### Pattern: Arithmetic Without Checks ```bash # Find all arithmetic operations grep -rn " + \| - \| \* " src/*.rs # Exclude test files and comments grep -rn " + \| - \| \* " src/*.rs | grep -v "test\|//" ``` ### 3.3 Systematic Variant Hunting #### Step 1: Characterize the Bug Answer these questions: 1. What is the vulnerable code pattern? 2. What makes it exploitable? 3. What would a fix look like? #### Step 2: Create Detection Queries **Grep-based:** ```bash # Pattern: [specific code pattern] grep -rn "[pattern]" src/ # Negative pattern (should be present but isn't) grep -L "[expected_pattern]" src/*.rs ``` **Semgrep-based:** ```yaml rules: - id: variant-pattern patterns: - pattern: - pattern-not: ``` #### Step 3: Triage Results | Result | Classification | Action | |--------|----------------|--------| | Clearly vulnerable | True Positive | Report | | Needs context | Investigate | Manual review | | Has mitigation | False Positive | Document why | | Different pattern | Not a variant | Skip | #### Step 4: Document Findings ```markdown ## Variant Analysis: [Bug Class Name] ### Initial Finding - Location: [file:line] - Description: [what's wrong] ### Pattern Description [Abstract description of what makes this a bug] ### Search Method ```bash [grep/semgrep commands used] ``` ### Variants Found | Location | Status | Notes | |----------|--------|-------| | file1.rs:23 | Confirmed | Same pattern | | file2.rs:45 | Confirmed | Slight variation | | file3.rs:67 | FP | Has validation elsewhere | ### Remediation [How to fix all instances] ``` ### 3.4 Automation for Future Prevention #### Convert to CI/CD Check ```yaml # .github/workflows/security.yml - name: Check for vulnerability patterns run: | # Run semgrep with custom rules semgrep --config rules/mvx-security.yaml src/ # Grep-based checks if grep -rn "unsafe_pattern" src/; then echo "Found potential vulnerability" exit 1 fi ``` ### 3.5 Variant Analysis Checklist After finding any bug: - [ ] Abstract the pattern (what makes it a bug?) - [ ] Create search queries (grep, semgrep) - [ ] Search entire codebase - [ ] Triage each result (TP/FP/needs investigation) - [ ] Verify true positives are exploitable - [ ] Document all variants - [ ] Create prevention rule for CI/CD - [ ] Recommend fix for all instances ### 3.6 Common Variant Categories #### Input Validation Variants - Missing in one endpoint → Check ALL endpoints - Missing for one parameter → Check ALL parameters #### Access Control Variants - Missing on one admin function → Check ALL admin functions - Inconsistent role checks → Audit entire role system #### State Management Variants - Reentrancy in one function → Check ALL external calls - Missing callback handling → Check ALL callbacks #### Arithmetic Variants - Overflow in one calculation → Check ALL math operations - Precision loss in one formula → Check ALL division operations ### 3.7 Reporting Multiple Variants #### Consolidated Report ```markdown # Bug Class: [Name] ## Summary Found [N] instances of [bug description] across the codebase. ## Root Cause [Why this pattern is vulnerable] ## Instances ### Instance 1 (file1.rs:23) [Details] ### Instance 2 (file2.rs:45) [Details] ## Recommended Fix [Generic fix pattern] ## Prevention [How to prevent this class of bugs in the future] ``` #### Severity Aggregation | Individual Severity | Count | Aggregate Severity | |---------------------|-------|-------------------| | Critical | 3+ | Critical | | High | 5+ | Critical | | Medium | 10+ | High | | Low | Any | Low | ### 3.8 Example: Complete Variant Analysis **Initial Bug: Missing amount validation in stake()** ```rust // Found in stake.rs:45 #[payable("EGLD")] fn stake(&self) { let payment = self.call_value().egld(); // Bug: No check for amount > 0 self.staked().update(|s| *s += payment.amount.as_big_uint()); } ``` **Pattern: Missing amount > 0 check on payable endpoint** > **Note**: With SDK v0.64+ Payment type, `amount` is `NonZeroBigUint` — the `> 0` check is redundant for typed payments. The pattern below applies to older SDK versions or raw `call_value()` usage. **Search:** ```bash grep -rn "#\[payable" src/ | cut -d: -f1 | sort -u | while read file; do echo "=== $file ===" grep -A 30 "#\[payable" "$file" | head -40 done > payable_review.txt ``` **Variants Found:** 1. `stake.rs:45` - stake() - CONFIRMED 2. `stake.rs:78` - add_stake() - CONFIRMED 3. `rewards.rs:23` - deposit_rewards() - CONFIRMED 4. `fees.rs:12` - pay_fee() - FALSE POSITIVE (has check on line 15) **Fix Applied to All:** ```rust #[payable("EGLD")] fn stake(&self) { let payment = self.call_value().egld(); require!(payment.amount.as_big_uint() > 0, "Amount must be positive"); self.staked().update(|s| *s += payment.amount.as_big_uint()); } ``` **CI Rule Created:** `rules/mvx-amount-validation.yaml`