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

H-04: `graduateAndUpgrade` Missing Crucial Invariant Checks Before Upgrade

albert

Summary

The graduateAndUpgrade function fails to enforce several critical invariants before processing wages and attempting an upgrade:

  1. It does not check if the school session has actually ended (block.timestamp >= sessionEnd).

  2. It does not verify that all students have received the required 4 reviews.

  3. It lacks logic to prevent students who don't meet the cutOffScore from being considered "graduated" or carried over.

Vulnerability Details

The README specifies these invariants:

  • "A school session lasts 4 weeks" and "System upgrade cannot take place unless school's sessionEnd has reached". The function graduateAndUpgrade does not check block.timestamp >= sessionEnd.

  • "Students must have gotten all reviews before system upgrade. System upgrade should not occur if any student has not gotten 4 reviews". The function does not iterate through listOfStudents to check reviewCount[student] == 4.

  • "Any student who doesn't meet the cutOffScore should not be upgraded". The function does not filter listOfStudents or pass information to LevelTwo to handle this.

Impact

  1. Premature Upgrade/Payout: Wages can be paid and an upgrade attempted before the 4-week session is over.

  2. Incomplete Reviews: The system can be upgraded even if students haven't received all their reviews, violating a core academic rule.

  3. Ungraduated Students Carried Over: Students failing to meet the cutOffScore might be treated as graduated and carried over to LevelTwo by default due to shared storage, undermining the academic integrity of the system.

Tools Used

Manual Review, Comparison with README specifications.

Recommendations

Implement these checks at the beginning of graduateAndUpgrade:

  1. Add require(block.timestamp >= sessionEnd, "HH__SessionNotEnded");.

  2. Iterate through listOfStudents and verify reviewCount[student] == 4 (requires H-05 and M-01 to be fixed).

  3. Ensure students not meeting cutOffScore are not considered "upgraded". This is best handled by LevelTwo's reinitializer by filtering the listOfStudents it inherits based on studentScore and the cutOffScore from LevelOne. LevelOne should pass the cutOffScore to LevelTwo's reinitializer.

Consolidated Code Modification for LevelOne.sol::graduateAndUpgrade (addressing H-02, H-03, H-04, L-03):

// src/LevelOne.sol
// ... (other parts of the contract) ...
​
function graduateAndUpgrade(address _levelTwo, bytes memory dataForLevelTwoInitialize) public onlyPrincipal {
if (_levelTwo == address(0)) {
revert HH__ZeroAddress();
}
​
// --- START OF MODIFICATION FOR H-04 (Invariant Checks) ---
require(inSession, "HH__NotInSession"); // Added: Ensure session was started
require(block.timestamp >= sessionEnd, "HH__SessionNotEnded");
​
for (uint256 i = 0; i < listOfStudents.length; i++) {
address student = listOfStudents[i];
// Assuming H-05 (reviewCount increment) and M-01 (review limit to 4) are fixed.
require(reviewCount[student] == 4, "HH__StudentReviewIncomplete");
}
// Note: The filtering of students who don't meet cutOffScore is handled
// by LevelTwo's reinitializer using the inherited studentScore and cutOffScore.
// LevelOne must pass its cutOffScore to LevelTwo.
// The `dataForLevelTwoInitialize` should include this.
// --- END OF MODIFICATION FOR H-04 ---
​
uint256 totalTeachers = listOfTeachers.length;
uint256 currentBursary = bursary; // Use a temporary variable for calculations
uint256 principalPay = (currentBursary * PRINCIPAL_WAGE) / PRECISION;
​
// --- START OF MODIFICATION FOR H-02 (Correct Teacher Wage Calculation) ---
uint256 totalTeacherShare = (currentBursary * TEACHER_WAGE) / PRECISION;
uint256 payPerTeacher = 0;
if (totalTeachers > 0) {
payPerTeacher = totalTeacherShare / totalTeachers;
}
// --- END OF MODIFICATION FOR H-02 ---
​
// --- START OF MODIFICATION FOR L-03 (Update bursary state variable) ---
// This reflects that 40% (35% teachers + 5% principal) is paid out.
// The remaining 60% stays in the bursary.
bursary = currentBursary - principalPay - totalTeacherShare;
// --- END OF MODIFICATION FOR L-03 ---
​
// Pay teachers
// If payPerTeacher is 0 (no teachers or zero share), loop won't run or transfers 0.
// Dust from division (if totalTeacherShare % totalTeachers != 0) will remain from totalTeacherShare
// and effectively add to the remaining bursary.
for (uint256 n = 0; n < totalTeachers; n++) {
if (payPerTeacher > 0) { // Avoid 0 value transfers if not needed by token
usdc.safeTransfer(listOfTeachers[n], payPerTeacher);
}
}
​
// Pay principal
if (principalPay > 0) { // Avoid 0 value transfers
usdc.safeTransfer(principal, principalPay);
}
// --- START OF MODIFICATION FOR H-03 (Actual Upgrade Call) ---
// _authorizeUpgrade is an internal function and will be called by _upgradeToAndCallUUPS.
// The `onlyPrincipal` modifier on this function already gates who can call it.
// `dataForLevelTwoInitialize` should be abi.encodeWithSignature("reinitializerFunctionName(types...)", args...)
// For example, for LevelTwo's graduate(): abi.encodeWithSignature("graduate(address,address,uint256)", newPrincipal, usdcAddress, currentCutOffScore)
_upgradeToAndCallUUPS(_levelTwo, dataForLevelTwoInitialize, false);
// --- END OF MODIFICATION FOR H-03 ---
​
emit Graduated(_levelTwo); // Event was present in README example, ensure it's emitted
}
​
// _authorizeUpgrade is an override required by UUPSUpgradeable
// It's called by the UUPS mechanism during an upgrade, not directly by us before calling _upgradeToAndCallUUPS.
// The onlyPrincipal modifier on graduateAndUpgrade already ensures only principal can initiate.
function _authorizeUpgrade(address newImplementation) internal override onlyPrincipal {}
​
// ... (other parts of the contract) ...
Updates

Lead Judging Commences

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

can graduate without session end

`graduateAndUpgrade()` can be called successfully even when the school session has not ended

Support

FAQs

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