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

Bursary not updated aftert paying the teacher and principal wages, causing incorrect funds accounting

cyfe45

Summary

In the LevelOne.sol::graduateAndUpgrade function, the contract pays the teacher and principal wages using the bursary balance, but fails to update the bursary afterward. This results in incorrect funds remaining in the bursary, causing future wage calculations to overestimate available funds and violate accurate accounting principles.

Vulnerability Details

The LevelOne.sol::graduateAndUpgrade function calculates the teacher and principal wages using the bursary balance.

uint256 payPerTeacher = (bursary * TEACHER_WAGE) / PRECISION;
uint256 principalPay = (bursary * PRINCIPAL_WAGE) / PRECISION;

However, the bursary value is not updated after the wage payments are made via external safeTransfer calls. As a result:

  • Incorrect bursary balance remains, causing subsequent to use outdated values.

  • Future wage payments will be overestimated.

  • The contract also violates the Check-Effects-Interactions pattern.

Not updating the internal state before making external calls can be dangerous as it can lead to unexpected behaviour and vulnerabilities.

Impacted code:

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

Impact: High – Incorrect bursary funds affect all future calculations and disbursements.
Likelihood: High – This function is expected to be called routinely during upgrades, consistently introducing accounting errors.

Tools Used

  • Manual review

  • Foundry test

Proof of Concept:

Adding the below code to the LeveOnelAndGraduateTest.t.sol file demonstrate that the bursary is unchanged after paying the teacher and principal wages:

contract LevelOneAndGraduateTest is Test {
...
​
modifier oneTeacher() {
_oneTeachersAdded();
_studentsEnrolled();
​
vm.prank(principal);
levelOneProxy.startSession(70);
​
_;
}
​
...
​
function test_fees_after_graduate_one_teacher() public oneTeacher {
levelTwoImplementation = new LevelTwo();
levelTwoImplementationAddress = address(levelTwoImplementation);
​
bytes memory data = abi.encodeCall(LevelTwo.graduate, ());
​
vm.prank(principal);
levelOneProxy.graduateAndUpgrade(levelTwoImplementationAddress, data);
​
LevelTwo levelTwoProxy = LevelTwo(proxyAddress);
​
assertEq(levelTwoProxy.bursary(), 3e22); // Bursary balance is not updated
assertEq(usdc.balanceOf(address(alice)), 105e20); // Alice has received her teacher wage
assertEq(usdc.balanceOf(address(principal)), 15e20); // Principal has received their wage
}
​
function _oneTeachersAdded() internal {
vm.startPrank(principal);
levelOneProxy.addTeacher(alice);
vm.stopPrank();
}
...
}

Recommendations

1. Update the bursary before any external calls:

By adding the line bursary -= (totalTeacherPay + principalPay); before the wage payments, the bursary will be updated before any external calls and stay in line with the Checks-Effects-Interactions pattern.

function graduateAndUpgrade(address _levelTwo, bytes memory) public onlyPrincipal {
if (_levelTwo == address(0)) {
revert HH__ZeroAddress();
}
​
uint256 totalTeachers = listOfTeachers.length;
​
uint256 totalTeacherPay = 0;
uint256 payPerTeacher = 0;
​
uint256 principalPay = (bursary * PRINCIPAL_WAGE) / PRECISION;
​
if (totalTeachers > 0) {
uint256 totalTeacherPay = (bursary * TEACHER_WAGE) / PRECISION;
uint256 payPerTeacher = totalTeacherPay / totalTeachers;
}
​
// EFFECTS: update bursary before any external calls
@> 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);
}

Note: The above code has also been updated to reflect the correct calculation of the teacher wages as per my other finding related to this function.

Updates

Lead Judging Commences

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

bursary not updated

The bursary is not updated after wages have been paid in `graduateAndUpgrade()` function

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

bursary not updated

The bursary is not updated after wages have been paid in `graduateAndUpgrade()` function

Support

FAQs

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