Hawk High

Summary

The graduateAndUpgrade function in the LevelOne contract does not reset student-related state variables (studentScore, reviewCount, and lastReviewTime) before upgrading to LevelTwo. This causes these variables to persist in storage even after upgrade, potentially leading to unintended behavior in the LevelTwo contract.

Vulnerability Detail

In an upgradeable contract pattern using UUPS, storage layouts must be carefully managed. When LevelOne upgrades to LevelTwo, the storage slots containing studentScore, reviewCount, and lastReviewTime are preserved.

The graduateAndUpgrade function in LevelOne:

This function distributes payments to teachers and principal, but does not reset:

1. studentScore mapping

2. reviewCount mapping (private)

3. lastReviewTime mapping (private)

Looking at the LevelTwo contract, we can see that it has studentScore but lacks reviewCount and lastReviewTime:

Impact

1. Data Inconsistency: Students graduating from LevelOne will carry their scores into LevelTwo, even though they should presumably start afresh.

2. Hidden Storage Conflict: The reviewCount and lastReviewTime mappings are not declared in LevelTwo but will still occupy their storage slots. If LevelTwo later adds variables in these slots, it will overwrite or be overwritten by these values.

3. Business Logic Flaws: Any business logic in LevelTwo that assumes fresh student scores will be compromised. For example, if a student had a low score in LevelOne but should start with a default score in LevelTwo, this won't happen.

4. Storage Slot Collision: LevelTwo's graduate() function with reinitializer(2) should be setting up new initial states, but existing stale data will persist.

Recommendation

Modify the graduateAndUpgrade function to clear all student-related state:

Alternatively, ensure that LevelTwo's graduate() reinitializer function properly handles the existing data by either clearing it or accommodating it in the new business logic.