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

System can upgrade without students receiving reviews.

5am

Summary

"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)" - There is no check to see if all students have received all 4 reviews in graduateAndUpgrade(). This means that students can graduate without being given a single review. This breaks the protocols core functionality.

Vulnerability Details

The number of reviews a student receives is stored in reviewCount, although this mapping is never checked in graduateAndUpgrade(). This means that a student can graduate even if they only have one review.

This PoC only works if the vulnerablities affecting the storage layout and invalid implementation are fixed, otherwise we can't upgrade the contract and therefore cannot check it's state.

PoC:

function test_system_can_upgrade_without_student_reviews() public {
// Adds 2 teachers to the school
vm.startPrank(principal);
levelOneProxy.addTeacher(alice);
levelOneProxy.addTeacher(bob);
vm.stopPrank();
​
// Enroll a student
vm.startPrank(clara);
usdc.approve(address(levelOneProxy), schoolFees);
levelOneProxy.enroll();
vm.stopPrank();
​
// Warp forward 1 week
vm.warp(block.timestamp + 1 weeks);
​
// 1 teacher leaves a review
vm.startPrank(alice);
levelOneProxy.giveReview(clara, false);
vm.stopPrank();
​
// Principal calls graduateAndUpgrade
vm.startPrank(principal);
levelTwoImplementation = new LevelTwo();
levelTwoImplementationAddress = address(levelTwoImplementation);
bytes memory data = abi.encodeCall(LevelTwo.graduate, ());
levelOneProxy.graduateAndUpgrade(levelTwoImplementationAddress, data);
// Principal performs actual upgrade
// This passes after fixing wrong storage layout and invalid implementation.
levelOneProxy.upgradeToAndCall(levelTwoImplementationAddress, data);
vm.stopPrank();
​
// Check that the student is in the new contract
LevelTwo levelTwoProxy = LevelTwo(proxyAddress);
assert(levelTwoProxy.getTotalStudents() == 1);
}

Impact

When graduateAndUpgrade() is called, all students will graduate regardless of the number of reviews they have received. This means that if it is called before all reviews are given, students without the correct number of reviews will be automatically graduated. This allows students who would have potentially received negative reviews to graduate before they are given. This breaks the protocol's functionality.

Tools Used

Manual review

Recommendations

Require all students to have at least 4 reviews in graduateAndUpgrade().

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("Not all students have been reviewed");
+ }
+ }
​
uint256 totalTeachers = listOfTeachers.length;
​
uint256 payPerTeacher = (bursary * TEACHER_WAGE) / PRECISION;
uint256 principalPay = (bursary * PRINCIPAL_WAGE) / PRECISION;
​
_authorizeUpgrade(_levelTwo);
​
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.

Appeal created

5am Submitter
6 months ago
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.

Support

FAQs

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