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

Absence of Graduation Logic in `LevelOne::graduateAndUpgrade` Function Causing Broken System Upgrade

rubencrxz

Summary

The LevelOne::graduateAndUpgrade function lacks logic to handle student graduation, which allows the protocol to be upgraded without controlling which students should graduate.

Vulnerability Details

In the LevelOne::graduateAndUpgrade function, there is no code to manage the graduation of students, which leads to an upgrade of the contract without controlling who should graduate. A student that does not have four reviews or that does not pass the cutOffScore mark is not marked as not graduated, which goes against the protocol intentention. Also, the system does not check that the session has ended to update the system.

This breaks the protocol's invariants and omits a crucial function from the system.

Impact

This issue severely breaks the protocol's intended behavior by bypassing a critical function of the system. This will occur every time the system is upgraded.

As will be shown in the Proof of Code, principal actor is able to call LevelOne::graduateAndUpdate function in an undesirable scenario in which the student fin does not outperform LevelOne::cutOffScore, does not have four reviews in LevelOne::reviewCount and in which the session has not ended yet.

Proof of Concept

This test demonstrates a vulnerability that allows the contract upgrade process to proceed even when the necessary conditions for student evaluation are not met. The goal is to bypass validation checks during the session period and confirm that an upgrade can occur undesirably. The steps are as follows:

  1. Increase the cutOffScore to 100 within the schoolInSession modifier in the LevelOneGraduateTest.t.sol contract. This raises the minimum score required for a student to qualify for graduation.

  2. Set the session duration to 1 week, ensuring that the session is still active and has not yet ended (sessionEnd has not occurred).

  3. Give the student fin only 1 negative review, resulting in:

    • Fewer than 4 total reviews (fails the reviewCount invariant)

    • A studentScore lower than the cutOffScore

  4. Trigger the contract upgrade as the principal, and verify that the upgrade proceeds despite the unmet conditions, thus confirming the vulnerability.

Proof of Code

To confirm the vulnerability, apply the following changes in the LevelOneGraduateTest.t.sol contract:

  1. Update the schoolInSession modifier by setting the cutOffScore to 100:

modifier schoolInSession() {
_teachersAdded();
_studentsEnrolled();
​
vm.prank(principal);
- levelOneProxy.startSession(70);
+ levelOneProxy.startSession(100);
​
_;
}

2.Add the following test function to simulate the undesired upgrade:

function test_confirm_can_graduate_without_following_invariants() public schoolInSession {
uint256 currentSessionTime = block.timestamp + 1 weeks;
vm.warp(currentSessionTime);
vm.prank(bob);
levelOneProxy.giveReview(fin, false);
​
console2.log("studentScore: ", levelOneProxy.studentScore(fin));
// levelOneProxy.reviewCount(fin) will return 0 as giveReview does not update reviewCount after being called
console2.log("reviewCount: ", levelOneProxy.reviewCount(fin));
console2.log("cutOffScore: ", levelOneProxy.cutOffScore());
console2.log("sessionEnd: ", levelOneProxy.sessionEnd());
console2.log("Actual Time: ", currentSessionTime);
assertGt(levelOneProxy.cutOffScore(), levelOneProxy.studentScore(fin));
assertGt(levelOneProxy.sessionEnd(), currentSessionTime);
​
levelTwoImplementation = new LevelTwo();
levelTwoImplementationAddress = address(levelTwoImplementation);
​
bytes memory data = abi.encodeCall(LevelTwo.graduate, ());
vm.startPrank(principal);
levelOneProxy.graduateAndUpgrade(levelTwoImplementationAddress, data);
vm.stopPrank();
​
LevelTwo levelTwoProxy = LevelTwo(proxyAddress);
}

Tools Used

The issue was discovered through a manual code review.

Recommendations

It is recommended to split the graduation and upgrade functionalities into two distinct functions for better clarity and security of the code.

  1. In the LevelOne.sol contract, add an array to store all graduated students and a mapping to register whether a student has graduated:

address principal;
bool inSession;
uint256 schoolFees;
uint256 public immutable reviewTime = 1 weeks;
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) public reviewCount;
mapping(address => uint256) public lastReviewTime;
+ mapping(address => bool) public isGraduated; // False by default.
address[] listOfStudents;
address[] listOfTeachers;
+ address[] graduatedStudents;

