Twyne Aave Integration

The wrapped aToken contract's reward accounting system is incompatible with the protocol's architecture, leading to lost farming rewards and incorrect reward distribution.

When users borrow wrapped aTokens from the intermediate vault, rebalanceATokens_CV() unwraps them on the collateral vault so AAVE recognizes the collateral. This leaves the wrapped aToken contract virtually backed rather than physically backed, as the underlying aTokens are held by the collateral vault.

The forked StataToken reward accounting assumes 100% of aTokens remain in the wrapped contract. However, aTokens used for borrowing are held by the collateral vault, which receives their farming rewards.

The contract ERC20AaveLMUpgradeable that is inherited by the wrapped aToken uses the INCENTIVES_CONTROLLER's getAssetIndex() function to determine rewards. When looking at the INCENTIVES_CONTROLLER function code, we can observe that this value is determined using the total value of the aToken.

_getAssetIndex(
        rewardData,
        IScaledBalanceToken(asset).scaledTotalSupply(),
        10 ** _assets[asset].decimals
      );

This creates several issues:

• The wrapped contract incorrectly tracks rewards for aTokens it no longer holds
• Collateral vaults receive farming rewards but cannot claim them
• Users claiming rewards from the wrapped contract receive inflated amounts on a first-come-first-serve basis
• Wrapped aTokens lent on the intermediate vault generate rewards that the vault cannot claim

Medium. The farming rewards system is broken, resulting in lost rewards and incorrect distributions.

Remove the current farming reward system from the wrapped aToken contract.

Instead, implement an onlyOwner() function allowing the Twyne admin to claim farming rewards from both the wrapped contract and collateral vaults. Consider redistributing rewards through a simpler system like Merkl that is compatible with intermediate vaults.

Fixed in PR#2 & PR#194.

In redeemUnderlying() and handleExternalLiquidation(), the functions attempt to redeem wrapped aTokens for underlying assets and transfer them to the user. However, during periods of high utilization of the intermediate pool, most aTokens may be transferred to collateral vaults and are unavailable on the wrapped contract.

Both functions call redeem() before rebalanceATokens_CV(), which requires the wrapped tokens to be redeemed without first retrieving aTokens from the collateral vault. During high intermediate vault utilization, this causes redemption to fail and the transaction to revert, even though the tokens are technically available on the collateral vault.

Medium. redeemUnderlying() and handleExternalLiquidation() may revert when intermediate vault utilization is high.

Call rebalanceATokens_CV() before redeem() in handleExternalLiquidation().
Override redeemUnderlying() in the AAVE integration vault and call _handleExcessCredit() before redeem().

Fixed in PR#189.

The categoryId mapping may lack precision for future AAVE market integrations on the same chain.

The CollateralVaultFactory] has an internal mapping categoryId that can be set through setCategoryId() by the owner. This mapping defines which category ID new AAVE collateral vaults should use based on collateral and target asset. Category 0 uses the default LTV, while other categories enable eMode for higher LTV on specific assets.

The contract doesn't account for AAVE having multiple markets on some chains. For example, mainnet has Core, Prime, Horizon RWA, and EtherFi markets. While Twyne currently targets only Core, future support for other AAVE markets would cause the factory to use the same category ID for all collateral/target asset pairs across different markets, potentially resulting in invalid IDs or unintended categories.

Low. Future AAVE integrations on the same chain may cause issues.

Add a targetVault parameter to the mapping:

- mapping(address collateralAsset => mapping( address targetAsset => uint8 categoryId)) public categoryId;
+ mapping(address targetVault => mapping( address collateralAsset => mapping( address targetAsset => uint8 categoryId))) public categoryId;

And update the setCategoryId() function.

Fixed in PR#191.

When querying the external liquidation LTV, the implementation of _getAaveLiqLTV() uses the corresponding E-mode liquidation threshold if a category is configured in the vault.

