Genesis Vaults Updates

VaultModule.withdraw returns the pre-penalty 'assets' value but transfers only the net assets after applying the global withdrawal penalty. GenesisVaultSYToken._redeem uses the returned value as the amount to forward to the receiver, leading it to attempt transferring more tokens than it actually received. This will either revert due to insufficient balance or, if the SY contract holds any underlying balance, overpay the user and drain those funds.

Make the amounts consistent. Either (a) change VaultModule.withdraw to return the netAssets actually transferred to msg.sender, or (b) change GenesisVaultSYToken._redeem to derive the net amount by tracking the SY contract’s asset balance delta (before and after withdraw) and transfer only that net amount. Also base the slippage check on the net amount.

The getUserSharesForClaim function is intended to be invoked by LayerZero’s lzRead function to distribute the state to other chains. It includes several revert cases if the vaultId is incorrect or the vault is not yet fully migrated.

However lzRead target functions should never revert, otherwise they would prevent DVNs from verifying the result and halt the read messaging channel, thus stopping the migration until it is manually unblocked, at which point it can be halted again.

Ensure the getUserSharesForClaim never reverts and instead returns an empty/sentinel value which can be ignored for invalid cases.

In the burnForMigration function there is no validation that the user has passed the withdrawal delay before burning their tokens for the migration.

Therefore the migration may offer a pathway for the user to exit their position in the VaultSYToken before the delay is up.

Consider if the withdrawalDelay should be validated before a user is allowed to invoke the burnForMigration function.

The deposit function allows the caller to provide an arbitrary receiver address and the receiver is the account which will have their withdrawal cooldown assigned, preventing them from withdrawing for the withdrawalDelay period.

Furthermore, there is no minimum deposit amount, which means that a malicious actor could prevent users from withdrawing their assets by continually depositing 1 wei of assets on behalf the user, specifying the victim user as the receiver and resetting their lastDepositTime.

Consider requiring that the receiver is the account invoking the deposit function, or that the caller is in a whitelisted group for the receiver so that this griefing cannot occur.

The cooldown mechanism for withdrawals can be bypassed if a user specifies the GenesisVaultSYToken contract as the receiver and then invokes the redeem function with burnFromInternalBalance as true.

This way the lastDepositTime entry is set for the GenesisVaultSYToken contract but never read, since the redeem function relies on the entry for the msg.sender.

This allows users to cause a deposit on behalf of the GenesisVaultSYToken contract and withdrawal to arbitrary receiver within a single transaction, while bypassing the withdrawal cooldown which may be unexpected for the system.

Consider validating that the receiver is not address(this) in the deposit function to prevent users from circumventing the withdrawalDelay this way.

The AlreadyBurned check in the burnForMigration function can be bypassed by transferring the tokens to another EOA.

Consider to prevent sending and receiving token if the user already burned or consider if this validation is necessary in general. Another solution could be to prevent transfers after the migration.

The _beforeTokenTransfer function of the GenesisVaultSYToken will update the recipient's lastDepositTime to the one of the sender if it's bigger than the current one.

This enables a withdrawal griefing DoS:

• Malicious actor deposits the minimum amount into the system and by doing so the attacker's lastDepositTime is set to now
• Another user tries to withdraw tokens
• The malicious actor front runs the call and transfers 1 wei to the user
• The victims lastDepositTime is updated and therefore the withdraw fails as the withdrawal delay has not passed yet.

Consider to create a whitelist mechanism to allow users to configure who can send them tokens. Or to add a minimum transfer amount to make this DoS attack too expensive to realistically take place.

If the given vault used a strategy it will be unset in the migrateVault flow. However the strategyInUse mapping is not set to false and therefore the strategy can never be used again and needs to be redeployed.

Consider to set the strategyInUse mapping to false in that case.

In GenesisVaultSYToken, the redeem function enforces withdrawal delay using lastDepositTime[msg.sender] even when burning shares directly from the contract (burnFromInternalBalance=true). An attacker can transfer their shares to the Factory contract, and then immediately redeem from a fresh account to bypass the cooldown.

When burning from internal balance, enforce cooldown based on the balance owner whose tokens are being burned or disallow burnFromInternalBalance path.

The burnForMigration function states that Users must burn their full balance to track for cross-chain claims, and the function does indeed burn the entire user balance of the user. However there is no validation that prevents the same user from depositing again through the deposit function.

This user will not be able to invoke the burnForMigration function again, however it may be unexpected for the migration that the user now has a balance in excess of what was burned previously.

Consider if this is expected for the migration and avoid this edge case in the migration logic. If it should be prevented, consider validating that the burnedBalances entry for the user is zero before allowing a deposit to occur.

The getUserAccountSummary function calculates and returns the earnedYield of the user.

The calculation compares the deposited and withdrawn amount of the user and sums it up with the current worth of the users shares.

The comparison of the withdrawn and deposited amount takes into account the paid fees while the worth of the user's shares does not.

Consider to apply the withdraw penalty.

The collectPerformanceFee function handles the case that no performance fee is set: if (recipient == address(0) || rate == 0 || yield == 0) { return (0, yield);}

However the harvestAndCompound function which calls it early returns if no performance fee is configured: if (vault.strategy == address(0) || !hasPerformanceFeeConfigured(vault)) { return (0, 0); }

Consider to allow not configuring a performance and giving all yield to the users in that case.

The MIN_MIGRATION_DELAY is set to one day, while the documentation states out it is 3 days.

Consider to update the constant or the comment.

In VaultModule, withdraw function enforces minWithdraw on gross assets before applying the withdrawal penalty. If grossAssets is greater than or equal to minWithdraw but the penalty reduces netAssets below minWithdraw, the withdrawal still succeeds, allowing dust withdrawals contrary to intended behaviour.

Consider enforcing minWithdraw on the post penalty amount.

The deposit takes in a receiver parameter but reverts if it's not the msg.sender.

Consider to use the msg.sender instead.

There is unused code in multiple places:

• The VaultNotMigrated error in Errors.sol
• The IERC165 import in ConfigurationModule.sol

Consider to remove unused code.

The lastDepositTime of the SY token recipient is now only increased if the recipient currently does not own tokens.

This allows to bypass the withdraw delay by transferring to another EOA if the user has multiple accounts holding the SY tokens.

Consider to acknowledge this behavior or to create a whitelist mechanism to allow users to configure who can send them tokens and to revert the recent changes.

The getUserSharesForClaim function is intended to be invoked by LayerZero’s lzRead function to distribute the state to other chains and reverts if the vaultId is incorrect.

However lzRead target functions should never revert, otherwise they would prevent DVNs from verifying the result and halt the read messaging channel, thus stopping the migration until it is manually unblocked, at which point it can be halted again.

Ensure the getUserSharesForClaim never reverts and instead returns an empty/sentinel value which can be ignored for invalid cases.