wighawag / hardhat-deploy

hardhat deployment plugin
MIT License
1.19k stars 292 forks source link

Defaulting to using deployed library instead of redeploying even if code is updated? #317

Open NIC619 opened 2 years ago

NIC619 commented 2 years ago

Describe the bug Saddle finance's recent exploit report said that hardhat-deploy defaults to using deployed libraries instead of redeploying new ones if code is updated.

The reason the old libraries persisted was because deployments are controlled by the “hardhat-deploy” plugin. This plugin has the behavior of defaulting to using previously-deployed instances of libraries, instead of redeploying new libraries if code is updated.

Can anyone help clarify this? Is this a possible bug or an operational error on Saddle's side? What's the best practice if we are deploying a fixed library?

Expected behavior If library is updated, hardhat-deploy should deploy the new library instead of reusing an existing one.

Additional context Saddle Finance exploit report: https://blog.saddle.finance/4-30-2022-post-mortem-of-mainnet-susdv2-metapool-exploit/

wighawag commented 2 years ago

hmm, that's not true

hardhat-deploy do not even try to deploy libraries, this has to be done by the user

and if it use the default option when deploying the library it will redeploy on code changes

EDIT: They are actually explicitly setting the option to not redeploy even if code changes : https://github.com/saddle-finance/saddle-contract/blob/32524bc9b0c97f8ad01943790f0fe06aaf8ec252/deploy/mainnet/004_deploy_MetaSwapUtils.ts#L12

Hence why it will not deploy the new version

NIC619 commented 2 years ago

Thanks for the clarification.

Indeed it is mentioned in the documentation that code change will not result in redeployment. Though personally I feel like it's not so easy to get the implication from the name (skipIfAlreadyDeployed) along. I get that it's the whole point of reading the documentation beforehand. Just thinking if there are alternatives for the name, something like skipEvenIfCodeChanged.