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

UpgradeBranch.sol does not use _disableInitializers()

AuditTemple

Summary

UpgradeBranch.sol does not disable initializers which allows a third party to become the owner of this implementation contract.

Vulnerability Details

UpgradeBranch.sol is one of the implementation contracts that the rootProxy will point to once the rootProxy gets deployed. Since UpgradeBranch.sol does not disable initializers in its constructor, this opens the possibility for a third party to make himself the owner of UpgradeBranch.sol by calling initialize().

contract UpgradeBranch is Initializable, OwnableUpgradeable {
using RootUpgrade for RootUpgrade.Data;
​
@> function initialize(address owner) external initializer {
__Ownable_init(owner);
}
}
​

The problem here is that the UpgradeBranch itself contains the upgrade logic function:

function upgrade(
RootProxy.BranchUpgrade[] memory branchUpgrades,
address[] memory initializables,
bytes[] memory initializePayloads
)
external
{
_authorizeUpgrade(branchUpgrades);
RootUpgrade.Data storage rootUpgrade = RootUpgrade.load();
​
rootUpgrade.upgrade(branchUpgrades, initializables, initializePayloads);
}

It executes a delegatecall to any address, which means that the account that takes over the implementation can provide a malicious contract with selfdestruct()and destroy the implementation. The problem here is that the implementation contract itself is used to execute the upgrade logic( UUPS) instead the logic living on the Proxy (Transperent Upgradeable Proxy).

Sad simply, In order for the proxy to upgrade to another contract it relies on the implementation of UpgradeBranch, but if the implementation gets destroyed, there would be no way (no logic) to update to another contract or simply replace it ( as would be possible if the upgrade logic lives in the proxy itself)

Impact

Implementation can get destoyed, compromising future updates

Tools Used

Manual Review

Recommended Mitigation

Make the following changes to UpgradeBranch.sol :

contract UpgradeBranch is Initializable, OwnableUpgradeable {
using RootUpgrade for RootUpgrade.Data;
+ constructor() {
+ _disableInitializers();
+ }
​
function initialize(address owner) external initializer {
__Ownable_init(owner);
}
​
function upgrade(
RootProxy.BranchUpgrade[] memory branchUpgrades,
address[] memory initializables,
bytes[] memory initializePayloads
)
external
{
_authorizeUpgrade(branchUpgrades);
RootUpgrade.Data storage rootUpgrade = RootUpgrade.load();
​
rootUpgrade.upgrade(branchUpgrades, initializables, initializePayloads);
}
​
function _authorizeUpgrade(RootProxy.BranchUpgrade[] memory) internal onlyOwner { }
}
​
Updates

Lead Judging Commences

inallhonesty Lead Judge over 1 year 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.

Give us feedback!