diff to checkout before mig by akiraonstarknet · Pull Request #6 · trovesfi/starknet_vault_kit

akiraonstarknet · 2025-10-23T07:00:41Z

No description provided.

Copilot

Pull Request Overview

This PR implements vault migration functionality and refactors code organization to support migration flows. The changes prepare the system for migrating liquidity between vault versions by adding a bring_liquidity interface, reorganizing merkle tree utilities into a shared module, and removing flash loan functionality that is no longer needed.

Key changes:

Added vault migration support with bring_liquidity function and ERC721 receiver capability in VaultAllocator
Refactored merkle tree generation and integration-specific leaf functions into reusable modules
Removed Vesu flash loan functionality from Manager and related tests

Reviewed Changes

Copilot reviewed 59 out of 60 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
vault_allocator/vault_allocator.cairo	Renamed module to VaultAllocatorMigration, added SRC5 component and ERC721 receiver support, implemented bring_liquidity function
vault_allocator/test/utils.cairo	Removed unused imports and helper functions, reorganized imports to use new merkle_tree module structure
vault_allocator/test/units/manager.cairo	Updated imports to use new merkle_tree module, removed flash loan related tests
vault_allocator/test/scenarios/*.cairo	Refactored to use new merkle tree integration APIs with configuration structs
vault_allocator/test/integrations/*.cairo	Updated to use new merkle_tree module structure and removed flash loan tests
vault_allocator/src/merkle_tree/*.cairo	New module containing base merkle tree utilities and integration-specific leaf generation functions
vault_allocator/src/manager/*.cairo	Removed flash loan functionality and Vesu singleton dependency
vault_allocator/src/middlewares/avnu_middleware/*.cairo	Refactored rate limiting to use window-based approach, added vault_allocator caller validation
vault_allocator/src/mocks/*.cairo	Added MockVault, removed FlashLoanSingletonMock
vault_allocator/src/decoders_and_sanitizers/*.cairo	Replaced flash_loan function with bring_liquidity in base decoder
vault/vault.cairo	Renamed to VaultMigration, added deposit limits, ERC4626 fee/limit implementations, and bring_liquidity authorization
vault/redeem_request/*.cairo	Added ERC721 enumerable support and disabled unsafe transfer_from
vault/aum_provider/*.cairo	New module for AUM reporting functionality

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-23T07:02:40Z

packages/vault_allocator/src/vault_allocator/vault_allocator.cairo

+    fn bring_liquidity(ref self: ContractState, vault: ContractAddress, amount: u256) {
+        self.ownable.assert_only_owner();
+        let underlying_asset = ERC4626ABIDispatcher { contract_address: vault }.asset();
+        ERC20ABIDispatcher { contract_address: underlying_asset }.approve(vault, amount);
+        IVaultMigrationDispatcher { contract_address: vault }.bring_liquidity(amount);


The function signature is inconsistent with the IVaultMigration interface defined at line 7. The interface declares bring_liquidity(ref self: TContractState, amount: u256) with one parameter, but the implementation has two parameters (ref self: ContractState, vault: ContractAddress, amount: u256). This will cause compilation errors.

Suggested change

fn bring_liquidity(ref self: ContractState, vault: ContractAddress, amount: u256) {

self.ownable.assert_only_owner();

let underlying_asset = ERC4626ABIDispatcher { contract_address: vault }.asset();

ERC20ABIDispatcher { contract_address: underlying_asset }.approve(vault, amount);

IVaultMigrationDispatcher { contract_address: vault }.bring_liquidity(amount);

fn bring_liquidity(ref self: ContractState, amount: u256) {

self.ownable.assert_only_owner();

// TODO: Determine vault address internally, e.g., from contract storage or configuration.

// let vault = self.vault.read(); // Example if vault address is stored

// let underlying_asset = ERC4626ABIDispatcher { contract_address: vault }.asset();

// ERC20ABIDispatcher { contract_address: underlying_asset }.approve(vault, amount);

// IVaultMigrationDispatcher { contract_address: vault }.bring_liquidity(amount);

// For now, function body is left empty or should be implemented as needed.

Copilot · 2025-10-23T07:02:40Z

packages/vault_allocator/src/vault_allocator/vault_allocator.cairo

+            self.src5.register_interface(IERC721_RECEIVER_ID);
            let result = call_contract_syscall(to, selector, calldata).unwrap_syscall();
+            self.src5.deregister_interface(IERC721_RECEIVER_ID);


Registering and deregistering the ERC721_RECEIVER interface around every contract call creates a race condition if multiple calls execute concurrently. The interface registration should be done once during contract initialization, not dynamically per call.

Copilot · 2025-10-23T07:02:41Z

packages/vault_allocator/src/middlewares/avnu_middleware/avnu_middleware.cairo

+            self.slippage.write(slippage);
+            self.period.write(period);
+            self.allowed_calls_per_period.write(allowed);
+            self.current_window_id.write(0);


In _set_config, resetting current_window_id to 0 instead of calculating the current window based on the new period will cause incorrect rate limiting. The window ID should be set to get_block_timestamp() / period to properly initialize the current window.

Suggested change

self.current_window_id.write(0);

self.current_window_id.write(get_block_timestamp() / period);

Copilot · 2025-10-23T07:02:41Z

packages/vault/src/vault/vault.cairo

+                // ERC20ABIDispatcher { contract_address: self.erc4626.asset() }
+                //     .transfer(alloc, remaining_buffer);


Commented-out code should be removed rather than left in the codebase. If this transfer functionality is intentionally removed, the comments at lines 826-827 and the related logic change at line 829 should be documented with an explanation of why the buffer is no longer transferred to the allocator.

Suggested change

// ERC20ABIDispatcher { contract_address: self.erc4626.asset() }

// .transfer(alloc, remaining_buffer);

// The transfer of the buffer to the allocator has been intentionally removed.

// Reason: Buffer is retained in the contract to ensure liquidity for pending or future redemptions,

// and to avoid potential issues with premature asset deployment. If allocator deployment is required,

// it should be triggered by a separate, explicit action.

Copilot · 2025-10-23T07:02:41Z

packages/vault/src/vault/vault.cairo

                management_fee_assets,
                total_supply + 1,
-                (total_assets - management_fee_assets) + 1,
+                (total_assets - (management_fee_assets + performance_fee_assets)) + 1,


The management fee calculation denominator has changed to subtract both management and performance fees. However, at this point in the code (line 847), performance_fee_assets has not been calculated yet - it's calculated later at lines 862-869. This will result in incorrect fee calculations. The performance fee should only be subtracted in the performance fee calculation, not in the management fee calculation.

Suggested change

(total_assets - (management_fee_assets + performance_fee_assets)) + 1,

(total_assets - management_fee_assets) + 1,

Copilot · 2025-10-23T07:02:42Z

packages/vault/src/redeem_request/redeem_request.cairo

+    fn _check_on_erc721_received(
+        from: ContractAddress, to: ContractAddress, token_id: u256, data: Span<felt252>,
+    ) -> bool {
+        let src5_dispatcher = ISRC5Dispatcher { contract_address: to };
+
+        if src5_dispatcher.supports_interface(IERC721_RECEIVER_ID) {
+            IERC721ReceiverDispatcher { contract_address: to }
+                .on_erc721_received(
+                    get_caller_address(), from, token_id, data,
+                ) == IERC721_RECEIVER_ID
+        } else {
+            src5_dispatcher.supports_interface(ISRC6_ID)
+        }
+    }


The function _check_on_erc721_received is defined but never used in this file. This appears to be dead code that should either be removed or integrated into the ERC721 transfer logic.

Suggested change

fn _check_on_erc721_received(

from: ContractAddress, to: ContractAddress, token_id: u256, data: Span<felt252>,

) -> bool {

let src5_dispatcher = ISRC5Dispatcher { contract_address: to };

if src5_dispatcher.supports_interface(IERC721_RECEIVER_ID) {

IERC721ReceiverDispatcher { contract_address: to }

.on_erc721_received(

get_caller_address(), from, token_id, data,

) == IERC721_RECEIVER_ID

} else {

src5_dispatcher.supports_interface(ISRC6_ID)

}

}

diff to checkout before mig

eed6b61

Copilot AI review requested due to automatic review settings October 23, 2025 07:00

Copilot AI reviewed Oct 23, 2025

View reviewed changes

actual audited diff

e8e1f51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

diff to checkout before mig#6

diff to checkout before mig#6
akiraonstarknet wants to merge 2 commits intozenith-auditedfrom
vesu-audit-diff

akiraonstarknet commented Oct 23, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Oct 23, 2025

Uh oh!

Copilot AI Oct 23, 2025

Uh oh!

Copilot AI Oct 23, 2025

Uh oh!

Copilot AI Oct 23, 2025

Uh oh!

Copilot AI Oct 23, 2025

Uh oh!

Copilot AI Oct 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

-    fn bring_liquidity(ref self: ContractState, vault: ContractAddress, amount: u256) {
-        self.ownable.assert_only_owner();
-        let underlying_asset = ERC4626ABIDispatcher { contract_address: vault }.asset();
-        ERC20ABIDispatcher { contract_address: underlying_asset }.approve(vault, amount);
-        IVaultMigrationDispatcher { contract_address: vault }.bring_liquidity(amount);
+    fn bring_liquidity(ref self: ContractState, amount: u256) {
+        self.ownable.assert_only_owner();
+        // TODO: Determine vault address internally, e.g., from contract storage or configuration.
+        // let vault = self.vault.read(); // Example if vault address is stored
+        // let underlying_asset = ERC4626ABIDispatcher { contract_address: vault }.asset();
+        // ERC20ABIDispatcher { contract_address: underlying_asset }.approve(vault, amount);
+        // IVaultMigrationDispatcher { contract_address: vault }.bring_liquidity(amount);
+        // For now, function body is left empty or should be implemented as needed.

	self.current_window_id.write(0);
	self.current_window_id.write(get_block_timestamp() / period);

		// ERC20ABIDispatcher { contract_address: self.erc4626.asset() }
		// .transfer(alloc, remaining_buffer);

-                // ERC20ABIDispatcher { contract_address: self.erc4626.asset() }
-                //     .transfer(alloc, remaining_buffer);
+                // The transfer of the buffer to the allocator has been intentionally removed.
+                // Reason: Buffer is retained in the contract to ensure liquidity for pending or future redemptions,
+                // and to avoid potential issues with premature asset deployment. If allocator deployment is required,
+                // it should be triggered by a separate, explicit action.

	(total_assets - (management_fee_assets + performance_fee_assets)) + 1,
	(total_assets - management_fee_assets) + 1,

Conversation

akiraonstarknet commented Oct 23, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant