fix: adjust allowance consumption

sakulstra · web-flow · commit b7beed9ee672 · 2025-06-25T12:56:49.000+02:00
Historically transferFrom etc have consumed ther users input as allowance.
That has never really been accurate, as the users input might not actually be transferrable due to rounding, to account for that we improved the logic to consumer "up to the raw scaled up transfer amount".
diff --git a/docs/3.5/Aave-v3.5-features.md b/docs/3.5/Aave-v3.5-features.md
@@ -74,6 +74,14 @@ In v3.5 we decided to double down on these robustness improvements:
 
 - on `repayWithAToken` and `withdraw`, the collateral flag is now properly set to `false` when burning all aTokens in the process. In previous versions of the protocol, there were edge cases in which the collateral flag was not properly updated.
 
+### Improved allowance
+
+Aave historically has never accurately tracked allowance. The reason for this is that in practice most operations are performed with the desired amount of `assets`, but the a/v token converting these amounts to `shares`.
+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.
+
 ### Misc changes
 
 - smaller refactoring in `LiquidationLogic` making the code more consistent
diff --git a/snapshots/AToken.transfer.json b/snapshots/AToken.transfer.json
@@ -1,11 +1,11 @@
 {
-  "full amount; receiver: ->enableCollateral": "138978",
-  "full amount; sender: ->disableCollateral;": "97383",
-  "full amount; sender: ->disableCollateral; receiver: ->enableCollateral": "139095",
-  "full amount; sender: ->disableCollateral; receiver: dirty, ->enableCollateral": "127143",
-  "full amount; sender: collateralDisabled": "97266",
-  "partial amount; sender: collateralDisabled;": "97242",
-  "partial amount; sender: collateralDisabled; receiver: ->enableCollateral": "138954",
-  "partial amount; sender: collateralEnabled;": "97450",
-  "partial amount; sender: collateralEnabled; receiver: ->enableCollateral": "139162"
+  "full amount; receiver: ->enableCollateral": "138977",
+  "full amount; sender: ->disableCollateral;": "97382",
+  "full amount; sender: ->disableCollateral; receiver: ->enableCollateral": "139094",
+  "full amount; sender: ->disableCollateral; receiver: dirty, ->enableCollateral": "127142",
+  "full amount; sender: collateralDisabled": "97265",
+  "partial amount; sender: collateralDisabled;": "97241",
+  "partial amount; sender: collateralDisabled; receiver: ->enableCollateral": "138953",
+  "partial amount; sender: collateralEnabled;": "97449",
+  "partial amount; sender: collateralEnabled; receiver: ->enableCollateral": "139161"
 }
diff --git a/snapshots/ATokenWithDelegation.transfer.json b/snapshots/ATokenWithDelegation.transfer.json
@@ -1,8 +1,8 @@
 {
-  "full amount; sender: with delegations, ->disableCollateral; receiver: without delegations, ->enableCollateral": "158388",
-  "full amount; sender: with delegations, delegatee, ->disableCollateral; receiver: with delegations, delegatee, ->enableCollateral": "154326",
-  "full amount; sender: with delegations, not delegatee, ->disableCollateral; receiver: with delegations, not delegatee, ->enableCollateral": "154326",
-  "full amount; sender: without delegations, ->disableCollateral; receiver: with delegations, ->enableCollateral": "141288",
-  "full amount; sender: without delegations, delegatee, ->disableCollateral; receiver: without delegations, delegatee, ->enableCollateral": "145351",
-  "full amount; sender: without delegations, not delegatee, ->disableCollateral; receiver: without delegations, not delegatee, ->enableCollateral": "145351"
+  "full amount; sender: with delegations, ->disableCollateral; receiver: without delegations, ->enableCollateral": "158387",
+  "full amount; sender: with delegations, delegatee, ->disableCollateral; receiver: with delegations, delegatee, ->enableCollateral": "154325",
+  "full amount; sender: with delegations, not delegatee, ->disableCollateral; receiver: with delegations, not delegatee, ->enableCollateral": "154325",
+  "full amount; sender: without delegations, ->disableCollateral; receiver: with delegations, ->enableCollateral": "141287",
+  "full amount; sender: without delegations, delegatee, ->disableCollateral; receiver: without delegations, delegatee, ->enableCollateral": "145350",
+  "full amount; sender: without delegations, not delegatee, ->disableCollateral; receiver: without delegations, not delegatee, ->enableCollateral": "145350"
 }
diff --git a/snapshots/Pool.Operations.json b/snapshots/Pool.Operations.json
@@ -6,12 +6,12 @@
   "flashLoan: flash loan for two assets": "257524",
   "flashLoan: flash loan for two assets and borrow": "461097",
   "flashLoanSimple: simple flash loan": "138283",
