-
Notifications
You must be signed in to change notification settings - Fork 0
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
MultisigManager may not be able to add a valid Multisig #521
Comments
GalloDaSballo marked the issue as primary issue |
Id argue Low since its unlikely |
I disagree @0xju1ie. I think it's an oversight not to have a way to delete old multisigs with the limit in place rather than a quality assurance issue. |
#349 has an interesting fix for this issue |
Which was: "Count only the validated/enabled multisigs in order to control the limit." |
The Warden has shown how, due to a logic flaw, the system can only ever add up to 10 multi sigs, even after disabling all, no more multi sigs could be added. Because this shows how an external condition can break the functionality of the MultisigManager, I agree with Medium Severity |
Acknowledged. Not fixing right now, we don't foresee having many multisigs at launch, and will upgrade as necessary to support more. |
GalloDaSballo marked the issue as selected for report |
Lines of code
https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MultisigManager.sol#L41-L43
Vulnerability details
Impact
When more than 10 mulitsig, it is impossible to modify or delete the old ones, making it impossible to create new valid ones.
Proof of Concept
MultisigManager limits the number of Multisig to 10, which cannot be deleted or replaced after they have been disable
This will have a problem, if the subsequent use of 10, all 10 for some reason, be disabled
Then it is impossible to add new ones and replace the old ones, so you have to continue using the old Multisig at risk
Tools Used
Recommended Mitigation Steps
add replace old mulitsig method
The text was updated successfully, but these errors were encountered: