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

Teachers can bypass session end restrictions via `LevelOne::giveReview`, breaking invariants

s3bc40

Summary

As stated in the documentation, a school session lasts 4 weeks and students can only be reviewed once per week. However, the current implementation allows teachers to give reviews after the session end and without taking into account the student reviewCount. This can lead to a situation where students receive more than 4 reviews, and review time limit is not enforced by sessionEnd variable. Furthermore, with bad reviews, students can be penalized and their scores can be reduced. Since the upgrade is manually done by the principal, this issue still remains in the code.

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

Impact

The giveReview function does not enforce the session end and allows teachers to give reviews endlessly. This can lead to students receiving more than 4 reviews, which violates the invariant that students should only receive one review per week. This can also lead to a situation where students are penalized with bad reviews, leading to a decrease in their scores.

Proof of Concept

Run the following test case:

function test_audit_teacher_can_give_review_endlessly() public schoolInSession {
// Count the number of weeks the teacher can give reviews
uint256 timestamp = block.timestamp;
uint256 countWeeks = 0;
for (uint256 i = 0; i < 10; i++) {
// This should break if we do have a 4 week limit
vm.warp(block.timestamp + 1 weeks);
vm.prank(alice);
levelOneProxy.giveReview(harriet, false);
countWeeks++;
}
uint256 overtime = countWeeks * 1 weeks + timestamp;
​
// Check that the teacher can give reviews endlessly
assertGt(countWeeks, 4);
assertGt(overtime, levelOneProxy.getSessionEnd());
console2.log("Teacher can give review endlessly: ", countWeeks);
}

  • We could also run more weeks to reach a negative score for the student, which is not expected and will cause a panic arithmetic error. See the same test output below with 11 weeks:

├─ [0] VM::prank(first_teacher: [0xeeEeC5A3afd714e3C63A0b1ef6d80722Bcc514b3])
│ └─ ← [Return]
├─ [5110] ERC1967Proxy::fallback(six_student: [0x983Cec4DF373E6f3809b4483fEBf6C9469B0769b], false)
│ ├─ [4633] LevelOne::giveReview(six_student: [0x983Cec4DF373E6f3809b4483fEBf6C9469B0769b], false) [delegatecall]
│ │ ├─ emit ReviewGiven(student: six_student: [0x983Cec4DF373E6f3809b4483fEBf6C9469B0769b], review: false, studentScore: 10)
│ │ └─ ← [Stop]
│ └─ ← [Return]
├─ [0] VM::warp(6048001 [6.048e6])
│ └─ ← [Return]
├─ [0] VM::prank(first_teacher: [0xeeEeC5A3afd714e3C63A0b1ef6d80722Bcc514b3])
│ └─ ← [Return]
├─ [5110] ERC1967Proxy::fallback(six_student: [0x983Cec4DF373E6f3809b4483fEBf6C9469B0769b], false)
│ ├─ [4633] LevelOne::giveReview(six_student: [0x983Cec4DF373E6f3809b4483fEBf6C9469B0769b], false) [delegatecall]
│ │ ├─ emit ReviewGiven(student: six_student: [0x983Cec4DF373E6f3809b4483fEBf6C9469B0769b], review: false, studentScore: 0)
│ │ └─ ← [Stop]
│ └─ ← [Return]
├─ [0] VM::warp(6652801 [6.652e6])
│ └─ ← [Return]
├─ [0] VM::prank(first_teacher: [0xeeEeC5A3afd714e3C63A0b1ef6d80722Bcc514b3])
│ └─ ← [Return]
├─ [2693] ERC1967Proxy::fallback(six_student: [0x983Cec4DF373E6f3809b4483fEBf6C9469B0769b], false)
│ ├─ [2209] LevelOne::giveReview(six_student: [0x983Cec4DF373E6f3809b4483fEBf6C9469B0769b], false) [delegatecall]
│ │ └─ ← [Revert] panic: arithmetic underflow or overflow (0x11)

Tools Used

Manual review

Recommendations

Consider implementing a check to ensure that the giveReview function respects the session end and the review count. This can be done by adding a check for the sessionEnd variable and ensuring that the review count does not exceed 4 by updating it in the function. Keep in mind that if the sessionEnd is taken into account, you have reconsider how to check the time elapsed since the last review.

Updates

Lead Judging Commences

yeahchibyke Lead Judge 7 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.

Give us feedback!