2.Modify the LevelOne::Graduated event and add an UpgradeToLevelTwo event:

event TeacherAdded(address indexed);
event TeacherRemoved(address indexed);
event Enrolled(address indexed);
event Expelled(address indexed);
event SchoolInSession(uint256 indexed startTime, uint256 indexed endTime);
event ReviewGiven(address indexed student, bool indexed review, uint256 indexed studentScore);
- event Graduated(address indexed levelTwo);
+ event Graduated(address indexed);
+ event UpgradeToLevelTwo(address indexed levelTwo, bytes data);
​

3.Ensure that LevelOne::reviewCount[_student] is updated each time a review is submitted within the LevelOne::giveReview function.
Note: This is an issue which is discussed in detail in a separate submission.

function giveReview(address _student, bool review) public onlyTeacher {
if (!isStudent[_student]) {
revert HH__StudentDoesNotExist();
}
require(reviewCount[_student] < 5, "Student review count exceeded!!!");
require(block.timestamp >= lastReviewTime[_student] + reviewTime, "Reviews can only be given once per week");
// where `false` is a bad review and true is a good review
if (!review) {
studentScore[_student] -= 10;
}
// Update last review time
lastReviewTime[_student] = block.timestamp;
+ // Update review Count
+ reviewCount[_student]++;
​
emit ReviewGiven(_student, review, studentScore[_student]);
}

4.Add a LevelOne::graduateStudent function in the LevelOne.sol contract that adheres to the protocol's invariants:

function graduateStudent(address _student) public onlyPrincipal {
require(block.timestamp >= sessionEnd, "Session has not ended yet");
require(reviewCount[_student] == 4, "Student must have exactly 4 reviews");
require(studentScore[_student] >= cutOffScore, "Score below cut-off");
require(!isGraduated[_student], "Already graduated");
​
isGraduated[_student] = true;
graduatedStudents.push(_student);
​
emit Graduated(_student);
}

5.Update the LevelOne::graduateAndUpgrade function:

It is recommended to modify the LevelOne::graduateAndUpgrade function to call the new function graduateStudent before proceeding with the contract upgrade. This ensures that only students who have completed all the necessary requirements are graduated before the contract upgrade takes place.

By calling graduateStudent, you can ensure that all necessary graduation conditions are met before allowing any changes or upgrades to the contract, preserving the integrity of the system.

After this, the invariants will be checked, and the LevelOne::graduateAndUpgrade function should be updated as follows:

- function graduateAndUpgrade(address _levelTwo, bytes memory ) public onlyPrincipal {
+ function Upgrade(address _levelTwo, bytes memory ) public onlyPrincipal {
if (_levelTwo == address(0)) {
revert HH__ZeroAddress();
}
​
uint256 totalTeachers = listOfTeachers.length;
​
uint256 payPerTeacher = (bursary * TEACHER_WAGE) / PRECISION;
uint256 principalPay = (bursary * PRINCIPAL_WAGE) / PRECISION;
+ event UpgradeToLevelTwo(_levelTwo, "");
_authorizeUpgrade(_levelTwo);
​
for (uint256 n = 0; n < totalTeachers; n++) {
usdc.safeTransfer(listOfTeachers[n], payPerTeacher);
}
​
usdc.safeTransfer(principal, principalPay);
}

Optionally, you can add the new variables to the LevelTwo.sol contract if they will be used in the updated implementation.

If you do, consider maintaining the same variable declaration order as in the LevelOne.sol contract, and ensure that variables from the LevelOne.sol contract are included to avoid storage collisions during the upgrade.

Note: You can check the storage slots by running the following commands in your console:

  • forge inspect LevelOne storage to inspect the storage of LevelOne.sol.

  • forge inspect LevelTwo storage to inspect the storage of LevelTwo.sol.

address principal;
bool inSession;
+ uint256 schoolFees;
+ uint256 private reviewTime = 1 weeks;
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) public reviewCount;
+ mapping(address => uint256) public lastReviewTime;
+ mapping(address => bool) public isGraduated; // False by default.
address[] listOfStudents;
address[] listOfTeachers;
+ address[] graduatedStudents;

Updates

Lead Judging Commences

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

cut-off criteria not applied

All students are graduated when the graduation function is called as the cut-off criteria is not applied.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.