Zaros Part 1

Severity: Medium

Summary

The RootUpgrade::replace and RootUpgrade::remove functions use address(this) to access the address of the current contract, UpgradeBranch. Because the functions are executed in a delegateCall, address(this) returns the RootProxy address instead of the UpgradeBranch.

Vulnerability Details

The RootUpgrade::replace and RootUpgrade::remove functions are intended to ensure that the UpgradeBranch branch's selectors cannot be replaced nor removed. This is to prevent owner from accidentally removing the selector for upgrade function, if removed then the upgrade function cannot be called and the upgrade functionality will be bricked.

The functions compare the target branch with the address(this) to determine if the UpgradeBranch is being modified.

Because the functions are executed in a delegatecall from the RootProxy, address(this) will return RootProxy address not the address of UpgradeBranch. As a result, the comparison will not succeed allowing for possibility of bricking the upgradeability feature.

Snippet of RootUpgrade::replace using address(this): RootUpgrade.sol#L150-L152

Snippet of RootUpgrade::remove using address(this): RootUpgrade.sol#L178-L180

Impact

The RootUpgrade::replace, and RootUpgrade::remove functions fail to prevent modifying functions in UpgradeBranch branch allowing for the owner to brick the upgradeability feature.

Tools Used

Manual Review

Recommendations

Add an immutable variable to UpgradeBranch contract and initialze it with address(this) in the constructor. Use the immutable variable value to perform the comparison in the vulnerable functions instead of address(this).