Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions snapshots/PositionManager.Operations.json
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
{
"AllowancePositionManager: approveWithdraw": "46836",
"AllowancePositionManager: approveWithdrawWithSig": "63047",
"AllowancePositionManager: borrowOnBehalfOf": "308793",
"AllowancePositionManager: creditDelegation": "46814",
"AllowancePositionManager: creditDelegationWithSig": "63134",
"AllowancePositionManager: renounceCreditDelegation": "24967",
"AllowancePositionManager: renounceWithdrawAllowance": "24989",
"AllowancePositionManager: withdrawOnBehalfOf: full": "123488",
"AllowancePositionManager: withdrawOnBehalfOf: partial": "134160",
"AllowancePositionManager: approveWithdraw": "46858",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can easily add gas snapshots for temporary approvals, but how to properly test gas usage of borrowOnBehalfOf and withdrawOnBehalfOf, while temporary approvals are in place. Challenge is that we are in isolate mode, there are not cheatcodes for transient storage manipulation, and we don't want the temporary approval to count towards the gas consumed by consumer functions. Only solution I see is: create a bundle function in the wrapper, measure bundle fn gas, measure approve fn gas, and then subtract the two. Any suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

wrapper can call vm.snapshotGasLastCall, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm not sure it works. if vm.snapshotGasLastCall works correctly for this purpose, it must not consider the temporaryApprove call. If it does not consider it, borrow/withdraw will fail (since transient storage is cleared).

Copy link
Member

@DhairyaSethi DhairyaSethi Dec 15, 2025

Choose a reason for hiding this comment

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

for the wrapper tempApprove & consumeAllowance are 2 ext calls, if snapshotGasLastCall considers what's inside its context this would work but could be wrong here

Copy link
Member

Choose a reason for hiding this comment

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

if not then we could do run this test w isolate=false and manually vm.cold() targets but this is more error prone; thinking more but probably subtract from cumulative usage is a good enough estimate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah ended up doing the last suggestion as well 26743fa. Not sure if cooling is necessary

Copy link
Member

Choose a reason for hiding this comment

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

cooling is very necessary otherwise loading the target for eth_call (of consumeAllowance) is warm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if temporary approval is active, then it must have happened in the same tx, so not cooling might make sense here

"AllowancePositionManager: approveWithdrawWithSig": "63069",
"AllowancePositionManager: borrowOnBehalfOf": "309383",
"AllowancePositionManager: creditDelegation": "46836",
"AllowancePositionManager: creditDelegationWithSig": "63156",
"AllowancePositionManager: renounceCreditDelegation": "24989",
"AllowancePositionManager: renounceWithdrawAllowance": "24900",
"AllowancePositionManager: withdrawOnBehalfOf: full": "123872",
"AllowancePositionManager: withdrawOnBehalfOf: partial": "134639",
"PositionConfigPositionManager: renounceGlobalPermission": "24423",
"PositionConfigPositionManager: renounceUserDynamicConfigPermission": "24468",
"PositionConfigPositionManager: renounceUserRiskPremiumPermission": "24467",
Expand Down
130 changes: 112 additions & 18 deletions src/position-manager/AllowancePositionManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ pragma solidity 0.8.28;

import {SignatureChecker} from 'src/dependencies/openzeppelin/SignatureChecker.sol';
import {SafeERC20, IERC20} from 'src/dependencies/openzeppelin/SafeERC20.sol';
import {SlotDerivation} from 'src/dependencies/openzeppelin/SlotDerivation.sol';
import {TransientSlot} from 'src/dependencies/openzeppelin/TransientSlot.sol';
import {EIP712} from 'src/dependencies/solady/EIP712.sol';
import {MathUtils} from 'src/libraries/math/MathUtils.sol';
import {NoncesKeyed} from 'src/utils/NoncesKeyed.sol';
Expand All @@ -24,15 +26,27 @@ contract AllowancePositionManager is
using SafeERC20 for IERC20;
using EIP712Hash for *;
using MathUtils for uint256;
using SlotDerivation for bytes32;
using TransientSlot for *;

/// @notice Mapping of withdraw allowances.
mapping(address owner => mapping(address spender => mapping(uint256 reserveId => uint256 amount)))
private _withdrawAllowances;

/// @notice Slot for the temporary withdraw allowances.
/// @dev keccak256('temporary.withdrawAllowances')
bytes32 private constant _TEMPORARY_WITHDRAW_ALLOWANCES_SLOT =
0x1c6a61279a13a86a789311ddf30aee38e2f4a9f6c4aad1ff4a2e75a4018e68c3;
Copy link
Member

Choose a reason for hiding this comment

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

-1 & ~0ff for collision resistance :gigachad:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed, I'm not convinced what values these bring. Curious to understand other's thoughts as well

Copy link
Member

Choose a reason for hiding this comment

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

they bring collision resistance from potentially using this slot (which we clearly don't here) & are generally considered best practice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we do -1 & ~0xff everywhere, these measures do not bring any value: we will still have a collision if same string is used.

Copy link
Member

Choose a reason for hiding this comment

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

why would we do it everywhere? we just it exceptionally here

Copy link
Member

Choose a reason for hiding this comment

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

the idea of this was not a best practice to use strings + mappings, its just to bring additional randomness on slot derivation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why would we do it everywhere?

if we follow this pattern (-1 & ~0xff) everywhere, then collisions are still guaranteed when the same underlying string is used.

its just to bring additional randomness on slot derivation

additional randomness relative to what?

Copy link
Member

Choose a reason for hiding this comment

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

to potentially using a string for slot derivation, we dont know but its just good practice.
'if we follow this pattern (-1 & ~0xff) everywhere' ==> see we wouldn't use it everywhere, just here
oz also does

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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


/// @notice Mapping of credit delegations.
mapping(address owner => mapping(address spender => mapping(uint256 reserveId => uint256 amount)))
private _creditDelegations;

/// @notice Slot for the temporary credit delegations.
/// @dev keccak256('temporary.creditDelegations')
bytes32 private constant _TEMPORARY_CREDIT_DELEGATIONS_SLOT =
0xcd470af8670f5baa744a0341af8a2e3f5d7ca086178908432a5cfaf39cb9299d;

/// @dev Constructor.
/// @param spoke_ The address of the spoke contract.
constructor(address spoke_) PositionManagerBase(spoke_) {}
Expand All @@ -58,6 +72,11 @@ contract AllowancePositionManager is
_updateWithdrawAllowance(params.owner, params.spender, params.reserveId, params.amount, true);
}

/// @inheritdoc IAllowancePositionManager
function temporaryApproveWithdraw(address spender, uint256 reserveId, uint256 amount) external {
_temporaryWithdrawAllowancesSlot(msg.sender, spender, reserveId).tstore(amount);
}

/// @inheritdoc IAllowancePositionManager
function creditDelegation(address spender, uint256 reserveId, uint256 amount) external {
_updateCreditDelegation(msg.sender, spender, reserveId, amount, true);
Expand All @@ -79,6 +98,11 @@ contract AllowancePositionManager is
_updateCreditDelegation(params.owner, params.spender, params.reserveId, params.amount, true);
}

/// @inheritdoc IAllowancePositionManager
function temporaryCreditDelegation(address spender, uint256 reserveId, uint256 amount) external {
_temporaryCreditDelegationsSlot(msg.sender, spender, reserveId).tstore(amount);
}

/// @inheritdoc IAllowancePositionManager
function renounceWithdrawAllowance(address owner, uint256 reserveId) external {
_updateWithdrawAllowance(
Expand Down Expand Up @@ -108,15 +132,7 @@ contract AllowancePositionManager is
address onBehalfOf
) external returns (uint256, uint256) {
require(amount > 0, InvalidAmount());
uint256 currentAllowance = _withdrawAllowances[onBehalfOf][msg.sender][reserveId];
require(currentAllowance >= amount, InsufficientWithdrawAllowance(currentAllowance, amount));
_updateWithdrawAllowance(
onBehalfOf,
msg.sender,
reserveId,
currentAllowance.uncheckedSub(amount),
true
);
_spendWithdrawAllowance(onBehalfOf, msg.sender, reserveId, amount);

IERC20 asset = _getReserveUnderlying(reserveId);
(uint256 withdrawnShares, uint256 withdrawnAmount) = ISpokeBase(SPOKE).withdraw(
Expand All @@ -136,15 +152,7 @@ contract AllowancePositionManager is
address onBehalfOf
) external returns (uint256, uint256) {
require(amount > 0, InvalidAmount());
uint256 currentAllowance = _creditDelegations[onBehalfOf][msg.sender][reserveId];
require(currentAllowance >= amount, InsufficientCreditDelegation(currentAllowance, amount));
_updateCreditDelegation(
onBehalfOf,
msg.sender,
reserveId,
currentAllowance.uncheckedSub(amount),
true
);
_spendCreditDelegation(onBehalfOf, msg.sender, reserveId, amount);

IERC20 asset = _getReserveUnderlying(reserveId);
(uint256 borrowedShares, uint256 borrowedAmount) = ISpokeBase(SPOKE).borrow(
Expand Down Expand Up @@ -219,4 +227,90 @@ contract AllowancePositionManager is
function _domainNameAndVersion() internal pure override returns (string memory, string memory) {
return ('AllowancePositionManager', '1');
}

/// @dev Temporary allowance takes precedence over stored allowance, and does not cumulate.
function _spendWithdrawAllowance(
address onBehalfOf,
address spender,
uint256 reserveId,
uint256 amount
) internal {
uint256 temporaryAllowance = _temporaryWithdrawAllowancesSlot(onBehalfOf, spender, reserveId)
.tload();
if (temporaryAllowance > 0) {
require(
temporaryAllowance >= amount,
InsufficientWithdrawAllowance(temporaryAllowance, amount)
);
_temporaryWithdrawAllowancesSlot(onBehalfOf, spender, reserveId).tstore(
temporaryAllowance.uncheckedSub(amount)
);
} else {
uint256 allowance = _withdrawAllowances[onBehalfOf][spender][reserveId];
require(allowance >= amount, InsufficientWithdrawAllowance(allowance, amount));
_updateWithdrawAllowance({
owner: onBehalfOf,
spender: spender,
reserveId: reserveId,
newAllowance: allowance.uncheckedSub(amount),
emitEvent: true
});
}
}

/// @dev Temporary allowance takes precedence over stored allowance, and does not cumulate.
function _spendCreditDelegation(
address onBehalfOf,
address spender,
uint256 reserveId,
uint256 amount
) internal {
uint256 temporaryAllowance = _temporaryCreditDelegationsSlot(onBehalfOf, spender, reserveId)
.tload();
if (temporaryAllowance > 0) {
require(
temporaryAllowance >= amount,
InsufficientCreditDelegation(temporaryAllowance, amount)
);
_temporaryCreditDelegationsSlot(onBehalfOf, spender, reserveId).tstore(
temporaryAllowance.uncheckedSub(amount)
);
} else {
uint256 allowance = _creditDelegations[onBehalfOf][spender][reserveId];
require(allowance >= amount, InsufficientCreditDelegation(allowance, amount));
_updateCreditDelegation({
owner: onBehalfOf,
spender: spender,
reserveId: reserveId,
newCreditDelegation: allowance.uncheckedSub(amount),
emitEvent: true
});
}
}

function _temporaryWithdrawAllowancesSlot(
address owner,
address spender,
uint256 reserveId
) internal pure returns (TransientSlot.Uint256Slot) {
return
_TEMPORARY_WITHDRAW_ALLOWANCES_SLOT
.deriveMapping(owner)
.deriveMapping(spender)
.deriveMapping(reserveId)
.asUint256();
}

function _temporaryCreditDelegationsSlot(
address owner,
address spender,
uint256 reserveId
) internal pure returns (TransientSlot.Uint256Slot) {
return
_TEMPORARY_CREDIT_DELEGATIONS_SLOT
.deriveMapping(owner)
.deriveMapping(spender)
.deriveMapping(reserveId)
.asUint256();
}
}
14 changes: 14 additions & 0 deletions src/position-manager/interfaces/IAllowancePositionManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,13 @@ interface IAllowancePositionManager is IPositionManagerBase {
bytes calldata signature
) external;

/// @notice Temporarily approves a spender to withdraw assets from the specified reserve on the spoke.
/// @dev The allowance is discarded after the transaction.
/// @param spender The address of the spender to receive the allowance.
/// @param reserveId The identifier of the reserve.
/// @param amount The amount of allowance.
function temporaryApproveWithdraw(address spender, uint256 reserveId, uint256 amount) external;

/// @notice Approves a credit delegation allowance for a spender.
/// @param spender The address of the spender to receive the allowance.
/// @param reserveId The identifier of the reserve.
Expand All @@ -58,6 +65,13 @@ interface IAllowancePositionManager is IPositionManagerBase {
bytes calldata signature
) external;

/// @notice Temporarily approves a credit delegation allowance for a spender.
/// @dev The allowance is discarded after the transaction.
/// @param spender The address of the spender to receive the allowance.
/// @param reserveId The identifier of the reserve.
/// @param amount The amount of allowance.
function temporaryCreditDelegation(address spender, uint256 reserveId, uint256 amount) external;

/// @notice Renounces the withdraw allowance given by the owner.
/// @param owner The address of the owner.
/// @param reserveId The identifier of the reserve.
Expand Down
28 changes: 28 additions & 0 deletions tests/mocks/AllowancePositionManagerWrapper.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// SPDX-License-Identifier: UNLICENSED
// Copyright (c) 2025 Aave Labs
pragma solidity ^0.8.0;

import {TransientSlot} from 'src/dependencies/openzeppelin/TransientSlot.sol';
import {AllowancePositionManager} from 'src/position-manager/AllowancePositionManager.sol';

contract AllowancePositionManagerWrapper is AllowancePositionManager {
using TransientSlot for *;

constructor(address spoke_) AllowancePositionManager(spoke_) {}

function temporaryWithdrawAllowance(
address owner,
address spender,
uint256 reserveId
) external view returns (uint256) {
return _temporaryWithdrawAllowancesSlot(owner, spender, reserveId).tload();
}

function temporaryCreditDelegation(
address owner,
address spender,
uint256 reserveId
) external view returns (uint256) {
return _temporaryCreditDelegationsSlot(owner, spender, reserveId).tload();
}
}
Loading
Loading