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 governor related issue reported by C4 #742

Merged
merged 1 commit into from
Dec 21, 2023

Conversation

rayeaster
Copy link
Contributor

Copy link
Collaborator

@GalloDaSballo GalloDaSballo left a comment

Choose a reason for hiding this comment

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

NVM - The Fix in this PR avoids the bug introduced by the recommended fix from C4

@GalloDaSballo
Copy link
Collaborator

Comments

  • 86 is tied to how uint8.max was a possible role which was skipped
    The naive mitigation would have caused overflow
    The implemented one avoids it

  • 244 is tied to always calling
    enabledFunctionSigsByTarget[target].remove(bytes32(functionSig));
    Which would signal the fact that the function is not enabled, a view issue mostly

That said, the change

            if (getRolesWithCapability[target][functionSig] == bytes32(0)) {
                enabledFunctionSigsByTarget[target].remove(bytes32(functionSig));
            }

Avoids removing the target unless the bitmap is zeroed out

Copy link
Collaborator

@GalloDaSballo GalloDaSballo left a comment

Choose a reason for hiding this comment

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

See my comments above

@dapp-whisperer dapp-whisperer changed the base branch from main to release-0.6 December 20, 2023 23:52
@dapp-whisperer dapp-whisperer merged commit 7b2697a into release-0.6 Dec 21, 2023
3 checks passed
@dapp-whisperer dapp-whisperer mentioned this pull request Dec 21, 2023
@dapp-whisperer dapp-whisperer mentioned this pull request Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants