Submission Details
Severity:
Upgrade causes storage layout incompatibility
Summary
When upgrading the function, we don't account for all the storage variables in LevelOne.sol in LevelTwo.sol. This causes inconsistency with the storage slots when we upgrade.
Vulnerability Details
Calling forge inspect src/LevelOne.sol:LevelOne storage-layout shows us the variables and their corresponding storage slots for the LevelOne implementation:
╭----------------+-----------------------------+------+--------+-------+---------------------------╮
| Name | Type | Slot | Offset | Bytes | Contract |
+==================================================================================================+
| principal | address | 0 | 0 | 20 | src/LevelOne.sol:LevelOne |
|----------------+-----------------------------+------+--------+-------+---------------------------|
| inSession | bool | 0 | 20 | 1 | src/LevelOne.sol:LevelOne |
|----------------+-----------------------------+------+--------+-------+---------------------------|
| schoolFees | uint256 | 1 | 0 | 32 | src/LevelOne.sol:LevelOne |
|----------------+-----------------------------+------+--------+-------+---------------------------|
| sessionEnd | uint256 | 2 | 0 | 32 | src/LevelOne.sol:LevelOne |
|----------------+-----------------------------+------+--------+-------+---------------------------|
| bursary | uint256 | 3 | 0 | 32 | src/LevelOne.sol:LevelOne |
|----------------+-----------------------------+------+--------+-------+---------------------------|
| cutOffScore | uint256 | 4 | 0 | 32 | src/LevelOne.sol:LevelOne |
|----------------+-----------------------------+------+--------+-------+---------------------------|
| isTeacher | mapping(address => bool) | 5 | 0 | 32 | src/LevelOne.sol:LevelOne |
|----------------+-----------------------------+------+--------+-------+---------------------------|
| isStudent | mapping(address => bool) | 6 | 0 | 32 | src/LevelOne.sol:LevelOne |
|----------------+-----------------------------+------+--------+-------+---------------------------|
| studentScore | mapping(address => uint256) | 7 | 0 | 32 | src/LevelOne.sol:LevelOne |
|----------------+-----------------------------+------+--------+-------+---------------------------|
| reviewCount | mapping(address => uint256) | 8 | 0 | 32 | src/LevelOne.sol:LevelOne |
|----------------+-----------------------------+------+--------+-------+---------------------------|
| lastReviewTime | mapping(address => uint256) | 9 | 0 | 32 | src/LevelOne.sol:LevelOne |
|----------------+-----------------------------+------+--------+-------+---------------------------|
| listOfStudents | address[] | 10 | 0 | 32 | src/LevelOne.sol:LevelOne |
|----------------+-----------------------------+------+--------+-------+---------------------------|
| listOfTeachers | address[] | 11 | 0 | 32 | src/LevelOne.sol:LevelOne |
|----------------+-----------------------------+------+--------+-------+---------------------------|
| usdc | contract IERC20 | 12 | 0 | 20 | src/LevelOne.sol:LevelOne |
╰----------------+-----------------------------+------+--------+-------+---------------------------╯
We can then do the same for LevelTwo and see that the values have shifted:
╭----------------+-----------------------------+------+--------+-------+---------------------------╮
| Name | Type | Slot | Offset | Bytes | Contract |
+==================================================================================================+
| principal | address | 0 | 0 | 20 | src/LevelTwo.sol:LevelTwo |
|----------------+-----------------------------+------+--------+-------+---------------------------|
| inSession | bool | 0 | 20 | 1 | src/LevelTwo.sol:LevelTwo |
|----------------+-----------------------------+------+--------+-------+---------------------------|
| sessionEnd | uint256 | 1 | 0 | 32 | src/LevelTwo.sol:LevelTwo |
|----------------+-----------------------------+------+--------+-------+---------------------------|
| bursary | uint256 | 2 | 0 | 32 | src/LevelTwo.sol:LevelTwo |
|----------------+-----------------------------+------+--------+-------+---------------------------|
| cutOffScore | uint256 | 3 | 0 | 32 | src/LevelTwo.sol:LevelTwo |
|----------------+-----------------------------+------+--------+-------+---------------------------|
| isTeacher | mapping(address => bool) | 4 | 0 | 32 | src/LevelTwo.sol:LevelTwo |
|----------------+-----------------------------+------+--------+-------+---------------------------|
| isStudent | mapping(address => bool) | 5 | 0 | 32 | src/LevelTwo.sol:LevelTwo |
|----------------+-----------------------------+------+--------+-------+---------------------------|
| studentScore | mapping(address => uint256) | 6 | 0 | 32 | src/LevelTwo.sol:LevelTwo |
|----------------+-----------------------------+------+--------+-------+---------------------------|
| listOfStudents | address[] | 7 | 0 | 32 | src/LevelTwo.sol:LevelTwo |
|----------------+-----------------------------+------+--------+-------+---------------------------|
| listOfTeachers | address[] | 8 | 0 | 32 | src/LevelTwo.sol:LevelTwo |
|----------------+-----------------------------+------+--------+-------+---------------------------|
| usdc | contract IERC20 | 9 | 0 | 20 | src/LevelTwo.sol:LevelTwo |
╰----------------+-----------------------------+------+--------+-------+---------------------------╯
Impact
This will shift all the values in each storage slots i.e: bursary => sessionEnd, cutOffScore => bursary. This changes the value of all the variables and breaks the protocol entirely.
Tools Used
Manual review
Recommendations
contract LevelTwo is Initializable {
using SafeERC20 for IERC20;
+ // --- Variables from LevelOne to maintain storage layout ---
address principal;
bool inSession;
+ uint256 schoolFees; // Kept for storage compatibility
+ // reviewTime was immutable in LevelOne, so no storage slot
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; // Kept for storage compatibility
+ mapping(address => uint256) private lastReviewTime; // Kept for storage compatibility
address[] listOfStudents;
address[] listOfTeachers;
Updates
Lead Judging Commences
yeahchibyke about 2 months ago
Submission Judgement Published Assigned finding tags:
storage collision
Rewards breakdown
nSLOC
243This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.