Conversation
…liquidity management
…lt for liquidity management
Summary of ChangesHello @dmytro-horbatenko, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the redemption capabilities by integrating with Aave V3 and Morpho vaults. These new redemption mechanisms allow the system to source liquidity from these popular DeFi protocols when the vault's internal balance is insufficient, enhancing flexibility and robustness. The changes encompass new smart contracts, updated deployment infrastructure, and thorough testing to ensure proper functionality and security. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces new redemption vaults that integrate with Aave and Morpho, allowing the system to tap into external liquidity sources. The implementation is well-structured and includes comprehensive tests. I've identified a critical issue in the Aave integration regarding the handling of withdrawal amounts, along with some opportunities for code cleanup to improve readability and gas efficiency in both new vault contracts.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new vault types for integrating with Aave and Morpho, as well as a vault that can invest in other mTokens. This is a significant feature addition that expands the capabilities of the platform. The changes include new contracts for these vaults, updates to configuration and deployment scripts, and extensive tests.
My review focuses on the new Solidity contracts. I've identified a critical re-initialization vulnerability in DepositVaultWithAave and DepositVaultWithMToken due to a missing initializer modifier. I've also pointed out a potential high-severity re-entrancy risk across all new vault contracts due to the lack of re-entrancy guards. Additionally, I've suggested improvements to the allowance-setting pattern for better security and recommended removing redundant variables for code clarity and gas optimization.
The rest of the changes, including the TypeScript files for deployment and testing, seem well-structured and correctly implement support for the new vaults. The refactoring to use a vault-resolver is a good improvement for maintainability.
contracts/DepositVaultWithAave.sol
Outdated
| IERC20(tokenIn).safeIncreaseAllowance( | ||
| address(aavePool), | ||
| transferredAmount | ||
| ); |
There was a problem hiding this comment.
Using safeIncreaseAllowance can be problematic if a previous transaction that set an allowance failed, leaving a non-zero allowance. A more robust pattern is to reset the allowance to zero before setting the new value. This prevents unexpected behavior from leftover allowances.
IERC20(tokenIn).safeApprove(address(aavePool), 0);
IERC20(tokenIn).safeApprove(address(aavePool), transferredAmount);
| IERC20(tokenIn).safeIncreaseAllowance( | ||
| address(mTokenDepositVault), | ||
| transferredAmount | ||
| ); |
There was a problem hiding this comment.
Using safeIncreaseAllowance can be problematic if a previous transaction that set an allowance failed, leaving a non-zero allowance. A more robust pattern is to reset the allowance to zero before setting the new value. This prevents unexpected behavior from leftover allowances.
IERC20(tokenIn).safeApprove(address(mTokenDepositVault), 0);
IERC20(tokenIn).safeApprove(address(mTokenDepositVault), transferredAmount);
| IERC20(tokenIn).safeIncreaseAllowance( | ||
| address(vault), | ||
| transferredAmount | ||
| ); |
There was a problem hiding this comment.
Using safeIncreaseAllowance can be problematic if a previous transaction that set an allowance failed, leaving a non-zero allowance. A more robust pattern is to reset the allowance to zero before setting the new value. This prevents unexpected behavior from leftover allowances.
IERC20(tokenIn).safeApprove(address(vault), 0);
IERC20(tokenIn).safeApprove(address(vault), transferredAmount);
| uint256 amountMTokenInCopy = amountMTokenIn; | ||
| address tokenOutCopy = tokenOut; | ||
| uint256 minReceiveAmountCopy = minReceiveAmount; |
There was a problem hiding this comment.
The local variables amountMTokenInCopy, tokenOutCopy, and minReceiveAmountCopy are initialized with the values of the function parameters and then used in place of them. Since the original parameters are not modified, these copies are redundant. Removing them will improve code clarity and provide minor gas savings. The original parameters amountMTokenIn, tokenOut, and minReceiveAmount can be used directly throughout the function. This also applies to RedemptionVaultWithMToken and RedemptionVaultWithMorpho.
| IERC20(mTokenA).safeIncreaseAllowance( | ||
| address(redemptionVault), | ||
| mTokenAAmount | ||
| ); |
There was a problem hiding this comment.
Using safeIncreaseAllowance can be problematic if a previous transaction that set an allowance failed, leaving a non-zero allowance. A more robust pattern is to reset the allowance to zero before setting the new value. This prevents unexpected behavior from leftover allowances.
IERC20(mTokenA).safeApprove(address(redemptionVault), 0);
IERC20(mTokenA).safeApprove(address(redemptionVault), mTokenAAmount);
…ew test for exact division case
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new vault types for interacting with Aave and Morpho protocols, as well as a new mToken-based strategy. The changes are extensive, touching contracts, tests, deployment scripts, and configurations. The new contracts are well-structured and inherit from existing base vaults, which is a good design choice. I've identified a critical issue in the RedemptionVaultWithAave contract related to an incorrect balance check that could cause legitimate transactions to fail. Additionally, I've pointed out the use of a deprecated function across several new contracts and suggested a safer alternative. Overall, this is a significant feature addition that expands the platform's capabilities.
Note: Security Review did not run due to the size of the PR.
…tests for edge cases
dmytro-horbatenko
force-pushed
the
feat/aave-and-morpho-vaults
branch
from
3c8e79d to
675402c
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new vault types that integrate with Aave and Morpho for yield generation, and also allows vault-to-vault deposits between mTokens. The changes are extensive, covering new smart contracts, interfaces, tests, and deployment scripts. The new contracts are well-structured and follow security best practices like using storage gaps for upgradeability and access control. The addition of comprehensive tests and updates to the deployment infrastructure is a great sign of quality. I have a couple of suggestions for improvement regarding code duplication and the use of deprecated functions, which would enhance maintainability and adherence to the latest best practices.
Note: Security Review did not run due to the size of the PR.
| tokensDecimals | ||
| ); | ||
|
|
||
| IERC20(tokenIn).safeIncreaseAllowance(address(pool), transferredAmount); |
There was a problem hiding this comment.
safeIncreaseAllowance is deprecated in recent OpenZeppelin Contracts versions in favor of safeApprove. While its use here is not necessarily unsafe, switching to safeApprove would be more idiomatic and future-proof. To be absolutely safe against all token implementations, it's best practice to reset the allowance to 0 before setting a new value. This pattern should be applied across all new vault contracts in this PR that use safeIncreaseAllowance (DepositVaultWithMToken, DepositVaultWithMorpho, RedemptionVaultWithMToken).
IERC20(tokenIn).safeApprove(address(pool), 0);
IERC20(tokenIn).safeApprove(address(pool), transferredAmount);
| function _redeemInstant( | ||
| address tokenOut, | ||
| uint256 amountMTokenIn, | ||
| uint256 minReceiveAmount, | ||
| address recipient | ||
| ) | ||
| internal | ||
| override | ||
| returns ( | ||
| CalcAndValidateRedeemResult memory calcResult, | ||
| uint256 amountTokenOutWithoutFee | ||
| ) | ||
| { | ||
| address user = msg.sender; | ||
|
|
||
| calcResult = _calcAndValidateRedeem( | ||
| user, | ||
| tokenOut, | ||
| amountMTokenIn, | ||
| true, | ||
| false | ||
| ); | ||
|
|
||
| _requireAndUpdateLimit(amountMTokenIn); | ||
|
|
||
| uint256 tokenDecimals = _tokenDecimals(tokenOut); | ||
|
|
||
| uint256 amountMTokenInCopy = amountMTokenIn; | ||
| address tokenOutCopy = tokenOut; | ||
| uint256 minReceiveAmountCopy = minReceiveAmount; | ||
|
|
||
| (uint256 amountMTokenInUsd, uint256 mTokenRate) = _convertMTokenToUsd( | ||
| amountMTokenInCopy | ||
| ); | ||
| (uint256 amountTokenOut, uint256 tokenOutRate) = _convertUsdToToken( | ||
| amountMTokenInUsd, | ||
| tokenOutCopy | ||
| ); | ||
|
|
||
| _requireAndUpdateAllowance(tokenOutCopy, amountTokenOut); | ||
|
|
||
| mToken.burn(user, calcResult.amountMTokenWithoutFee); | ||
| if (calcResult.feeAmount > 0) | ||
| _tokenTransferFromUser( | ||
| address(mToken), | ||
| feeReceiver, | ||
| calcResult.feeAmount, | ||
| 18 | ||
| ); | ||
|
|
||
| uint256 amountTokenOutWithoutFeeFrom18 = ((calcResult | ||
| .amountMTokenWithoutFee * mTokenRate) / tokenOutRate) | ||
| .convertFromBase18(tokenDecimals); | ||
|
|
||
| amountTokenOutWithoutFee = amountTokenOutWithoutFeeFrom18 | ||
| .convertToBase18(tokenDecimals); | ||
|
|
||
| require( | ||
| amountTokenOutWithoutFee >= minReceiveAmountCopy, | ||
| "RVA: minReceiveAmount > actual" | ||
| ); | ||
|
|
||
| _checkAndRedeemAave(tokenOutCopy, amountTokenOutWithoutFeeFrom18); | ||
|
|
||
| _tokenTransferToUser( | ||
| tokenOutCopy, | ||
| recipient, | ||
| amountTokenOutWithoutFee, | ||
| tokenDecimals | ||
| ); | ||
| } |
There was a problem hiding this comment.
The _redeemInstant function contains a large block of code that is duplicated across RedemptionVaultWithAave, RedemptionVaultWithMorpho, and RedemptionVaultWithMToken. This duplication makes the code harder to maintain, as any change to the core redemption logic needs to be applied in multiple places. Consider refactoring this common logic into a shared internal function within the base RedemptionVault contract, perhaps using a hook that child contracts can override for their specific withdrawal logic. For example, an internal function like _prepareRedemption could handle the common calculations and checks, returning the necessary values to the caller.
2 participants
No description provided.