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

removeVault() will block because of incorrect vaults length #96

Merged
merged 4 commits into from
Dec 6, 2022

Conversation

tushargargght
Copy link
Contributor

Describe the bug

In SameAssetUnderlyingsAbstractVault.sol, removeVaults() accepts a vaultIndex to remove the corresponding vault from _activeUnderlyingVaults, and the index is required to be less than the length of _activeUnderlyingVaults.

uint256 newUnderlyingVaultsLen = _activeUnderlyingVaults.length - 1;
require(vaultIndex <= newUnderlyingVaultsLen, "Invalid from vault index");

We also noticed the same vaultIndex is used to get the underlyingVaultIndex from the vaultIndexMap later.

uint256 underlyingVaultIndex = vaultIndexMapMem.map(vaultIndex);

This means if the governor wants to remove the nth vault in the vaultMap, it requires having at least n active vaults. However, this is not always the case, as previously removed vaults will still occupy their indexes on the map, and that will block the governor from removing active vaults. Since there is a limit of total active vaults, the issue will also block the governor from adding new vaults once after adding 15 vaults

@naddison36 naddison36 merged commit 3226f67 into develop Dec 6, 2022
@naddison36 naddison36 deleted the issue/216/removeVaultIndexBlock branch December 6, 2022 07:52
@naddison36 naddison36 mentioned this pull request Jan 10, 2023
naddison36 added a commit that referenced this pull request Jan 10, 2023
* fix: npm publish action (#86)
* chore: adds publish web to publish action
* feat: fraxBP vaults (#71)
* feat: test for update assetsPerShare after removal of underlyingVault
* feat: ConvexFrax based vaults
* chore: Refactor Curve lib tests (#74)
* chore: using Hardhat network helpers for setBalance, setStorageAt and stopImpersonateAccount (#82)
* fix: missing awaits in savePlus fork tests
* refact: reduce gas consumption while donating tokens (#87)
* chore: updated docs for second audit
* chore: adds deployment address (#93)
* fix: removeVault() will block because of incorrect vaults length (#96)
* fix: modified require to use total vaults length instead of active vaults
* feat: test for nth vault removal
* fix: outdated assets amount in _redeem and _mint for PeriodicAllocationAbstractVault (#99)
* feat: recalculate assets amount of successful assetsPerShare update
* feat: tests for recalculating assetsPerShare for redeem and mint
* fix: Meta vaults cannot re-add underlying vaults after removal (#100)
* feat: removed allowance in remove vault
* fix: test for remove and re-add vault
* fix: liq-donate-tokens task with multiple rewards (#101)
* fix: liq-settle-swap task (#103)
* fix: removes death code from LiquidatorVaults (#104)
* fix: RoundUp for `mint`, `previewMint`, `withdraw` and `previewWithdraw` (#102)
* feat: contract changes for roundUp
* feat: LightBasicVault roundUp
* feat: roundUp unit-tests
* feat: 3Crv based vault changes
* feat: FraxBp based vault changes
* feat: roundUp tests and scaling bug fix
* fix: reset network before running test
* fix: [Macro] Medium and lower criticality fixes (#106)
* feat: [M-4] added check for performance fees too high during initialization
* feat: [L-2] cannot remove cached vault
* feat: [Q-1] removed zero-address check for code consistency
* feat: [Q-2] removed unused imports
* feat: [Q-3] corrected comment
* feat: [G-1] No check if performanceFee is 0
* feat: [G-3] Unnecessary control variable initialization of for loops And removed zero address check vaultManagerRole
* fix: fork-tests for sameAssetsUnderlyingVaults behavior for removeVault
* feat: [G-2] uint24 for performanceFee
* fix: uint24 performanceFee
* fix: unit-test errors
* fix: removed toNumber()
* chore: toNumber fix
* fix: minor fixes
* feat: updated diagrams
* feat: get historical token balances using token-balance HH task (#107)
* fix: liquidator retry approvals (#110)
* fix: liquidator retry approvals by pre-approving the async swapper to transfer reward tokens
* fix: typos in README (#112)

Co-authored-by: doncesarts <doncesarts@gmail.com>
Co-authored-by: tushargargght <tushargargght@gmail.com>
Co-authored-by: omahs <73983677+omahs@users.noreply.github.com>
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.

3 participants