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

`LevelOne:graduateAndUpgrade` Lacks Review Count Check

azriel20005

Description:
When the principal calls LevelOne:graduateAndUpgrade, there are no checks to ensure that all students have gotten 4 reviews.

Impact:
This directly violates the invariant "Students must have gotten all reviews before system upgrade. System upgrade should not occur if any student has not gotten 4 reviews (one for each week)"

Proof of Concept:
Since reviewCount is a private mapping, I changed it to a public mapping temporarily so that I can access the count value. By adding the following test code, we have shown that upgrading is possible even when students have less than 4 (in this case 0) reviews.

function test_upgrade_without_4_reviews() public schoolInSession {
levelTwoImplementation = new LevelTwo();
levelTwoImplementationAddress = address(levelTwoImplementation);
​
bytes memory data = abi.encodeCall(LevelTwo.graduate, ());
​
assertEq(levelOneProxy.reviewCount(harriet), 0);
​
vm.prank(principal);
levelOneProxy.graduateAndUpgrade(levelTwoImplementationAddress, data);
}

Recommended Mitigation:
By including the extra check below, we can ensure that all students have had 4 reviews before allowing the upgrade.

function graduateAndUpgrade(address _levelTwo, bytes memory) public onlyPrincipal {
if (_levelTwo == address(0)) {
revert HH__ZeroAddress();
}
​
++ for (uint256 n = 0; n < listOfStudents.length; n++) {
++ if (reviewCount[listOfStudents[n]] < 4) {
++ revert HH__NotEnoughReviews(); // adding new revert for this error
++ }
++ }
​
uint256 totalTeachers = listOfTeachers.length;
​
// @audit-info: teachers should share the 35% right? this is doing each teacher is getting paid 35%
uint256 payPerTeacher = (bursary * TEACHER_WAGE) / PRECISION;
uint256 principalPay = (bursary * PRINCIPAL_WAGE) / PRECISION;
​
_authorizeUpgrade(_levelTwo);
​
// @audit-info: push vs pull? would it be possible for a revert to occur thus giving DOS?
// possible if one of the addresses are blacklisted!
for (uint256 n = 0; n < totalTeachers; n++) {
usdc.safeTransfer(listOfTeachers[n], payPerTeacher);
}
​
usdc.safeTransfer(principal, principalPay);
}
Updates

Lead Judging Commences

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.

Support

FAQs

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