Zaros Part 1
Zaros
DeFiFoundry
60,000 USDC
View results
Previous Next
Submission Details
Severity: low
Valid

Incompatibility of Tree Proxy Pattern with OpenZeppelin's Initializable due to the Usage of Shared `INITIALIZABLE_STORAGE` Slot.

0xpep7

Summary

The tree proxy architecture, designed to facilitate the upgrade of multiple branches in the same release version, faces a critical compatibility issue with OpenZeppelin's Initializable design. This problem will arise in future upgrades because of the shared INITIALIZABLE_STORAGE slot. This incompatibility results in failed upgrades, making the pattern unsuitable for current use with the OpenZeppelin Initializable library.

Vulnerability Details

The root cause of the vulnerability is the misuse of the Initializable library, where multiple initializables are expected while ONLY one INITIALIZABLE_STORAGE slot is used.

// File: src/tree-proxy/leaves/RootUpgrade.sol
// Findings are labeled with '<= FOUND'
206: function initializeRootUpgrade(
...
212: {
213: for (uint256 i; i < initializables.length; i++) { // <= FOUND: multiple initializables are expected whereas ONLY one INITIALIZABLE_STORAGE slot is used
...
223: }

Let us walk through the issue with the following scenario:

  1. Alice, a developer, implements the tree proxy pattern using OpenZeppelin's Initializable library for managing upgradeable contracts.

  2. After the initial deployment, Alice has freshly developed more branches and attempts to add those in the same release version using the initialize function.

  3. Since the RootProxy manages all branches with the same slot of INITIALIZABLE_STORAGE, it is impossible to call initialize on them outside of construction because the INITIALIZABLE_STORAGE._initialed state has been set to 1 after the initial deployment of the RootProxy and address(RootProxy).code.length is not 0 anymore which makes both initialSetup and construction below not being met. This will cause Alice's call to upgrade to fail with InvalidInitialization error.

https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/625fb3c2b2696f1747ba2e72d1e1113066e6c177/contracts/proxy/utils/Initializable.sol#L118-L122

bool construction = initialized == 1 && address(this).code.length == 0;
​
​
if (!initialSetup && !construction) {
revert InvalidInitialization();
}

  1. Alice tries to workaround the issue by calling the reinitialize(version) function but still fails due to this version condition as it strictly requires the version input has to be incremented after each calls. Which means each new/upgraded branch version has to be unique even if they belong to the same release.

  2. Incrementing the version for each new branch contradicts versioning semantics, making upgrades infeasible.

POC

Add following code to 2024-07-zaros/test/integration/tree-proxy/upgrade-branch/upgrade/upgradeBranch_initializables.t.sol and run with forge test -vvv --mt test_addNewInitializables

The PASS result will confirm the upgradability issue.

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.25;
​
import { Base_Test } from "test/Base.t.sol";
import { UpgradeBranch } from "@zaros/tree-proxy/branches/UpgradeBranch.sol";
import { IPerpsEngine } from "@zaros/perpetuals/PerpsEngine.sol";
import { RootProxy } from "@zaros/tree-proxy/RootProxy.sol";
import { Initializable } from "@openzeppelin-upgradeable/proxy/utils/Initializable.sol";
​
import { console } from "forge-std/console.sol";
​
​
contract Upgrade_Initializables_Test is Base_Test {
NewInitialzableBranch newInitialzableBranch;
​
function setUp() public override {
Base_Test.setUp();
changePrank({ msgSender: users.owner.account });
​
newInitialzableBranch = new NewInitialzableBranch();
}
​
function test_addNewInitializables() external {
// Prepare new branch upgrade data for NewInitialzableBranch contract
RootProxy.BranchUpgrade[] memory branchUpgrades = new RootProxy.BranchUpgrade[](1);
address[] memory initializables = new address[](1);
bytes[] memory initializePayloads = new bytes[](1);
​
bytes4[] memory newInitializableBranchSelectors = new bytes4[](1);
newInitializableBranchSelectors[0] = newInitialzableBranch.newSelector.selector;
branchUpgrades[0] = RootProxy.BranchUpgrade({ branch: address(newInitialzableBranch), action: RootProxy.BranchUpgradeAction.Add, selectors: newInitializableBranchSelectors });
initializables[0] = address(newInitialzableBranch);
initializePayloads[0] = abi.encodeWithSelector(newInitialzableBranch.initialize.selector);
​
// @audit: expect a revert due to same Initializable state from initial deployment being used
vm.expectRevert({ revertData: Initializable.InvalidInitialization.selector });
IPerpsEngine(address(perpsEngine)).upgrade(branchUpgrades, initializables, initializePayloads);
}
}
​
contract NewInitialzableBranch is Initializable {
// @audit: initialize will fail due to same slot being used in RootProxy
// and initializer has already executed in the initial deployment
function initialize() external initializer {
}
​
function newSelector() external {
}
}
​

Impact

This vulnerability has a Medium severity because it fundamentally breaks the upgrade process in the tree proxy pattern. The inability to (re)initialize branches with the same version prevents seamless upgrades, potentially leading to significant operational disruptions and increased complexity in version management. This will also reduce the number of potential adaptations of the Tree Proxy architecture.

Tools Used

Foundry test

Recommendations

To mitigate this critical issue, consider customizing the Initializable library to make the INITIALIZABLE_STORAGE slot unique for each new initializable branch, ensuring the Root does not use a conflicted slot. Alternatively, develop a new Initializable pattern compatible with the Tree Proxy pattern.

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

`initializer` modifiers will revert if not deployed in the same constructor

Support

FAQs

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