Incorrect Data Type Handling in `writeVarInt` Function

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x0fad636e222fe8446b48b613e8af237bc7fdfcce8e5c59d45c3d4c88501aa3a6 Severity: medium

Description: Description\ There is a bug in the writeVarInt function of the Buffer.sol contract. The function incorrectly calls Endian.reverse32 with a uint16 argument instead of a uint32 argument. This can lead to incorrect data being written to the buffer and potential out-of-bounds errors.

Attack Scenario\ Describe how the vulnerability can be exploited.

Attachments

1. Proof of Concept (PoC) File

   function writeVarInt(BufferIO memory buffer, uint256 value) internal pure {
       if (value <= 0xFC) {
           write(buffer, bytes.concat(bytes1(uint8(value))));
       } else if (value >= 253 && value <= 0xFFFF) {
           write(buffer, bytes.concat(bytes1(uint8(0xFD))));
           write(buffer, bytes.concat(bytes2(Endian.reverse16(uint16(value)))));
       } else if (value >= 65536 && value <= 0xFFFFFFFF) {
           write(buffer, bytes.concat(bytes1(uint8(0xFE))));
           write(buffer, bytes.concat(bytes4(Endian.reverse32(uint16(value)))));<------
       } else if (value >= 4_294_967_296 && value <= 0xFFFFFFFFFFFFFFFF) {
           write(buffer, bytes.concat(bytes1(uint8(0xFF))));
           write(buffer, bytes.concat(bytes8(Endian.reverse64(uint64(value)))));
       } else {
           revert("Value too large");
       }
   }

2. Allocate a buffer using the alloc function.
3. Attempt to write a variable integer with a value between 65536 and 0xFFFFFFFF using the writeVarInt function.
4. Observe that the data written to the buffer is incorrect due to the improper handling of the data type.

Expected Behavior: The writeVarInt function should correctly handle the data type and write the appropriate bytes to the buffer. Actual Behavior: The writeVarInt function incorrectly calls Endian.reverse32 with a uint16 argument, leading to incorrect data being written to the buffer.

2. Revised Code File (Optional)

   Update the writeVarInt function to call Endian.reverse32 with a uint32 argument instead of a uint16 argument.

[AresAudits]

The same issue of incorrect type usage (uint16 instead of uint32) is present in the StorageWritableBufferStream.sol too.

function writeVarInt(WritableBufferStream storage buffer, uint256 value) internal {
        if (value <= 0xFC) {
            _write(buffer, bytes.concat(bytes1(uint8(value))));
        } else if (value >= 253 && value <= 0xFFFF) {
            _write(buffer, bytes.concat(bytes1(uint8(0xFD))));
            _write(buffer, bytes.concat(bytes2(Endian.reverse16(uint16(value)))));
        } else if (value >= 65536 && value <= 0xFFFFFFFF) {
            _write(buffer, bytes.concat(bytes1(uint8(0xFE))));
            _write(buffer, bytes.concat(bytes4(Endian.reverse32(uint16(value)))));<----
        } else if (value >= 4_294_967_296 && value <= 0xFFFFFFFFFFFFFFFF) {
            _write(buffer, bytes.concat(bytes1(uint8(0xFF))));
            _write(buffer, bytes.concat(bytes8(Endian.reverse64(uint64(value)))));
        } else {
            revert("Value too large");
        }
    }

[party-for-illuminati]

Low severity because it doesn't get to this branch

[AresAudits]

@party-for-illuminati ,Thank you for your response. While I understand that the current flow may not reach this branch(as of now), I believe this issue should still be considered medium severity for the following reasons:

• We cannot be sure that this will not be executed in the future, potentially introducing unexpected behavior or transaction revertions, especially since this is used in crucial functions.

• This issue is also present in another contract, StorageWritableBufferStream.sol, increasing the potential impact.

Considering the potential impact and the presence of this issue in multiple contracts, I believe it deserves a medium severity rating.I would really appreciate it if you could reconsider the severity level.