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

Selector Collision Vulnerability in `RootUpgrade` Library

kevinkkien

Summary

The addBranch and replaceBranch functions in the RootUpgrade library do not check for potential selector collisions. This means that different branches could be assigned the same function selectors, leading to incorrect function calls and unexpected behavior in the proxy contract.

Vulnerability Details

In the addBranch and replaceBranch functions, new branches are added or existing branches are replaced with new selectors. However, there is no mechanism to verify that these selectors are not already in use by other branches. If a collision occurs, where the same selector is associated with multiple branches, the proxy contract will not be able to determine which branch's function to call, resulting in unpredictable behavior.

https://github.com/Cyfrin/2024-07-zaros/blob/main/src/tree-proxy/leaves/RootUpgrade.sol#L115-L135

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)) {
revert Errors.FunctionAlreadyExists(selector);
}
​
self.selectorToBranch[selector] = branch;
// slither-disable-next-line unused-return
self.branchSelectors[branch].add(selector);
}
}

https://github.com/Cyfrin/2024-07-zaros/blob/main/src/tree-proxy/leaves/RootUpgrade.sol#L137-L175

function replaceBranch(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];
address oldBranch = self.selectorToBranch[selector];
​
if (selector == bytes4(0)) {
revert Errors.SelectorIsZero();
}
if (oldBranch == address(this)) {
revert Errors.ImmutableBranch();
}
if (oldBranch == branch) {
revert Errors.FunctionFromSameBranch(selector);
}
if (oldBranch == address(0)) {
revert Errors.NonExistingFunction(selector);
}
​
// overwrite selector to new branch
self.selectorToBranch[selector] = branch;
​
// slither-disable-next-line unused-return
self.branchSelectors[branch].add(selector);
​
// slither-disable-next-line unused-return
self.branchSelectors[oldBranch].remove(selector);
​
// if no more selectors, remove old branch address
if (self.branchSelectors[oldBranch].length() == 0) {
// slither-disable-next-line unused-return
self.branches.remove(oldBranch);
}
}
}

Impact

If a selector collision occurs, the proxy contract may call the wrong function implementation, leading to unexpected and potentially harmful behavior.

Tools Used

Manual Review

Recommendations

Add logic to the addBranch and replaceBranch functions to check if the new selectors are already associated with other branches. If a collision is detected, revert the transaction or handle it gracefully.

function addBranch(Data storage self, address branch, bytes4[] memory selectors) internal onlyOwner {
// ...
​
for (uint256 i; i < cachedSelectorsLength; i++) {
bytes4 selector = selectors[i];
​
// ...
​
// Check for selector conflicts
if (self.selectorToBranch[selector] != address(0)) {
revert Errors.FunctionAlreadyExists(selector);
}
​
// ...
}
}
​
function replaceBranch(Data storage self, address branch, bytes4[] memory selectors) internal onlyOwner {
// ...
​
for (uint256 i; i < cachedSelectorsLength; i++) {
bytes4 selector = selectors[i];
​
// ... (
​
// Check for selector conflicts on other branches
if (self.selectorToBranch[selector] != address(0) &&
self.selectorToBranch[selector] != oldBranch) { // Only overwriting of the same branch is allowed
revert Errors.FunctionAlreadyExists(selector);
}
​
// ...
}
}
Updates

Lead Judging Commences

inallhonesty Lead Judge
10 months ago
inallhonesty Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
Assigned finding tags:

Function Selector Collision Vulnerability in `RootUpgrade::replaceBranch` Library

Support

FAQs

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