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

Memory Slot Layout Inconsistency Between LevelOne and LevelTwo Contracts

linxun

Summary

There is a severe memory slot layout inconsistency between the LevelOne and LevelTwo contracts during the upgrade process. The LevelTwo contract is missing several critical state variables that exist in the LevelOne contract, which will cause state variable misalignment after contract upgrade, leading to data reading errors and potential fund loss.

Vulnerability Details

By comparing the state variable layouts of LevelOne.sol and LevelTwo.sol, we discovered that the LevelTwo contract is missing the following variables that exist in LevelOne:

  1. uint256 schoolFees;

  2. uint256 public immutable reviewTime = 1 weeks;

  3. mapping(address => uint256) private reviewCount;

  4. mapping(address => uint256) private lastReviewTime;

The contract uses the UUPS upgradeable proxy pattern for upgrades, but this pattern requires maintaining consistent state variable layouts in the upgraded contract, or it will lead to incorrect interpretation of state data.

In LevelOne.sol, state variables are stored in the following memory slots in order:

// Storage slot order
address principal; // slot 0
bool inSession; // slot 0 (packed after principal)
uint256 schoolFees; // slot 1 ⚠️ Missing in LevelTwo
uint256 public immutable reviewTime = 1 weeks; // slot 2 ⚠️ Missing in LevelTwo (though immutables don't use storage slots)
uint256 public sessionEnd; // slot 3
uint256 public bursary; // slot 4
uint256 public cutOffScore; // slot 5
// Mappings and arrays occupy slots after these

Whereas in LevelTwo.sol, the state variable order is:

// Storage slot order
address principal; // slot 0
bool inSession; // slot 0 (packed after principal)
// Missing schoolFees // slot 1 position vacant
// Missing reviewTime (though it's immutable and doesn't use a storage slot)
uint256 public sessionEnd; // slot 1 (incorrect position, should be slot 3)
uint256 public bursary; // slot 2 (incorrect position, should be slot 4)
uint256 public cutOffScore; // slot 3 (incorrect position, should be slot 5)
// Mappings and arrays occupy slots after these, also misaligned

Impact

When the LevelOne contract is upgraded to the LevelTwo contract through the graduateAndUpgrade function, it will result in the following serious consequences:

  1. The sessionEnd variable will read the original value of schoolFees, causing errors in the session end time.

  2. The bursary variable will read the original value of reviewTime (if it weren't immutable), potentially causing errors in fund allocation calculations.

  3. All subsequent variables will be misread, including the critical cutOffScore and teacher/student identity and score mappings.

  4. The reviewCount and lastReviewTime mapping data will be completely lost and inaccessible after the upgrade.

These issues may lead to:

  • Complete failure of contract business logic

  • Incorrect fund allocation calculations

  • Confusion in teacher and student identity information

  • Failure of the student scoring system

Tools Used

Manual code review

Recommendation

Add the missing variables to the LevelTwo contract to ensure that the state variable layout remains consistent with the LevelOne contract:

contract LevelTwo is Initializable {
using SafeERC20 for IERC20;
​
address principal;
bool inSession;
uint256 schoolFees; // Add this variable
uint256 public immutable reviewTime = 1 weeks; // Add this variable
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; // Add this mapping
mapping(address => uint256) private lastReviewTime; // Add this mapping
address[] listOfStudents;
address[] listOfTeachers;
// Rest of the code...
}

Even if the LevelTwo contract doesn't use these variables, they must be retained to maintain storage layout consistency. If you want to remove these variables, you must use placeholder variables to ensure that storage slots are not offset.

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.