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

Missing Session End Validation in giveReview Function

umarbinkhattab

Severity: High

Likelihood: High

Summary

The giveReview function in the LevelOne contract lacks a critical check to verify if the current school session is still active. Teachers can continue giving reviews even after the sessionEnd timestamp has passed, violating the intended time-bound constraints of the educational system.

Vulnerability Details

In the contract, a school session has a defined end time:

function startSession(uint256 _cutOffScore) public onlyPrincipal notYetInSession {
sessionEnd = block.timestamp + 4 weeks;
inSession = true;
cutOffScore = _cutOffScore;
​
emit SchoolInSession(block.timestamp, sessionEnd);
}
​

However, the giveReview function does not validate whether the current time is before sessionEnd:

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]);
}
​

This means teachers can continue to affect student scores even after a session has officially ended, which is a significant temporal boundary violation.

Impact

The impact of this vulnerability is high for several reasons:

  1. Time Boundary Violation: Teachers can modify student scores outside the designated session period

  2. Post-Session Score Manipulation: Student scores can be altered after the session has ended, potentially affecting graduation decisions

  3. System State Inconsistency: The system enters a state where students cannot enroll (due to inSession being true) but teachers can still modify scores

  4. Educational Fairness Breach: Students expect their final scores to be locked at session end, but they can still be modified

  5. Contract Logic Violation: This breaks the intended lifecycle of the educational system where reviews should only happen during active sessions

Code Reference

The giveReview function should check if the current time is before sessionEnd:

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");
// Missing check: require(block.timestamp <= sessionEnd, "School session has ended");
​
// ... rest of function
}
​

Proof of Concept

  1. Principal calls startSession setting sessionEnd to block.timestamp + 4 weeks

  2. After 4 weeks pass, block.timestamp > sessionEnd

  3. Teachers can still call giveReview and affect student scores

  4. This creates an undefined period where the session has technically ended, but reviews can still be given

Recommendations

Add a session end validation check to the giveReview function:

function giveReview(address _student, bool review) public onlyTeacher {
if (!isStudent[_student]) {
revert HH__StudentDoesNotExist();
}
// Add check to ensure session is still active
require(block.timestamp <= sessionEnd, "School session has ended");
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]);
}
​

Additionally, consider adding an explicit check for inSession == true to maintain consistency with other functions in the contract.

Updates

Lead Judging Commences

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

session state not updated

`inSession` not updated after during upgrade

Support

FAQs

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