sherlock-audit / 2024-02-rubicon-finance-judging

5 stars 3 forks source link

Tychai0s - Lack of Storage gap in ProxyConstructor.sol #23

Closed sherlock-admin2 closed 7 months ago

sherlock-admin2 commented 7 months ago

Tychai0s

medium

Lack of Storage gap in ProxyConstructor.sol

Title

Audit Report: Storage Slot Clash Risk in ProxyConstructor.sol

Summary

This report highlights a vulnerability in the ProxyConstructor.sol smart contract, specifically related to the lack of a storage gap after the initialized variable.

Affected Code

https://github.com/sherlock-audit/2024-02-rubicon-finance/blob/main/gladius-contracts-internal/src/lib/ProxyConstructor.sol#L7

Vulnerability Detail

In ProxyConstructor.sol:7, the contract defines an initialized boolean variable without a subsequent storage gap. This poses a risk for future upgrades; if new variables are added after initialized, their storage slots may clash with those of variables in inheriting contracts. This can lead to unpredictable behavior and data loss.

Impact

The absence of a storage gap following the initialized variable can result in severe issues during contract upgrades, including but not limited to data corruption, loss of funds, or unintended contract behavior. This vulnerability undermines the upgradability feature by introducing potential conflicts in storage layout, which could be exploited or result in accidental loss of data integrity.

Code Snippet

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

abstract contract ProxyConstructor {
    error AlreadyInitialized();

    // @audit-issue L-02 There is no storage gap, if a variable is added after the `initialized` variable in future upgrades it's storage slot will clash with other variables in the inheriting contracts.
    bool public initialized;

    function initialize(address addr0, address addr1) external virtual {}
}

Tools Used

Manual Review

Recommendation

To mitigate this risk, it is recommended to introduce a storage gap immediately after the initialized variable. A common practice is to allocate one or more slots (e.g., uint256[50] private __gap;) as a buffer between the end of the last defined variable and the start of the next contract's storage layout. This strategy ensures that future variables added to the contract or its inheritors do not clash with existing storage slots, preserving data integrity across upgrades.

sherlock-admin commented 7 months ago

3 comment(s) were left on this issue during the judging contest.

tsvetanovv commented:

Low. "Storage Slot Collision issue" is Low according to the Sherlock documentation

PNS commented:

Simple contracts with one of the parent contract not implementing storage gaps are considered low/informational.

0xAadi commented:

Invalid: OOS