Hawk High
First Flight #39
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: low
Valid

Incorrect or Redundant Logic in `giveReview` Function in `Session1.sol`

bambooboo

Summary

The giveReview function includes a check on reviewCount[_student] to limit the number of reviews a student can receive. However, this variable is never updated in the function, rendering the check ineffective. Additionally, due to an existing time-based constraint, the reviewCount variable appears redundant and may be safely removed.

Vulnerability Details

The giveReview function is implemented as follows:

Source Link (Line 277)

function giveReview(address _student, bool review) public onlyTeacher {
if (!isStudent[_student]) {
revert HH__StudentDoesNotExist();
}
require(reviewCount[_student] < 5, "Student review count exceeded!!!");
require(block.timestamp >= lastReviewTime[_student] + reviewTime, "Reviews can only be given once per week");
​
// where `false` is a bad review and true is a good review
if (!review) {
studentScore[_student] -= 10;
}
​
// Update last review time
lastReviewTime[_student] = block.timestamp;
​
emit ReviewGiven(_student, review, studentScore[_student]);
}

In this function:

  • Line 281 checks that reviewCount[_student] < 5.

  • However, reviewCount[_student] is never incremented or updated.

  • As a result, this check has no effect and does not prevent a student from receiving more than 5 reviews.

Moreover, the function already enforces a time-based restriction:

require(block.timestamp >= lastReviewTime[_student] + reviewTime, "Reviews can only be given once per week");

This ensures that reviews can only be submitted at intervals defined by reviewTime. Combined with a bounded session duration (e.g., via sessionEnd), the total number of reviews per student is inherently limited, making the reviewCount variable redundant.

Impact

  • Logical Inconsistency: A variable is checked without being updated, leading to dead code.

  • Wasted Storage: The reviewCount mapping consumes unnecessary storage and gas without contributing functional value.

  • Developer Confusion: Future developers may assume the check is effective and misinterpret the contract's behavior.

Tools Used

  • Manual Code Review

Recommendations

Option 1: Keep the review count logic and fix the bug. If the goal is to enforce a maximum number of reviews per student (independent of time), the contract should increment reviewCount[_student] after a review is given:

reviewCount[_student] += 1;

Place this after the review is applied and before emitting the event.

Option 2: Remove the reviewCount logic entirely. If the intention is to limit reviews based solely on time constraints, remove the reviewCount check and mapping to reduce contract complexity and save gas.

// Remove this line:
require(reviewCount[_student] < 5, "Student review count exceeded!!!");
​
// Remove the reviewCount mapping from the contract state
Updates

Lead Judging Commences

yeahchibyke Lead Judge 7 months ago
Submission Judgement Published
Validated
Assigned finding tags:

reviewCount not updated

`reviewCount` for students is not updated after each review session

Support

FAQs

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

Give us feedback!