code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-06-lukso/blob/9dbc96410b3052fc0fd9d423249d1fa42958cae8/contracts/LSP20CallVerification/LSP20CallVerification.sol#L80

Vulnerability details

Impact

Let's break down this part of the function:

if (
    returnedData.length < 32 ||
    bytes28(bytes32(returnedData) << 32) != bytes28(0)
) revert LSP20InvalidMagicValue(postCall, returnedData);

This if statement is intended to do two things, as indicated by the two conditions separated by || (OR):

1) Check if returnedData.length is less than 32. If it is, the function reverts because the returned data should contain at least 32 bytes.

2) bytes32(returnedData): This casts returnedData to a bytes32 type. If returnedData is less than 32 bytes, it's right-padded with zeros to reach 32 bytes. If it's more than 32 bytes, only the first 32 bytes are taken. bytes32(returnedData) << 32: This shifts the bytes32 value 32 bits to the left, which effectively discards the leftmost 4 bytes (32 bits) of the bytes32 value and shifts the remaining bytes to the left, filling in zeros from the right. bytes28(bytes32(returnedData) << 32): This casts the result to bytes28, effectively taking the leftmost 28 bytes of the bytes32 value. bytes28(bytes32(returnedData) << 32) != bytes28(0): This checks if the bytes28 value is not all zeros. Now, let's consider the comments above the if statement, which suggest that the purpose of this condition is to check if the first 4 bytes of returnedData (when interpreted as a bytes32 value) contain a non-zero bytes4 value.

The current code does not fulfill this purpose because the left shift operation discards these 4 bytes, so they are not part of the bytes28 value that is being checked. In other words, the condition is not checking the right part of the data.

The correct approach would be to right-shift the bytes32 value by 224 bits (which is equivalent to 28 bytes), so that the leftmost 4 bytes (which contain the potential bytes4 value) are moved to the rightmost part of the bytes32 value. Then, you can cast the result to bytes4 and check if it's not equal to bytes4(0), like so:

if (
    returnedData.length < 32 ||
    bytes4(bytes32(returnedData) >> 224) != bytes4(0)
) revert LSP20InvalidMagicValue(postCall, returnedData);

This code correctly checks if the first 4 bytes of returnedData contain a non-zero bytes4 value. If returnedData is less than 32 bytes, or its first 4 bytes are all zeros when interpreted as a bytes4 value, the function will revert.