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

The deployment of the perpsEngin will fail because both the `GlobalConfigurationBranch` and the `UpgradeBranch` have the same function selectors

benrai

Lines of code

https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/GlobalConfigurationBranch.sol#L24

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

https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/tree-proxy/leaves/RootUpgrade.sol#L127-L129

Impact

Because both the GlobalConfigurationBranch and the UpgradeBranch inherit the functions of OwnableUpgradeable, they both have the same function selector. Since the selectorToBranch mapping in the PerpsEngine can only hold one branch per function selector, trying to add the second branch will revert and therefore the deployment of the PerpsEngine will revert.

Proof of Concept

The PerpsEngine contract uses the Tree Proxy Pattern. A RootProxy and multiple branches store the smart contract functions. Each branch holds a set of functions with the corresponding function selectors. When calling the PerpsEngine, the fallback function is activated and the call is delegated to the corresponding branch based on the provided function selector. To determine which branch to delegate the call to, the RootProxy utilizes the selectorToBranch mapping, which maps a function selector to the corresponding branch address. This mapping is initialized during deployment.

The issue arises from the fact that both GlobalConfigurationBranch and the UpgradeBranch inherit external functions from the OwnableUpgradeable contract and therefore share the same function selectors, for example transferOwnership(address). Since in the selectorToBranch mapping only one branch can be mapped to a specific branch, adding the second branch will revert resulting in a failed deployment of the PerpsEngine.

function addBranch(Data storage self, address branch, bytes4[] memory selectors) internal {
// slither-disable-next-line unused-return
self.branches.add(branch);
​
uint256 cachedSelectorsLength = selectors.length;
​
for (uint256 i; i < cachedSelectorsLength; i++) {
bytes4 selector = selectors[i];
​
if (selector == bytes4(0)) {
revert Errors.SelectorIsZero();
}
if (self.selectorToBranch[selector] != address(0)) { <==@audit will revert here because of dublicat selector
revert Errors.FunctionAlreadyExists(selector);
}
​
self.selectorToBranch[selector] = branch;
// slither-disable-next-line unused-return
self.branchSelectors[branch].add(selector);
}
}

Recommended Mitigation Steps

To ensure the full functionality of the inherited OwnableUpgradeable contract for both the GlobalConfigurationBranch and the UpgradeBranch, consider adding additional functions to the branch contracts which call the duplicate functions. Only add the function selectors of those new functions to the selectorToBranch mapping.

For example, add a transferOwnershipGlobalConf function to the GlobalConfigurationBranch which calls the transferOwnership function:

function transferOwnershipGlobalConf(address newOwner) public onlyOwner{
transferOwnership(newOwner);
}
Updates

Lead Judging Commences

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

Support

FAQs

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