Hawk High
First Flight #39
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

Storage Collision Report: Variable Layout Mismatch Between LevelOne and LevelTwo

khalidwalid

Severity: High

Likelihood: High

Summary

The LevelOne and LevelTwo contracts have significantly different storage layouts, with inconsistencies in variable declarations and ordering. Upon upgrading from LevelOne to LevelTwo via the UUPS pattern, these mismatches will cause severe storage collisions where variables in LevelTwo will unintentionally access or overwrite storage slots previously used by different variables in LevelOne, leading to data corruption and critical system failures.

Vulnerability Detail

Storage Layout Principles in Upgradeable Contracts

In Solidity, storage variables are assigned slots sequentially in the order they're declared. When using upgradeable contract patterns like UUPS, the proxy contract's storage layout must be preserved across implementations to maintain data integrity. Any mismatch in variable declarations between implementation versions will cause storage collisions.

Comprehensive Storage Layout Analysis

LevelOne Storage Layout:

// First storage variables in LevelOne
address principal; // slot 0
bool inSession; // slot 1
uint256 schoolFees; // slot 2
uint256 public immutable reviewTime; // Not stored in contract storage (immutable)
uint256 public sessionEnd; // slot 3
uint256 public bursary; // slot 4
uint256 public cutOffScore; // slot 5
mapping(address => bool) public isTeacher; // slot 6
mapping(address => bool) public isStudent; // slot 7
mapping(address => uint256) public studentScore; // slot 8
mapping(address => uint256) private reviewCount; // slot 9
mapping(address => uint256) private lastReviewTime; // slot 10
address[] listOfStudents; // slot 11
address[] listOfTeachers; // slot 12
// Constants don't take storage slots
uint256 public constant TEACHER_WAGE = 35;
uint256 public constant PRINCIPAL_WAGE = 5;
uint256 public constant PRECISION = 100;
IERC20 usdc; // slot 13
​

LevelTwo Storage Layout:

// First storage variables in LevelTwo
address principal; // slot 0 - matches LevelOne
bool inSession; // slot 1 - matches LevelOne
uint256 public sessionEnd; // slot 2 - COLLISION! Was slot 3 in LevelOne
uint256 public bursary; // slot 3 - COLLISION! Was slot 4 in LevelOne
uint256 public cutOffScore; // slot 4 - COLLISION! Was slot 5 in LevelOne
mapping(address => bool) public isTeacher; // slot 5 - COLLISION! Was slot 6 in LevelOne
mapping(address => bool) public isStudent; // slot 6 - COLLISION! Was slot 7 in LevelOne
mapping(address => uint256) public studentScore; // slot 7 - COLLISION! Was slot 8 in LevelOne
address[] listOfStudents; // slot 8 - COLLISION! Was slot 11 in LevelOne
address[] listOfTeachers; // slot 9 - COLLISION! Was slot 12 in LevelOne
// Constants don't take storage slots
uint256 public constant TEACHER_WAGE_L2 = 40;
uint256 public constant PRINCIPAL_WAGE_L2 = 5;
uint256 public constant PRECISION = 100;
IERC20 usdc; // slot 10 - COLLISION! Was slot 13 in LevelOne
​

Critical Storage Conflicts

  1. Missing Variable in LevelTwo: LevelOne has uint256 schoolFees in slot 2, which is completely missing in LevelTwo, causing a cascading collision for all subsequent variables.

  2. Missing Private Mappings: LevelOne's reviewCount and lastReviewTime mappings (slots 9 and 10) are missing in LevelTwo, further exacerbating the misalignment.

  3. Array Position Shifts: The dynamic arrays listOfStudents and listOfTeachers have shifted from slots 11 and 12 in LevelOne to slots 8 and 9 in LevelTwo.

  4. IERC20 Storage Shift: The usdc variable has shifted from slot 13 to slot 10.

Technical Explanation of Impact

