Hawk High

Summary

(HIGH) Upgrading to LevelTwo corrupts critical state variables (usdc token address, totalTeachers), breaking functionality

Vulnerability Details

File: src/LevelTwo.sol (State Variables)

Impact

Technical Root Cause
The vulnerability is due to a storage layout incompatibility between LevelOne and LevelTwo when using the UUPS (Universal Upgradeable Proxy Standard) pattern. UUPS proxies allow upgrading the implementation contract while preserving the storage and address of the proxy. For this to work safely, the new implementation contract (LevelTwo) must maintain a compatible storage layout with the old one (LevelOne) for any state variables it intends to continue using or for new variables it appends.

Specifically:

1. Initializable contract's storage: Both LevelOne and LevelTwo inherit Initializable, which typically uses storage slot 0 for _initialized and _initializing. This part is compatible.

2. LevelOne storage layout (after Initializable's slot 0):

   • Slot 1: address public principal;

   • Slot 2: uint256 public schoolFees;

   • Slot 3: IERC20 public usdc; (effectively an address)

   • Slot 4: mapping(address => bool) public isTeacher; (conceptual slot for mapping)

   • Slot 5: address[] public teachers; (length and data pointer)

   • Slot 6: uint256 public totalTeachers;

   • And so on for other variables.

3. LevelTwo storage layout (after Initializable's slot 0):

   • Slot 1: address public principal; (Compatible with LevelOne.principal)

   • Slot 2: IERC20 public usdc; (INCOMPATIBLE: LevelOne has uint256 schoolFees at this slot)

   • Slot 3: uint256 public totalTeachers; (INCOMPATIBLE: LevelOne has IERC20 usdc at this slot)

   • Slot 4: uint256 public totalStudents; (INCOMPATIBLE: LevelOne has mapping(address => bool) isTeacher associated with this slot, followed by teachers array)

   • Slot 5: address[] public teachers; (INCOMPATIBLE: LevelOne has uint256 totalTeachers at slot 6, teachers array was at slot 5. LevelTwo.teachers will try to read its length from LevelOne.totalTeachers slot if packing allows, or a subsequent slot, leading to further corruption.)

This mismatch means that when LevelTwo's code executes, it reads and writes to storage slots assuming its own layout, thereby misinterpreting or corrupting the data previously stored by LevelOne. For instance, LevelTwo.usdc will read the uint256 value of LevelOne.schoolFees and interpret its lower 20 bytes as an address.

Likelihood of Exploitation: High. This is not an "exploitation" by a malicious external actor but an operational failure during a planned upgrade. If the incompatible LevelTwo contract is deployed as an upgrade, the corruption is guaranteed to occur. The likelihood of deploying such an incompatible upgrade depends on the team's diligence and testing practices.

Tools Used

1. AI assistance for identification an impact analaysis

2. Manual review

Recommendations

To fix this vulnerability, ensure that the storage layout of LevelTwo.sol is compatible with LevelOne.sol. This means:

1. Preserve Order and Type: Existing state variables from LevelOne that are still needed in LevelTwo must be declared in the exact same order and with the exact same types.

2. Append New Variables: New state variables in LevelTwo must be appended after all the variables from LevelOne.

3. Careful with Deletion/Modification: Deleting or changing the type of an existing state variable from LevelOne in LevelTwo will cause a storage layout shift and is generally unsafe unless the variable is no longer used and its slot can be repurposed (an advanced and risky technique). It's safer to mark unused variables as deprecated and leave them in place to preserve layout, or use techniques like "gap" variables (uint256[50] private __gap;) in earlier versions to reserve space for future variables.