`Identity` fallback returns too many bytes

[code423n4]

Handle

cmichel

Vulnerability details

The Identity.fallback returns 0x20 = 32 bytes being read from memory position 0, where the first four bytes have been set to the msg.sig.

The functions like onERC1155Received are supposed to return a bytes4 value.

(Also note that the other 28 bytes are not cleared but returned and could affect the result. This does not seem to be possible in this function though.)

Impact

It probably doesn't matter as the other bits are initialized with 0 and the superfluous data is dropped when the caller decodes it as a bytes4.

Recommended Mitigation Steps

We still recommend avoiding assembly here and returning the correct amount of bytes by implementing the three functions (onERC721Received, etc.) as a normal function. It's cleaner.

[Ivshti]

all words are bytes32 sized, so doesn't a regular solidity function that returns bytes4 also return the same thing?

[Ivshti]

this is a wontfix for a few reasons:

1. when doing it with solidity, eg function onERC721Received(address, address, uint256, bytes memory) public returns (bytes4) { return this.onERC721Received.selector; }, solidity also verifies the arguments and performs an ABI parse on it, which wastes gas
2. solidity already pads return data to word size (32 bytes) because it ABI encodes everything, so this function returns the same as a normal solidity function would

[GalloDaSballo]

I also believe this is a non issue as the return value will get padded out

Haven't run the test on gas so won't comment

Downgrading to non-critical