-
Notifications
You must be signed in to change notification settings - Fork 48
Don't use unsafe default balancer address #250
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
base: main
Are you sure you want to change the base?
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
Not sure about this change since this will change the deployed bytecode, thus the deployed address by default. |
|
It would probably make more sense to add a mandatory step in the docs with checking the deployed BalancerV2Vault address and source code. |
fedgiac
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, good idea now that we know that the vault could be compromised.
Let's merge this also to the new-chain-deployments branch.
Not sure about this change since this will change the deployed bytecode, thus the deployed address by default.
No worries, changing bytecode mainly applies to .sol files that aren't tests or scripts.
It would probably make more sense to add a mandatory step in the docs with checking the deployed BalancerV2Vault address and source code.
Yep, can you add a mention to the readme?
I already updated the internal guide for deploying on new chains (here if you want more context).
Co-authored-by: Federico Giacon <[email protected]>
d082d54 to
d2cb037
Compare
kaze-cow
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest we have the ability to somehow override the balancer address through a command line argument or similar (ex. set to address 0). Just in case balancer is not deploying to a network and we still want to get the contract deployed. Based on how we have the change set up right now, it appears it will throw and not allow the user to continue if there is no balancer deployment.
Description
The key that the balancer vault used to be deployed from is compromised, so we can be sure that the contract deployed under this address is legit or if the contract is not deployed at all we can't be sure an attacker won't deploy to that address later. This PR removes the default address and asks the user to get a balancer address from the balancer team.