The diamond dispatch logic present in the DiamondProxy contract, used to route functions call, contains a clashing between two different scenarios, causing an invocation with empty calldata (usually meant to transfer ETH) to be interpreted as a function call to a function whose selector has the zero value.
The ZkSync Era main entrypoint in Ethereum is implemented using the Diamond pattern. Under this model, function calls are routed to the proper facet (i.e. a module) based on the function selector present in calldata (first 4 bytes, or msg.sig).
This routing is implemented in the DiamondProxy contract:
20: fallback() external payable {
21: Diamond.DiamondStorage storage diamondStorage = Diamond.getDiamondStorage();
22: // Check whether the data contains a "full" selector or it is empty.
23: // Required because Diamond proxy finds a facet by function signature,
24: // which is not defined for data length in range [1, 3].
25: require(msg.data.length >= 4 || msg.data.length == 0, "Ut");
26: // Get facet from function selector
27: Diamond.SelectorToFacet memory facet = diamondStorage.selectorToFacet[msg.sig];
28: address facetAddress = facet.facetAddress;
29:
30: require(facetAddress != address(0), "F"); // Proxy has no facet for this selector
31: require(!diamondStorage.isFrozen || !facet.isFreezable, "q1"); // Facet is frozen
32:
33: assembly {
34: // The pointer to the free memory slot
35: let ptr := mload(0x40)
36: // Copy function signature and arguments from calldata at zero position into memory at pointer position
37: calldatacopy(ptr, 0, calldatasize())
38: // Delegatecall method of the implementation contract returns 0 on error
39: let result := delegatecall(gas(), facetAddress, ptr, calldatasize(), 0, 0)
40: // Get the size of the last return data
41: let size := returndatasize()
42: // Copy the size length of bytes from return data at zero position to pointer position
43: returndatacopy(ptr, 0, size)
44: // Depending on the result value
45: switch result
46: case 0 {
47: // End execution and revert state changes
48: revert(ptr, size)
49: }
50: default {
51: // Return data with length of size at pointers position
52: return(ptr, size)
53: }
54: }
55: }As we can see in the previous snippet of code, line 27 fetches the facet based on the msg.sig value. If this selector is correctly mapped (line 30) and the facet is not frozen (line 31), then the call is dispatched to the facet using a delegatecall, similar to standard proxy pattern (lines 33-54).
It should be noted that calls can potentially have empty calldata. This is in fact a supported action by the diamond, since the check present in line 25 allows a calldata length of zero (require(msg.data.length >= 4 || msg.data.length == 0, "Ut")). In this scenario, msg.sig will be bytes4(0), and the implementation will behave exactly as if it were a function call to the zero selector (i.e. when calldata = 0x00000000...). The issue leads to a clash between a call with empty calldata and a real function call to a function whose selector value is zero.
The described conflict has the following impact:
- A call with empty
calldata(e.g. a transfer of ETH), will be interpreted as a call to a function whose selector is zero. The implementation will query the facet in line 27, and will ultimately be reverted by the check in line 30. - In the event there is a registered function whose selector is zero (unlikely, but possible), a call with empty
calldatawill be mistakenly routed to the registered facet for the selector with zero value.
In the following test, the diamond receives a call with empty calldata (this is an ETH transfer) that is incorrectly interpreted as a function call to a function with zero selector. The transaction fails due to the check in line 30 with reason "F", as there is no facet associated with a selector of value zero.
function test_Diamond_FailsEmptyCalldata() public {
// Build a diamond, this will be empty but is enough to demo the issue
AllowList allowList = new AllowList(owner);
DiamondInit diamondInit = new DiamondInit();
bytes8 dummyHash = 0x1234567890123456;
address dummyAddress = makeAddr("dummyAddress");
bytes memory diamondInitData = abi.encodeWithSelector(
diamondInit.initialize.selector,
dummyAddress, //verifier
owner,
owner,
0,
0,
0,
allowList,
VerifierParams({recursionNodeLevelVkHash: 0, recursionLeafLevelVkHash: 0, recursionCircuitsSetVksHash: 0}),
false,
dummyHash,
dummyHash,
1000000
);
Diamond.FacetCut[] memory facetCuts = new Diamond.FacetCut[](0);
Diamond.DiamondCutData memory diamondCutData = Diamond.DiamondCutData({
facetCuts: facetCuts,
initAddress: address(diamondInit),
initCalldata: diamondInitData
});
uint256 chainId = block.chainid;
DiamondProxy diamondProxy = new DiamondProxy(chainId, diamondCutData);
vm.deal(user, 1 ether);
vm.prank(user);
(bool success, bytes memory response) = address(diamondProxy).call{value: 1 ether}("");
// this fails, ETH transfer will be associated with a call to a function with a zero selector and fail
assertFalse(success);
}The key question here is who receives a call with empty calldata, since a diamond is expected to contain multiple facets. The following are different alternatives depending on the intended use case:
- Have the DiamondProxy receive the call without actually invoking any facet. This will solve the clashing issue, while providing a behavior similar to having an empty
receive()function. - Have a way to register which facet should handle this type of invocations. When receiving a call with
msg.data.length == 0, route the call to the registered facet. - Similarly to the previous point, instead of registering a facet, have a default facet to handle this, i.e. ReceiveFacet. When receiving a call with
msg.data.length == 0, route the call to this default facet. - If these types of calls should not be needed, then prevent calls with empty
calldata(i.e. disallow calls withmsg.data.length == 0) to avoid the potential clashing with a function with zero value selector.