Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #730. accruedToTreasury not being properly considered together with aToken supply #733

Merged

Conversation

eboadom
Copy link
Collaborator

@eboadom eboadom commented Nov 11, 2022

#730

The changes included in this PR are the following:

  • On both FlashLoan and BridgeLogic the accumulation to the liquidity index now includes both aToken supply and scaled-up accruedToTreasury.
  • On ValidationLogic, now validateSupply() takes into account accruedToTreasury, to not allow deposits when (aToken supply + scaled-up accruedToTreasury) is bigger than the supply cap.
  • Also on ValidationLogic, validateDropReserve now checks that both aToken supply and accruedToTreasury are 0.
  • On PoolConfigurator, _checkNoSuppliers() checks that accruedToTreasury is 0, as factually the Collector is a depositor in that case, even if not holding aToken.
  • Tests for the most important fix, affecting flash loan fee (and bridge fee, but it is exactly the same)
  • Test for fixes on validations

…e flash loan fee (same as bridging) imprecision
@@ -72,7 +73,9 @@ library ValidationLogic {
supplyCap == 0 ||
(IAToken(reserveCache.aTokenAddress).scaledTotalSupply().rayMul(
reserveCache.nextLiquidityIndex
) + amount) <=
) +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess adding up the scaled amounts and multiplying will be the same and save 1 operation. Is there a reason you didn't do it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, makes more sense. Will change

Copy link
Contributor

@miguelmtzinf miguelmtzinf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Looking forward to the validation tests

@miguelmtzinf
Copy link
Contributor

I'm in doubt about if the aToken should also reflects this in its totalSupply() and scaledTotalSupply() functions.

Three issues:

  1. The aToken real totalSupply should account for accruedToTreasury, becuase its actually part of the aToken supply (just a deferred mint)
  2. The AaveIncentivesController handleAction() call is wrong, because we are passing a smaller value: https://github.com/aave/aave-v3-core/blob/master/contracts/protocol/tokenization/base/IncentivizedERC20.sol#L206
  3. Integrations can become cumbersome if they use the aToken totalSupply without taking the consideration of accounting the accruedToTreasury. This is the case of the Rewards contracts (similar to 2):
    https://github.com/aave/aave-v3-periphery/blob/master/contracts/rewards/RewardsController.sol#L85
    https://github.com/aave/aave-v3-periphery/blob/master/contracts/rewards/RewardsDistributor.sol#L189

@eboadom
Copy link
Collaborator Author

eboadom commented Nov 14, 2022

I'm in doubt about if the aToken should also reflects this in its totalSupply() and scaledTotalSupply() functions.

Three issues:

  1. The aToken real totalSupply should account for accruedToTreasury, becuase its actually part of the aToken supply (just a deferred mint)
  2. The AaveIncentivesController handleAction() call is wrong, because we are passing a smaller value: https://github.com/aave/aave-v3-core/blob/master/contracts/protocol/tokenization/base/IncentivizedERC20.sol#L206
  3. Integrations can become cumbersome if they use the aToken totalSupply without taking the consideration of accounting the accruedToTreasury. This is the case of the Rewards contracts (similar to 2):
    https://github.com/aave/aave-v3-periphery/blob/master/contracts/rewards/RewardsController.sol#L85
    https://github.com/aave/aave-v3-periphery/blob/master/contracts/rewards/RewardsDistributor.sol#L189
  1. I thought about it, but don't agree. The aToken supply should only represent the total supply of minted tokens. Withdrawal rights are then managed by the pool, so the token should not really be aware of the accruedToTreasury
  2. It is fine, just that the accruedToTreasury will start to accrue rewards whenever minted. I think it is legit, if not, will break 1).
  3. It is not really a problem. It is way worse for integrators if there is some extra component to form the total of scaledTotalSupply()

@miguelmtzinf
Copy link
Contributor

I'm in doubt about if the aToken should also reflects this in its totalSupply() and scaledTotalSupply() functions.
Three issues:

  1. The aToken real totalSupply should account for accruedToTreasury, becuase its actually part of the aToken supply (just a deferred mint)
  2. The AaveIncentivesController handleAction() call is wrong, because we are passing a smaller value: https://github.com/aave/aave-v3-core/blob/master/contracts/protocol/tokenization/base/IncentivizedERC20.sol#L206
  3. Integrations can become cumbersome if they use the aToken totalSupply without taking the consideration of accounting the accruedToTreasury. This is the case of the Rewards contracts (similar to 2):
    https://github.com/aave/aave-v3-periphery/blob/master/contracts/rewards/RewardsController.sol#L85
    https://github.com/aave/aave-v3-periphery/blob/master/contracts/rewards/RewardsDistributor.sol#L189
  1. I thought about it, but don't agree. The aToken supply should only represent the total supply of minted tokens. Withdrawal rights are then managed by the pool, so the token should not really be aware of the accruedToTreasury
  2. It is fine, just that the accruedToTreasury will start to accrue rewards whenever minted. I think it is legit, if not, will break 1).
  3. It is not really a problem. It is way worse for integrators if there is some extra component to form the total of scaledTotalSupply()

Fair enough. Although debatable, this is probably the approach that makes most sense. Thanks

- Optimised calculations on ValidationLogic
- Renamed error id to be more precise
Tests:
- Added tests for changes on validations.
@eboadom eboadom changed the title WIP. Fixes #730. accruedToTreasury not being properly considered together with aToken supply Fixes #730. accruedToTreasury not being properly considered together with aToken supply Nov 15, 2022
helpers/types.ts Outdated Show resolved Hide resolved
@miguelmtzinf miguelmtzinf merged commit 1b950ab into aave:feat/3.0.1 Nov 15, 2022
@eboadom eboadom mentioned this pull request Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants