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

Security Review of Hawk High by Divya Ranjan

divyaranjan

Summary

While Hawk High follows good practices for their upgradeable contracts, such as using OpenZeppelin's reliable library, it still has major issues in it's implementation. In it's business logic of paying teachers, it has an arithmetic issue of exceeding more than 100% of the bursary which would fail due to insufficient funds.

Vulnerability Details

The payment structure of the school declares that a teachers would share 35% of the bursary, but in it's logic of graduateAndUpgrade:

uint256 payPerTeacher = (bursary * TEACHER_WAGE) / PRECISION;

This implies that each teacher receives 35% of the entire bursary, which means if the number of teachers is more than or equal to 3, then the amount to pay them would be more than 100% of the bursary, which would be impossible alongside the 5% cut for the school's principal.

One can verify that this indeed is what occurs, by adding another teacher to the existing list of two teachers in the foundry test suite:

@@ -27,6 +27,7 @@ contract LevelOneAndGraduateTest is Test {
// teachers
address alice;
address bob;
+ address charlie;
// students
address clara;
address dan;
@@ -49,6 +50,7 @@ contract LevelOneAndGraduateTest is Test {
alice = makeAddr("first_teacher");
bob = makeAddr("second_teacher");
+ charlie = makeAddr("third_teacher");
clara = makeAddr("first_student");
dan = makeAddr("second_student");
@@ -178,6 +180,37 @@ contract LevelOneAndGraduateTest is Test {
console2.log(levelTwoProxy.getTotalStudents());
}
+ // New test case for payment overflow vulnerability
+ function test_teacher_payment_overflow() public {
+ // Add 3 teachers
+ vm.startPrank(principal);
+ levelOneProxy.addTeacher(alice);
+ levelOneProxy.addTeacher(bob);
+ levelOneProxy.addTeacher(charlie);
+ vm.stopPrank();
+
+ // Enroll students (6 students)
+ _studentsEnrolled();
+
+ // Start session
+ vm.prank(principal);
+ levelOneProxy.startSession(70);
+
+ // Prepare LevelTwo upgrade
+ levelTwoImplementation = new LevelTwo();
+ levelTwoImplementationAddress = address(levelTwoImplementation);
+ bytes memory data = abi.encodeCall(LevelTwo.graduate, ());
+
+ // Verify contract balance
+ uint256 totalBursary = 6 * schoolFees;
+ assertEq(usdc.balanceOf(address(levelOneProxy)), totalBursary);
+
+ // Attempt graduation (should revert)
+ vm.prank(principal);
+ vm.expectRevert("SafeERC20: ERC20 operation did not succeed");
+ levelOneProxy.graduateAndUpgrade(levelTwoImplementationAddress, data);
+ }
+

When one runs this with forge test one gets:

[FAIL: Error != expected error: ERC20InsufficientBalance(0x90193C961A926261B756D1E5bb255e67ff9498A1, 9000000000000000000000 [9e21], 10500000000000000000000 [1.05e22]) != SafeERC20: ERC20 operation did not succeed] test_teacher_payment_overflow() (gas: 1589898)

Impact

Critical

Tools Used

  • git

  • foundry

  • slither

Recommendations

The appropriate way to handle this bug is to not provide 35% of the entire bursary to each teacher, instead to distributeit proporationally so that all the teachers cumulatively constitute 35% of the bursary. This is in accordance with the payment structure that says:

teachers share of 35% of bursary

To make this change, one needs to simply divide by the total number of teachers, so the logic in graduateAndUpgrade changes to:

uint256 payPerTeacher = (bursary * TEACHER_WAGE) / (PRECISION * totalTeachers);

This splits the 35% of the bursary equally among all the teachers, and ensures that the total payments made to the teachers never exceeds the limit of 35% of bursary.

Updates

Lead Judging Commences

yeahchibyke Lead Judge 3 months ago
Submission Judgement Published
Validated
Assigned finding tags:

incorrect teacher pay calculation

`payPerTeacher` in `graduateAndUpgrade()` is incorrectly calculated.

yeahchibyke Lead Judge 3 months ago
Submission Judgement Published
Validated
Assigned finding tags:

incorrect teacher pay calculation

`payPerTeacher` in `graduateAndUpgrade()` is incorrectly calculated.

Support

FAQs

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