Category: Business Logic
Functions cannot be removed during upgrades
Medium Severity
Medium Impact
Medium Likelihood
Description
In getUpgrade(...),
bytes4[] storage existingSelectors = LibAppStorage
.ds()
.upgradeParam
.existingSelectors[msg.sender];it is populated with null values.
Therefore, the following loop to set the remove functions will never initiate:
for (uint256 i = 0; i < existingSelectors.length; ) {
if (!newSelectorSet[existingSelectors[i]]) {
LibAppStorage.ds().upgradeParam.selectorsRemoved[i].push(
existingSelectors[i]
);
}And the final IDiamondCut.FacetCut[] returned will not contain any of the remove instructions.
Impact
It will not be possible to remove functions from Voyage's interface using the intended functionality. It would be possible, however, to replace them with functions that do not perform any operations. This approach will, however, result in a very cluttered and confusing interface and should be avoided.
Recommendations
Populate existingSelectors by populating adding existingSelectors.push(selector) to the following:
for (uint256 i = 0; i < currentFacets.length; ) {
IDiamondLoupe.Facet memory facet = currentFacets[i];
for (uint256 j = 0; j < facet.functionSelectors.length; ) {
bytes4 selector = facet.functionSelectors[j];
newSelectors.push(selector);
existingSelectorFacetMap[selector] = facet.facetAddress;
existingSelectors.push(selector);
unchecked {
++j;
}
}
unchecked {
++i;
}
}Remediation
The upgrade functionality has been dropped entirely from the project, the issue has therefore been remediated.