Critical Risk of Storage Collision in Upgradable Contract Due to Inadequate Storage Management

[ptisserand]

From https://codehawks.cyfrin.io/c/2024-07-ark-project/s/435

The Starklane contract, designed to be an upgradable contract, inherits from multiple contracts (UUPSOwnableProxied, StarklaneState, StarklaneEscrow, StarklaneMessaging, and CollectionManager). Each of these contracts defines its own set of storage variables, but none of them reserve storage gaps for future upgrades. This makes it impossible to safely add new variables to any of the inherited contracts without causing storage collisions. The Starklane contract itself is the only safe place to add new variables, significantly limiting the flexibility of future upgrades and increasing the risk of storage collision.

Unit tests must be provided.

[Iwueseiter]

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

I'm a frontend and smart contract developer. I've contributed to Projects here on onlydust
and with that experience, I would handle this task as expected. This would also be my first time
contributing to this project.

How I plan on tackling this issue

1. Introduce Storage Gaps
   A widely accepted approach is to introduce storage gaps in each contract involved.
   Storage gaps allow for future upgrades without risking storage collisions.
   I will can implement this by adding a fixed-size array of reserved slots at the end of
   the storage layout of each contract.

2. Modify Inherited Contracts
   Apply the same storage gap strategy to the other contracts
   that Starklane inherits from (e.g., StarklaneEscrow, StarklaneMessaging, etc.).
   Each contract should have its own reserved storage for future variables.

3. Delegate New Storage Variables to Starklane
   If reserving storage gaps is not feasible in the inherited contracts for any reason,
   I could keep all new storage variables in the Starklane contract itself, since it is
   the root contract.

4. Use a Custom Storage Pattern
   Another advanced solution is to isolate storage for each module using a custom storage
   pattern. Each contract can define its own storage structure, where the storage
   variables are accessed by a dedicated unique identifier (like a struct in a mapping).

5. Check for Storage Collisions Before Upgrading
   To ensure safety during future upgrades, I will use tools such as OpenZeppelin's Upgrades Plugins. They can detect potential storage layout collisions
   when upgrading smart contracts and help avoid unintended consequences.

[aji70]

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

i'm a solidity and cairo smart contract developer with over 2 years experience and believe i have the skill set for the task and i am also very good with smart contract testing and have worked with upgradable smart contracts in the past

[DanielEmmanuel1]

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

GM GM ArkProject My name is Deon and I'd like to apply formally for the task presented. I am a Web and blockchain engineer with a passion for building user interfaces and Dapps that deliver meaningful experiences. With a background in Computer Science (BSc) and hands-on experience. If given the chance to contribute this will be my second official contribution via onlydust and I'm confident in my ability to deliver on the feature you're looking for.

How I plan on tackling this issue

1. Firstly I will analyze the inheritance structure of the Starklane contract and how storage slots are currently assigned to variables from inherited contracts like UUPSOwnableProxied, StarklaneState, StarklaneEscrow, and others. This will help identify areas where storage collisions could occur when upgrading the contract.

2. I will add storage gaps (e.g., uint256[50] private ______gap;) in each inherited contract. These reserved storage slots will allow safe future upgrades without risking storage overlap, providing flexibility for adding new variables.

3. I will explicitly assign storage slots to critical variables across the inherited contracts. By manually specifying storage positions, I can prevent accidental overwriting of data when new variables are introduced.

4. I will also write unit tests to simulate contract upgrades and validate that no storage collisions occur. This includes ensuring that new variables can be safely added without causing data corruption or breaking the functionality of inherited contracts.

5. Finally I will ensure all contracts in the inheritance chain are covered, with a focus on safe storage management and upgradeability. This will mitigate the risk of data corruption or storage collisions in future upgrades.

With this approach, I will prevent storage collisions and ensure safe upgrades to the upgradable contract.

[onlydustapp]

The maintainer ptisserand has assigned Iwueseiter to this issue via OnlyDust Platform.
Good luck!

[ptisserand]

Hi @Iwueseiter any update on this ?

[Iwueseiter]

Hi @Iwueseiter any update on this ?

I’ve started working on it.
I was going to reach out because I have a question.
My task involves calculating storage slots so is the unit test necessary?

I’m unable to message you and the telegram group as well. Please leave a message so I’d be able to text you.