Aave's share tokens are rebasing breaking current strategy code

[code423n4]

Handle

cmichel

Vulnerability details

When depositing into Aave through the AaveYield.lockTokens contract strategy, one receives the sharesReceived amount corresponding to the diff of aToken balance, which is just always the deposited amount as aave is a rebasing token and 1.0 aToken = 1.0 underlying at each deposit / withdrawal.

Note that this sharesReceived (the underlying deposit amount) is cached in a balanceInShares map in SavingsAccount.deposit which makes this share static and not dynamically rebasing anymore:

function deposit(
    uint256 _amount,
    address _token,
    address _strategy,
    address _to
) external payable override nonReentrant returns (uint256) {
    require(_to != address(0), 'SavingsAccount::deposit receiver address should not be zero address');
    uint256 _sharesReceived = _deposit(_amount, _token, _strategy);
    balanceInShares[_to][_token][_strategy] = balanceInShares[_to][_token][_strategy].add(_sharesReceived);
    emit Deposited(_to, _sharesReceived, _token, _strategy);
    return _sharesReceived;
}

function getTokensForShares(uint256 shares, address asset) public view override returns (uint256 amount) {
    if (shares == 0) return 0;
    address aToken = liquidityToken(asset);

    (, , , , , , , uint256 liquidityIndex, , ) = IProtocolDataProvider(protocolDataProvider).getReserveData(asset);

    // @audit-info tries to do (user shares / total shares) * underlying amount where underlying amount = scaledBalance * liquidityIndex
    amount = IScaledBalanceToken(aToken).scaledBalanceOf(address(this)).mul(liquidityIndex).mul(shares).div(
        IERC20(aToken).balanceOf(address(this))
    );
}

However, the getTokensForShares function uses a rebasing total share supply of IERC20(aToken).balanceOf(this).

POC

• SavingsAccount deposits 1000 DAI for user and user receives 1000 aDAI as shares. These shares are cached in balanceInShares[user][dai][aave].
• Time passes, Aave accrues interest for lenders, and the initial 1000 aTokens balance has rebased to 1200 aTokens
• SavingsAccount withdraws 1000 aDAI shares for user which calls AaveYield.unlockTokens. The user receives only 1000 DAI. The interest owed to the user is not paid out.
• Note that getTokensForShares also returns the wrong amount as 1200 * 1000 / 1200 = 1000

Impact

Interest is not paid out to users. Pool collateral is measured without the interest accrued as it uses getTokensForShares which will lead to early liquidations and further loss.

Recommended Mitigation Steps

If the user shares are not rebasing, you cannot have the "total shares supply" (the shares in the contract) be rebasing as in getTokensForShares. Also withdrawing the share amount directly from Aave as in _withdrawERC does not withdraw the yield. A fix could be to create a non-rebasing wrapper LP token that is paid out to the user proportional to the current strategy TVL at time of user deposit.

[ritik99]

We've been aware of this issue for some time.. ended up including the AaveYield file in the scope by mistake! We do not plan to include the Aave strategy in our launch (we maintain a strategy registry that allows us to add/drop yield strategies), and as noted in #128, we will be utilizing wrapper contracts that mimics behaviour of non-rebasing LP tokens

[0xean]

going to side with the warden since they believed the contract to be in scope and it's a valid concern.