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

Storage Layout Collision Between LevelOne and LevelTwo

teoslaf

Summary

LevelTwo has an incompatible storage layout with LevelOne, causing a severe storage collision that would corrupt contract state during an upgrade. Specifically, LevelTwo's sessionEnd variable would overwrite the same storage slot used by LevelOne's schoolFees.

Vulnerability Details

When upgrading contracts using the UUPS pattern, maintaining the exact same storage layout is critical. The LevelOne and LevelTwo contracts have mismatched storage layouts:

LevelOne storage layout:

address principal; // Slot 0
bool inSession; // Slot 1
uint256 schoolFees; // Slot 2
uint256 immutable reviewTime; // Not in storage (immutable)
uint256 public sessionEnd; // Slot 3
uint256 public bursary; // Slot 4
// ... additional variables
mapping(address => uint256) private reviewCount; // Missing in LevelTwo
mapping(address => uint256) private lastReviewTime; // Missing in LevelTwo

LevelTwo storage layout:

address principal; // Slot 0
bool inSession; // Slot 1
uint256 public sessionEnd; // Slot 2 - COLLISION with schoolFees
uint256 public bursary; // Slot 3 - COLLISION with sessionEnd
// ... additional variables

This causes two critical problems:

  1. sessionEnd in LevelTwo would read from the storage slot containing schoolFees in LevelOne

  2. bursary in LevelTwo would read from the slot containing sessionEnd in LevelOne

The result is a "shifted" storage layout where all variables after schoolFees would be corrupted after an upgrade.

Impact

If an upgrade from LevelOne to LevelTwo were to occur:

  1. Contract state would be corrupted, with sessionEnd reading the incorrect value (previously schoolFees)

  2. The bursary variable would contain the old sessionEnd value, causing financial calculations to be incorrect

  3. All subsequent state variables would be similarly corrupted

  4. The entire behavior of the contract would be unpredictable after upgrade

  5. Financial operations would be compromised, potentially leading to fund loss

Tools Used

  • Manual storage layout analysis

  • Contract comparison

  • Storage slot tracing

Recommendations

Fix the storage layout in LevelTwo to exactly match LevelOne by preserving all state variables in the same order, even if they're unused:

contract LevelTwo is Initializable, UUPSUpgradeable {
using SafeERC20 for IERC20;
​
address principal;
bool inSession;
uint256 schoolFees; // Must keep this even if unused
uint256 public immutable reviewTime = 1 weeks; // Immutable (doesn't affect storage layout)
uint256 public sessionEnd; // Now correctly in slot 3
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; // Must keep this even if unused
mapping(address => uint256) private lastReviewTime; // Must keep this even if unused
address[] listOfStudents;
address[] listOfTeachers;
// ... rest of the contract
}

This ensures that all storage slots remain in the exact same positions after the upgrade, preventing any data corruption.

Updates

Lead Judging Commences

yeahchibyke Lead Judge 7 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.

Give us feedback!