Skip to content

Clarify ERC-4626 fees flow #1602

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 26, 2025
Merged

Clarify ERC-4626 fees flow #1602

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7e7a08d
Add a note to ERC4626HooksTrait
immrsd Nov 21, 2025
52bd984
Add clarifying comments to ERC4626 mocks
immrsd Nov 21, 2025
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
97 changes: 95 additions & 2 deletions packages/test_common/src/mocks/erc4626.cairo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ pub mod ERC4626AssetsFeesMock {
use openzeppelin_token::erc20::{DefaultConfig as ERC20DefaultConfig, ERC20HooksEmptyImpl};
use starknet::ContractAddress;
use starknet::storage::{StoragePointerReadAccess, StoragePointerWriteAccess};
use super::{fee_on_raw, fee_on_total};
use super::{ONE_SHARE, fee_on_raw, fee_on_total};

// ERC4626
#[abi(embed_v0)]
Expand Down Expand Up @@ -307,6 +307,21 @@ pub mod ERC4626AssetsFeesMock {

/// Hooks
impl ERC4626HooksImpl of ERC4626Component::ERC4626HooksTrait<ContractState> {
fn before_deposit(
ref self: ERC4626Component::ComponentState<ContractState>,
caller: ContractAddress,
receiver: ContractAddress,
assets: u256,
shares: u256,
fee: Option<Fee>,
) {
// Ensure conversion rate is correct before deposit has started
assert!(
ONE_SHARE == self.convert_to_assets(ONE_SHARE),
"ERC4626AssetsFeesMock: incorrect conversion rate in before_deposit",
);
}

fn after_deposit(
ref self: ERC4626Component::ComponentState<ContractState>,
caller: ContractAddress,
Expand All @@ -315,6 +330,8 @@ pub mod ERC4626AssetsFeesMock {
shares: u256,
fee: Option<Fee>,
) {
// The conversion rate is temporarily incorrect as the asset fee remains in the vault
// awaiting transfer to the fee recipient.
let fee = fee
.unwrap_or_else(
|| panic!("ERC4626AssetsFeesMock expects fee in after_deposit to not be None"),
Expand Down Expand Up @@ -342,6 +359,11 @@ pub mod ERC4626AssetsFeesMock {
let asset_dispatcher = IERC20Dispatcher { contract_address: asset_address };
assert(asset_dispatcher.transfer(fee_recipient, fee), 'Fee transfer failed');
}
// Ensure conversion rate is correct after fee has been transferred
assert!(
ONE_SHARE == self.convert_to_assets(ONE_SHARE),
"ERC4626AssetsFeesMock: incorrect conversion rate in after_deposit (end)",
);
}

fn before_withdraw(
Expand All @@ -353,6 +375,11 @@ pub mod ERC4626AssetsFeesMock {
shares: u256,
fee: Option<Fee>,
) {
// Ensure conversion rate is correct before withdraw has started
assert!(
ONE_SHARE == self.convert_to_assets(ONE_SHARE),
"ERC4626AssetsFeesMock: incorrect conversion rate in before_withdraw (start)",
);
let fee = fee
.unwrap_or_else(
|| panic!(
Expand Down Expand Up @@ -382,6 +409,23 @@ pub mod ERC4626AssetsFeesMock {
let asset_dispatcher = IERC20Dispatcher { contract_address: asset_address };
assert(asset_dispatcher.transfer(fee_recipient, fee), 'Fee transfer failed');
}
// Rate is incorrect here: fee transferred out but corresponding shares yet to be burned
}

fn after_withdraw(
ref self: ERC4626Component::ComponentState<ContractState>,
caller: ContractAddress,
receiver: ContractAddress,
owner: ContractAddress,
assets: u256,
shares: u256,
fee: Option<Fee>,
) {
// Ensure conversion rate is correct after withdraw has completed
assert!(
ONE_SHARE == self.convert_to_assets(ONE_SHARE),
"ERC4626AssetsFeesMock: incorrect conversion rate in after_withdraw",
);
}
}

Expand Down Expand Up @@ -433,7 +477,7 @@ pub mod ERC4626SharesFeesMock {
use openzeppelin_token::erc20::{DefaultConfig as ERC20DefaultConfig, ERC20HooksEmptyImpl};
use starknet::ContractAddress;
use starknet::storage::{StoragePointerReadAccess, StoragePointerWriteAccess};
use super::{fee_on_raw, fee_on_total};
use super::{ONE_SHARE, fee_on_raw, fee_on_total};

// ERC4626
#[abi(embed_v0)]
Expand Down Expand Up @@ -481,6 +525,21 @@ pub mod ERC4626SharesFeesMock {

/// Hooks
impl ERC4626HooksImpl of ERC4626Component::ERC4626HooksTrait<ContractState> {
fn before_deposit(
ref self: ERC4626Component::ComponentState<ContractState>,
caller: ContractAddress,
receiver: ContractAddress,
assets: u256,
shares: u256,
fee: Option<Fee>,
) {
// Ensure conversion rate is correct before deposit has started
assert!(
ONE_SHARE == self.convert_to_assets(ONE_SHARE),
"ERC4626FeesSharesMock: incorrect conversion rate in before_deposit",
);
}

fn after_deposit(
ref self: ERC4626Component::ComponentState<ContractState>,
caller: ContractAddress,
Expand All @@ -489,6 +548,8 @@ pub mod ERC4626SharesFeesMock {
shares: u256,
fee: Option<Fee>,
) {
// The conversion rate is temporarily incorrect as the assets were transferred in
// but the corresponding share fee has not been minted yet.
let fee = fee
.unwrap_or_else(
|| panic!("ERC4626FeesSharesMock expects fee in after_deposit to not be None"),
Expand All @@ -510,6 +571,11 @@ pub mod ERC4626SharesFeesMock {
let fee_recipient = contract_state.entry_fee_recipient.read();
contract_state.erc20.mint(fee_recipient, fee);
}
// Ensure conversion rate is correct after fee has been minted
assert!(
ONE_SHARE == self.convert_to_assets(ONE_SHARE),
"ERC4626FeesSharesMock: incorrect conversion rate in after_deposit (end)",
);
}

fn before_withdraw(
Expand All @@ -521,6 +587,11 @@ pub mod ERC4626SharesFeesMock {
shares: u256,
fee: Option<Fee>,
) {
// Ensure conversion rate is correct
assert!(
ONE_SHARE == self.convert_to_assets(ONE_SHARE),
"ERC4626FeesSharesMock: incorrect conversion rate in before_withdraw (start)",
);
let fee = fee
.unwrap_or_else(
|| panic!(
Expand All @@ -547,6 +618,27 @@ pub mod ERC4626SharesFeesMock {
}
contract_state.erc20._transfer(owner, fee_recipient, fee);
}
// Ensure conversion rate is correct
assert!(
ONE_SHARE == self.convert_to_assets(ONE_SHARE),
"ERC4626FeesSharesMock: incorrect conversion rate in before_withdraw (end)",
);
}

fn after_withdraw(
ref self: ERC4626Component::ComponentState<ContractState>,
caller: ContractAddress,
receiver: ContractAddress,
owner: ContractAddress,
assets: u256,
shares: u256,
fee: Option<Fee>,
) {
// Ensure conversion rate is correct after withdraw has completed
assert!(
ONE_SHARE == self.convert_to_assets(ONE_SHARE),
"ERC4626FeesSharesMock: incorrect conversion rate in after_withdraw",
);
}
}

Expand Down Expand Up @@ -735,6 +827,7 @@ use openzeppelin_utils::math;
use openzeppelin_utils::math::Rounding;

const _BASIS_POINT_SCALE: u256 = 10_000;
const ONE_SHARE: u256 = 1_000_000_000_000_000_000;

/// Calculates the fees that should be added to an amount `assets` that does not already
/// include fees.
Expand Down
9 changes: 9 additions & 0 deletions packages/token/src/erc20/extensions/erc4626/erc4626.cairo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,15 @@ pub mod ERC4626Component {
/// Fees are calculated using `FeeConfigTrait` methods and automatically adjust the final
/// asset and share amounts. Fee transfers are handled in `ERC4626HooksTrait` methods.
///
/// NOTE: When a vault implements fees on deposits or withdrawals (either in shares or
/// assets), fee transfers must be handled in these hooks by library clients. This creates
/// a non-atomic operation flow consisting of multiple state-changing steps: transferring
/// assets, minting or burning shares, and transferring (or minting) fees. Between these steps,
/// the vault's state is temporarily inconsistent: the asset-to-share conversion rate does not
/// accurately reflect the vault's final state until all steps have completed. Therefore, it is
/// critical to avoid making any external calls (including to the vault contract itself) or
/// querying conversion rates during hook execution.
///
/// CAUTION: Special care must be taken when calling external contracts in these hooks. In
/// that case, consider implementing reentrancy protections. For example, in the
/// `withdraw` flow, the `withdraw_limit` is checked *before* the `before_withdraw` hook
Expand Down