sherlock-audit / 2023-12-ubiquity-judging

2 stars 2 forks source link

bareli - Function Selector Clashes: #219

Closed sherlock-admin closed 9 months ago

sherlock-admin commented 9 months ago

bareli

medium

Function Selector Clashes:

Summary

Function Selector Clashes: The code assumes that function selectors are unique and does not handle the case where two different functions could have the same selector due to a hash collision. This is a theoretical risk, as collisions are highly unlikely, but it's something to be aware of.

Vulnerability Detail

function addFunctions( address _facetAddress, bytes4[] memory _functionSelectors ) internal { require( _functionSelectors.length > 0, "LibDiamondCut: No selectors in facet to cut" ); DiamondStorage storage ds = diamondStorage(); require( _facetAddress != address(0), "LibDiamondCut: Add facet can't be address(0)" ); uint96 selectorPosition = uint96( ds.facetFunctionSelectors[_facetAddress].functionSelectors.length ); // add new facet address if it does not exist if (selectorPosition == 0) { addFacet(ds, _facetAddress); } for ( uint256 selectorIndex; selectorIndex < _functionSelectors.length; selectorIndex++ ) { bytes4 selector = _functionSelectors[selectorIndex]; address oldFacetAddress = ds .selectorToFacetAndPosition[selector] .facetAddress; require( oldFacetAddress == address(0), "LibDiamondCut: Can't add function that already exists" ); addFunction(ds, selector, selectorPosition, _facetAddress); selectorPosition++; } }

Impact

function selectors are unique and does not handle the case where two different functions could have the same selector due to a hash collision. This is a theoretical risk, as collisions are highly unlikely, but it's something to be aware of.

Code Snippet

https://github.com/sherlock-audit/2023-12-ubiquity/blob/main/ubiquity-dollar/packages/contracts/src/dollar/libraries/LibDiamond.sol#L154

Tool used

Manual Review

Recommendation

molecula451 commented 9 months ago

Invalid, we are aware when a function selector clash, and we have dealt with this before

sherlock-admin2 commented 8 months ago

1 comment(s) were left on this issue during the judging contest.

auditsea commented:

Unlikely happen

sherlock-admin2 commented 8 months ago

1 comment(s) were left on this issue during the judging contest.

auditsea commented:

Unlikely happen

nevillehuang commented 8 months ago

Invalid, no issue highlighted here, simply speculating on function selector clash without proof of its possibility.