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

remediations #4

Merged
merged 60 commits into from
Jul 8, 2024
Merged

remediations #4

merged 60 commits into from
Jul 8, 2024

Conversation

zeroknots
Copy link
Member

@zeroknots zeroknots commented Jun 17, 2024

Implementing remediations for Ackee's Safe7579 security audit, as well as other security improvements.

Copy link
Member Author

@zeroknots zeroknots left a comment

Choose a reason for hiding this comment

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

Note: H1

I think adding this access control should make remediate the frontrunning / race condition problem entirely.

98f2eb9

I still think having a launchpad like single transaction install process for existing safe accounts is a good idea, but delegatecall to multisend should be sufficient for that?!

fixing access control for launchpadValidators() as well

wip
@zeroknots zeroknots force-pushed the feature/remediation branch from 6767458 to bab5361 Compare June 18, 2024 01:14
// ISafe7579(initData.safe7579).launchpadValidators(initData.validators);
// but we need to append msg.sender (entrypoint) to ERC2771 style access control, to protect
// the launchpadValidator function
(bool success,) = address(initData.safe7579).call(
Copy link
Member Author

Choose a reason for hiding this comment

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

adding ERC2771 style appending of msg.sender to calldata. this should offer sufficient frontrun protection for the launchpadValidators function

function launchpadValidators(ModuleInit[] calldata validators)
external
override
onlyEntryPointOrSelf
Copy link
Member Author

@zeroknots zeroknots Jun 18, 2024

Choose a reason for hiding this comment

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

added access control here

@zeroknots zeroknots requested a review from kopy-kat June 18, 2024 05:41
zeroknots and others added 2 commits June 18, 2024 12:50
adding test case to remove multi type module
@kopy-kat kopy-kat merged commit e17b03c into main Jul 8, 2024
3 checks passed
@kopy-kat kopy-kat deleted the feature/remediation branch July 8, 2024 20:40
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.

2 participants