Skip to content

Commit 66bceaf

Browse files
immrsdimmrsd
authored
Clarify ERC-4626 fees flow (#1602)
* Add a note to ERC4626HooksTrait * Add clarifying comments to ERC4626 mocks
1 parent e5235c1 commit 66bceaf

File tree

2 files changed

+104
-2
lines changed

2 files changed

+104
-2
lines changed

‎packages/test_common/src/mocks/erc4626.cairo‎

Lines changed: 95 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -259,7 +259,7 @@ pub mod ERC4626AssetsFeesMock {
259259
use openzeppelin_token::erc20::{DefaultConfig as ERC20DefaultConfig, ERC20HooksEmptyImpl};
260260
use starknet::ContractAddress;
261261
use starknet::storage::{StoragePointerReadAccess, StoragePointerWriteAccess};
262-
use super::{fee_on_raw, fee_on_total};
262+
use super::{ONE_SHARE, fee_on_raw, fee_on_total};
263263

264264
// ERC4626
265265
#[abi(embed_v0)]
@@ -307,6 +307,21 @@ pub mod ERC4626AssetsFeesMock {
307307

308308
/// Hooks
309309
impl ERC4626HooksImpl of ERC4626Component::ERC4626HooksTrait<ContractState> {
310+
fn before_deposit(
311+
ref self: ERC4626Component::ComponentState<ContractState>,
312+
caller: ContractAddress,
313+
receiver: ContractAddress,
314+
assets: u256,
315+
shares: u256,
316+
fee: Option<Fee>,
317+
) {
318+
// Ensure conversion rate is correct before deposit has started
319+
assert!(
320+
ONE_SHARE == self.convert_to_assets(ONE_SHARE),
321+
"ERC4626AssetsFeesMock: incorrect conversion rate in before_deposit",
322+
);
323+
}
324+
310325
fn after_deposit(
311326
ref self: ERC4626Component::ComponentState<ContractState>,
312327
caller: ContractAddress,
@@ -315,6 +330,8 @@ pub mod ERC4626AssetsFeesMock {
315330
shares: u256,
316331
fee: Option<Fee>,
317332
) {
333+
// The conversion rate is temporarily incorrect as the asset fee remains in the vault
334+
// awaiting transfer to the fee recipient.
318335
let fee = fee
319336
.unwrap_or_else(
320337
|| panic!("ERC4626AssetsFeesMock expects fee in after_deposit to not be None"),
@@ -342,6 +359,11 @@ pub mod ERC4626AssetsFeesMock {
342359
let asset_dispatcher = IERC20Dispatcher { contract_address: asset_address };
343360
assert(asset_dispatcher.transfer(fee_recipient, fee), 'Fee transfer failed');
344361
}
362+
// Ensure conversion rate is correct after fee has been transferred
363+
assert!(
364+
ONE_SHARE == self.convert_to_assets(ONE_SHARE),
365+
"ERC4626AssetsFeesMock: incorrect conversion rate in after_deposit (end)",
366+
);
345367
}
346368

347369
fn before_withdraw(
@@ -353,6 +375,11 @@ pub mod ERC4626AssetsFeesMock {
353375
shares: u256,
354376
fee: Option<Fee>,
355377
) {
378+
// Ensure conversion rate is correct before withdraw has started
379+
assert!(
380+
ONE_SHARE == self.convert_to_assets(ONE_SHARE),
381+
"ERC4626AssetsFeesMock: incorrect conversion rate in before_withdraw (start)",
382+
);
356383
let fee = fee
357384
.unwrap_or_else(
358385
|| panic!(
@@ -382,6 +409,23 @@ pub mod ERC4626AssetsFeesMock {
382409
let asset_dispatcher = IERC20Dispatcher { contract_address: asset_address };
383410
assert(asset_dispatcher.transfer(fee_recipient, fee), 'Fee transfer failed');
384411
}
412+
// Rate is incorrect here: fee transferred out but corresponding shares yet to be burned
413+
}
414+
415+
fn after_withdraw(
416+
ref self: ERC4626Component::ComponentState<ContractState>,
417+
caller: ContractAddress,
418+
receiver: ContractAddress,
419+
owner: ContractAddress,
420+
assets: u256,
421+
shares: u256,
422+
fee: Option<Fee>,
423+
) {
424+
// Ensure conversion rate is correct after withdraw has completed
425+
assert!(
426+
ONE_SHARE == self.convert_to_assets(ONE_SHARE),
427+
"ERC4626AssetsFeesMock: incorrect conversion rate in after_withdraw",
428+
);
385429
}
386430
}
387431

@@ -433,7 +477,7 @@ pub mod ERC4626SharesFeesMock {
433477
use openzeppelin_token::erc20::{DefaultConfig as ERC20DefaultConfig, ERC20HooksEmptyImpl};
434478
use starknet::ContractAddress;
435479
use starknet::storage::{StoragePointerReadAccess, StoragePointerWriteAccess};
436-
use super::{fee_on_raw, fee_on_total};
480+
use super::{ONE_SHARE, fee_on_raw, fee_on_total};
437481

438482
// ERC4626
439483
#[abi(embed_v0)]
@@ -481,6 +525,21 @@ pub mod ERC4626SharesFeesMock {
481525

482526
/// Hooks
483527
impl ERC4626HooksImpl of ERC4626Component::ERC4626HooksTrait<ContractState> {
528+
fn before_deposit(
529+
ref self: ERC4626Component::ComponentState<ContractState>,
530+
caller: ContractAddress,
531+
receiver: ContractAddress,
532+
assets: u256,
533+
shares: u256,
534+
fee: Option<Fee>,
535+
) {
536+
// Ensure conversion rate is correct before deposit has started
537+
assert!(
538+
ONE_SHARE == self.convert_to_assets(ONE_SHARE),
539+
"ERC4626FeesSharesMock: incorrect conversion rate in before_deposit",
540+
);
541+
}
542+
484543
fn after_deposit(
485544
ref self: ERC4626Component::ComponentState<ContractState>,
486545
caller: ContractAddress,
@@ -489,6 +548,8 @@ pub mod ERC4626SharesFeesMock {
489548
shares: u256,
490549
fee: Option<Fee>,
491550
) {
551+
// The conversion rate is temporarily incorrect as the assets were transferred in
552+
// but the corresponding share fee has not been minted yet.
492553
let fee = fee
493554
.unwrap_or_else(
494555
|| panic!("ERC4626FeesSharesMock expects fee in after_deposit to not be None"),
@@ -510,6 +571,11 @@ pub mod ERC4626SharesFeesMock {
510571
let fee_recipient = contract_state.entry_fee_recipient.read();
511572
contract_state.erc20.mint(fee_recipient, fee);
512573
}
574+
// Ensure conversion rate is correct after fee has been minted
575+
assert!(
576+
ONE_SHARE == self.convert_to_assets(ONE_SHARE),
577+
"ERC4626FeesSharesMock: incorrect conversion rate in after_deposit (end)",
578+
);
513579
}
514580

515581
fn before_withdraw(
@@ -521,6 +587,11 @@ pub mod ERC4626SharesFeesMock {
521587
shares: u256,
522588
fee: Option<Fee>,
523589
) {
590+
// Ensure conversion rate is correct
591+
assert!(
592+
ONE_SHARE == self.convert_to_assets(ONE_SHARE),
593+
"ERC4626FeesSharesMock: incorrect conversion rate in before_withdraw (start)",
594+
);
524595
let fee = fee
525596
.unwrap_or_else(
526597
|| panic!(
@@ -547,6 +618,27 @@ pub mod ERC4626SharesFeesMock {
547618
}
548619
contract_state.erc20._transfer(owner, fee_recipient, fee);
549620
}
621+
// Ensure conversion rate is correct
622+
assert!(
623+
ONE_SHARE == self.convert_to_assets(ONE_SHARE),
624+
"ERC4626FeesSharesMock: incorrect conversion rate in before_withdraw (end)",
625+
);
626+
}
627+
628+
fn after_withdraw(
629+
ref self: ERC4626Component::ComponentState<ContractState>,
630+
caller: ContractAddress,
631+
receiver: ContractAddress,
632+
owner: ContractAddress,
633+
assets: u256,
634+
shares: u256,
635+
fee: Option<Fee>,
636+
) {
637+
// Ensure conversion rate is correct after withdraw has completed
638+
assert!(
639+
ONE_SHARE == self.convert_to_assets(ONE_SHARE),
640+
"ERC4626FeesSharesMock: incorrect conversion rate in after_withdraw",
641+
);
550642
}
551643
}
552644

@@ -735,6 +827,7 @@ use openzeppelin_utils::math;
735827
use openzeppelin_utils::math::Rounding;
736828

737829
const _BASIS_POINT_SCALE: u256 = 10_000;
830+
const ONE_SHARE: u256 = 1_000_000_000_000_000_000;
738831

739832
/// Calculates the fees that should be added to an amount `assets` that does not already
740833
/// include fees.

‎packages/token/src/erc20/extensions/erc4626/erc4626.cairo‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -241,6 +241,15 @@ pub mod ERC4626Component {
241241
/// Fees are calculated using `FeeConfigTrait` methods and automatically adjust the final
242242
/// asset and share amounts. Fee transfers are handled in `ERC4626HooksTrait` methods.
243243
///
244+
/// NOTE: When a vault implements fees on deposits or withdrawals (either in shares or
245+
/// assets), fee transfers must be handled in these hooks by library clients. This creates
246+
/// a non-atomic operation flow consisting of multiple state-changing steps: transferring
247+
/// assets, minting or burning shares, and transferring (or minting) fees. Between these steps,
248+
/// the vault's state is temporarily inconsistent: the asset-to-share conversion rate does not
249+
/// accurately reflect the vault's final state until all steps have completed. Therefore, it is
250+
/// critical to avoid making any external calls (including to the vault contract itself) or
251+
/// querying conversion rates during hook execution.
252+
///
244253
/// CAUTION: Special care must be taken when calling external contracts in these hooks. In
245254
/// that case, consider implementing reentrancy protections. For example, in the
246255
/// `withdraw` flow, the `withdraw_limit` is checked *before* the `before_withdraw` hook

0 commit comments

Comments
 (0)