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

Check if the same implementation address before upgrade in Transparent #5288

Open
Confucian-e opened this issue Oct 31, 2024 · 2 comments
Open

Comments

@Confucian-e
Copy link

🧐 Motivation

Currently, there is no check during contract upgrades to see if the new address is the same as the old address, which has led to several instances where I upgraded to a duplicate address and captured the Upgrade event, but the upgrade did not actually succeed.

📝 Details

check this: Confucian-e@1564887

If you agree with my point of view, I would be happy to submit a PR.

@ernestognw
Copy link
Member

Hi @Confucian-e,

We usually handle these issues on our upgrades plugins package where multiple upgradeability checks are run. Right now it doesn't check if the implementation is the same as the old implementation when upgrading, but could be added as an configurable option.

Before doing that, I'd like to understand better what's your setup and how are you upgrading. It seems weird that you have had several instances upgrading to the same implementation. Can you elaborate on that?

@Confucian-e
Copy link
Author

Thank you for your response. I understand that the upgrades plugins package includes various upgradeability checks, which is indeed helpful. However, not everyone upgrades contracts through your plugins. Some developers, like me, might use custom scripts or other methods, which could bypass these additional checks.

For instance, I previously wrote a script to upgrade a contract by reading the new implementation address from a file. Unfortunately, there was a bug in the script, and it accidentally fetched the address of the already-deployed implementation. Since the contract didn’t enforce a check to prevent upgrading to the same implementation, the transaction succeeded, and I mistakenly believed the upgrade was successful after seeing the Upgraded event.

I realized the issue only later when the contract didn’t exhibit the expected behavior. This could have been avoided if the upgrade logic at the contract level validated that the new implementation is different from the current one.

To prevent such scenarios, I suggest adding this restriction directly in the TransparentUpgradeableProxy contract. It would make the upgrade process more robust and safer, especially for users who are not using the plugins. I’d be happy to submit a PR if this idea aligns with your roadmap.

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

No branches or pull requests

2 participants