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

Calculation of teacher fees causes excess fees to be paid out when there are more than one teacher and potentially reverts if the TEACHER_WAGE is too high due to insufficient bursary funds

cyfe45

Summary

The LevelOne.sol::graduateAndUpgrade function incorrectly calculates the teacher fee by not incorporating the number of teachers and with the current variable assumptions it will revert when there are more than 2 teachers onboarded (with the current TEACHER_WAGE variable).

Vulnerability Details

The LevelOne.sol::graduateAndUpgrade function incorrectly calculates the teacher fee by not incorporating the number of teachers. This causes the contract to pay out too much in teacher fees. Furthermore, when there are more than 2 teachers onboarded to Hawk High (assuming >= 34% TEACHER_WAGE), the function will revert due to insufficient funds. Causing unpaid wages to both the teacher and principal.

The teacher fees are calculated as follows in the LevelOne.sol::graduateAndUpgrade function:

...
uint256 public constant TEACHER_WAGE = 35; // 35%
...
​
function graduateAndUpgrade(address _levelTwo, bytes memory) public onlyPrincipal {
...
uint256 totalTeachers = listOfTeachers.length;
​
@> uint256 payPerTeacher = (bursary * TEACHER_WAGE) / PRECISION;
...
}

The above function should also divide the payPerTeacher calculation by the number of teachers.

Impact

The LevelOne.sol::graduateAndUpgrade only works when there is only one teacher onboarded. When more than one teacher is onboarded then the function incorrectly calcuylate the teacher fees as it does not divide the fees by the number of teachers. This ultimately will cause the function to overpay the teacher wages and drain all the bursary when there are more than 2 teachers onboarded (with the current variables).

Impact is high as the function overpays the teacher wages and drains the bursary.
Likelihood is also high as the function will always calculate the teacher fee incorrectly when there are more than 1 teacher onboarded.

Tools Used

  • Manual review

  • Foundry test

Proof of Concept:

Adding the below code to the LeveOnelAndGraduateTest.t.sol file will cause the function to revert..

...
contract LevelOneAndGraduateTest is Test {
...
// teachers
...
address charlie;
...
function setUp() public {
...
charlie = makeAddr("third_teacher");
...
}
...
modifier schoolSetUp() {
_threeTeachersAdded();
_studentsEnrolled(); //same as per the original test code on github
​
vm.prank(principal);
levelOneProxy.startSession(70);
​
_;
}
​
function test_fees_after_graduate() public schoolSetUp {
levelTwoImplementation = new LevelTwo();
levelTwoImplementationAddress = address(levelTwoImplementation);
​
bytes memory data = abi.encodeCall(LevelTwo.graduate, ());
​
vm.prank(principal);
levelOneProxy.graduateAndUpgrade(levelTwoImplementationAddress, data);
​
LevelTwo levelTwoProxy = LevelTwo(proxyAddress);
​
console2.log(levelTwoProxy.bursary());
console2.log(levelTwoProxy.getTotalStudents());
console2.log(usdc.balanceOf(address(alice)));
console2.log(levelOneProxy.getListOfTeachers().length);
}
​
function _threeTeachersAdded() internal {
vm.startPrank(principal);
levelOneProxy.addTeacher(alice);
levelOneProxy.addTeacher(bob);
levelOneProxy.addTeacher(charlie);
vm.stopPrank();
}
...
}

Recommendations

1. Adding the number of teachers to the calculation of the teacher fees:

By updating the graduateAndUpgrade function to calculate the teacher fees correctly, the function will no longer overpay the teacher wages. Furthermore, to be in line with Checks-Effects-Interactions, the function should update also the bursary before any external calls.

function graduateAndUpgrade(address _levelTwo, bytes memory) public onlyPrincipal {
if (_levelTwo == address(0)) {
revert HH__ZeroAddress();
}
​
uint256 totalTeachers = listOfTeachers.length;
​
uint256 totalTeacherPay = 0; // new variable
uint256 payPerTeacher = 0; // updated definiton of payPerTeacher to initialise to 0
​
uint256 principalPay = (bursary * PRINCIPAL_WAGE) / PRECISION;
​
if (totalTeachers > 0) {// adding the if statement to check if there are any teachers before dividing by the number of teachers
totalTeacherPay = (bursary * TEACHER_WAGE) / PRECISION;
payPerTeacher = totalTeacherPay / totalTeachers;
}
​
// EFFECTS: update bursary before any external calls
// Note: I will report this lack of bursary update in another finding
bursary -= (totalTeacherPay + principalPay);
​
_authorizeUpgrade(_levelTwo);
​
// INTERACTIONS: transfer funds to the teachers and principal
for (uint256 n = 0; n < totalTeachers; n++) {
usdc.safeTransfer(listOfTeachers[n], payPerTeacher);
}
​
usdc.safeTransfer(principal, principalPay);
}

The above calculation correct the teacher wage (assuming 3 teachers):

(3e22 * 35) / 3 / 100 = 35e20

Updates

Lead Judging Commences

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