Hawk High

Summary

The LevelTwo contract, intended as an upgrade for LevelOne, exhibits a critical storage layout incompatibility. The order, types, and existence of state variables in LevelTwo do not align with those in LevelOne. If LevelOne were upgraded to point to LevelTwo, the proxy would interpret the existing storage data using LevelTwo's layout, leading to complete state corruption. This would break all contract functionality, likely result in incorrect interpretation or loss of funds (e.g., the bursary), and render the system unusable.

Vulnerability Details

Upgradeable smart contracts using proxy patterns (like UUPS) rely on the principle that the storage layout of a new implementation contract must be compatible with the previous one. This means:

• Existing state variables must remain in the same order and be of the same type.

• New state variables can only be added at the end of the existing variable declarations.

• Existing state variables cannot be removed or have their types changed in a way that alters their storage footprint or interpretation.

A comparison of LevelOne and LevelTwo state variables reveals significant incompatibilities:

LevelOne State Variables (Simplified Order):

1. address principal;

2. bool inSession;

3. uint256 schoolFees;

4. uint256 public immutable reviewTime = 1 weeks; (Immutables don't occupy standard storage slots in the same way but affect bytecode, and their absence/presence matters for exact bytecode matching if that were a criteria, though less so for pure storage slot aliasing).

5. uint256 public sessionEnd;

6. uint256 public bursary;

7. uint256 public cutOffScore;

8. mapping(address => bool) public isTeacher;

9. mapping(address => bool) public isStudent;

10. mapping(address => uint256) public studentScore;

11. mapping(address => uint256) private reviewCount;

12. mapping(address => uint256) private lastReviewTime;

13. address[] listOfStudents;

14. address[] listOfTeachers;

15. IERC20 usdc;
    (Note: UUPSUpgradeable also adds its own storage variables, which must also be respected).

LevelTwo State Variables (Simplified Order):

1. address principal;

2. bool inSession;

3. uint256 public sessionEnd; // MISMATCH: Reads LevelOne.schoolFees slot

4. uint256 public bursary; // MISMATCH: Reads LevelOne.sessionEnd slot

5. uint256 public cutOffScore;// MISMATCH: Reads LevelOne.bursary slot

6. mapping(address => bool) public isTeacher; // MISMATCH: Reads LevelOne.cutOffScore slot

7. mapping(address => bool) public isStudent;

8. mapping(address => uint256) public studentScore;

9. address[] listOfStudents;

10. address[] listOfTeachers;

11. IERC20 usdc;

Missing Variables in LevelTwo:

• schoolFees (uint256)

• reviewCount (mapping)

• lastReviewTime (mapping)

The Problem During Upgrade:
When LevelOne is upgraded to LevelTwo, the proxy contract simply changes the implementation address it points to. The actual data stored in the proxy's storage slots remains untouched. LevelTwo's code will then attempt to read this existing data according to its own declared variable layout.

For example:

• The storage slot that LevelOne used for schoolFees (a uint256) will be interpreted by LevelTwo as its sessionEnd variable.

• The storage slot for LevelOne.sessionEnd will be read as LevelTwo.bursary.

• The data for LevelOne.reviewCount and LevelOne.lastReviewTime will effectively be lost or misread as part of other variables if their slots are overlaid by LevelTwo's subsequent variable declarations.

This misalignment means that virtually every piece of state data will be misinterpreted, leading to chaotic and unpredictable behavior.

Impact

The consequences of such a storage layout incompatibility during an upgrade are catastrophic:

1. Total State Corruption:

   • All meaningful data from LevelOne (principal address, session status, fees, scores, lists of users, token addresses) will be read incorrectly by LevelTwo. The contract will operate with nonsensical or arbitrary values for its state variables.

2. Complete Loss of Functionality:

   • Functions in LevelTwo will operate on corrupted data, leading to incorrect logic execution, reverts, or unintended state changes. The contract will cease to function as intended.

3. Potential Loss or Misappropriation of Funds:

   • If the bursary variable in LevelTwo reads from a slot that previously held a different type of data (e.g., sessionEnd timestamp from LevelOne), its value will be arbitrary. If usdc token address is misread, token transfers will fail or target incorrect tokens. This can lead to funds being permanently locked, sent to wrong addresses, or calculations for payments becoming wildly inaccurate.

4. Security Vulnerabilities:

   • Corrupted state can create new, unforeseen security vulnerabilities. For instance, if an access control variable like principal is misread, it could potentially grant unauthorized access or lock out legitimate administrators.

5. Irreversible Damage (Without Complex Recovery):

   • Once the state is corrupted in this manner, it's extremely difficult and risky to try and "fix" it with a subsequent upgrade, as you'd need to design a new implementation that precisely understands the nature of the corruption to unscramble it. Often, the only recourse is a full migration to a new, correctly deployed system, which is a major undertaking.

6. Breach of Trust:

   • Such a fundamental error in the upgrade process would completely destroy user and stakeholder trust in the system's reliability and the development team's competence.

Severity: Critical. This is one of the most severe vulnerabilities in upgradeable contract development, as it directly leads to the complete breakdown and potential irreversible corruption of the contract's state and functionality.

Tools Used

manual review

Recommendations

To prevent catastrophic state corruption due to storage layout incompatibilities during upgrades, the following recommendations are paramount:

1. Strict Adherence to Storage Layout Compatibility Rules:

   • Maintain Order and Type: When designing LevelTwo (or any subsequent upgrade), ensure that all state variables inherited or carried over from LevelOne are declared in the exact same order and with the exact same data types.

   • Append New Variables: New state variables in LevelTwo must only be added at the end of the sequence of variables inherited from LevelOne.

   • Do Not Remove or Reorder: Do not remove state variables present in LevelOne from LevelTwo. Do not reorder existing state variables.

   • Consider Initializable and UUPSUpgradeable Storage: Remember that base contracts like Initializable and UUPSUpgradeable also declare state variables. These must also be accounted for in the storage layout of both LevelOne and LevelTwo. (Typically, LevelTwo would inherit these in the same way LevelOne did).

2. Re-Design LevelTwo for Compatibility:

   • The LevelTwo contract needs a complete redesign of its state variable declarations to match LevelOne's layout for all shared variables, and then append any new variables.

   • Specifically, LevelTwo must include:

     • address principal;

     • bool inSession;

     • uint256 schoolFees; (If this concept is still relevant to LevelTwo)

     • uint256 public sessionEnd;

     • uint256 public bursary;

     • uint256 public cutOffScore;

     • mapping(address => bool) public isTeacher;

     • mapping(address => bool) public isStudent;

     • mapping(address => uint256) public studentScore;

     • mapping(address => uint256) private reviewCount; (If review functionality is to be maintained or evolved)

     • mapping(address => uint256) private lastReviewTime; (If review functionality is to be maintained or evolved)

     • address[] listOfStudents;

     • address[] listOfTeachers;

     • IERC20 usdc;

     • Then, any new variables specific to LevelTwo can be added after these.

3. Utilize Upgrade Safety Tools:

   • Employ tools provided by development frameworks like Hardhat or Truffle (e.g., OpenZeppelin Upgrades Plugins) that perform checks for storage layout compatibility before deploying an upgrade. These tools can automatically detect many common storage layout errors.

     • Example: npx hardhat verify --network <your_network> <PROXY_ADDRESS> <NEW_IMPLEMENTATION_NAME> (or similar commands provided by plugins).

4. Thorough Code Review and Manual Verification:

   • Before any upgrade, conduct a meticulous manual review of the state variable declarations in both the current and new implementations to confirm compatibility. Pay close attention to order, types, and any inherited contracts.

5. Comprehensive Testnet Upgrades:

   • Always perform upgrade tests on a testnet that mirrors the mainnet state as closely as possible.

   • After a testnet upgrade:

     • Verify that all critical state variables retain their correct values.

     • Test all core functionalities of the contract to ensure they operate as expected with the existing data.

6. Develop an Upgrade Checklist:

   • Maintain a strict checklist for the upgrade process that includes a mandatory step for verifying storage layout compatibility.

By rigorously following these recommendations, the development team can avoid storage layout conflicts and ensure that upgrades are safe, preserving the integrity of the contract's state and functionality. This is a non-negotiable aspect of secure upgradeable smart contract development.