Differential Review

Name: multiversx-diff-review
Rating: 92
Author: multiversx

Analyze differences between two versions of a MultiversX codebase, focusing on security implications of changes, storage layout compatibility, and upgrade safety.

When to Use

•Reviewing pull requests with contract changes
•Auditing upgrade proposals before deployment
•Analyzing differences between deployed code and proposed updates
•Verifying fix implementations don't introduce regressions

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<BigUint>;

// v2 - DANGEROUS: Changed storage key
#[storage_mapper("userBalance")]  // Different key = new empty storage!
fn user_balance(&self, user: &ManagedAddress) -> SingleValueMapper<BigUint>;

Initialization on Upgrade

CRITICAL: #[init] is NOT called on upgrade. Only #[upgrade] runs.

rust

// v1 - Original contract
#[init]
fn init(&self) {
    self.config().set(DefaultConfig::new());
}

// v2 - Added new storage mapper
#[storage_mapper("newFeatureEnabled")]
fn new_feature_enabled(&self) -> SingleValueMapper<bool>;

// 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

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

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

Create a change inventory:

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:

•Find all read locations
•Find all write locations
•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

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_esdt();
    require!(payment.token_identifier == self.accepted_token().get(), "Wrong token");
}

// v2 - DANGEROUS: Removed validation
#[payable("*")]
fn deposit(&self) {
    let payment = self.call_value().single_esdt();
    // 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!

5. Deliverable 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