Calldata length is not validated in Diamond dispatch logic

Summary

The implementation of the Diamond dispatch logic doesn't validate the length of calldata to ensure the presence of a selector, potentially creating a conflict in which an empty or shorter calldata will be interpreted as a dispatch to a function

Vulnerability Detail

Since the protocol has removed the receive() function from the reference Diamond implementation (see https://github.com/mudgen/diamond-3-hardhat/blob/main/contracts/Diamond.sol#L62), the fallback function will also take calls that have empty calldata.

Under this scenario, msg.sig will still be interpreted as bytes4(0), which will try to incorrectly fetch a selector using this key.

Similarly, it is also possible to specify a non-empty but shorter calldata, anything shorter than 4 bytes will be interpreted as a 4 byte value padded with 0. So for example, 0x112233 will be interpreted as a function selector of value 0x11223300.

Impact

Calls to the diamond proxy with calldata shorter than 4 bytes will incorrectly fetch a selector using a shorter key, which could cause potential conflicts with valid selectors.

Code Snippet

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

Tool used

Manual Review

Recommendation

Ensure the length of the calldata is at least 4 bytes long.

fallback() external payable {
+   require(msg.data.length >= 4, "missing selector");
    ...
}

Duplicate of #183

sherlock-admin2 commented 6 months ago

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

auditsea commented:

Validation is not required