Hawk High

Summary

This report highlights a clarity and maintainability issue in the LevelOne.sol contract. The function graduateAndUpgrade() has a name that implies it handles the process of student graduation (e.g., checking scores against cutOffScore, marking students as graduated). However, its current implementation only processes financial payouts to teachers and the principal, and authorizes a contract upgrade to a new version (e.g., LevelTwo.sol). The actual student graduation logic based on performance is deferred to the graduate() reinitializer function in the LevelTwo.sol contract. This discrepancy can lead to confusion for developers, auditors, and users regarding the function's true purpose and the state of students after its execution.

Vulnerability Details / Issue Description

The function graduateAndUpgrade(address _levelTwo, bytes memory) in LevelOne.sol is intended to be called by the principal at the end of a school session.

The name graduateAndUpgrade strongly suggests that student graduation is part of its responsibilities. Users, including the principal calling the function, or developers integrating with the system, might expect that after this function successfully executes:

1. Students' scores have been evaluated against the cutOffScore.

2. Students who meet the criteria are marked as "graduated" within the LevelOne.sol state or an event is emitted to this effect.
   However, the function only performs:

3. Payouts from the bursary.

4. Authorization for the UUPS proxy to upgrade to the _levelTwo contract address.
   The actual student graduation logic (evaluating scores, updating student lists based on performance) is handled by the graduate() reinitializer function within the LevelTwo.sol contract, which is called after the proxy has been upgraded.

Proof Of Concept / Scenario

1. A school session in LevelOne.sol concludes.

2. The principal calls graduateAndUpgrade(levelTwoAddress, "").

3. The transaction succeeds (assuming other bugs like payout logic are fixed and prerequisites are met).

4. An observer or the principal checks the state of students in LevelOne.sol (e.g., via getListOfStudents() or by querying isStudent and studentScore). They might expect to see a change in student status or a reduced list of active students reflecting graduation.

5. However, no such change related to academic graduation occurs in LevelOne.sol as a direct result of this function call. The listOfStudents and their isStudent status remain unchanged by this function with respect to their academic performance.

This leads to a misunderstanding of the function's scope and the system's state transition.

Impact

• Developer Confusion: Developers working on or integrating with the contract may misinterpret the function's behavior, leading to incorrect assumptions when building further logic or off-chain services.

• Increased Audit Effort: Auditors might spend additional time verifying if graduation logic is indeed missing or located elsewhere, as the name suggests its presence.

• Potential for Operational Misunderstanding: The principal or other administrative users might expect student graduation to be finalized by this call, leading to incorrect assumptions about which students have successfully completed the course at this stage.

• Reduced Code Readability: A function name should clearly and accurately describe its actions. A misleading name reduces the overall readability and self-documentation of the code.
  Tools Used
  Manual Code Review

Recommendations

To improve clarity and accurately reflect the function's operations, consider renaming graduateAndUpgrade in LevelOne.sol. A name that more precisely describes its actions would be beneficial.

Suggested Renaming Options:

1. processSessionEndAndAuthorizeUpgrade

2. finalizeSessionPayoutsAndAuthorizeUpgrade

3. triggerPayoutsAndAuthorizeUpgrade