Hawk High

Summary

The LevelOne::graduateAndUpgrade function contains a critical miscalculation in the payPerTeacher formula. If more than two teachers are added, the total funds required to pay them exceed the available bursary, causing the transaction to revert. This effectively locks the LevelOne::graduateAndUpgrade function, resulting in a Denial of Service.

Vulnerability Details

Inside the LevelOne::graduateAndUpgrade function, the contract loops through the listOfTeachers array and attempts to distribute teacher salaries using a payPerTeacher amount. This value is calculated as payPerTeacher = (bursary * TEACHER_WAGE) / PRECISION, where TEACHER_WAGE = 35 and PRECISION = 100.

However, this calculation assumes only one teacher will be paid the entire teacher allocation of the bursary. When multiple teachers are present, this formula is applied to each teacher individually, leading to total payouts that exceed the available bursary. As a result, if more than two teachers are added, the third or subsequent transfer fails due to insufficient funds, causing the transaction to revert.

Impact

This miscalculation leads to a Denial of Service. When the condition is triggered (more than two teachers added), the LevelOne::graduateAndUpgrade function becomes permanently unusable. This prevents the contract from distributing schoolFees and upgrading to the next level, effectively locking the system.

Proof of Concept

To demonstrate the vulnerability:

1. Add a third teacher to the contract.

2. Call the graduateAndUpgrade function.

3. Observe the transaction revert due to an out-of-funds error when paying the third teacher.

Proof of Code

In LevelOneAndGraduateTest.t.sol contract, add the following code:

Tools Used

This issue was found by manual review.

Recommendations

Although there exists an invariant that states "Wages are to be paid only when the graduateAndUpgrade function is called by the principal", combining two critical operations (contract upgrade and funds distribution) in a single function increases the risk of cascading failures. In this case, a miscalculation in teacher payments can revert the entire transaction, permanently blocking the contract upgrade.

1. Consider splitting the logic of the upgrade and the bursary distribution in different functions. LevelOne::graduateAndUpgrade will keep only the upgrade logic:

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

   }

2.To ensure fair and efficient distribution of the bursary, a proper formula will be introduced in a new function bursaryDistribution. Additionally, the current approach of paying teachers via a loop presents scalability challenges as the number of teachers grows, the transaction may become prohibitively expensive or exceed the gas limit, leading to another Denial of Service.

To address this, the following optimizations will be implemented:

• Storing Payments in a Mapping: Instead of iterating over the listOfTeachers array to distribute funds in a single transaction, a mapping will store each teacher’s allocated payment.

• Self-Service Claim Mechanism: Teachers will be able to independently withdraw their salaries using a new claimTeacherPay function. This eliminates the need for costly batch transactions and reduces gas consumption.

This approach improves scalability and ensures the contract remains efficient even as the number of teachers increases.

If the desire is to preserve the upgrade and payment logic within the LevelOne::graduateAndUpgrade function, as one invariant of the protocol states, the following changes will be implemented:

2.1. Introduce a teachersTotalPay variable, which will replicate the logic from LevelOne::payPerTeacher to determine the total disbursable amount for teachers.

2.2. Compute the individual payPerTeacher using a new proper formula.

2.3. Retain the existing loop over the listOfTeachers array to distribute payments directly within the function.