Summary
The principal can remove teachers at anytime using LevelOne.sol:removeTeacher() function.
Wages are paid automatically during session upgrade (i.e when graduateAndUpgrade is called).
Vulnerability Details
Teachers will do thier work perfectly until the end of the session.
However, since the principal has the freedom of removing the teachers at any time, principal can remove all the teachers just before the session upgrade.
This action leads to unpaid wages of teachers.
Vulnerable Code:
function removeTeacher(address _teacher) public onlyPrincipal {
if (_teacher == address(0)) {
revert HH__ZeroAddress();
}
if (!isTeacher[_teacher]) {
revert HH__TeacherDoesNotExist();
}
uint256 teacherLength = listOfTeachers.length;
for (uint256 n = 0; n < teacherLength; n++) {
if (listOfTeachers[n] == _teacher) {
listOfTeachers[n] = listOfTeachers[teacherLength - 1];
listOfTeachers.pop();
break;
}
}
isTeacher[_teacher] = false;
emit TeacherRemoved(_teacher);
}
Proof of Code :
add this test to LevelOneAndGraduateTest.t.sol and run it
function test_confirm_can_remove_teachers_and_upgrade() public schoolInSession {
vm.warp(block.timestamp + 4 weeks- 5 minutes);
vm.startPrank(principal);
levelOneProxy.removeTeacher(alice);
levelOneProxy.removeTeacher(bob);
vm.stopPrank();
assertEq(levelOneProxy.getTotalTeachers(), 0);
uint256 teacherAliceBalanceBeforeGettingWages = MockUSDC(levelOneProxy.getSchoolFeesToken()).balanceOf(alice);
levelTwoImplementation = new LevelTwo();
levelTwoImplementationAddress = address(levelTwoImplementation);
bytes memory data = abi.encodeCall(LevelTwo.graduate, ());
vm.prank(principal);
levelOneProxy.graduateAndUpgrade(levelTwoImplementationAddress, data);
assert(MockUSDC(levelOneProxy.getSchoolFeesToken()).balanceOf(alice) <= teacherAliceBalanceBeforeGettingWages);
LevelTwo levelTwoProxy = LevelTwo(proxyAddress);
}
This PoC demonstrates that the teachers are unpaid even after the session is upgraded because principal has removed all the teachers just before the session upgrade.
Impact
Tools Used
Manual Analysis
Foundry unit testing
Recommendations
Update the removeTeacher function so that teachers can only be removed before the session starts.
If need more flexibility, allow removal only before the first week of the session.
function removeTeacher(address _teacher) public onlyPrincipal {
if (_teacher == address(0)) {
revert HH__ZeroAddress();
}
if (!isTeacher[_teacher]) {
revert HH__TeacherDoesNotExist();
}
+ //can remove only before 3 weeks of session end / 1st week of the session
+ if(block.timestamp >= (sessionEnd - 3 weeks)){
+ revert("cannot remove teacher at this time");
+ }
uint256 teacherLength = listOfTeachers.length;
for (uint256 n = 0; n < teacherLength; n++) {
if (listOfTeachers[n] == _teacher) {
listOfTeachers[n] = listOfTeachers[teacherLength - 1];
listOfTeachers.pop();
break;
}
}
isTeacher[_teacher] = false;
emit TeacherRemoved(_teacher);
}
