Hawk High
First Flight #39
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

Storage collision in LevelTwo violates the required bursary invariant after the upgrade

jfornells

Drscription

The LevelTwo contract manually redefines the storage structure without preserving the layout of LevelOne. It removes important storage variables such as schoolFees, reviewTime, reviewCount, and lastReviewTime, as well as other intermediate fields.

This collision causes the bursary value, which should retain 60% of the school funds as per the specified invariant, to be corrupted and replaced by other values like sessionEnd. Consequently, the contract does not correctly reflect the remaining resources after the upgrade, violating a critical functional requirement of the system.

This misalignment causes severe collisions with persistent proxy data as well.

Impact:

  • The bursary is corrupted and no longer contains the expected value.

  • Stored data is read incorrectly.

  • Values in utilized slots are overwritten.

  • The contract may operate with incorrect roles, scores, or balances.

  • The resulting behavior is erratic and difficult to detect.

  • No visible errors occur, even though the state is corrupted.

  • It is very difficult to recover or fix the state once deployed.

Proof of Concept:

The following demonstrates a storage collision between LevelOne and LevelTwo, showing how the bursary and sessionEnd values are corrupted after the upgrade:

  1. Capture the values of bursary (slot 5) and sessionEnd (slot 4) before performing the upgrade:

uint256 bursary = LevelOne(proxy).bursary();
console2.log("bursary LevelOne:.....", bursary);
uint256 sessionEnd = LevelOne(proxy).sessionEnd();
console2.log("Session End LevelOne:", sessionEnd);

  1. Perform the upgrade to LevelTwo:

bytes memory data = abi.encodeWithSignature("graduate()");
LevelOne(proxy).graduateAndUpgrade(address(levelTwo), data);

  1. Query the value of bursary again from LevelTwo:

uint256 bursary2 = LevelTwo(proxy).bursary();
console2.log("bursary LevelTwo:..", bursary2);

Result:

Logs:
bursary LevelOne:..... 30000000000000000000000
Session End LevelOne:. 2419201
bursary LevelTwo:..... 2419201

This demonstrates that the expected value of bursary has been overwritten by the value of sessionEnd, confirming a storage collision caused by the incorrect redefinition of the layout in LevelTwo.

Tools Used:

Manual Review, Foundry

Recommended Mitigation:

To ensure compatibility with the state stored in the proxy, none of the existing storage variables should be removed in the new implementation (LevelTwo). All variables from LevelOne must be preserved in the same order.

If new variables need to be added, they should only be appended at the end of the layout, after the last existing variable. This practice ensures that previous values are not overwritten and the contract's state remains intact.

Constants (constant) are not part of the storage and do not affect slot layout.

address principal;
bool inSession;
+ uint256 schoolFees;
+ uint256 public immutable 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[] listOfStudents;
address[] listOfTeachers;
Updates

Lead Judging Commences

yeahchibyke Lead Judge 6 months ago
Submission Judgement Published
Validated
Assigned finding tags:

storage collision

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.