Skip to content

fix: DeployVault, updated Vault and Factory contracts#15

Open
apoorvlathey wants to merge 1 commit intomasterfrom
fix-deploy-vault
Open

fix: DeployVault, updated Vault and Factory contracts#15
apoorvlathey wants to merge 1 commit intomasterfrom
fix-deploy-vault

Conversation

@apoorvlathey
Copy link
Member

Issue:
After an upgrade to the Vault implementation, new deploys for the vaults started reverting. The issue was that during vault initialization, within the onlyPrivileged check, if manager was not set yet then the msg.sender (in this case the VaultFactory) was checked against the owner which is the vaultFactory.owner().

Fix:

  1. We set the manager to be the msg.sender in the vault's initialization function
  2. The vault factory then sets the actual user as the manager
  3. The transferOwnership call after that has been removed as all the vaults refer to the vault factory's owner. This allows us to transfer the ownership of the vault factory once, and all the vaults automatically refer to the new owner.

Tests:
Added local + forked mainnet tests which are passing successfully.

@apoorvlathey apoorvlathey requested a review from tomwade October 2, 2025 12:07
Copy link
Collaborator

@tomwade tomwade left a comment

Choose a reason for hiding this comment

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

Tests seem solid, so if they are passing my comments are probably moot. As I'm new to the codebase though, just asked 2 questions 👍

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