Beanstalk Notion
Beanstalk Notion
📄

Report #12421

Report Date
October 14, 2022
Status
Closed
Payout

Bypassing facets registry in a `farm` function

‣
Report Info

Report ID

#12421

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 _farm function in a FarmFacet contract that performs a delegatecall on facet address. Here is the way how the facet address is calculated:

bytes4 functionSelector;
assembly {
    functionSelector := calldataload(data.offset)
}
address facet = ds
    .selectorToFacetAndPosition[functionSelector]
    .facetAddress;

In case when the length of data array is smaller than 4 bytes calldataload will read the rest from calldata, which in general case can store arbitrary data. So it is possible to manipulate the value of functionSelector in case of such short data array.

Attack scenario

  • Diamond proxy has just a standard selector, like 0x11223344.
  • Vulnerable facet has a fallback function.
  • It is not expected that someone can call this fallback function, since the diamond hasn't been set zero selector as a valid one.

Then it is possible to call _farm with data equals to concatenation of the following parts: 0000000000000000000000000000000000000000000000000000000000000020 (means data offset) 0000000000000000000000000000000000000000000000000000000000000000 (means data length) 11223344 (additional content of calldata using which functionSelector will be determined)

So the diamond proxy will find a facet and make a dellegatecall to the address of facet. Finally, the facet will not recognize empty calldata as a call with some selector and, as result, will execute the fallback funciton logic.

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;

contract FarmFacetMock {
    function _farm(bytes calldata data) public {
        bytes4 functionSelector;
        assembly {
            functionSelector := calldataload(data.offset)
        }

        require(data.length == 0);
        require(functionSelector == bytes4(0x11223344));
    }
}

contract PoC {
    FarmFacetMock public farmFacetMock;

    constructor() {
        farmFacetMock = new FarmFacetMock();
    }

    function hack() external {
        bytes memory data = abi.encodePacked(
            FarmFacetMock._farm.selector,
            bytes32(0x0000000000000000000000000000000000000000000000000000000000000020), // offset
            bytes32(0x0000000000000000000000000000000000000000000000000000000000000000), // length
            bytes32(0x1122334400000000000000000000000000000000000000000000000000000000) // additional calldata
        );
        (bool s, ) = address(farmFacetMock).call(data);
        require(s);
    }
}

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