sherlock-audit / 2023-12-ubiquity-judging

2 stars 2 forks source link

Bauchibred - Diamond.sol's `fallback()` seems to be erroneously implemented #160

Closed sherlock-admin2 closed 10 months ago

sherlock-admin2 commented 10 months ago

Bauchibred

medium

Diamond.sol's fallback() seems to be erroneously implemented

Proof of Concept

Take a look at Diamond.sol#L42-L65

    /**
     * @notice Finds facet for function that is called and executes the
     * function if a facet is found and returns any value
     */
     //@audit evidently, function is payable
    fallback() external payable {
        LibDiamond.DiamondStorage storage ds;
        bytes32 position = LibDiamond.DIAMOND_STORAGE_POSITION;
        assembly {
            ds.slot := position
        }
        address facet = ds.selectorToFacetAndPosition[msg.sig].facetAddress;
        require(facet != address(0), "Diamond: Function does not exist");
        //@audit below no value is passed while delegate calling
        assembly {
            calldatacopy(0, 0, calldatasize())
            let result := delegatecall(gas(), facet, 0, calldatasize(), 0, 0)
            returndatacopy(0, 0, returndatasize())
            switch result
            case 0 {
                revert(0, returndatasize())
            }
            default {
                return(0, returndatasize())
            }
        }

As seen thiks function is used toi find a faceet for any fnunction that is called abd theb it later on executes the function after finding the facet while returning the value from the function.

Issue as has been hinted by the @audit tag is that while this function is caling anyfact, being payable some of the function are going to need the msg.value to be passed, but while delegate calling the function msg.value is not passed and as such any function that needs value ends up reverting

Impact

Asides what's been explained in the Proof of Concept, functions that need value passed to them in order to execute are being wrongly queried and protocol can't rightly access this, i.e since no value are forwarded breaks protocol's core functionality

Code Snippet

https://github.com/sherlock-audit/2023-12-ubiquity/blob/d9c39e8dfd5601e7e8db2e4b3390e7d8dff42a8e/ubiquity-dollar/packages/contracts/src/dollar/Diamond.sol#L42-L65

Recommended Mitigation Steps

While delegate calling, pass the msg.value provided

sherlock-admin2 commented 10 months ago

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

auditsea commented:

No facet functions require native value

sherlock-admin2 commented 10 months ago

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

auditsea commented:

No facet functions require native value

nevillehuang commented 10 months ago

Invalid, native eth is not supported as a collateral type