Summary
In the Hawk High LevelOne::graduateAndUpgrade function, it can be executed before a session starts/ends and without any student reviews given breaking multiple invariants.
Description
An invariant is a property that must always hold true no matter the state of the contract.
The Hawk High documentation states that some of its invariant's are:
A school session is 4 weeks
Students must have gotten all reviews before system upgrade. System upgrade should not occur if any student has not gotten 4 reviews (one for each week)
System upgrade cannot take place unless school's sessionEnd has reached
The LevelOne::graduateAndUpgrade function is missing core logic to enforce these invariants and as such the function can be called
Before a started session has completed
Before a session has started
Before all students have been reviewed.
essentially breaking all three invariants of the contract.
Furthermore, this highlights a centralization issue as the Principal is assumed to be trusted at all times. The contract does mitigate the centralization issue as it does not enforce any rules preventing the principal from misbehaving.
Affected Code Area
Below is the vulnerable function where the above described issue lie.
function graduateAndUpgrade(address _levelTwo, bytes memory) public onlyPrincipal {
if (_levelTwo == address(0)) {
revert HH__ZeroAddress();
}
uint256 totalTeachers = listOfTeachers.length;
uint256 payPerTeacher = (bursary * TEACHER_WAGE) / PRECISION;
uint256 principalPay = (bursary * PRINCIPAL_WAGE) / PRECISION;
_authorizeUpgrade(_levelTwo);
for (uint256 n = 0; n < totalTeachers; n++) {
usdc.safeTransfer(listOfTeachers[n], payPerTeacher);
}
usdc.safeTransfer(principal, principalPay);
}
Impact
The severity of this bug is High due to breaking 3 core invariants and resulting in the contract not functioning as expected.
Furthermore, the current behavior creates trust problems as any parent/student reviewing the contract may view it as a scam and a way to get money out of people.
This will affect Hawk Highs reputation and affect the attendance of students along with the future of Hawk High in terms of success having a low probability.
The contract is very centralized as the Principal holds power over most functionality; as the contract does not re-enforce expected Principal behavior it is easy to misbehave.
Tools Used
Manual Review
Foundry
Proof of Concept
Description
I have created a runnable Proof-of-concept that executes two scenarios.
1st Scenario: Students have enrolled, teachers have been added and a session has started. The Principal executes graduateAndUpgrade before the session has ended and before students have been reviewed
2nd Scenario: Students have enrolled, a session has NOT yet started, and the principal calls graduateAndUpgrade. (Note: a principal can also add themselves as a teacher to get a higher payment (40%)).
Run 1st Scenario: forge test --mt testCanGraduateAndUpgradeBeforeSessionEnds -vvv
Run 2nd Scenario: forge test --mt testCanGraduateAndUpgradeBeforeSesionStarts -vvv
Code
pragma solidity 0.8.26;
import {Test, console2} from "forge-std/Test.sol";
import {LevelOne} from "src/LevelOne.sol";
import {LevelTwo} from "src/LevelTwo.sol";
import {DeployLevelOne} from "script/DeployLevelOne.s.sol";
import {MockUSDC} from "test/mocks/MockUSDC.sol";
import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
contract DoSByStudentRemoval is Test {
DeployLevelOne deployBot;
LevelOne levelOneProxy;
LevelTwo levelTwoLogic;
MockUSDC usdc;
address proxyAddress;
address levelOneImplementationAddress;
address principal;
address teacherJasmine = makeAddr("1st_teacher");
address[] students;
uint256 schoolFees;
function setUp() public {
deployBot = new DeployLevelOne();
proxyAddress = deployBot.deployLevelOne();
levelOneProxy = LevelOne(proxyAddress);
usdc = deployBot.getUSDC();
principal = deployBot.principal();
schoolFees = deployBot.getSchoolFees();
}
modifier startSession() {
vm.startPrank(principal);
levelOneProxy.startSession(5);
_;
}
modifier enrollStudents() {
uint256 numberOfStudents = 10;
students = new address[](numberOfStudents);
for (uint256 x = 1; x < numberOfStudents; x++) {
students[x] = address(uint160(x));
vm.startPrank(students[x]);
usdc.mint(students[x], schoolFees);
usdc.approve(address(levelOneProxy), schoolFees);
levelOneProxy.enroll();
vm.stopPrank();
}
_;
}
modifier addTeacher() {
vm.startPrank(principal);
levelOneProxy.addTeacher(teacherJasmine);
_;
}
function testCanGraduateAndUpgradeBeforeSessionEnds() public enrollStudents addTeacher startSession {
uint256 principalStartingBalance = usdc.balanceOf(principal);
uint256 teacherJasmineStartingBalance = usdc.balanceOf(teacherJasmine);
uint256 paidSchoolFees = (students.length * levelOneProxy.getSchoolFeesCost());
assert(principalStartingBalance == 0 && teacherJasmineStartingBalance == 0);
uint256 expectedTimeSessionToEnd = levelOneProxy.getSessionEnd();
if (block.timestamp < expectedTimeSessionToEnd) {
vm.startPrank(principal);
levelTwoLogic = new LevelTwo();
levelOneProxy.graduateAndUpgrade(address(levelTwoLogic), "");
}
uint256 principalFinalBalance = usdc.balanceOf(principal);
uint256 teacherJasmineFinalBalance = usdc.balanceOf(teacherJasmine);
console2.log("Starting Balance for Principal: ", principalStartingBalance);
console2.log("Starting Balance for teacherJasmine: ", teacherJasmineStartingBalance);
console2.log("School fees paid: ", paidSchoolFees);
console2.log("---AFTER GRADUATE AND UPGRADE, PRINCIPAL AND TEACHERS ARE PAID---");
console2.log("Final Balance for Principal: ", principalFinalBalance);
console2.log("Final Balance for teacherJasmine: ", teacherJasmineFinalBalance);
}
function testCanGraduateAndUpgradeBeforeSesionStarts() public enrollStudents {
uint256 principalStartingBalance = usdc.balanceOf(principal);
uint256 paidSchoolFees = (students.length * levelOneProxy.getSchoolFeesCost());
vm.startPrank(principal);
levelTwoLogic = new LevelTwo();
levelOneProxy.graduateAndUpgrade(address(levelTwoLogic), "");
uint256 principalFinalBalance = usdc.balanceOf(principal);
uint256 remainingBursary = levelOneProxy.bursary();
assert(remainingBursary != paidSchoolFees);
console2.log("Starting Balance for Principal: ", principalStartingBalance);
console2.log("School fees paid: ", paidSchoolFees);
console2.log("---AFTER GRADUATE AND UPGRADE, PRINCIPAL IS PAID---");
console2.log("Final Balance for Principal: ", principalFinalBalance);
console2.log("Remaining bursary: ", remainingBursary);
}
}
Mitigation
In order to enforce the invariants and to mitigate the centralization issue by enforcing the Principals roles/level of access:
The graduateAndUpgrade function should have logic to check that a session has started
The graduateAndUpgrade function should have logic to check that a session has completed
The graduateAndUpgrade function should have logic to check that all students have received 4 reviews
Custom Errors
error HH__NotAllowed();
+ error HH_CannotUpgrade_UnreviewedStudents();
+ error HH_SessionHasNotStarted();
Corrected graduateAndUpgrade logic
function graduateAndUpgrade(address _levelTwo, bytes memory) public onlyPrincipal {
if (_levelTwo == address(0)) {
revert HH__ZeroAddress();
}
+ if (inSession == false) {
+ revert HH_SessionHasNotStarted(); //can only upgrade once session has started
+ }
//Checking session has ended
+ require(block.timestamp >= sessionEnd,"Session has not yet completed");
//Checking each student has been reviewed and they have met the cut off score
+ uint256 totalStudents = listOfStudents.length;
+ for(uint256 x = 0;x < totalStudents;x++){
+ if(reviewCount[listOfStudents[x]] != 4){
+ revert HH_CannotUpgrade_UnreviewedStudent();
+ }
+ }
uint256 totalTeachers = listOfTeachers.length;
uint256 payPerTeacher = (bursary * TEACHER_WAGE) / PRECISION;
uint256 principalPay = (bursary * PRINCIPAL_WAGE) / PRECISION;
_authorizeUpgrade(_levelTwo);
for (uint256 n = 0; n < totalTeachers; n++) {
usdc.safeTransfer(listOfTeachers[n], payPerTeacher);
}
usdc.safeTransfer(principal, principalPay);
}
