Summary
graduateAndUpgrade() does not check if the students have all received their 4 reviews before the upgrade is authirzed. The function does not also check if the session was even started and has come to an end so as to upgrade after students have their reviews.
Vulnerability Details
graduateAndUpgrade()can be called by the principal whether or not the session has ended or whether the students have received their 4 reviews or not.
Impact
The contract's invariant is broken because, and a malicious principal can call the graduateAndUpgrade()function to get paid whenever they want.
poc
Looking at the test suite below, it shows that the principal can call graduateAndUpgrade()anytime if students are enrolled without any checks and get paid. this test suite when run in LevelOneAndGraduateTest.t.solpasses.
function test_principalCanCallGraduateAndUpgradeAnytimeAndGetPaid_poc() public {
_teachersAdded();
_studentsEnrolled();
uint256 principalBalanceBefore = usdc.balanceOf(principal);
uint256 aliceBalanceBefore = usdc.balanceOf(alice);
uint256 bobBalanceBefore = usdc.balanceOf(bob);
uint256 bursary = levelOneProxy.bursary();
uint256 totalTeachers = levelOneProxy.getTotalTeachers();
uint256 teacherWagePercentage = levelOneProxy.TEACHER_WAGE();
uint256 principalWagePercentage = levelOneProxy.PRINCIPAL_WAGE();
uint256 precision = levelOneProxy.PRECISION();
uint256 expectedPayPerTeacher = (bursary * teacherWagePercentage) / precision;
uint256 expectedPrincipalPay = (bursary * principalWagePercentage) / precision;
levelTwoImplementation = new LevelTwo();
levelTwoImplementationAddress = address(levelTwoImplementation);
bytes memory data = abi.encodeCall(LevelTwo.graduate, ());
vm.startPrank(principal);
levelOneProxy.graduateAndUpgrade(levelTwoImplementationAddress, data);
LevelTwo levelTwoProxy = LevelTwo(proxyAddress);
vm.stopPrank();
uint256 principalBalanceAfter = usdc.balanceOf(principal);
uint256 aliceBalanceAfter = usdc.balanceOf(alice);
uint256 bobBalanceAfter = usdc.balanceOf(bob);
console2.log("Principal balance before:", principalBalanceBefore);
console2.log("Principal balance after:", principalBalanceAfter);
console2.log("Alice balance before:", aliceBalanceBefore);
console2.log("Alice balance after:", aliceBalanceAfter);
console2.log("Bob balance before:", bobBalanceBefore);
console2.log("Bob balance after:", bobBalanceAfter);
console2.log("Expected pay per teacher:", expectedPayPerTeacher);
console2.log("Expected principal pay:", expectedPrincipalPay);
assert(principalBalanceAfter == principalBalanceBefore + expectedPrincipalPay);
assert(aliceBalanceAfter == aliceBalanceBefore + expectedPayPerTeacher);
assert(bobBalanceAfter == bobBalanceBefore + expectedPayPerTeacher);
}
Tools Used
manual review
Recommendations
Add checks into the function to make sure that the conditions in the invariant is met. consider adding the lines of code shown below for the necessary checks;
+ error HH__CannotUpgradeUntilAllReviewsAreGiven();
function graduateAndUpgrade(address _levelTwo, bytes memory) public onlyPrincipal {
if (_levelTwo == address(0)) {
revert HH__ZeroAddress();
}
+ if (inSession == true) {
+ revert HH__AlreadyInSession();
+ }
+ if (listOfStudents.length == 0) {
+ revert HH__ZeroValue();
+ }
+ if (listOfTeachers.length == 0) {
+ revert HH__ZeroValue();
+ }
+
+ uint256 totalStudents = listOfStudents.length;
+
+ for (uint256 n = 0; n < totalStudents; n++) {
+ if (reviewCount[listOfStudents[n]] != 4) {
+ revert HH__CannotUpgradeUntilAllReviewsAreGiven();
+ }
+ }
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);
}
