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

Indefinite Review Window Due to Missing Session Boundary Enforcement

khalidwalid

Severity: High

Likelihood: High

Summary

The LevelOne contract fails to enforce session end boundaries in the giveReview function, creating an "eternal review period" where teachers can continue modifying student scores indefinitely after a session has officially ended.

Vulnerability Details

The LevelOne contract implements a school session system with a clear start and end:

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

However, while the sessionEnd variable is set, the giveReview function does not check whether the current time has exceeded this boundary:

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 stands in stark contrast to other functions like expel, which explicitly check whether the session is active:

function expel(address _student) public onlyPrincipal {
if (inSession == false) {
revert();
}
// ... rest of function
}
​

Impact

This vulnerability has severe implications for the contract's intended behavior:

  1. Infinite Review Window: Teachers can continue giving reviews weeks, months, or even years after a session has ended

  2. Graduation Ceremony Disruption: If the graduateAndUpgrade function relies on final student scores, these scores can be manipulated right before graduation

  3. Inconsistent State Management: The contract has an ambiguous state where students can't enroll (due to inSession being true) but teachers can still modify scores after the session should be closed

  4. Contract Lifecycle Violation: The intended lifecycle of start session → conduct reviews → end session → graduate is broken

  5. Unpredictable Final Scores: Students cannot know when their final score is actually "final"

Code Reference

The contract maintains two session-related state variables:

bool inSession;
uint256 public sessionEnd;
​

The giveReview function should check both that the session is active (inSession == true) and that the current time is before the session end (block.timestamp <= sessionEnd):

function giveReview(address _student, bool review) public onlyTeacher {
// Current code with no session boundary checks
// ...
}
​

Proof of Concept

  1. Principal calls startSession with _cutOffScore = 70

  2. sessionEnd is set to block.timestamp + 4 weeks

  3. After 4 weeks pass and sessionEnd is reached, teachers can still call giveReview

  4. Even if the principal is preparing to call graduateAndUpgrade, teachers can still modify student scores

  5. A teacher could give negative reviews to students who would have passed the cutoff score, causing them to fail just before graduation

Recommendations

Implement proper session boundary enforcement in the giveReview function by adding these checks:

function giveReview(address _student, bool review) public onlyTeacher {
// Check that we are in an active session
require(inSession == true, "School is not in session");
// Check that the session end time has not been reached
require(block.timestamp <= sessionEnd, "School session has ended");
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]);
}
​

Additionally, consider adding functionality to properly close a session when sessionEnd is reached, transitioning the contract to a state where reviews are no longer possible but graduation processing can occur.

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.