code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-06-lukso/blob/main/contracts/LSP6KeyManager/LSP6Modules/LSP6SetDataModule.sol#L222

Vulnerability details

Impact

The below situations do not have checks on their inputs:

When bytes12(inputDataKey) == _LSP6KEY_ADDRESSPERMISSIONS_PERMISSIONS_PREFIX:

            // AddressPermissions:Permissions:<address>
            if (
                bytes12(inputDataKey) ==
                _LSP6KEY_ADDRESSPERMISSIONS_PERMISSIONS_PREFIX
            ) {
                // controller already has the permissions needed. Do not run internal function.
                if (hasBothAddControllerAndEditPermissions) return (bytes32(0));

According to LSP6, inputDataValue is expected to be exactly 32 bytes.

When inputDataKey == _LSP1_UNIVERSAL_RECEIVER_DELEGATE_KEY || bytes12(inputDataKey) == _LSP1_UNIVERSAL_RECEIVER_DELEGATE_PREFIX:

            // LSP1UniversalReceiverDelegate or LSP1UniversalReceiverDelegate:<typeId>
        } else if (
            inputDataKey == _LSP1_UNIVERSAL_RECEIVER_DELEGATE_KEY ||
            bytes12(inputDataKey) == _LSP1_UNIVERSAL_RECEIVER_DELEGATE_PREFIX
        ) {
            // same as above. If controller has both permissions, do not read the `target` storage
            // to save gas by avoiding an extra external `view` call.
            if (
                controllerPermissions.hasPermission(
                    _PERMISSION_ADDUNIVERSALRECEIVERDELEGATE |
                        _PERMISSION_CHANGEUNIVERSALRECEIVERDELEGATE
                )
            ) {
                return bytes32(0);

According to LSP1, inputDataValue is expected to be exactly 20 bytes (an address).

When bytes12(inputDataKey) == _LSP17_EXTENSION_PREFIX:

            // LSP17Extension:<bytes4>
        } else if (bytes12(inputDataKey) == _LSP17_EXTENSION_PREFIX) {
            // same as above. If controller has both permissions, do not read the `target` storage
            // to save gas by avoiding an extra external `view` call.
            if (
                controllerPermissions.hasPermission(
                    _PERMISSION_ADDEXTENSIONS | _PERMISSION_CHANGEEXTENSIONS
                )
            ) {
                return bytes32(0);

According to LSP17, inputDataValue is expected to be exactly 20 bytes (an address).

When inputDataKey == _LSP6KEY_ADDRESSPERMISSIONS_ARRAY:

        // AddressPermissions[] -> array length
        if (inputDataKey == _LSP6KEY_ADDRESSPERMISSIONS_ARRAY) {
            // if the controller already has both permissions from one of the two required,
            // No permission required as CHECK is already done. We don't need to read `target` storage.
            if (hasBothAddControllerAndEditPermissions) return bytes32(0);

According to LSP2, inputDataValue is expected to be exactly 16 bytes (the array length).