feat(StakeManager)!: introduce a per user vault limit #88
Conversation
| CONSTANTS | ||
| //////////////////////////////////////////////////////////////////////////*/ | ||
|
|
||
| uint8 public constant MAX_VAULTS_PER_USER = 5; |
There was a problem hiding this comment.
@gravityblast @3esmit What should the max be? I went with 5 as it's more than enough I think.
Also, shall we call this MAX_VAULTS_PER_ACCOUNT instead/
There was a problem hiding this comment.
yes I think MAX_VAULTS_PER_ACCOUNT makes sense.
for the number, can't we have it variable so an admin can change it?
it feels too random now to take a decision on a number like that
| IStakeVault(migrateTo).migrateFromVault(migrationData); | ||
|
|
||
| delete vaultData[msg.sender]; | ||
| _deregisterVault(msg.sender); |
There was a problem hiding this comment.
Now that we have limited amount of vaults, deregistering is actually feasible, which allows us to clean up after ourselves in migrateToVault() and leave()
| vaults[vaultOwner].push(address(vault)); | ||
| emit VaultRegistered(address(vault), vaultOwner); | ||
| return vault; | ||
| } |
There was a problem hiding this comment.
Notice that this function is only called by StakeVault.replaceVault().
As mentioned in the commit, StakeVault should actually be calling VaultFactory for this, but since there's a chance we'll replace the factory in the future, we need StakeVault to do this via StakeManager instead.
Key insights here are:
createMigrationVault()really just creates a vault via VaultFactory, without the max limit check- It also does the
registerVault()logic. I've inlined it here, because otherwise the VaultFactory would doregisterVault(), which is a bit of an unnecessary back in forth (alsoregisterVault()has the max limit check, which we otherwise need to make conditional)
| address migrateTo = address(stakeManager.createMigrationVault()); | ||
| _migrateToVault(migrateTo); | ||
| return migrateTo; | ||
| } |
There was a problem hiding this comment.
Notice that this largely leverages functionality that was already there.
We just create the migration vault in a slightly different way.
| */ | ||
| function createMigrationVault(address vaultOwner) external onlyStakeManager returns (StakeVault clone) { | ||
| return _createVault(vaultOwner); | ||
| } |
There was a problem hiding this comment.
createMigrationVautl() really just means createVaultWithoutRegistration()
Happy to rename this @gravityblast @3esmit
0x-r4bbit
force-pushed
the
fix/vault-max-limits
branch
3 times, most recently
from
412247d to
6b0c7d6
Compare
0x-r4bbit
force-pushed
the
fix/vault-owner-bug
branch
from
81c7b3d to
c000893
Compare
0x-r4bbit
force-pushed
the
fix/vault-max-limits
branch
from
6b0c7d6 to
4355c78
Compare
This commit introduces a maximum limit of vaults an account can own in the staking system. This is to fix the situation that accounts can create a large amount of vaults to avoid getting slashed. To enable vault limits, the following notable changes were made: - `StakeManager.maxVaultsPerUser()` is a configurable field to set the max amount of vaults per user - `StakeManager.registerVault()` now when trying to register more than `maxVaultsPerUser` vaults - `StakeManager.migrateTo(address)` now deregisters the vault that the account is migrating from - `StakeVault.replaceVault()` has been introduced to allow for migrating to a new vault instance if the max vault limit is already reached - To make `StakeVault.replaceVault()` possible `StakeManager.createMigrationVault()` was introduced, which bypasses the max vault limit temporarily, to replace an older vault - Respectively, `VaultFactory.createVaultWithoutRegistration()` has been introduced as well (we could've gotten away with just this addition, but since we can't `StakeVault`'s if the vault factory changes, they have to make them go through `StakeManager` instead, which ultimately calls the vault factory) - `StakeManager.leave()` also deregisters the vault that is leaving. This prevents accounts from locking themselves out of the system by creating max limit vaults and have them all leave. - Tests have been added accordingly BREAKING CHANGES: - `StakeManager.registerVault()` now only allows for `StakeManager.maxVaultsPerUser()` vaults per user and reverts otherwise - `StakeManager.migrateTo(address)` now deregisters the vault that calls this function - `StakeManager.leave()` also deregisters the vault that calls this function Addresses Cyfrin/audit-2025-12-statusl2#11 Closes #73
0x-r4bbit
force-pushed
the
fix/vault-max-limits
branch
from
4355c78 to
e8e89e3
Compare
2 participants
This commit introduces a maximum limit of vaults an account can own in the staking system. This is to fix the situation that accounts can create a large amount of vaults to avoid getting slashed.
To enable vault limits, the following notable changes were made:
StakeManager.MAX_VAULTS_PER_USER()is a constant that contains the limitStakeManager.registerVault()now when trying to register more thanMAX_VAULTS_PER_USERvaultsStakeManager.migrateTo(address)now deregisters the vault that the account is migrating fromStakeVault.replaceVault()has been introduced to allow for migrating to a new vault instance if the max vault limit is already reachedStakeVault.replaceVault()possibleStakeManager.createMigrationVault()was introduced, which bypasses the max vault limit temporarily, to replace an older vaultVaultFactory.createMigrationVault()has been introduced as well (we could've gotten away with just this addition, but since we can'tStakeVault's if the vault factory changes, they have to make them go throughStakeManagerinstead, which ultimately calls the vault factory)StakeManager.leave()also deregisters the vault that is leaving. This prevents accounts from locking themselves out of the system by creating max limit vaults and have them all leave.BREAKING CHANGES:
StakeManager.registerVault()now only allows forStakeManager.MAX_VAULTS_PER_USER()vaults per user and reverts otherwiseStakeManager.migrateTo(address)now deregisters the vault that calls this functionStakeManager.leave()also deregisters the vault that calls this functionAddresses https://github.com/Cyfrin/audit-2025-12-statusl2/issues/11
Closes #73