aave / aave-vault Goto Github PK

Within IATokenVault.sol , some events do not have their parameters indexed. For instance:

https://github.com/aave/wrapped-atoken-vault/blob/f39678dd1279ff9d8bcc13dfa6fd0eaf75037b69/src/interfaces/IATokenVault.sol#L38

https://github.com/aave/wrapped-atoken-vault/blob/f39678dd1279ff9d8bcc13dfa6fd0eaf75037b69/src/interfaces/IATokenVault.sol#L55

It is possible for the deposit function to pull more assets from the user than needed for the same amount of shares because of rounding issues.
The amount of shares are being calculated on the line 530, but due to rounding issues (previewDeposit is rounding down), the function could make use of more assets than needed.
https://github.com/aave/wrapped-atoken-vault/blob/8374fb5ead3bb67848bfc4cd782035c62bff9da7/src/ATokenVault.sol#L522-L533

If there is small amount of tokens in the pool attacker can lock funds of other users. This can be done by sending directly tokens to the contract and by this he can manipulate and change the ratio between the amount of shares to the balanceOf(ATOKEN).
In some cases it might lead to a situation that an attacker can get more tokens than he should.

    function previewDeposit(uint256 assets) public view virtual override returns (uint256) {
        return _convertToShares(assets, Math.Rounding.Down);
    }

    function _convertToShares(uint256 assets, Math.Rounding rounding) internal view virtual returns (uint256 shares) {
        uint256 supply = totalSupply();
        return
            (assets == 0 || supply == 0)
                ? _initialConvertToShares(assets, rounding)
                : assets.mulDiv(supply, totalAssets(), rounding);
    }

    function totalAssets() public view virtual override returns (uint256) {
        return _asset.balanceOf(address(this));
    }

The bug caused by using balanceOf as the totalAssets.

The code needs some significant refactoring, in particular in areas of using the pre-existing ERC4626 functions instead of redefining logic. The code needs a refresher in general as well.

In the vault contract, the deposit function signature is given with the override keyword, but not in the corresponding vault interface.
Consider adding the override keyword to the deposit function signature in the interface to remain consistent.

Within Constants.sol there is no SPDX license identifier.

To avoid legal issues regarding copyright and follow best practices, consider adding SPDX license identifiers to files as suggested by the Solidity documentation.

The current implementation calculates the fee as a percentage, then takes that in the form of shares withdrawn/redeemed. This is a somewhat naive implementation. A user might not have earned enough yield in the vault to compensate for the fee taken, leaving them with fewer assets than initially deposited.

An alternative fee approach would be to keep track of the yield earned per user, and only levy the fee as a percentage of the yield the user has earned in the vault. This would require a refactor to the fee logic.

To be discussed.

• Pass in 2 sigs from the user - permit and vault function sigs
• Use try-catch to handle reverts on permit blocking vault functions
• consider non-permit tokens and DAI's non standard permit

Description
In some cases after depositing an underlying token other calls(deposit, withdraw) will revert because it is updating the _s.lastVaultBalance by a different amount than it is updating the balanceof(ATOKEN).

So the lastVaultBalance can be greater than the balanceof(ATOKEN) and it will cause a revert due to underflow from the _accrueYield function.

    function _baseDeposit(uint256 assets, uint256 shares, address depositor, address receiver, bool asAToken) private {
        // Need to transfer before minting or ERC777s could reenter.
        if (asAToken) {
            ATOKEN.transferFrom(depositor, address(this), assets);
        } else {
            UNDERLYING.safeTransferFrom(depositor, address(this), assets);
            AAVE_POOL.supply(address(UNDERLYING), assets, address(this), REFERRAL_CODE);
        }

        _s.lastVaultBalance += uint128(assets);
        _mint(receiver, shares);

        emit Deposit(depositor, receiver, assets, shares);
    }

Impact
There will be a DOS until the index is large enough, it will cause the balanceof(ATOKEN) to be equal or greater than the _s.lastVaultBalance.

Use token amounts before/after to determine the actual change in ATokens held in the vault

Description:
In the initialize function there is no check for owner = address(0)

    function initialize(
        address owner,
        uint256 initialFee,
        string memory shareName,
        string memory shareSymbol,
        uint256 initialLockDeposit
    ) external initializer {
        require(initialLockDeposit != 0, "ZERO_INITIAL_LOCK_DEPOSIT");
        _transferOwnership(owner); // In this line
        __ERC4626_init(UNDERLYING);
        __ERC20_init(shareName, shareSymbol);
        __EIP712_init(shareName, "1");
        _setFee(initialFee);

        UNDERLYING.safeApprove(address(AAVE_POOL), type(uint256).max);

        _handleDeposit(initialLockDeposit, address(this), msg.sender, false);
    }

