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

[Macro] Medium and lower criticality fixes #106

Merged
merged 15 commits into from
Dec 31, 2022
Merged

Conversation

tushargargght
Copy link
Contributor

@tushargargght tushargargght commented Dec 30, 2022

All the fixes/improvements/recommendation edits from the Macro audit.

Specifically it includes tests and fixes for:

  • [M-4] No upper bound for the initialization of performance fee
  • [L-2] removeVault() may block single source withdraws
  • [Q-1] Inconsistent zero address check for vault manager
  • [Q-2] Unused imports
  • [Q-3] Misleading comment
  • [G-1] No check if performanceFee is 0
  • [G-2] Unnecessary use of uint256 for performanceFee
  • [G-3] Unnecessary control variable initialization of for loops

@tushargargght tushargargght marked this pull request as ready for review December 30, 2022 10:53
Copy link
Contributor

@naddison36 naddison36 left a comment

Choose a reason for hiding this comment

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

Looks good

Note changing the performanceFee from uint256 to uint24 reduces the gas costs of chargePerformanceFees as there is one less slot to read. That is, performanceFee is now packed into the same slot as feeReceiver.

If the existing Meta Vault contract is upgraded, performanceFee will have to be kept at uint256 to preserve the existing slot layout. New contracts can be deployed with performanceFee of uint24 type.

@naddison36
Copy link
Contributor

The new PeriodicAllocationPerfFeeMetaVault storage slot layout
PeriodicAllocationPerfFeeMetaVaultStorage

@tushargargght tushargargght merged commit 6e8b6b5 into develop Dec 31, 2022
@tushargargght tushargargght deleted the macro_221_bulk branch December 31, 2022 05:59
@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.

2 participants