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: check threshold percentage #101

Merged
merged 1 commit into from
Apr 7, 2024
Merged

Conversation

renlulu
Copy link
Contributor

@renlulu renlulu commented Apr 7, 2024

Issue: Missing input sanitization in updateQuorumThresholdPercentage

The function updateQuorumThresholdPercentage() does not have any checks on the input parameter thresholdPercentage. The expected range of an uint8 value is 0 to 255, which means that the quorumThresholdPercentage value could theoretically be set to any value within this range. However, as this value is used as a percentage, acceptable values should be limited to the range 0 to 100.

If no checks are implemented, potential issues could occur. For instance, if the thresholdPercentage is set above 100, it might break the functions that use quorumThresholdPercentage as they might never reach the required quorum.

contracts/src/core/MachServiceManager.sol (Lines 147-150):

function updateQuorumThresholdPercentage(uint8 thresholdPercentage) external onlyOwner {
    quorumThresholdPercentage = thresholdPercentage;
    emit QuorumThresholdPercentageChanged(thresholdPercentage);
}

Valid ranges are 0-100 as implied by this comment on the individual threshold percentages:

contracts/src/core/MachServiceManager.sol (Lines 222-224):

for (uint256 i = 0; i < alertHeader.quorumThresholdPercentages.length; i++) {
    // we don't check that the quorumThresholdPercentages are not >100 because a greater value would trivially fail the check, implying
    // signed stake > total stake

It is recommended, to implement a sanity check to verify that the thresholdPercentage is within valid range (0 to 100). One could even further restrict the range for better security. If this condition is not met, the function should revert the operation.

@github-actions github-actions bot added bug Something isn't working XS labels Apr 7, 2024
@bb111189 bb111189 added the Audit label Apr 7, 2024
@bb111189 bb111189 merged commit 178a732 into m2-dev Apr 7, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Audit bug Something isn't working XS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants