Submission Details
Severity:
[H-5] Mixing up storage locations causes storage collisions for every variable after `LevelTwo::inSession`, freezing the protocol
Description: LevelTwo.sol does not have the schoolFees, reviewTime variables changing the storage layout from LevelOne.sol. This causes a storage collision when upgrading the protocol, making it so that the slots of the variables are gonna be different for the ones after LevelTwo::inSession.
Impact: After the upgrade, variables will have different storage slots than before upgrading.
Proof of Concept: A proof of code is not possible since the LevelTwo contract isn't even upgradeable, but if it were also inheriting UUPSUpgradable from OpenZeppelin a PoC would look like this:
function testStorageCollisionAfterUpgrade() public schoolInSession {
levelTwoImplementation = new LevelTwo();
levelTwoImplementationAddress = address(levelTwoImplementation);
bytes memory data = abi.encodeCall(LevelTwo.graduate, ());
uint256 cutOffScoreBefore = levelOneProxy.cutOffScore();
vm.prank(principal);
levelOneProxy.upgradeToAndCall(levelTwoImplementationAddress, data);
uint256 cutOffScoreAfter = levelOneProxy.cutOffScore();
assertNotEq(cutOffScoreBefore, cutOffScoreAfter);
}
Using the forge inspect command on both protocols helps us see the difference in storage slots between the two protocols.
LevelOne.sol storage layout:
╭----------------+-----------------------------+------+--------+-------+---------------------------╮
| 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 |
╰----------------+-----------------------------+------+--------+-------+---------------------------╯
LevelTwo.sol storage layout:
╭----------------+-----------------------------+------+--------+-------+---------------------------╮
| 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 |
╰----------------+-----------------------------+------+--------+-------+---------------------------╯
Recommended Mitigation: Try to keep the order of the storage variables in LevelTwo.sol the same as in LevelOne.sol.
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.