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

Inconsistent Session State Validation in Teacher Removal Function

umarbinkhattab

Issue Description

The removeTeacher() function lacks a session state validation check, while the similar administrative function expel() properly verifies that the school is in session. This inconsistency creates a logical flaw in the contract's permission model and violates the implied design pattern.

Impact - Medium

This inconsistency allows the principal to remove teachers at any time, including during active school sessions when teachers are required for student reviews. Removing teachers mid-session could disrupt the academic process, preventing students from receiving their required reviews and potentially blocking system upgrades.

Likelihood - Medium

The likelihood of exploitation is medium since it requires the principal to deliberately or accidentally remove teachers during an active session. However, given that the function lacks protection, any principal could make this mistake without warning.

Detailed Analysis

Let's compare the implementations of both functions:

// Does NOT check session state
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);
}
​
// Properly checks session state
function expel(address _student) public onlyPrincipal {
if (inSession == false) {
revert();
}
if (_student == address(0)) {
revert HH__ZeroAddress();
}
// ... rest of function
}

The expel() function begins with an explicit check if (inSession == false) { revert(); } to ensure that a school session is active before expelling a student. This check logically makes sense, as student management should only occur during active sessions.

However, removeTeacher() has no such check, creating an inconsistent pattern in the contract's administrative controls. This allows teachers to be removed at any time, which could disrupt an ongoing session if teachers are removed while students still need reviews.

Given the invariant that "Students must have gotten all reviews before system upgrade," removing teachers mid-session could make it impossible for all students to receive their required reviews, potentially blocking graduation and system upgrades.

Recommendation

Add a session state check to the removeTeacher() function to align with the contract's design pattern:

function removeTeacher(address _teacher) public onlyPrincipal {
// Ensure consistency with expel() function
if (inSession == true) {
revert HH__AlreadyInSession(); // Or create a new specific error
}
if (_teacher == address(0)) {
revert HH__ZeroAddress();
}
// Rest of function remains unchanged
// ...
}

This ensures that teachers can only be removed outside of active school sessions, maintaining the contract's logical consistency and preventing potential disruption to the academic process.

Updates

Lead Judging Commences

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

Support

FAQs

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

Give us feedback!