Hawk High
First Flight #39
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Impact: high
Likelihood: medium
Invalid

Inconsistent authorization in LevelOne::addTeacher and LevelOne::removeTeacher potentially leading to bricked contract

bavoen

Summary

The LevelOne::addTeacher is protected by the notYetInSession modifier, while the LevelOne::removeTeacher is not. This means that we can not add teachers when the school is in session but we can remove them. This can lead to a bricked contract if all teachers are removed from the contract.

Vulnerability Details

Since teachers can be removed and not added this can lead to a situation where there are no teachers left in the contract, which would make it impossible to give reviews to students. The school session can only be ended when all students have received 4 reviews. This means that if all teachers are removed from the contract, it will be impossible to give reviews to students and the school session can not be ended. This would lead to a bricked contract.

The relevant code is located in the LevelOne contract:

function addTeacher(address _teacher) public onlyPrincipal notYetInSession {
if (_teacher == address(0)) {
revert HH__ZeroAddress();
}
​
if (isTeacher[_teacher]) {
revert HH__TeacherExists();
}
​
if (isStudent[_teacher]) {
revert HH__NotAllowed();
}
​
listOfTeachers.push(_teacher);
isTeacher[_teacher] = true;
​
emit TeacherAdded(_teacher);
}
​
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);
}

Impact

Altough this would only be a problem if the principal would remove all teachers from the contract (by accident or on purpose) this is still a problem since there is no way to recover from this situation. This would lead to a bricked contract since the school session can not be ended and all students and coins would be stuck in the contract.

Tools Used

Manually reviewed the code.

Recommendations

It is recommended to add the notYetInSession modifier to the removeTeacher function to ensure that teachers cannot be removed while the school is in session. This will prevent the potential bricking of the contract by ensuring that at least one teacher remains in the contract during an active session.

-function removeTeacher(address _teacher) public onlyPrincipal {
+function removeTeacher(address _teacher) public onlyPrincipal notYetInSession {
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);
}
Updates

Lead Judging Commences

yeahchibyke Lead Judge 6 months ago
Submission Judgement Published
Invalidated
Reason: Design choice

Appeal created

bavoen Submitter
6 months ago
yeahchibyke Lead Judge
6 months ago
yeahchibyke Lead Judge 6 months ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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