Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: underflow error for AllowedERC725YKey + allow to set the zero key 0x00000000...00000000 via the Key Manager #226

Merged
merged 1 commit into from
Jul 12, 2022

Conversation

CJ42
Copy link
Member

@CJ42 CJ42 commented Jul 8, 2022

What does this PR introduce?

The AllowedERC725YKey feature in the LSP6 Key Manager currently does not allowed to set 0x0000000000000000000000000000000000000000000000000000000000000000 (= bytes32(0) or 32 x 00 bytes) as an allowed ERC725YKey.

If a controller address has this data key in its list of allowed ERC725Y data key, the check against the allowed data key leads to integer underflow when counting the number of zero trailing bytes 🚫 in the allowed data key (see screenshot below).

unknown

As a result, this can lead the controller to be stuck and prevent the controller address from setting data on the account linked to this KeyManager.

This PR fixes this bug, while also allowing to whitelist any allowed ERC725Y data key by setting the zero-key 0x00000000...00000000 in the list of allowed ERC725Y data keys.

@CJ42 CJ42 added bug Something isn't working enhancement New feature or request labels Jul 8, 2022
@CJ42 CJ42 requested review from frozeman, YamenMerhi and a team July 8, 2022 12:18
Copy link
Member

@YamenMerhi YamenMerhi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

@CJ42 CJ42 merged commit c549873 into develop Jul 12, 2022
@CJ42 CJ42 deleted the fix/zero-allowed-key branch July 12, 2022 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

2 participants