-  "liquidationCall: deficit on liquidated asset": "352448",
-  "liquidationCall: deficit on liquidated asset + other asset": "438268",
-  "liquidationCall: full liquidation": "352448",
-  "liquidationCall: full liquidation and receive ATokens": "349752",
-  "liquidationCall: partial liquidation": "343371",
-  "liquidationCall: partial liquidation and receive ATokens": "340675",
+  "liquidationCall: deficit on liquidated asset": "352447",
+  "liquidationCall: deficit on liquidated asset + other asset": "438267",
+  "liquidationCall: full liquidation": "352447",
+  "liquidationCall: full liquidation and receive ATokens": "349750",
+  "liquidationCall: partial liquidation": "343370",
+  "liquidationCall: partial liquidation and receive ATokens": "340673",
   "repay: full repay": "161770",
   "repay: full repay with ATokens": "163597",
   "repay: partial repay": "158122",
diff --git a/snapshots/Pool.OperationsComposition.json b/snapshots/Pool.OperationsComposition.json
@@ -1,5 +1,5 @@
 {
-  "batchLiquidate: liquidate 2 users": "486407",
+  "batchLiquidate: liquidate 2 users": "486405",
   "repayAndWithdraw: borrow disabled, collateral disabled": "290170",
   "supplyAndBorrow: first supply->collateralEnabled, first borrow": "372109"
 }
diff --git a/snapshots/StataTokenV2.json b/snapshots/StataTokenV2.json
@@ -1,7 +1,7 @@
 {
   "claimRewards": "359625",
   "deposit": "274833",
-  "depositATokens": "218541",
+  "depositATokens": "220899",
   "redeem": "197171",
-  "redeemAToken": "147142"
+  "redeemAToken": "147141"
 }
diff --git a/snapshots/WrappedTokenGatewayV3.json b/snapshots/WrappedTokenGatewayV3.json
@@ -1,6 +1,6 @@
 {
-  "borrowETH": "236036",
+  "borrowETH": "236535",
   "depositETH": "195025",
   "repayETH": "179154",
-  "withdrawETH": "244706"
+  "withdrawETH": "247524"
 }
diff --git a/src/contracts/interfaces/ICreditDelegationToken.sol b/src/contracts/interfaces/ICreditDelegationToken.sol
@@ -21,6 +21,14 @@ interface ICreditDelegationToken {
     uint256 amount
   );
 
+  /**
+   * @dev Indicates a failure with the `spender`’s `allowance`. Used in borrowing.
+   * @param spender Address that may be allowed to operate on tokens without being their owner.
+   * @param allowance Amount of tokens a `spender` is allowed to operate with.
+   * @param needed Minimum amount required to perform a transfer.
+   */
+  error InsufficientBorrowAllowance(address spender, uint256 allowance, uint256 needed);
+
   /**
    * @notice Delegates borrowing power to a user on the specific debt token.
    * Delegation will still respect the liquidation constraints (even if delegated, a
diff --git a/src/contracts/protocol/tokenization/AToken.sol b/src/contracts/protocol/tokenization/AToken.sol
@@ -190,6 +190,32 @@ abstract contract AToken is VersionedInitializable, ScaledBalanceTokenBase, EIP7
     _approve(owner, spender, value);
   }
 
+  /// @inheritdoc IERC20
+  function transferFrom(
+    address sender,
+    address recipient,
+    uint256 amount
+  ) external virtual override(IERC20, IncentivizedERC20) returns (bool) {
+    uint256 index = POOL.getReserveNormalizedIncome(_underlyingAsset);
+    _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.
+      // - In order to not introduce a breaking change for existing integrations, the deducted allowance is based on the available allowance as `Max(availableAllowance, (amount, correctAmount))`
+      amount.getATokenTransferScaledAmount(index).getATokenBalance(index)
+    );
+    _transfer(sender, recipient, amount.toUint120());
+    return true;
+  }
+
   /**
    * @notice Overrides the parent _transfer to force validated transfer() and transferFrom()
    * @param from The source address
diff --git a/src/contracts/protocol/tokenization/VariableDebtToken.sol b/src/contracts/protocol/tokenization/VariableDebtToken.sol
@@ -84,7 +84,23 @@ abstract contract VariableDebtToken is DebtTokenBase, ScaledBalanceTokenBase, IV
     uint256 index
   ) external virtual override onlyPool returns (uint256) {
     if (user != onBehalfOf) {
-      _decreaseBorrowAllowance(onBehalfOf, user, amount);
+      uint256 borrowAllowance = _borrowAllowances[onBehalfOf][user];
+      if (borrowAllowance < amount) {
+        revert InsufficientBorrowAllowance(user, borrowAllowance, amount);
+      }
+      // When borrowing on behalf of a user, the borrower specified an "amount", which is measured in the underlying the user wants to receive.
+      // The protocol internally works, with a "scaled down" representation of the amount, which in most cases loses precision.
+      // In practice this means that when borrowing `n`, the user might receive `n+m` debt.
+      // Similar to the aToken `transferFrom` function, handling this scenario exactly is impossible.
+      // 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 `scaledUpCeil(scaledAmount)`. This replicates the behavior on borrow followed by a balanceOf.
+      // - In order to not introduce a breaking change for existing integrations, the deducted allowance is based on the available allowance as `Max(availableAllowance, (amount, correctAmount))`
+      uint256 scaledUp = scaledAmount.getVTokenBalance(index);
+      _decreaseBorrowAllowance(
+        onBehalfOf,
+        user,
+        borrowAllowance >= scaledUp ? scaledUp : borrowAllowance
+      );
     }
     _mintScaled({
       caller: user,
diff --git a/src/contracts/protocol/tokenization/base/IncentivizedERC20.sol b/src/contracts/protocol/tokenization/base/IncentivizedERC20.sol
@@ -22,6 +22,14 @@ abstract contract IncentivizedERC20 is Context, IERC20Detailed {
   using WadRayMath for uint256;
   using SafeCast for uint256;
 
+  /**
+   * @dev Indicates a failure with the `spender`’s `allowance`. Used in transfers.
+   * @param spender Address that may be allowed to operate on tokens without being their owner.
+   * @param allowance Amount of tokens a `spender` is allowed to operate with.
+   * @param needed Minimum amount required to perform a transfer.
+   */
+  error ERC20InsufficientAllowance(address spender, uint256 allowance, uint256 needed);
+
   /**
    * @dev Only pool admin can call functions marked by this modifier.
    */
