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

QA Report #60

Open
howlbot-integration bot opened this issue Oct 27, 2024 · 3 comments
Open

QA Report #60

howlbot-integration bot opened this issue Oct 27, 2024 · 3 comments
Labels
2nd place bug Something isn't working grade-b Q-04 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sufficient quality report This report is of sufficient quality

Comments

@howlbot-integration
Copy link

howlbot-integration bot commented Oct 27, 2024

See the markdown file with the details of this report here.

@howlbot-integration howlbot-integration bot added bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sufficient quality report This report is of sufficient quality labels Oct 27, 2024
howlbot-integration bot added a commit that referenced this issue Oct 27, 2024
howlbot-integration bot added a commit that referenced this issue Oct 27, 2024
@c4-judge
Copy link

c4-judge commented Nov 4, 2024

GalloDaSballo marked the issue as grade-b

@GalloDaSballo
Copy link

GalloDaSballo commented Nov 10, 2024

Finding Heading
TODO
calculateAddress() function is vulnerable to frontrunning
I
Safe calling itself with empty calldata is potentially dangerous if a fallback handler is configured
I
Insufficient validation for owners in RecoverySpellFactory.sol
L
Consider restricting unbounded loops to protect against user error
I
Bad proposal timestamp cleanup leads to inconsistent behavior
-3
Q-01 Arbitrum and Optimism chainIds missing in SystemDeploy.s.sol
L
Q-02 InstanceDeployer immutable variable verification missing in SystemDeploy.s.sol
I
Q-03 Consider making RecoverySpell deployment permissioned
I
Q-04 Wrong comment in ConfigurablePause.sol
NC
Q-05 Unused Code
I
Q-06 Unnecessary salt in InstanceDeployer.sol
I, wouldn't make the change over the recommendation due to lack of backing
Q-07 Reachable assert() functions in InstanceDeployer.sol will consume all of the remaining gas
I
Q-08 s parameter in signature can be non-zero due to corrupted memory
I
Q-09 Rename newRecoveryThreshold to recoveryThreshold
I
Q-10 Add recovery spell owners to the recovery spell signature
TODO
Q-11 Comment that safe owners can call executeWhitelistedBatch() is wrong
TODO
Q-12 Comment that safe owners can execute whitelisted calldatas is wrong
TODO
Q-13 Comment that require() can not be reached is wrong
TODO
Q-14 indexes variable is redundant
TODO
Q-15 Delete _calldataList is redundant
TODO
Q-16 _setPauseTime() in setGuardian() is redundant
TODO
Q-17 checkCalldata() does not support calls with zero calldata
TODO
Q-18 Index copying logic can be shortcut
TODO

Stopping here, overall I think it would have been best to handpick half of these findings and add more info and context

TODO - 10
I - 9
L - 2
-3 - 1
NC - 1
I, wouldn't make the change over the recommendation due to lack of backing - 1

2L 1NC -3

@thebrittfactor
Copy link

For awarding purposes, C4 staff have marked as tied for 2nd place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2nd place bug Something isn't working grade-b Q-04 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

4 participants