No Storage Gap for Upgradeable Contracts

Summary

Storage of Boosted3TokenAuraVault and MetaStable2TokenAuraVault vaults might be corrupted during an upgrade.

Vulnerability Detail

For upgradeable contracts, having a storage gap is crucial. This gap enables developers to add new state variables in the future without disrupting the storage compatibility of existing deployments. Without storage gap, updating the implementation code could become extremely challenging.

If there is no storage gap, any new variables added to the base contract might overwrite existing variables in the child contract. This could lead to unintended and potentially severe consequences for the child contracts.

check this: https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable

Impact

any new variables added to the base contract might overwrite existing variables.

Code Snippet

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.25;

import "@openzeppelin/contracts/access/Ownable2Step.sol";

event ReplaceImplementationStarted(address indexed previousImplementation, address indexed newImplementation);
event ReplaceImplementation(address indexed previousImplementation, address indexed newImplementation);
error Unauthorized();

contract Upgradeable2Step is Ownable2Step {
    address public pendingImplementation;
    address public implementation;

    constructor() Ownable(msg.sender) {}

    // called on an inheriting proxy contract
    function replaceImplementation(address impl_) public onlyOwner {
        pendingImplementation = impl_;
        emit ReplaceImplementationStarted(implementation, impl_);
    }

    // called from an inheriting implementation contract
    function acceptImplementation() public {
        if (msg.sender != pendingImplementation) {
            revert OwnableUnauthorizedAccount(msg.sender);
        }
        emit ReplaceImplementation(implementation, msg.sender);
        delete pendingImplementation;
        implementation = msg.sender;
    }

    // called on an inheriting implementation contract
    function becomeImplementation(Upgradeable2Step proxy) public {
        if (msg.sender != proxy.owner()) {
            revert Unauthorized();
        }
        proxy.acceptImplementation();
    }
}

https://github.com/sherlock-audit/2024-05-sophon/blob/05059e53755f24ae9e3a3bb2996de15df0289a6c/farming-contracts/contracts/proxies/Upgradeable2Step.sol#L1-L39

Tool used

Manual Review

Recommendation

Its recommended to add an appropriate storage gap at the end of upgradeable contracts, as demonstrated below. For further guidance, please refer to OpenZeppelin's upgradeable contract templates.

uint256[50] private __gap;

sherlock-audit / 2024-05-sophon-judging

Avci - No Storage Gap for Upgradeable Contracts #218