abi.encodePacked() collision due to dynamic types usage that could lead to

[code423n4]

Lines of code

https://github.com/code-423n4/2023-06-lukso/blob/9dbc96410b3052fc0fd9d423249d1fa42958cae8/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol#L356-L363 https://github.com/code-423n4/2023-06-lukso/blob/9dbc96410b3052fc0fd9d423249d1fa42958cae8/contracts/LSP6KeyManager/LSP6Modules/LSP6SetDataModule.sol#L544-L551

Vulnerability details

Impact

The use of abi.encodePacked() could lead to collision due to the dynamic types usage. Through abi.encodePacked(), Solidity supports a non-standard packed mode where:

• Types shorter than 32 bytes are neither zero padded nor sign extended and
• Dynamic types are encoded in-place and without the length.
• Array elements are padded, but still encoded in-place
• Structs as well as nested arrays are not supported.

abi.encode is used if you are dealing with more than one dynamic data type as it prevents collision. abi.encodePacked takes all types of data and any amount of input.

Proof of Concept

Contract: LSP6KeyManager

File: LSP6KeyManagerCore.sol

Code link: https://github.com/code-423n4/2023-06-lukso/blob/9dbc96410b3052fc0fd9d423249d1fa42958cae8/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol#L356-L363 code: bytes memory encodedMessage = abi.encodePacked( LSP6_VERSION, block.chainid, nonce, validityTimestamps, msgValue, payload );

Contract: LSP6Modules

File: LSP6SetDataModule.sol

Code link: https://github.com/code-423n4/2023-06-lukso/blob/9dbc96410b3052fc0fd9d423249d1fa42958cae8/contracts/LSP6KeyManager/LSP6Modules/LSP6SetDataModule.sol#L544-L551 code: length = uint16( bytes2( abi.encodePacked( allowedERC725YDataKeysCompacted[pointer], allowedERC725YDataKeysCompacted[pointer + 1] ) ) );

Tools Used

Manual Review

Recommended Mitigation Steps

According to solidity documentation, if you use abi.encodePacked for signatures, authentication or data integrity, make sure to always use the same types and check that at most one of them is dynamic. Unless there is a compelling reason, abi.encode should be preferred. Do not use more than one dynamic type in abi.encodePacked(). Instead,use abi.encode() preferably.

Use:

code: bytes memory encodedMessage = abi.encode( LSP6_VERSION, block.chainid, nonce, validityTimestamps, msgValue, payload );

code: length = uint16( bytes2( abi.encode( allowedERC725YDataKeysCompacted[pointer], allowedERC725YDataKeysCompacted[pointer + 1] ) ) );

Assessed type

en/de-code

[c4-pre-sort]

minhquanym marked the issue as primary issue

[c4-pre-sort]

minhquanym marked the issue as low quality report

[c4-sponsor]

CJ42 marked the issue as disagree with severity

[c4-judge]

trust1995 marked the issue as unsatisfactory: Invalid