102:     function _getAaveLiqLTV() internal view returns (uint) {
103:         if (categoryId == 0) {
104:             (,,uint currentLiquidationThreshold,,,,,,,) = aaveDataProvider.getReserveConfigurationData(underlyingAsset);
105:             return currentLiquidationThreshold;
106:         } else {
107:             return IAaveV3Pool(targetVault).getEModeCategoryCollateralConfig(categoryId).liquidationThreshold;
108:         }
109:     }

However, Aave internally checks if the selected E-mode is actually enabled for the asset. The function calculateUserAccountData() checks if the collateral is associated with the borrower's E-mode to apply the category configuration.

121:             vars.isInEModeCategory =
122:               params.userEModeCategory != 0 &&
123:               EModeConfiguration.isReserveEnabledOnBitmap(vars.eModeCollateralBitmap, vars.i);

While it is expected that Twyne configures categories correctly, these settings can be modified at Aave after a vault has been created.

Low. The liquidation threshold might diverge in the unlikely event that an existing category is disabled.

Consider also checking if the asset is enabled for the given category to align the implementation with Aave's getUserAccountData().

Fixed in PR#193.

In the AaveV3ATokenWrapper contract, the rebalanceATokens_CV() is not affected by the emergency pause since it doesn't rely on _update().

Low.

Add the whenNotPaused modifier to enable pauses on this functionality.

Fixed in PR#2.

The variable AAVE_POOL is never used in the AaveV3Wrapper contract.

Informational.

Remove the variable.

Fixed in PR#192.

The CollateralVaultFactory contract was updated to return 2 as its version, but the VaultManager and EulerCollateralVault contracts still return 1 in version() despite being updated since the last deployment.

Informational. Inconsistent versioning between contracts.

Align contract versions for contracts that were deployed previously then modified.

Fixed in PR#192.

Natspec can be updated throughout the code to improve documentation:

• Natspec for handleExternalLiquidation() can be changed for AAVE integration:

- to be called if the vault is liquidated by Euler
+ to be called if the vault is liquidated by AAVE

• Missing natspec for setOracleResolvedVaultForOracleRouter().
• Missing VaultType _vaultType natspec in createCollateralVault().

Informational.

Update the natspec.

Fixed in PR#192.

The setCategoryId() function doesn't emit an event when updating the category configuration.

Informational.

Emit an event in setCategoryId().

Fixed in PR#192.

Given the refactor of _isNotExternallyLiquidated(), the skim() function can avoid duplicating the validation logic, as there is no benefit in re-using cached variables.

Informational.

-   function skim() external callThroughEVC whenNotPaused nonReentrant {
-       // copied from onlyBorrowerAndNotExtLiquidated modifier to cache balanceOf
-       require(_msgSender() == borrower, ReceiverNotBorrower());
-       require(_isNotExternallyLiquidated(), ExternallyLiquidated());
+   function skim() external onlyBorrowerAndNotExtLiquidated whenNotPaused nonReentrant {

Fixed in PR#192.

The implementation of createCollateralVault() uses a VaultType enum to determine the creation logic between Euler and Aave.

This can lead to a confusing interface, given that the creation logic differs between the two protocols. One example is the _targetAsset parameter, which is required for Aave but not used in Euler.

Informational.

Split the creation functionality between separate functions, such as createEulerCollateralVault() and createAaveCollateralVault().

Acknowledged.

The CustomERC4626StataTokenUpgradeable is a copy of Aave's ERC4626StataTokenUpgradeable with the max* functions re-defined to skip checks and allow any amount. While this saves gas, it should be noted that it breaks the ERC-4626 standard.

Informational.

Notify integrators about this behavior.

Acknowledged. This wrapper is an internal complexity.

In rebalanceATokens_CV(), the function transfers aTokens to the collateral vault when shares >= actualScaledBalance. When both values are equal, this results in a zero transfer operation.

Gas savings.

Replace the else condition with else if (shares > actualScaledBalance).

Fixed in commit 2f0f014171b9ecfef7ea6d88d719e536d69d3098c36be4c3aa19559f18db9015L138.