user with ADDEXTENSIONS and CHANGEEXTENSIONS will remove extension unintentional

[code423n4]

Lines of code

https://github.com/code-423n4/2023-06-lukso/blob/9dbc96410b3052fc0fd9d423249d1fa42958cae8/contracts/LSP6KeyManager/LSP6Modules/LSP6SetDataModule.sol#L302-L318

Vulnerability details

Summary

Adding extension use 4 bytes function selector to add new extension, and if user with ADDEXTENSIONS permission also has CHANGEEXTENSIONS permission and wants to add new extension and there is an extension with that function selector, extension will be removed unintentional.

Impact

• extension will be removed unintentional

Proof of Concept

user with ADDEXTENSIONS permission and CHANGEEXTENSIONS permission wants to add new extension(not changing it) and LSP6SetDataModule.sol checks that user has both permissions or not, so it will pass, and extension will be removed.

            // 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);
            }

            return
                _getPermissionToSetLSP17Extension(
                    controlledContract,
                    inputDataKey
                );

Tools Used

manual

Recommended Mitigation Steps

Don't pass if the user has both permission, and check that is there any extension with those function selector or not

Assessed type

Other

[c4-pre-sort]

minhquanym marked the issue as primary issue

[c4-sponsor]

CJ42 marked the issue as sponsor disputed

[CJ42]

This is intended behaviour. A controller with both ADD/CHANGE EXTENSION permission can do any of these behaviours:

• add a new address for a selector.
• edit an address for a selector.
• remove an address for a selector.

[c4-judge]

trust1995 marked the issue as unsatisfactory: Invalid