Hawk High

Summary

This report details a vulnerability found in the LevelT wo.sol contract when intended as a UUPS (Universal Upgradeable Proxy Standard) upgrade target for LevelOne.sol. The LevelTwo.sol contract does not maintain the storage layout of the LevelOne.sol contract. This incompatibility will lead to data corruption and unpredictable behavior if an upgrade is performed, rendering the contract system unusable.

Vulnerability Details

Upgradeable smart contracts using proxy patterns, such as UUPS, separate the contract's logic (implementation contract) from its state (stored in the proxy contract). When an upgrade occurs, the proxy is pointed to a new implementation contract, but the storage slots and their existing data remain unchanged at the proxy's address.

Solidity assigns storage slots to state variables based on their order of declaration in the contract. For a safe upgrade, the new implementation contract LevelTwo.sol must preserve the storage layout of the original contract LevelOne.sol. This means:

1. All existing state variables from the old contract must be declared in the new contract in the exact same order.

2. No state variables can be removed from the sequence.

3. New state variables can only be appended at the end of the declaration list.

Comparing the state variables:

LevelOne.sol declares (among others, in order):

• address public principal;

• bool public inSession;

• uint256 public schoolFees;

• uint256 public reviewTime = 1 weeks;

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

• mapping(address => uint256) private lastReviewTime;

• address[] public listOfStudents;

• address[] public listOfTeachers;

• IERC20 usdc;

LevelTwo.sol declares:

• address principal; (Note: missing public visibility, which doesn't affect storage slot but is an inconsistency)

• bool inSession; (Note: missing public visibility)

• uint256 public sessionEnd;

• uint256 public bursary;

• uint256 public cutOffScore;

• mapping(address => bool) public isTeacher;

• mapping(address => bool) public isStudent;

• mapping(address => uint256) public studentScore;

• address[] listOfStudents;

• address[] listOfTeachers;

• IERC20 usdc;

The LevelTwo.sol contract omits schoolFees, reviewTime, reviewCount, and lastReviewTime from their original positions in the storage layout. This causes a misalignment:

• The storage slot originally holding LevelOne.schoolFees will be interpreted by LevelTwo.sol as LevelTwo.sessionEnd.

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

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

• The slots for LevelOne.reviewCount and LevelOne.lastReviewTime (mappings) will be misinterpreted by LevelTwo.listOfStudents and LevelTwo.listOfTeachers respectively (or their metadata).

Proof Of Concept

Consider the following scenario:

1. The LevelOne.sol contract is deployed via a proxy and initialized.

   • principal is set.

   • schoolFees is set to 100 * 10**18 (100 tokens).

   • reviewTime is 1 weeks (604800 seconds).

   • sessionEnd is set to block.timestamp + 4 weeks.

   • Other variables like bursary, cutOffScore, mappings, and arrays are populated.

2. The principal calls graduateAndUpgrade() in LevelOne.sol, authorizing an upgrade to LevelTwo.sol.

3. The proxy's implementation is changed to LevelTwo.sol.

4. When LevelTwo.sol's logic attempts to read its state variables:

   • Reading LevelTwo.sessionEnd will yield the value 100 * 10**18 (which was LevelOne.schoolFees). This value, when interpreted as a timestamp, is nonsensical and will break any time-dependent logic.

   • Reading LevelTwo.bursary will yield 604800 (which was LevelOne.reviewTime).

   • Reading LevelTwo.cutOffScore will yield the original LevelOne.sessionEnd timestamp.

   • Operations on LevelTwo.listOfStudents will attempt to use storage slots that previously held data for LevelOne.reviewCount, leading to corrupted array data or reverts.

This misinterpretation of data will occur for all variables declared after the point of divergence in the storage layout.

Impact

The impact of this vulnerability is Critical:

• Data Corruption: The contract's state will be entirely corrupted. Values will be misinterpreted, leading to a nonsensical and inconsistent state.

• Broken Logic: All functions in LevelTwo.sol that rely on these state variables will behave unpredictably and incorrectly.

• Denial of Service: The contract will likely become unusable, as operations may revert due to type mismatches or corrupted data structures (e.g., array lengths).

• Potential Loss of Funds: If any financial logic (e.g., fee distribution, withdrawals) relies on these corrupted variables, it could lead to incorrect fund transfers or lockups.

Tools Used

• Manual Code Review

• Understanding of EVM storage layout principles

• Knowledge of OpenZeppelin UUPS upgrade patterns

Recommendations

To rectify this vulnerability, LevelTwo.sol must be modified to ensure its storage layout is compatible with LevelOne.sol.

1. Preserve Storage Layout: Declare all state variables from LevelOne.sol in LevelTwo.sol in the exact same order.

2. Handle "Removed" Variables: If variables from LevelOne.sol are no longer functionally needed in LevelTwo.sol, they must still be declared to occupy their original storage slots. These can be named with a __gap_ prefix or _deprecated suffix to indicate they are unused placeholders.

3. Append New Variables: Any new state variables specific to LevelTwo.sol must be added after all the preserved variables from LevelOne.sol.

4. Maintain UUPS Compliance for Future Upgrades: If LevelTwo.sol is intended to be upgradeable further, it must also inherit UUPSUpgradeable and implement the _authorizeUpgrade function.

5. Visibility: Consider making state variables in /LevelTwo.sol public if their corresponding variables in LevelOne.sol were public, to maintain consistent external interfaces (though this doesn't directly affect storage slot assignment for value types).

Proposed Code Change for LevelTwo.sol: