Hawk High
First Flight #39
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Impact: low
Likelihood: low
Invalid

Lack of Action on Positive Reviews in `LevelOne::giveReview` May Contradict Protocol Intent.

rubencrxz

Summary

In the LevelOne::giveReview function, a conditional decreases the LevelOne::studentScore if a bad review is submitted. However, there is no corresponding logic for a good review, which reduces code clarity and readability and may introduce issues in the future.

Vulnerability Details

If a teacher submits a bad review in the LevelOne::giveReview function, the student's LevelOne::studentScore decreases by 10 points. However, if a good review is submitted, no changes are made to the score. This behavior makes the protocol's intention unclear and could lead to potential vulnerabilities in the contract.

Impact

This issue may disrupt the system if good reviews are not handled as expected. While it might make sense for a good review to leave the score unchanged, the contract does not explicitly document this behavior. This could be problematic, especially if LevelOne::cutOffScore is ever set above the initial score (100), making impossible for students to improve its initial socre.

The issue can be reproduced by adding the following code to the LevelOneAngGraduateTest.t.sol contract, which demonstrates that the student's score remains at the initial value (100) after a good review:

Proof of Concept:

  1. Teacher alice submits a good review for student fin.

  2. The value of studentScore(fin) remains unchanged, as confirmed in the console output.

Proof of Code

function test_review_is_true_does_not_increase_score() public schoolInSession {
uint256 finScoreBeforeReview = levelOneProxy.studentScore(fin);
console2.log("studentScoreBeforeReview: ", finScoreBeforeReview);
vm.warp(block.timestamp + 1 weeks);
​
vm.prank(alice);
levelOneProxy.giveReview(fin, true);
​
uint256 finScoreAfterReview = levelOneProxy.studentScore(fin);
console2.log("studentScoreAfterReview: ", finScoreAfterReview);
}

Tools Used

The issue was identified through manual review.

Recommendations

1. Add NatSpec comments before the function to clearly document the intended behavior:

​
+ /* @notice Submit a review for a student
+ * @param _student Address of the student being reviewed
+ * @param good If true, studentScore[_student] remains the same; if false, decreases by 10
+ * @dev Scores cannot go below 60 or above 100
+ */
​
function giveReview(address _student, bool review) public onlyTeacher {

2. Optionally, add an else clause in LevelOne::giveReview function for better readability and clarity of intention. Two options are presented:

  • The first one does not modify the studentScore[_student] value when a positive review is submitted. If this behavior is intentional, no change is needed.

  • The second option increases the studentScore[_student] value by ten upon receiving a positive review.

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;
}
+ else {
+ // studentScore[_student] is not modified if the review is good
+ }
​
// Update last review time
lastReviewTime[_student] = block.timestamp;
​
emit ReviewGiven(_student, review, studentScore[_student]);
}
​
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;
}
+ else {
+ studentScore[_student] += 10;
+ }
​
// Update last review time
lastReviewTime[_student] = block.timestamp;
​
emit ReviewGiven(_student, review, studentScore[_student]);
}
Updates

Lead Judging Commences

yeahchibyke Lead Judge 6 months ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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