aave / aave-vault Goto Github PK
View Code? Open in Web Editor NEWERC4626 vault to hold aToken assets
License: Other
ERC4626 vault to hold aToken assets
License: Other
Within IATokenVault.sol , some events do not have their parameters indexed. For instance:
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.
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
:
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.
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:
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.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.
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.
A declarative, efficient, and flexible JavaScript library for building user interfaces.
๐ Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. ๐๐๐
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google โค๏ธ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.