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

School system upgrades causes storage collisions and breaks school records

37h3rn17y2

Summary

Storage slots of variables in LevelTwo does not match those of LevelOne. After school system upgrades, LevelTwo implementation contract will attempt to access variables in the wrong storage slot of proxy contract, resulting in storage collision and breaks the school records.

Vulnerability Details

Before the school system upgrades, LevelOne implementation contract has modified state of storage slots in proxy contract in a particular order. In more detail (ref), address, bool and uint256 are packed (if possible) and stored in 32 byte slots based on the order of declaration (excluding constant and immutable variables). arrays stores the array.length to the storage slot at declaration, and array data is stored sequentially starting at slot keccak256(abi.encode(ARRAY_SLOT_DECLARATION)). mapping of key:value pair leaves an empty slot at mapping declaration, with the value stored at keccak256(abi.encode(KEY, SLOT_INDEX_DECLARATION)).

After the school system upgrades, LevelTwo implementation contract does not have the same order of variables as LevelOne contract, leading to LevelTwo contract accessing the wrong storage slot locations of the proxy contract.

Below shows the storage slot layout in the proxy contract, the variables/functions in LevelOne and LevelTwo accessing the corresponding storage slots as well as the storage collision impact. Using two teachers and two students as example for brevity.

Slot LevelOne LevelTwo Collision
0 principal principal
1 inSession inSession
2 schoolFees sessionEnd <@ sessionEnd returns schoolFees (5000e18)
3 sessionEnd bursary <@ bursary returns sessionEnd (block.timestamp + 4 weeks if session started, 0 otherwise)
4 bursary cutOffScore <@ cutOffScore returns bursary
5 cutOffScore isTeacher declaration
6 isTeacher declaration isStudent declaration
7 isStudent declaration studentScore declaration
8 studentScore declaration listOfStudents.length <@ listOfStudents.length returns 0 (listOfStudents returns empty array)
9 reviewCount declaration listOfTeachers.length <@ listOfTeachers.length returns 0 (listOfTeachers returns empty array)
10 lastReviewTime declaration usdc <@ usdc returns 0 address
11 listOfStudents.length
12 listOfTeachers.length
13 usdc
ka(teacher0Addr,5) isTeacher[teacher0] <@ isTeacher[teacherAddr] returns false (teacher is de-registered)
ka(teacher1Addr,5) isTeacher[teacher1] <@ isTeacher[teacherAddr] returns false (teacher is de-registered)
ka(teacher0Addr,6) isTeacher[teacher0] isStudent[teacher0] <@ isStudent[teacherAddr] returns true (teacher registered as student)
ka(teacher1Addr,6) isTeacher[teacher1] isStudent[teacher1] <@ isStudent[teacherAddr] returns true (teacher registered as student)
ka(student0Addr,6) isStudent[student0] <@ isStudent[studentAddr] returns false (student is de-registered)
ka(student0Addr,7) isStudent[student0] studentScore[student0] <@ studentScore[studentAddr] returns 1 (student's score reduced)
ka(student0Addr,8) studentScore[student0]
ka(student0Addr,9) reviewCount[student0]
ka(student0Addr,10) lastReviewTime[student0]
ka(student1Addr,6) isStudent[student1] <@ isStudent[studentAddr] returns false (student is de-registered)
ka(student1Addr,7) isStudent[student1] studentScore[student1] <@ studentScore[studentAddr] returns 1 (student's score reduced)
ka(student1Addr,8) studentScore[student1]
ka(student1Addr,9) reviewCount[student1]
ka(student1Addr,10) lastReviewTime[student1]
ka(11) listOfStudents[0] i.e. student0Addr
ka(11) + 1 listOfStudents[1] i.e. student1Addr
ka(12) listOfTeachers[0] i.e. teacher0Addr
ka(12) + 1 listOfTeachers[1] i.e. teacher1Addr

**ka(params) denotes keccak256(abi.encode(params))

PoC

**Note: minor changes needed to the LevelTwo contract before running PoC to fix upgradeability issue. (_authorizeUpgrade left unprotected only for demonstration purposes)

+ import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
.
+ contract LevelTwo is Initializable, UUPSUpgradeable {
- contract LevelTwo is Initializable {
.
+ function _authorizeUpgrade(address newImplementation) internal override {}

Place the following into LevelOne|AndGraduateTest.t.sol and run

forge test --mt testStorageCollision

function testStorageCollision() public {
// Register teachers and students
_teachersAdded();
_studentsEnrolled();
uint256 totalStudents = levelOneProxy.getTotalStudents();
uint256 L1Bursary = levelOneProxy.bursary();
uint256 L1SessionEnd = levelOneProxy.sessionEnd();
​
// Graduate and Upgrade
levelTwoImplementation = new LevelTwo();
levelTwoImplementationAddress = address(levelTwoImplementation);
bytes memory data = abi.encodeCall(LevelTwo.graduate, ());
vm.prank(principal);
levelOneProxy.upgradeToAndCall(levelTwoImplementationAddress, data);
​
// 1. `LevelTwo::sessionEnd` returns `LevelOne::schoolFees` (5000e18)
assertEq(LevelTwo(proxyAddress).sessionEnd(), schoolFees);
​
// 2. `LevelTwo::bursary` returns `LevelOne::sessionEnd` (block.timestamp + 4 weeks if session started, 0 otherwise)
assertNotEq(LevelTwo(proxyAddress).bursary(), L1Bursary);
assertEq(LevelTwo(proxyAddress).bursary(), L1SessionEnd);
​
// 3. `LevelTwo::cutOffScore` returns `LevelOne::bursary`
assertEq(LevelTwo(proxyAddress).cutOffScore(), L1Bursary);
// 4. `LevelTwo::listOfStudents.length` returns 0 (breaks `LevelTwo::getTotalStudents`)
assertEq(LevelTwo(proxyAddress).getTotalStudents(), 0);
​
// 5. `LevelTwo::listOfStudents` returns empty array (breaks `LevelTwo::getListOfStudents`)
address[] memory emptyArray = new address[](0);
assertEq(LevelTwo(proxyAddress).getListOfStudents(), emptyArray);
// 6. `LevelTwo::listOfTeachers.length` returns 0 (breaks `LevelTwo::getTotalTeachers`)
assertEq(LevelTwo(proxyAddress).getTotalTeachers(), 0);
​
// 7. `LevelTwo::listOfTeachers` returns empty array (breaks `LevelTwo::getListOfTeachers`)
assertEq(LevelTwo(proxyAddress).getListOfTeachers(), emptyArray);
​
// 8. `LevelTwo::usdc` returns 0 address (breaks `LevelTwo::getSchoolFeesToken`)
assertEq(LevelTwo(proxyAddress).getSchoolFeesToken(), address(0));
​
// 9. `LevelTwo::isTeacher[teacherAddr]` returns false (teacher is de-registered)
assertFalse(LevelTwo(proxyAddress).isTeacher(alice));
assertFalse(LevelTwo(proxyAddress).isTeacher(bob));
​
// 10. `LevelTwo::isStudent[teacherAddr]` returns true (teacher registered as student)
assertTrue(LevelTwo(proxyAddress).isStudent(alice));
assertTrue(LevelTwo(proxyAddress).isStudent(bob));
​
// 11. `LevelTwo::isStudent[studentAddr]` returns false (student is de-registered)
assertFalse(LevelTwo(proxyAddress).isStudent(clara));
assertFalse(LevelTwo(proxyAddress).isStudent(dan));
assertFalse(LevelTwo(proxyAddress).isStudent(eli));
assertFalse(LevelTwo(proxyAddress).isStudent(fin));
assertFalse(LevelTwo(proxyAddress).isStudent(grey));
assertFalse(LevelTwo(proxyAddress).isStudent(harriet));
​
// 12. `LevelTwo::studentScore[studentAddr]` returns 1 (student's score reduced)
assertEq(LevelTwo(proxyAddress).studentScore(clara), 1);
assertEq(LevelTwo(proxyAddress).studentScore(dan), 1);
assertEq(LevelTwo(proxyAddress).studentScore(eli), 1);
assertEq(LevelTwo(proxyAddress).studentScore(fin), 1);
assertEq(LevelTwo(proxyAddress).studentScore(grey), 1);
assertEq(LevelTwo(proxyAddress).studentScore(harriet), 1);
}

Impact

Below is the list of impacts after school system upgrade caused by the storage collision

  1. LevelTwo::sessionEnd returns LevelOne::schoolFees (5000e18)

  2. LevelTwo::bursary returns LevelOne::sessionEnd (block.timestamp + 4 weeks if session started, 0 otherwise)

  3. LevelTwo::cutOffScore returns LevelOne::bursary

  4. LevelTwo::listOfStudents.length returns 0 (breaks LevelTwo::getTotalStudents)

  5. LevelTwo::listOfStudents returns empty array (breaks LevelTwo::getListOfStudents)

  6. LevelTwo::listOfTeachers.length returns 0 (breaks LevelTwo::getTotalTeachers)

  7. LevelTwo::listOfTeachers returns empty array (breaks LevelTwo::getListOfTeachers)

  8. LevelTwo::usdc returns 0 address (breaks LevelTwo::getSchoolFeesToken)

  9. LevelTwo::isTeacher[teacherAddr] returns false (teacher is de-registered)

  10. LevelTwo::isStudent[teacherAddr] returns true (teacher registered as student)

  11. LevelTwo::isStudent[studentAddr] returns false (student is de-registered)

  12. LevelTwo::studentScore[studentAddr] returns 1 (student's score reduced)

Impact: High, school records are broken
Likelihood: High, principal will upgrade school system at the end of school session (after 4 weeks)
Severity: High

Tools Used

Manual review

Recommendations

Add dummy variables to fill up storage slots and prevent storage collisions
LevelTwo.sol

address principal;
bool inSession;
+ uint256 dummyVariable1;
uint256 public sessionEnd;
uint256 public bursary;
uint256 public cutOffScore;
mapping(address => bool) public isTeacher;
mapping(address => bool) public isStudent;
mapping(address => uint256) public studentScore;
+ uint256 dummyVariable2;
+ uint256 dummyVariable3;
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;
Updates

Lead Judging Commences

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

storage collision

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

storage collision

Appeal created

37h3rn17y2 Submitter
6 months ago
37h3rn17y2 Submitter
6 months ago
yeahchibyke Lead Judge
6 months ago
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.