Hawk High

Summary

The removeTeacher function allows the principal to remove a teacher at any time, including during an active session or critically, just before or during the graduateAndUpgrade process. If listOfTeachers changes between wage calculation and payment, or if all teachers are removed right before payment, it could lead to incorrect distributions, division by zero errors (if the list becomes empty post-calculation but pre-division/payment), or violate the intent of the "teachers share 35% of bursary" invariant if teachers who contributed are removed right before payout.

Vulnerability Details

The removeTeacher function can be called by the principal at any time:

The graduateAndUpgrade function calculates teacher wages based on the listOfTeachers at the time of its execution:

Consider these scenarios:

1. A teacher is removed after totalTeachers is cached but before the payPerTeacher calculation or the payment loop. This could lead to payPerTeacher being based on an outdated count.

2. All teachers are removed after totalTeacherShare is calculated but before payPerTeacher is determined or payments are made. If totalTeachers becomes 0, this could lead to a division by zero if not handled carefully, or the totalTeacherShare (35% of bursary) would not be distributed.

3. The invariant states "teachers share of 35% of bursary". If a teacher contributes throughout the session but is removed by the principal moments before graduateAndUpgrade is called, they receive no pay according to the current logic. This might not align with the spirit of the invariant.

Impact

• Incorrect Wage Distribution: If the listOfTeachers changes at a critical moment during graduateAndUpgrade execution (though direct reentrancy from removeTeacher into graduateAndUpgrade isn't present, the timing between separate calls matters), the wage calculation could be based on stale data, leading to incorrect payments per teacher or failed payments.

• Potential for Teachers' Share to be Retained: If all teachers are removed just before payout, the 35% allocated share might remain in the contract, which might be an unintended consequence if these teachers had provided service.

• Violation of Fair Compensation Expectation (Implicit): Teachers might expect compensation for work done even if removed just before payout. The current contract doesn't cater to this.

While direct atomicity violations are hard without reentrancy within graduateAndUpgrade itself, the ability to change the teacher list so close to payout without a "snapshot" mechanism for who is eligible can be problematic for fairness and predictability.

Tools Used

Manual Review, Logical Analysis.

Recommendations

1. Clarify Intent: Decide if teachers removed just before payout (but after contributing) should receive wages.

2. Snapshotting (Robust Solution): If eligible teachers should be fixed at a certain point (e.g., when session ends or graduateAndUpgrade begins), consider snapshotting the listOfTeachers used for payment calculations at the start of graduateAndUpgrade and using that snapshot throughout the function. This makes the wage distribution robust against changes to listOfTeachers during the function's execution or immediately prior to it in a way that could be exploited.

3. Restrict removeTeacher Timing (Simpler): Consider disallowing removeTeacher calls when block.timestamp >= sessionEnd and graduateAndUpgrade has not yet been called, or once graduateAndUpgrade has begun (if it were to have multiple steps across transactions, which it doesn't here). This is a stricter approach.

A simple mitigation for the direct calculation issue within graduateAndUpgrade is to ensure totalTeachers is read once and used consistently, and handle the case where totalTeachers becomes 0 gracefully. The existing fix for H-02 (calculating totalTeacherShare then dividing by totalTeachers > 0 ? totalTeachers : 1) already somewhat handles the division by zero, but the share would remain undistributed if totalTeachers is 0.

Code Snippet (Conceptual - No direct code change for removeTeacher itself, but for graduateAndUpgrade consideration):
The fix for H-02 in graduateAndUpgrade already helps prevent reverts by checking totalTeachers > 0. The core of this L-04 finding is more about the timing and fairness aspect, which is a design consideration.

The main recommendation is to be aware of the implications of removeTeacher's flexibility and potentially snapshot the list of eligible teachers if fairness for removed-but-contributing teachers is a concern, or if the listOfTeachers could change in a way that breaks the payment loop (e.g., if the list shrinks mid-loop, which is not possible in a single transaction flow but highlights the sensitivity). The most practical fix is ensuring the calculation logic (as in H-02 fix) is robust to totalTeachers being 0.