@@ -183,6 +191,30 @@ abstract contract IncentivizedERC20 is Context, IERC20Detailed {
     return true;
   }
 
+  /**
+   * @dev Updates `owner`'s allowance for `spender` based on spent `value`.
+   *
+   * Does not update the allowance value in case of infinite allowance.
+   * Revert if not enough allowance is available.
+   *
+   * @param owner The owner of the tokens
+   * @param spender The user allowed to spend on behalf of owner
+   * @param amount The minimum amount being consumed from the allowance
+   * @param correctedAmount The maximum amount being consumed from the allowance
+   */
+  function _spendAllowance(
+    address owner,
+    address spender,
+    uint256 amount,
+    uint256 correctedAmount
+  ) internal virtual {
+    uint256 currentAllowance = _allowances[owner][spender];
+    if (currentAllowance < amount)
+      revert ERC20InsufficientAllowance(spender, currentAllowance, amount);
+    uint256 consumption = currentAllowance >= correctedAmount ? correctedAmount : currentAllowance;
+    _approve(owner, spender, currentAllowance - consumption);
+  }
+
   /**
    * @notice Transfers tokens between two users and apply incentives if defined.
    * @param sender The source address
diff --git a/tests/protocol/tokenization/AToken_TransferFrom.t.sol b/tests/protocol/tokenization/AToken_TransferFrom.t.sol
@@ -0,0 +1,86 @@
+// SPDX-License-Identifier: BUSL-1.1
+pragma solidity ^0.8.0;
+
+import 'forge-std/Test.sol';
+
+import {IAToken, IERC20} from '../../../src/contracts/interfaces/IAToken.sol';
+import {IScaledBalanceToken} from '../../../src/contracts/interfaces/IScaledBalanceToken.sol';
+import {IVariableDebtToken} from '../../../src/contracts/interfaces/IVariableDebtToken.sol';
+import {IPool} from '../../../src/contracts/interfaces/IPool.sol';
+import {Errors} from '../../../src/contracts/protocol/libraries/helpers/Errors.sol';
+import {SupplyLogic} from '../../../src/contracts/protocol/libraries/logic/SupplyLogic.sol';
+import {MathUtils} from '../../../src/contracts/protocol/libraries/math/MathUtils.sol';
+import {WadRayMath} from '../../../src/contracts/protocol/libraries/math/WadRayMath.sol';
+import {DataTypes} from '../../../src/contracts/protocol/libraries/types/DataTypes.sol';
+import {TestnetProcedures} from '../../utils/TestnetProcedures.sol';
+import {AaveSetters} from '../../utils/AaveSetters.sol';
+import {MockAToken} from '../../../src/contracts/mocks/tokens/MockAToken.sol';
+
+contract ATokenTransferFromTests is TestnetProcedures {
+  using WadRayMath for uint256;
+
+  error ERC20InsufficientAllowance(address spender, uint256 allowance, uint256 needed);
+
+  address public token;
+  IAToken public aToken;
+
+  function setUp() public {
+    initTestEnvironment(false);
+    token = tokenList.wbtc;
+    aToken = IAToken(contracts.poolProxy.getReserveAToken(token));
+  }
+
+  function test_transferFrom_shouldRevertIfSenderIsNotApproved() external {
+    address sender = address(0x1);
+    address owner = address(0x2);
+    uint256 amount = 100;
+    vm.expectRevert(abi.encodeWithSelector(ERC20InsufficientAllowance.selector, sender, 0, amount));
+    vm.prank(sender);
+    aToken.transferFrom(owner, sender, amount);
+  }
+
+  function test_transferFrom_shouldRevertIfSenderInsufficientAllowance() external {
+    address sender = address(0x1);
+    address owner = address(0x2);
+    uint256 amount = 100;
+    vm.prank(owner);
+    aToken.approve(sender, amount - 1);
+
+    vm.expectRevert(
+      abi.encodeWithSelector(ERC20InsufficientAllowance.selector, sender, amount - 1, amount)
+    );
+    vm.prank(sender);
+    aToken.transferFrom(owner, sender, amount);
+  }
+
+  function test_transferFrom(uint128 liquidityIndex, uint256 approval) external {
+    address sender = address(0x1);
+    address owner = address(0x2);
+    uint256 amount = 1e18;
+
+    liquidityIndex = uint128(bound(liquidityIndex, 1e27, 100_000e27));
+    AaveSetters.setATokenBalance(address(aToken), owner, amount, 1e27);
+    AaveSetters.setLiquidityIndex(address(contracts.poolProxy), token, liquidityIndex);
+    vm.prank(owner);
+    aToken.approve(sender, bound(approval, amount, amount.rayMulCeil(liquidityIndex)));
+
+    uint256 ownerScaledBalanceBefore = aToken.scaledBalanceOf(owner);
+    uint256 ownerAllowanceBefore = aToken.allowance(owner, sender);
+    uint256 senderBalanceBefore = aToken.balanceOf(sender);
+
+    vm.prank(sender);
+    aToken.transferFrom(owner, sender, amount);
+
+    uint256 ownerScaledBalanceAfter = aToken.scaledBalanceOf(owner);
+    uint256 ownerAllowanceAfter = aToken.allowance(owner, sender);
+    uint256 senderBalanceAfter = aToken.balanceOf(sender);
+
+    assertGe(ownerAllowanceBefore - ownerAllowanceAfter, amount);
+    assertGe(senderBalanceAfter - senderBalanceBefore, amount);
+
+    uint256 upscaledDiff = (ownerScaledBalanceBefore - ownerScaledBalanceAfter).rayMulFloor(
+      liquidityIndex
+    );
+    assertGe(upscaledDiff, amount);
+  }
+}
diff --git a/tests/protocol/tokenization/VariableDebtToken_MintOnBehalf.t.sol b/tests/protocol/tokenization/VariableDebtToken_MintOnBehalf.t.sol

Original file line number	Diff line number	Diff line change
`@@ -1,5 +1,5 @@`
`1`	`1`	`{`
`2`		`- "batchLiquidate: liquidate 2 users": "486407",`
	`2`	`+ "batchLiquidate: liquidate 2 users": "486405",`
`3`	`3`	`"repayAndWithdraw: borrow disabled, collateral disabled": "290170",`
`4`	`4`	`"supplyAndBorrow: first supply->collateralEnabled, first borrow": "372109"`
`5`	`5`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1,7 +1,7 @@`
`1`	`1`	`{`
`2`	`2`	`"claimRewards": "359625",`
`3`	`3`	`"deposit": "274833",`
`4`		`- "depositATokens": "218541",`
	`4`	`+ "depositATokens": "220899",`
`5`	`5`	`"redeem": "197171",`
`6`		`- "redeemAToken": "147142"`
	`6`	`+ "redeemAToken": "147141"`
`7`	`7`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1,6 +1,6 @@`
`1`	`1`	`{`
`2`		`- "borrowETH": "236036",`
	`2`	`+ "borrowETH": "236535",`
`3`	`3`	`"depositETH": "195025",`
`4`	`4`	`"repayETH": "179154",`
`5`		`- "withdrawETH": "244706"`
	`5`	`+ "withdrawETH": "247524"`
`6`	`6`	`}`