Impact:
It will be impossible to call any onlyowner function.
For example: No one will be able to claim the fees.

Currently the vault is susceptible to a frontrunning attack common in many ERC4626 vault contracts. See an explanation of the attack vector here.

I have reproduced the attack on a Polygon fork, and can confirm we are susceptible. I believe the most logical solution would be to enforce a minimum first deposit, something like 1e[decimals] sounds logical, but further investigation is needed.

Currently, the function setFee() simply updates the fee without cumulating yield, this results in the owner being able to take 100% of all cumulated yield since the last update.

The solution should be simple enough; adding a _cumulateYield() call to beginning of the setFee() function. This needs to be evaluated, though.

There are several opportunities for doc-string improvements. For example, in the ATokenVault :

• There is an extra comma on Line 47.
  https://github.com/aave/wrapped-atoken-vault/blob/f39678dd1279ff9d8bcc13dfa6fd0eaf75037b69/src/ATokenVault.sol#L47
• The comment on Line 70 is misleading as the initial deposit is in the underlying tokens instead of the 'underlying atokens'.
  https://github.com/aave/wrapped-atoken-vault/blob/f39678dd1279ff9d8bcc13dfa6fd0eaf75037b69/src/ATokenVault.sol#L70
• Many functions allow third parties to use a valid signature to call, for example, depositWithSig , depositATokensWithSig , etc. These functions require the signer of the message to have already approved the appropriate amount in either the underlying token or the aToken. Consider appropriately adding this requirement in the doc-string for relevant functions.

Throughout the codebase there are pragma statements that use an outdated version of Solidity.

Consider taking advantage of the latest Solidity version to improve the overall readability and security of the codebase. Regardless of which version of Solidity is used, consider pinning the version consistently throughout the codebase to prevent opening bugs due to incompatible future releases

Some functions are giving names to the return variable of the function, some not.

https://github.com/aave/wrapped-atoken-vault/blob/8473a3cb8cd167c6c894c7721f45ec95e21bffcc/src/ATokenVault.sol#L296-L302

https://github.com/aave/wrapped-atoken-vault/blob/8473a3cb8cd167c6c894c7721f45ec95e21bffcc/src/ATokenVault.sol#L359-L361

While our convention is not to give names to return variables, we can follow that approach to stay as close as possible to the ERC4626 for those functions that are part of the standard. We can follow our convention in the other cases.

The following functions do not conform to the EIP-4626 specifications:

• The maxWithdraw function in EIP-4626 should specify the amount of the underlying asset, in this case the ERC-20 token instead of the corresponding aToken, that can be withdrawn from the owner's balance through a withdraw call. The implementation is required to satisfy both the global and user-specific limits and MUST return 0 if disabled (even temporarily). However, the maxWithdraw function inherited from the base implementation only gives the limit in terms of the aToken amount, not the underlying token amount. In the case where withdraw from the pool would fail due to the pool configuration (e.g., the pool is paused), withdrawing from the vault would also fail. In such cases, the maxWithdraw should return 0 instead.
• The previewWithdraw in EIP-4626 should allow a user to simulate the effects of their withdrawal at the current block, given current on-chain conditions. However the previewWithdraw inherited from the base implementation does not account for the corresponding Aave-v3 pool states and liquidity position in that block. This applies similarly to the previewDeposit , previewMint and previewRedeem functions.

Consider incorporating the pool states and liquidity positions into the affected functions.

When depositing underlying tokens to the vault, an amount is transferred from the depositor to the vault and the same amount is then supplied to the pool. If the amount of underlying tokens that the vault receives is less than what is specified in the transfer (for example, if the underlying token is a fee-on transfer type), then supplying to the pool will revert and the deposit operation will fail.

Consider only supplying the actual amount transferred to the vault and updating the stored vault balance by the actual amount transferred.

Right now it is possible to pass in a valid sig from a depositor, then replace the intended recipient of shares with your own address. The signature itself does not check that shares flow to depositor, but only that the underlying asset flows from the depositor to the vault.

Users should be able to deposit the underlying ERC20 token, or the corresponding aToken.

Linked to #43

Within MetaTxHelpers.sol there is a require statement on line 31 that requires multiple conditions to be satisfied. To simplify the codebase and raise the most helpful error messages for failing require statements, consider employing a single require statement per condition.

https://github.com/aave/wrapped-atoken-vault/blob/f39678dd1279ff9d8bcc13dfa6fd0eaf75037b69/src/libraries/MetaTxHelpers.sol#L31

In some comments in the vault contract as well as the readme, EIP712 is accidentally referred to as EIP721.

If we do approvals at the contract level with permit signatures, we will need a custom implementation of this vault for DAI as it does not conform to the permit standard.