multiversx-diff-review

• 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

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

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

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

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

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

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

```

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!

```

```markdown

# Differential Review Report

Versions Compared: v1.0.0 → v2.0.0

Reviewer: [Name]

Date: [Date]

[One paragraph overview of changes]

1. [Finding with severity and recommendation]

• [ ] No struct field reordering
• [ ] New mappers initialized in #[upgrade]
• [ ] Storage keys unchanged

| Change | Impact | Migration Required |

|--------|--------|-------------------|

| ... | ... | ... |

1. [Specific actionable recommendation]

```

• 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