fix: better allowance mechanism + informal issue fixes

Author: TepNik

docs/3.5/Aave-v3.5-features.md

@@ -80,7 +80,8 @@ Aave historically has never accurately tracked allowance. The reason for this is
 For allowance / approval, this means that the consumed allowance is not always equal to the amount transferred. While this problem is not perfectly solvable without breaking changes, in v3.5 the protocol ensures that the exact consumed allowance is burned if available.
 
 Example: When a user calls `transferFrom(sender, recipient, 100)` in most cases the transfer will transfer slightly more than `100` tokens (e.g `101`). This is due to precision loss between assets/shares conversion.
-In Aave versions `< 3.5` this action would always result in burning `100` allowance. On Aave v3.5, the transfer will either burn `101` allowance or burn `100` allowance dependent on the users available allowance. This prevents undesired double spending of allowance, while maintaining backwards compatibility.
+In Aave versions `< 3.5` this action would always result in burning `100` allowance. On Aave v3.5, the transfer will check the balance difference on the sender and discount up to the difference from the allowance.
+Example: The user transfers `100`, but due to rounding he loses `102` balance. The allowance is reduced by up to `102`. If the original allowance was only `100`, the transaction will still pass for backwards compatibility.
 
 ### Misc changes
 

snapshots/Pool.Operations.json

@@ -1,10 +1,10 @@
 {
-  "borrow: first borrow->borrowingEnabled": "250129",
-  "borrow: recurrent borrow": "215047",
+  "borrow: first borrow->borrowingEnabled": "250372",
+  "borrow: recurrent borrow": "215290",
   "flashLoan: flash loan for one asset": "162245",
-  "flashLoan: flash loan for one asset and borrow": "267279",
+  "flashLoan: flash loan for one asset and borrow": "267522",
   "flashLoan: flash loan for two assets": "257244",
-  "flashLoan: flash loan for two assets and borrow": "461912",
+  "flashLoan: flash loan for two assets and borrow": "462398",
   "flashLoanSimple: simple flash loan": "138143",
   "liquidationCall: deficit on liquidated asset": "352397",
   "liquidationCall: deficit on liquidated asset + other asset": "438234",

snapshots/Pool.OperationsComposition.json

@@ -1,5 +1,5 @@
 {
   "batchLiquidate: liquidate 2 users": "486305",
   "repayAndWithdraw: borrow disabled, collateral disabled": "290170",
-  "supplyAndBorrow: first supply->collateralEnabled, first borrow": "377351"
+  "supplyAndBorrow: first supply->collateralEnabled, first borrow": "377594"
 }

snapshots/StataTokenV2.json

@@ -1,7 +1,7 @@
 {
   "claimRewards": "359625",
   "deposit": "274833",
-  "depositATokens": "220899",
+  "depositATokens": "221434",
   "redeem": "197171",
   "redeemAToken": "147141"
 }

snapshots/WrappedTokenGatewayV3.json

@@ -1,6 +1,6 @@
 {
-  "borrowETH": "237215",
+  "borrowETH": "239507",
   "depositETH": "195025",
   "repayETH": "179129",
-  "withdrawETH": "247524"
+  "withdrawETH": "248059"
 }

src/contracts/instances/VariableDebtTokenMainnetInstanceGHO.sol

@@ -48,6 +48,6 @@ contract VariableDebtTokenMainnetInstanceGHO is VariableDebtToken {
     );
   }
 
-  // @note deprecated discount hook being called by stkAAVE
+  // @note deprecated discount hook being called by stkAAVE, not used since v3.4
   function updateDiscountDistribution(address, address, uint256, uint256, uint256) external {}
 }

src/contracts/interfaces/IAToken.sol