When the proxy contract upgrades from LevelOne to LevelTwo:

  1. Data Corruption Chain Reaction:

    • LevelTwo's sessionEnd will read/write to slot 2, which contained schoolFees in LevelOne

    • Every subsequent variable will read/write to incorrect slots, creating a cascading failure

  2. Mapping Collision Examples:

    • LevelTwo's isTeacher will map to slot 5 but access data from slot 6 (LevelOne's isTeacher)

    • LevelTwo's studentScore will map to slot 7 but access data from slot 8 (LevelOne's studentScore)

  3. Dynamic Array Corruption:

    • Arrays in Solidity store their length at the declared slot and data at keccak256(slot)

    • After upgrade, listOfStudents will read length from slot 8 (previously slot 11), likely yielding incorrect data

    • This could potentially expose or corrupt the data previously stored in slot 8 (LevelOne's studentScore)

  4. Deep Storage Implications:

    • Mappings and arrays use sophisticated storage patterns that will be severely corrupted

    • For example, data at keccak256(slot) + key for mappings will be accessing entirely wrong sections of storage

Impact

  1. Complete System Failure: The storage collisions will render LevelTwo's functionality completely broken upon upgrade, as every state variable will access incorrect data.

  2. Financial Loss: Given that this contract deals with:

    • School fees (schoolFees)

    • Treasury funds (bursary)

    • Payment calculations (TEACHER_WAGE, PRINCIPAL_WAGE) The storage collisions will cause financial calculations to use incorrect data, potentially leading to financial losses.

  3. Student Record Corruption:

    • Student status (isStudent)

    • Academic scores (studentScore)

    • Course completion status and review history will all be corrupted

  4. Irreversible Data Loss: Since the upgrade process doesn't preserve data properly, critical information may be permanently lost or corrupted in the transition.

  5. Identity Confusion: Teacher and student identities will be mixed up, potentially giving unauthorized access to teacher-only functions to students and vice versa.

Code Reference

The issue spans both contracts' entire storage layout:

In LevelOne:

contract LevelOne is Initializable, UUPSUpgradeable {
using SafeERC20 for IERC20;
​
address principal;
bool inSession;
uint256 schoolFees; // Missing in LevelTwo
uint256 public immutable reviewTime = 1 weeks; // Immutable - not in storage
uint256 public sessionEnd;
uint256 public bursary;
uint256 public cutOffScore;
mapping(address => bool) public isTeacher;
mapping(address => bool) public isStudent;
mapping(address => uint256) public studentScore;
mapping(address => uint256) private reviewCount; // Missing in LevelTwo
mapping(address => uint256) private lastReviewTime; // Missing in LevelTwo
address[] listOfStudents;
address[] listOfTeachers;
​
uint256 public constant TEACHER_WAGE = 35;
uint256 public constant PRINCIPAL_WAGE = 5;
uint256 public constant PRECISION = 100;
​
IERC20 usdc;
}
​

In LevelTwo:

contract LevelTwo is Initializable {
using SafeERC20 for IERC20;
address principal;
bool inSession;
// Missing: uint256 schoolFees; - Critical omission!
uint256 public sessionEnd;
uint256 public bursary;
uint256 public cutOffScore;
mapping(address => bool) public isTeacher;
mapping(address => bool) public isStudent;
mapping(address => uint256) public studentScore;
// Missing: reviewCount and lastReviewTime mappings
address[] listOfStudents;
address[] listOfTeachers;
uint256 public constant TEACHER_WAGE_L2 = 40;
uint256 public constant PRINCIPAL_WAGE_L2 = 5;
uint256 public constant PRECISION = 100;
IERC20 usdc;
}
​

Recommendation

  1. Match Storage Layout Exactly: Ensure LevelTwo declares all variables from LevelOne in the exact same order, even if some aren't used in LevelTwo's logic:

contract LevelTwo is Initializable {
using SafeERC20 for IERC20;
address principal;
bool inSession;
uint256 schoolFees; // Preserved for storage compatibility
uint256 public immutable reviewTime = 1 weeks; // Immutable - doesn't affect storage
uint256 public sessionEnd;
uint256 public bursary;
uint256 public cutOffScore;
mapping(address => bool) public isTeacher;
mapping(address => bool) public isStudent;
mapping(address => uint256) public studentScore;
mapping(address => uint256) private reviewCount; // Preserved for storage compatibility
mapping(address => uint256) private lastReviewTime; // Preserved for storage compatibility
address[] listOfStudents;
address[] listOfTeachers;
uint256 public constant TEACHER_WAGE_L2 = 40; // Constants don't affect storage
uint256 public constant PRINCIPAL_WAGE_L2 = 5; // Constants don't affect storage
uint256 public constant PRECISION = 100; // Constants don't affect storage
IERC20 usdc;
}
​

  1. Use Storage Gaps: Implement storage gaps in both contracts to accommodate future additions without disrupting the layout:

// Add to both contracts
uint256[50] private __gap;
​

  1. Follow OpenZeppelin's Storage Upgrade Pattern: Use the structured approach to storage management from OpenZeppelin's upgradeable contracts library.

  2. Implement Storage Layouts Diagram: Maintain a diagram or documentation showing the exact storage layout of both contracts to prevent accidental mismatches in future upgrades.

Updates

Lead Judging Commences

yeahchibyke Lead Judge 6 months ago
Submission Judgement Published
Validated
Assigned finding tags:

storage collision

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.