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

UpgradeBranch Removal Protection Invariant Can Be Broken

0xpep7

Summary

The current implementation of the UpgradeBranch contract contains a critical vulnerability where the intended protection mechanism fails to function as expected. This issue arises because address(this) refers to the proxy address rather than the implementation address, leading to the UpgradeBranch remaining mutable and vulnerable to accidental modifications.

Vulnerability Details

The root cause of the vulnerability is that the condition branch == address(this) always evaluates to false because address(this) refers to the proxy address rather than the UpgradeBranch implementation address.

// File: src/tree-proxy/leaves/RootUpgrade.sol
137: function replaceBranch(Data storage self, address branch, bytes4[] memory selectors) internal {
...
148: revert Errors.SelectorIsZero();
149: }
150: if (oldBranch == address(this)) {// <= FOUND: Always false since address(this) refers to the proxy address
151: revert Errors.ImmutableBranch();
152: }
153: if (oldBranch == branch) {
154: revert Errors.FunctionFromSameBranch(selector);
...
175: }
// File: src/tree-proxy/leaves/RootUpgrade.sol
177: function removeBranch(Data storage self, address branch, bytes4[] memory selectors) internal {
178: if (branch == address(this)) { // <= FOUND: UpgradeBranch is not protected
179: revert Errors.ImmutableBranch();
180: }
181:
182: uint256 cachedSelectorsLength = selectors.length;
...
204: }

POC

The test_removeUpgradeBranch execution result of PASS will confirm UpgradeBranch and its corresponding upgrade selector were removed from the RootProxy.

// 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 { console } from "forge-std/console.sol";
​
​
contract Upgrade_Removal_Test is Base_Test {
​
function setUp() public override {
Base_Test.setUp();
changePrank({ msgSender: users.owner.account });
}
​
function test_removeUpgradeBranch() external {
// Look up UpgradeBranch impl contract via UpgradeBranch.upgrade selector
UpgradeBranch upgradeBranchImpl = UpgradeBranch(IPerpsEngine(address(perpsEngine)).branchAddress(UpgradeBranch.upgrade.selector));
RootProxy.BranchUpgrade[] memory branchUpgrades = new RootProxy.BranchUpgrade[](1);
address[] memory initializables = new address[](0);
bytes[] memory initializePayloads = new bytes[](0);
​
// Remove upgrade selector from upgradeBranch
bytes4[] memory upgradeBranchSelectors = new bytes4[](1);
upgradeBranchSelectors[0] = UpgradeBranch.upgrade.selector;
branchUpgrades[0] = RootProxy.BranchUpgrade({ branch: address(upgradeBranchImpl), action: RootProxy.BranchUpgradeAction.Remove, selectors: upgradeBranchSelectors });
​
IPerpsEngine(address(perpsEngine)).upgrade(branchUpgrades, initializables, initializePayloads);
​
// Lookup upgrade selector via branchAddress
address upgradeBranchByUpgradeSelector = IPerpsEngine(address(perpsEngine)).branchAddress(UpgradeBranch.upgrade.selector);
console.log("upgradeBranch lookup via upgrade.selector", upgradeBranchByUpgradeSelector);
assertEq(upgradeBranchByUpgradeSelector, address(0), "upgradeBranch not removed");
// Lookup selectors for upgrade branch address
bytes4[] memory selectorsByUpgradeBranch = IPerpsEngine(address(perpsEngine)).branchFunctionSelectors(address(upgradeBranchImpl));
assertEq(selectorsByUpgradeBranch.length, 0, "upgradeBranch selectors not removed");
// for (uint256 i; i < selectorsByUpgradeBranch.length; ++i) {
// console.log("selectors lookup via upgrade branch");
// console.logBytes4(selectorsByUpgradeBranch[i]);
// }
}
}

Impact

Even though it requires an admin/eDao mistake to occur, the severity should be Medium because:

1/ Had it occured, the impact is Critical as it would cause significant security risks and potentially rendering the proxy pattern inoperable.

2/ There exists an intended protection mechanism for the UpgradeBranch. Breaking the ImmutableBranch invariant could be considered a valid Medium.

Tools Used

Foundry test

Recommendations

To address this issue, modify the protection mechanism to correctly compare the implementation address rather than the proxy address. Ensure that the implementation address is appropriately protected to prevent accidental or malicious modifications.

Updates

Lead Judging Commences

inallhonesty Lead Judge
about 1 year ago
inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
Assigned finding tags:

`replaceBranch` and `removeBranch` use `address(this)` - Proxy Address - to compare against branch address

Support

FAQs

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