@@ -36,13 +36,14 @@ interface IAToken is IERC20, IScaledBalanceToken, IInitializableAToken {
   ) external returns (bool);
 
   /**
-   * @notice Burns aTokens from `user` and sends the equivalent amount of underlying to `receiverOfUnderlying`
-   * @dev In some instances, the mint event could be emitted from a burn transaction
-   * if the amount to burn is less than the interest that the user accrued
+   * @notice Burns aTokens from `user` and sends the equivalent amount of underlying to `receiverOfUnderlying`.
+   * @dev Passing both the unscaled and scaled amounts enhances precision. The `scaledAmount` is used for precise balance updates,
+   * while the `amount` is used for the underlying asset transfer, preventing cumulative rounding errors.
+   * @dev In some instances, a mint event may be emitted from a burn transaction if the amount to burn is less than the interest that the user accrued.
    * @param from The address from which the aTokens will be burned
    * @param receiverOfUnderlying The address that will receive the underlying
-   * @param amount The amount being burned
-   * @param scaledAmount The scaled amount being burned
+   * @param amount The amount of underlying to be burned (non scaled)
+   * @param scaledAmount The scaled amount of aTokens to be burned (scaled)
    * @param index The next liquidity index of the reserve
    * @return `true` if the the new balance of the user is 0
    */
