Submitted by codegpt
There is an insufficient length check when validating the allowed data keys that can be set by a caller. In particular, if a decoded length in the compact bytes array happens to be 0, the caller will be validated against any data key.
Although a decoded length is not possible when setting allowed data keys via the LSP6SetDataModule contract, there is no guarantee that data values before ownership transfer to an LSP6 contract are valid. This allows a scamming opportunity, as malicious actors who set-up an ERC725 contract for another can create a backdoor that cannot be easily seen, as the allowed data keys will simply have an extra 0x0000.
When verifying allowed data keys that a caller can set, a length check is done on the first 2 bytes of a compact bytes array to ensure that the length does not exceed a data keys length.
This length value is used to decide the bit mask when validating data keys.
In the case, that length == 0, then mask == bytes32(0).
Then, the validation check for all data keys will pass, as the validation will be reduced to whether or not allowedKey & bytes32(0) == inputDataKey & bytes32(0), which is always true.
This allows the caller to set data values for all data keys except those used for LSP1, LSP6, and LSP17, due to the initial checks in LSP6SetDataModule._getPermissionRequiredToSetDataKey(). In particular, they would be able to obstruct data values for the following established keys:
Suppose userA and userB wish to share a universal profile and decide to use an LSP6 key manager to manage the profile. userA is decided to set-up the contracts. However, userA is malicious and does the following:
If userB does not trust userA, they are able to check userAs permissions and allowed data keys, but will find an extra 0x0000 in the allowed data keys, which does not appear malicious. However, this allows userA the ability to change the data value for almost all data keys.
The following fuzz test written in foundry shows that after adding 0x0000 to the allowed data keys of malicious, malicious is able to set the data value of most data keys:
It is recommended to add a check requiring that length > 0.
CJ42 (LUKSO) disagreed with severity and commented:
Trust (judge) decreased severity to Medium and commented:
