Hawk High

First Flight #39

Beginner FriendlySolidity

100 EXP

View results

Previous Next

Submission Details

Severity: low

Valid

[L] `giveReview()` does not update reviewCount

wiasliaw

Vulnerability Details

When the teacher calls giveReview and outputs a review, giveReview does not update the value of reviewCount. As a result, the check require(reviewCount[_student] < 5) may still pass.

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");

if (!review) {

// @note effect on `studentScore`

studentScore[_student] -= 10;

}

// @note effect on `lastReviewTime`

lastReviewTime[_student] = block.timestamp;

// @note there is no effect on `reviewCount`

emit ReviewGiven(_student, review, studentScore[_student]);

}

Impact

The invariant that “System upgrade should not occur if any student has not gotten 4 reviews” can be broke. If reviewCount is not updated correctly, the system will not know whether all students have received 4 reviews to perform upgrade.

Additionally, graduateAndUpgrade does not check reviewCount of each student.

PoC

Alter reviewCount's visibility to public:

contract LevelOne is Initializable, UUPSUpgradeable {

mapping(address => uint256) public reviewCount;

}

PoC:

// SPDX-License-Identifier: SEE LICENSE IN LICENSE

pragma solidity 0.8.26;

import {Test, console2} from "forge-std/Test.sol";

import {DeployLevelOne} from "../script/DeployLevelOne.s.sol";

import {GraduateToLevelTwo} from "../script/GraduateToLevelTwo.s.sol";

import {LevelOne} from "../src/LevelOne.sol";

import {LevelTwo} from "../src/LevelTwo.sol";

import {MockUSDC} from "./mocks/MockUSDC.sol";

contract LevelOneAndGraduateTest is Test {

DeployLevelOne deployBot;

GraduateToLevelTwo graduateBot;

LevelOne levelOneProxy;

LevelTwo levelTwoImplementation;

address proxyAddress;

address levelOneImplementationAddress;

address levelTwoImplementationAddress;

MockUSDC usdc;

address principal;

uint256 schoolFees;

// teachers

address alice;

address bob;

// students

address clara;

address dan;

address eli;

address fin;

address grey;

address harriet;

function setUp() public {

deployBot = new DeployLevelOne();

proxyAddress = deployBot.deployLevelOne();

levelOneProxy = LevelOne(proxyAddress);

// graduateBot = new GraduateToLevelTwo();

usdc = deployBot.getUSDC();

principal = deployBot.principal();

schoolFees = deployBot.getSchoolFees();

levelOneImplementationAddress = deployBot.getImplementationAddress();

alice = makeAddr("first_teacher");

bob = makeAddr("second_teacher");

clara = makeAddr("first_student");

dan = makeAddr("second_student");

eli = makeAddr("third_student");

fin = makeAddr("fourth_student");

grey = makeAddr("fifth_student");

harriet = makeAddr("six_student");

usdc.mint(clara, schoolFees);

usdc.mint(dan, schoolFees);

usdc.mint(eli, schoolFees);

usdc.mint(fin, schoolFees);

usdc.mint(grey, schoolFees);

usdc.mint(harriet, schoolFees);

}

function test_poc_giveReview() external schoolInSession {

vm.startPrank(alice);

// 1st call

vm.warp(block.timestamp + 1 weeks);

levelOneProxy.giveReview(harriet, false);

uint256 count1 = levelOneProxy.reviewCount(harriet);

// 2nd call

vm.warp(block.timestamp + 2 weeks);

levelOneProxy.giveReview(harriet, false);

uint256 count2 = levelOneProxy.reviewCount(harriet);

vm.stopPrank();

// assertion: should not revert

assertNotEq(count1, count2);

}

modifier schoolInSession() {

_teachersAdded();

_studentsEnrolled();

vm.prank(principal);

levelOneProxy.startSession(70);

}

function _teachersAdded() internal {

vm.startPrank(principal);

levelOneProxy.addTeacher(alice);

levelOneProxy.addTeacher(bob);

vm.stopPrank();

}

function _studentsEnrolled() internal {

vm.startPrank(clara);