@@ -62,11 +63,13 @@ interface IAToken is IERC20, IScaledBalanceToken, IInitializableAToken {
   function mintToTreasury(uint256 scaledAmount, uint256 index) external;
 
   /**
-   * @notice Transfers aTokens in the event of a borrow being liquidated, in case the liquidators reclaims the aToken
+   * @notice Transfers aTokens in the event of a borrow being liquidated, in case the liquidator reclaims the aToken.
+   * @dev Passing both the unscaled and scaled amounts enhances precision. The `scaledAmount` is used for precise balance updates,
+   * while the `amount` is used for logging and consistency, preventing cumulative rounding errors.
    * @param from The address getting liquidated, current owner of the aTokens
    * @param to The recipient
-   * @param amount The amount of tokens getting transferred
-   * @param scaledAmount The scaled amount of tokens getting transferred
+   * @param amount The amount of tokens getting transferred (non-scaled)
+   * @param scaledAmount The scaled amount of tokens getting transferred (scaled)
    * @param index The next liquidity index of the reserve
    */
   function transferOnLiquidation(

src/contracts/interfaces/IVariableDebtToken.sol

@@ -11,12 +11,14 @@ import {IInitializableDebtToken} from './IInitializableDebtToken.sol';
  */
 interface IVariableDebtToken is IScaledBalanceToken, IInitializableDebtToken {
   /**
-   * @notice Mints debt token to the `onBehalfOf` address
+   * @notice Mints debt token to the `onBehalfOf` address.
+   * @dev Passing both the unscaled and scaled amounts enhances precision. The `scaledAmount` is used for precise balance updates,
+   * while the `amount` is used for allowance checks, preventing cumulative rounding errors.
    * @param user The address receiving the borrowed underlying, being the delegatee in case
    * of credit delegate, or same as `onBehalfOf` otherwise
    * @param onBehalfOf The address receiving the debt tokens
-   * @param amount The amount of debt being accounted for on the allowance
-   * @param scaledAmount The scaledAmount of debt being minted
+   * @param amount The unscaled amount of debt to be accounted for allowance
+   * @param scaledAmount The scaled amount of debt tokens to mint
    * @param index The variable debt index of the reserve
    * @return The scaled total debt of the reserve
    */
@@ -29,11 +31,11 @@ interface IVariableDebtToken is IScaledBalanceToken, IInitializableDebtToken {
   ) external returns (uint256);
 
   /**
-   * @notice Burns user variable debt
-   * @dev In some instances, a burn transaction will emit a mint event
-   * if the amount to burn is less than the interest that the user accrued
+   * @notice Burns user variable debt.
+   * @dev Passing the scaled amount allows for more precise calculations and avoids cumulative errors from repeated conversions.
+   * @dev In some instances, a burn transaction will emit a mint event if the amount to burn is less than the interest that the user accrued.
    * @param from The address from which the debt will be burned
-   * @param scaledAmount The scaled amount getting burned
+   * @param scaledAmount The scaled amount of debt getting burned
    * @param index The variable debt index of the reserve
    * @return True if the new balance is zero
    * @return The scaled total debt of the reserve

src/contracts/protocol/libraries/helpers/TokenMath.sol

@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: MIT
-pragma solidity ^0.8.10;
+pragma solidity ^0.8.0;
 
 import {WadRayMath} from '../../libraries/math/WadRayMath.sol';
 

src/contracts/protocol/tokenization/AToken.sol

@@ -191,20 +191,41 @@ abstract contract AToken is VersionedInitializable, ScaledBalanceTokenBase, EIP7
     uint256 amount
   ) external virtual override(IERC20, IncentivizedERC20) returns (bool) {
     uint256 index = POOL.getReserveNormalizedIncome(_underlyingAsset);
+    uint256 scaledBalanceOfSender = super.balanceOf(sender);
     _spendAllowance(
       sender,
       _msgSender(),
       amount,
-      // According to the ERC20 specification, the spent allowance should reflect the amount transferred.
-      // Following the spec exactly is impossible though, as the allowance references the "scaled up" amount while the transfer operates with "scaled down" amount.
-      // Because of this different handling of amounts, there are amounts that are impossible to accurately reflect on the balance.
-      // As an example: transferFrom(..., 1) at a liquidity index of 2e27, can never transfer exactly `1` as the smallest unit that can be accounted for would be 2.
-      // In addition to that the existing balance has an effect on the final "scaled up" balance after transfer.
-      // As an example: transferFrom(..., 1) at a liquidity index of 1.1 where the recipient has a "scaled up" balance of `9 * 1.1 = 9.9 = 9` before the transfer, will have a balance of `10 * 1.1. = 11` after the Transfer.
-      // While this problem is not solvable without introducing breaking changes, on Aave v3.5 the situation is improved in the following way:
-      // - The `correct` amount to be deducted is considered to be `scaledUpFloor(scaledDownCeil(input.amount))`. This replicates the behavior on transfer, followed by a balanceOf.
-      // - To avoid breaking existing integrations, the amount deducted from the allowance is the minimum of the available allowance and the actual up-scaled asset amount.
-      amount.getATokenTransferScaledAmount(index).getATokenBalance(index)
+      // This comment explains the logic behind the allowance spent calculation.
+      //
+      // Problem:
+      // Simply decreasing the allowance by the input `amount` is not ideal for scaled-balance tokens.
+      // Due to rounding, the actual decrease in the sender's balance (`amount_out`) can be slightly
+      // larger than the input `amount`.
+      //
+      // Definitions:
+      // - `amount`: The unscaled amount to be transferred, passed as an argument.
+      // - `amount_out`: The actual unscaled amount deducted from the sender's balance.
+      // - `amount_in`: The actual unscaled amount added to the recipient's balance.
+      // - `allowance_spent`: The unscaled amount deducted from the spender's allowance.
+      // - `amount_logged`: The amount logged in the `Transfer` event.
+      //
+      // Solution:
+      // To fix this, `allowance_spent` must be exactly equal to `amount_out`.
+      // We calculate `amount_out` precisely by simulating the effect of the transfer on the sender's balance.
+      // `actualBalanceDecrease` in the code corresponds to `amount_out`.
+      // By passing `actualBalanceDecrease` to `_spendAllowance`, we ensure `allowance_spent` is as close as possible to `amount_out`.
+      // `amount_logged` is equal to `amount`. `amount_in` is the actual balance increase for the recipient, which is >= `amount` due to rounding.
+      //
+      // Backward Compatibility & Guarantees:
+      // This implementation is backward-compatible and secure. The `_spendAllowance` function has a critical feature:
+      // 1. It REQUIRES the allowance to be >= `amount` (the user's requested transfer amount).
+      // 2. The amount consumed from the allowance is `actualBalanceDecrease`, but it is capped at the `currentAllowance`.
+      // This means if a user has an allowance of 100 wei and calls `transferFrom` with an `amount` of 100, the call will succeed
+      // even if the calculated `actualBalanceDecrease` is 101 wei. In that specific scenario, the allowance consumed will be 100 wei (since that is the `currentAllowance`),
+      // and the transaction will not revert. But if the allowance is 101 wei, then the allowance consumed will be 101 wei.
+      scaledBalanceOfSender.getATokenBalance(index) -
+        (scaledBalanceOfSender - amount.getATokenTransferScaledAmount(index)).getATokenBalance(index)
     );
     _transfer(sender, recipient, amount.toUint120());
     return true;