Hawk High

Summary

The LevelOne::giveReview function incorrectly handles the update of the LevelOne::reviewCount for students, which can break the intended checks within the function.

Specifically, the function includes a requirement that LevelOne::reviewCount must be less than five. However, this counter is never updated when a review is submitted, rendering the check ineffective.

Vulnerability Details

LevelOne::giveReview function is supposed to allow the teachers to submit only one review per student per week. However, it does not update LevelOne::reviewCount mapping as expected. Although other variables in the contact such as LevelOne::lastReviewTime or LevelOne::sessionEnd provide protection to it indirectly, the function should still update LevelOne::reviewCount every time it is called. This omission creates an inconsistency in the contract's state.

Impact

Although the current implementation restricts teachers from calling LevelOne::giveReview more than once per week, which decreases the likelihood of this issue being exploited by a malicious actor, the logic of the contract becomes fragile. If variables like LevelOne::sessionEnd or LevelOne::lastReviewTime are modified, the protection breaks down.

The intended restriction (reviewCount < 5) is entirely bypassed, rendering the reviewCount mapping functionally useless.

This issue can be confirmed by adding the following test to the LevelOneAndGraduateTest.t.sol contract. The console will show that "reviewCount:", "reviewCount2:", and "reviewCount3:" all return 0.

Proof of Concept:

1. Teacher alice submits three reviews for student fin.

2. After the third review, the console shows reviewCount(fin) is still 0, despite three reviews being recorded.

Proof of Code

Tools Used

This issue was found by manual review.

Recommendations

Consider to modify LevelOne::giveReview function to properly update LevelOne::reviewCount value every time the function is called: