AfiUSD

Description

Allowing users to cancel their withdraw request after the funds arrive results in assets being temporarily stuck in the vault:

1. User requests withdraw
2. Manager withdraws from external protocols and sends funds to AFI contract
3. User calls cancelWithdrawal or transfers the tokens to another address which means redeem and redeemFor won't work anymore

There is no way for the Manager to recover those funds and make them "productive" again. The recovery path is that when the next requestWithdraw is made, the manager needs to look at the balance of asset tokens already sitting in the afiToken contract and only withdraw more tokens from the external protocols if it exceeds what is already there.

This whole process becomes even more difficult since there is no way to introspect the total pending assets that have active withdraw requests, so the manager software will have to (off chain) watch for WithdrawRequest and events WithdrawCancelled events, track the shares emitted in the former and the users for the latter (looking up the previously emitted shares), and then compare those values to a shares to assets conversion to get the assets needed to be withdraw to clear current withdraw requests.

Impact Explanation

This will create a UX that will look like funds are locked in the contract and the user only has the option to re-initiate a withdraw request, wait the cooldown period again then retrieve funds. However, this also allows the user to earn yield on "safe" funds while blocking the operator's ability to redeemFor. If the manager's off-chain operator/admin are not able to catch this scenario, a malicious user could cycle requests and effectively lock all funds back in the AFI preventing yield earning and requiring all users to withdraw and redeposit.

Likelihood Explanation

Through the normal course of events, this is likely to happen, but given the profit angle of keeping funds safe while earning yield, this becomes high.

Proof of Concept (if required)

// added to test-afiToken.t.sol    function test_user_cancel_withdrawal_stuck_funds() public {        uint256 managerBalanceBefore = asset.balanceOf(address(manager));        vm.startPrank(user1);        vault.deposit(DEPOSIT_AMOUNT, user1);        vm.stopPrank();
        assertEq(asset.balanceOf(address(vault)), 0, "Vault should have transferred funds right away to the manager");        assertEq(asset.balanceOf(address(manager)), managerBalanceBefore + DEPOSIT_AMOUNT, "Manager should have the deposited amount");
        vm.startPrank(user1);        // User requests full withdrawal        vault.requestWithdrawal(vault.balanceOf(user1));        vm.stopPrank();
        // Manager sees withdraw request and sends those funds back to vault        vm.startPrank(admin);        manager.withdrawAssets(address(vault), DEPOSIT_AMOUNT, address(asset));        vm.stopPrank();
        assertEq(asset.balanceOf(address(vault)), DEPOSIT_AMOUNT, "Vault should have the amount waiting to be withdrawn");        assertEq(asset.balanceOf(address(manager)), managerBalanceBefore, "Manager should only have starting amount");
        vm.startPrank(user1);        vault.cancelWithdrawal();        vm.stopPrank();
        assertEq(asset.balanceOf(address(vault)), DEPOSIT_AMOUNT, "Vault still has the amount waiting to be withdrawn");        assertEq(asset.balanceOf(address(manager)), managerBalanceBefore, "Manager should only have starting amount");    }

Recommendation

This likely needs more functionality added to the contract, some initial options:

• a function the manager calls when moving assets to the contract for a withdraw request that updates the withdraw request struct so it is known that those assets have arrived and can/should be sent back if the request is cancelled
• a total shares amount requested
• more info in the cancellation event so the off chain listeners have less work to do

Removing the ability of the user to revoke a request would force honest users to be re-deposit if they have changed their mind and allow the Manager to redeem for dishonest users. However, an important consideration in the fix here is how to prevent an indirect revoke of a request by the user transferring the tokens to a new address. Two options to address this for malicious users is to either burn the shares on request rather than on completion or to update the internal workings of the ERC-20 transfer function to prevent a user from transferring shares they have already committed to withdrawing.

AFI-X: Fixed in commit 089fe2f. Fix scenario where user funds could become stuck in AFI contract while continuing to accrue interest, enabling timely withdrawal and resolving fund lockup risk. Added a recovery function to recover stuck tokens/eth.

Cantina Managed: This allows the ADMIN of the afi contract to pull any tokens from the contract. It does not prevent them from pulling funds that should be available to a user who has requested redemption which adds to the trust placed in the ADMIN address. Further, it allows recovering ETH from the contract, which should (in almost all cases) not be able to get to the contract since it does not have a payable function.

It also removes the ability of a user to cancel the redeem request which prevents the funds from being stuck in the first place.

Description

Through the use of the execute function, the OPERATOR and the ADMIN can call functions in the afiToken that should only be called because of the Yield contract's actions. the manageAssetAndShares function in the Manager contract attempts to prevent any address other than the yield contract from calling the aifUSD contract's functions: updateTotalAssets, mintVaultTokens, and burnVaultTokens. The Yield contract ensures that the manager can only mint or burn tokens to/from the treasury.

However, because the afiToken only checks that these functions are called by the Manager, the OPERATOR and the ADMIN are able to call them directly from within the execute function. This allows the ADMIN and OPERATOR to mint and burn any user's tokens.

While these are theoretically "permissioned" addresses, the Manager contract's docs and requires suggest this is a concerning permission escalation. The impact of this is giving the OPERATOR and ADMIN full control to mint, burn and change totalAssets in the afiUSD contract, allowing them to drain all value and wipe out all AFI tokens.

Recommendation

These afiUSD functions or the execute functions might need additional protections to prevent this path of access. Execute could ensure that the target is not the afiToken contract (even it is whitelisted) or that these function calls are in the data. The afiToken could check the to/from against the treasury value for the mint/burn (though this can't protect against total asset changes). Perhaps, this functionality can simply cut the Manager out all together and the Yield token gets permission to do this directly in the token contract.

OPERATOR and ADMIN already have great power of being able to pull funds from the Manager, so regardless of this issue, great care needs to be taken with how those addresses are controlled.

AFI-X: Fixed in commit 23ba61b.Fix permission escalation vulnerabilities for OPERATOR and ADMIN roles in Manager to prevent unauthorized privilege gain.

Cantina Managed: The changes address this by separating the permissions in the manager between the operator and the admin and by adding a check to the afiToken for the treasury address. This does not completely eliminate the risk of arbitrary minting/burning but makes it much less likely since an attacker would have to control multiple permissioned addresses. Care should be taken that these addresses (OPERATOR, ADMIN, and afiToken.ADMIN_ROLE) are separated and properly protected.

Description

The Manager.manageAssetAndShares() function should only be called by the Yield contract. Inside the Yield contract, there is only one place that it is called: distributeYield(). Inside distributeYield, the values for _order.updateAsset and _order.isMint are hardcoded to both be true. This means that the conditional checking if(_order.updateAsset) is unnecessary; it will always be true. Further, the conditional to see if (_order.isMint) will also always be true, so IafiUSD().burnVaultToken can never be called by the Yield contract and should not be called by the manager contract directly as it would destroy user value. This is a critical path (allowing the burning of arbitrary users' tokens) that is not needed at all and should be removed.

Recommendation

Remove burnVaultTokens from the afiUSD token contract to make that more secure. Change Manager.manageAssetAndShares to something like (note: not tested/optimized code):

function manageAssetAndShares(address _to, ManageAssetAndShares memory _order) external {    require(msg.sender == yield, "only yield contract can call");
    IafiUSD(_order.VaultToken).updateTotalAssets(_order.assetAmount, _order.isMint);    IafiUSD(_order.VaultToken).mintVaultToken(_to, _order.shares);    emit AssetAndShareManaged(        msg.sender, _order.VaultToken, _order.shares, _order.assetAmount, _order.updateAsset, _order.isMint    );}

Consider removing _order.isMint and _order.updateAsset as they are not used/changing.

AFI-X: Fixed in commit 48553ab.

Cantina Managed: The PR adds a check to burnVaultToken and mintVaultToken to ensure that it is only minting or burning Treasury tokens. In combination with restricting the Manager admin from calling execute and preventing execute from calling manageAssetAndShares, this severely reduces the possibility that burnVaultToken or mintVaultToken are called with arbitrary, dangerous values. Yield.distributeYield has also been updated to include a path where a loss could result in burning shares from the treasury. As long as REBALANCER_ROLE in the Yield contract and the ADMIN in the Manager contract are not controlled by the same entity, there should not be a way for arbitrary user tokens to be burned. Manager.updateAssetAndShares still contains an unnecessary conditional for updateAsset since both paths in the Yield contract hard code that to true. This is not directly an issue, but may indicate further changes need to be made to completely tighten the logic flows between these contracts.

This change does introduce one new mechanism that was not evaluated in the initial audit - burning treasury shares when yield rewards are negative. While, burning the Treasury's tokens as a sort of first loss capital is a nice mechanism, this code will result in a revert if the treasury does not have tokens to burn, meaning losses will not be accounted for in the loss. There is not a treasury contract present in this audit so it is not possible to evaluate its behavior; however, it is conceivable that it could front run a distributeYield call that would cause its shares to be burned by moving those shares preventing a loss. It is unknown how the REBALANCER_ROLE contract would handle this or what the implications of a revert here would mean for future yield distribution. It is recommended that these dynamics be further explained and reviewed for potential issues.

Description

The exchangeRate function in the afiToken contract returns the rate including fees, which differs from the exchangeRateScaled value. This occurs because exchangeRate calls previewMint instead of super.previewMint. Since previewMint includes fees when calculating shares, it produces a higher value than expected.

Integrators relying on this exchange rate will receive inaccurate values, potentially breaking their integrations.

Recommendation

Use super.previewMint instead of previewMint to get the correct exchange rate without fees.

AFI-X: Fixed in commit dd9d6f0. Fixed by replacing previewMint by super.previewMint

Cantina Managed: Fix verified.

Description

Modifying the vestingPeriod during an ongoing distribution alters the totalAssets() calculation, directly affecting the share price.

function totalAssets() public view virtual override returns (uint256) {	//@audit getUnvestedAmount use the vestingPeriod for calculation  return virtualAssets - getUnvestedAmount();}

Malicious users could exploit this vulnerability by withdrawing all their shares and re-depositing after the vestingPeriod update to obtain more shares at a discounted price.

This vulnerability becomes particularly severe if combined with issue #1.

Consider this example of totalAssets calculation:

// Initial scenariolastDistributionTimestamp = 0currentTimestamp = 3 daysvestingPeriod = 6 daysvestingAmount = 100 tokens
// Shares calculationgetUnvestedAmount = 100 * (6 - 3) / 6 = 50 tokenstotalAssets = 1000 - 50totalSupply = 1000Shares ratio: 0.95
// After updating vestingPeriod to 12 dayslastDistributionTimestamp = 0currentTimestamp = 3 daysvestingPeriod = 12 daysvestingAmount = 100 tokens
// New shares calculationgetUnvestedAmount = 100 * (12 - 3) / 12 = 75 tokenstotalAssets = 1000 - 75totalSupply = 1000Shares ratio: 0.925

The immediate reduction in totalAssets after updating the vesting period means users can receive more shares for the same deposit amount, creating an arbitrage opportunity.

Recommendation

Prevent this vulnerability by accruing the vesting amount before updating the vestingPeriod:

function setVestingPeriod(uint256 newPeriod) external onlyRole(ADMIN_ROLE) {  if (newPeriod > 30 days) revert InvalidVestingPeriod();+ _updateVestingAmount(0);   vestingPeriod = newPeriod;  emit VestingPeriodUpdated(newPeriod);}

AFI-X: Fixed in commit c3307df. Prevent shares manipulation when updating vestingPeriod.

Cantina Managed: Fix verified.

Description

A user who deposits then requests a withdraw can prevent their funds from being exposed to 3rd party protocol risk (whatever the manager's operator is using to generate yield) while still earning a share of the yield.

This hurts other users in some ways, the vault is holding unproductive assets so it is earning less yield than it could be if all assets were deployed. Second, users whose assets are deployed will receive a smaller portion of the yield than if they had deployed the assets directly since they have to share that yield with the free-loading user. Additionally, user could also leverage third-party markets, such as a Uniswap pool with afiToken/USDC, flash-loaning afiToken and immediately creating a withdrawal request, amplifying the damage to other users.

A user who has initiated withdrawal could also be exposed to loss if the case if the Yield is not profitable. Perhaps this is good as it prevents user's from requesting a withdraw right before a loss occurs, but it could also be gamed by the Manager's operator to slow down withdraws requested by users until the losses are registered and affect those users. Further because positive yield is vested and losses are immediate a normal user is more likely to share in losses after requesting a withdrawal than they are to share in the gains.

Proof of Concept

// added to test-afiToken.t.sol    function test_user_deposit_withdraw_with_yield() public {        uint256 user1SharesBefore = vault.balanceOf(user1);        console2.log("user1SharesBefore", user1SharesBefore);        uint256 expectedShares = vault.previewDeposit(DEPOSIT_AMOUNT);        console2.log("expectedShares", expectedShares);        vm.startPrank(user1);        vault.deposit(DEPOSIT_AMOUNT, user1);        vm.stopPrank();        uint256 user1SharesAfterDeposit = vault.balanceOf(user1);        console2.log("user1SharesAfterDeposit", user1SharesAfterDeposit);        assertEq(expectedShares, user1SharesAfterDeposit, "User1 shares should be equal to expected shares");
        vm.startPrank(user2);        vault.deposit(DEPOSIT_AMOUNT, user2);        vm.stopPrank();
        uint256 user1WithdrawalRequestAssets = vault.previewRedeem(expectedShares);        vm.startPrank(user1);        vault.requestWithdrawal(expectedShares);        vm.stopPrank();
        // Manager sees withdraw request and sends those funds back to vault        vm.startPrank(admin);        manager.withdrawAssets(address(vault), user1WithdrawalRequestAssets, address(asset));        vm.stopPrank();
        // Assume the Manager earns yield with the remaining funds         vm.warp(block.timestamp + vault.vestingPeriod() + 1);        // Manager distributes yield        _distributeYieldWithAssets(YIELD_AMOUNT, 0, 1, true);
        uint256 user1BalanceIfRedeemed = vault.previewRedeem(vault.balanceOf(user1));        uint256 user2BalanceIfRedeemed = vault.previewRedeem(vault.balanceOf(user2));        console2.log("user1BalanceIfRedeemed", user1BalanceIfRedeemed);        console2.log("user2BalanceIfRedeemed", user2BalanceIfRedeemed);        assertEq(user1BalanceIfRedeemed, user2BalanceIfRedeemed, "User1 should get the same amount of yield as user2");
        // Manager distributes yield        _distributeYieldWithAssets(YIELD_AMOUNT, 0, 2, true);
        user1BalanceIfRedeemed = vault.previewRedeem(vault.balanceOf(user1));        user2BalanceIfRedeemed = vault.previewRedeem(vault.balanceOf(user2));        console2.log("user1BalanceIfRedeemed", user1BalanceIfRedeemed);        console2.log("user2BalanceIfRedeemed", user2BalanceIfRedeemed);        assertEq(user1BalanceIfRedeemed, user2BalanceIfRedeemed, "User1 should get the same amount of yield as user2");    }

Recommendation

The Manager.redeemFor function allows the operator of the manager to force the freeloading user out, reducing the protocol's assets to just those that are earning yield. The Operator software needs to have a way to track which users have requested withdraws and waited their cooldown period so they can call redeemFor before adding yield to the contract. This can be blocked by the freeloading user cancelling the withdraw request once the funds arrive, essentially locking them in the AFI contract, a user taking advantage of this issue is able to keep freeloading forever.

There could be more extensive changes that only give the user yield credit until their funds are delivered to the AFI contract or that excludes funds pending withdraw from earning yield. These each have their trade offs and complexities.

It might be simpler to remove funds in the withdraw process from both earnings and losses. If a user tries to game the system by removing their funds just before a loss, they will still have to pay the fee to rejoin the system and earn yield again.

AFI-X: Fixed in commit 089fe2f and 0e5d3da. Prevent pending withdrawal requests from claiming deployed funds yield, and ensure they are not disproportionately exposed to potential losses by removing cancel withdrawal function. Also, in redeemFor() removed cooldown requirements, so that manager can offload users without waiting for cooldown period.

Cantina Managed: The change burns shares on redeem request which prevents the user from being exposed to positive or negative yield while the withdrawal is pending. However, the withdraw and redeem functions still use the _withdraw function when sending the assets to the user. This internal function still checks allowance which creates a weird situation where the user might have revoked permission for the shares and that could prevent them from being able to get their withdrawn tokens even though the shares have already been burned.

Description

There are not dedicated tests for the Manager.sol file, especially the critical functionality paths like execute and withdrawAssets.

Recommendation

Write comprehensive unit and integration tests that ensure that these functions operator correctly, the permissions provide robust protection against unwanted actions and failure modes are properly tested.

AFI-X: Fixed in commit 23ba61b. Increase test coverage for Manager contract to improve reliability and catch potential regressions earlier.

Cantina Managed: The fix greatly increases the test coverage for the Manager. One improvement that could still be made would be to have a full flow/integration that shows the manager using the execute to perform a write call in the system (such as depositing or withdrawing). The current tests only use read calls in execute which does not simulate the real world use case.

Description

The upgrade flow of UUPSUpgradeable goes through the Proxy contract and calls the implementation contract in the upgradeAndCall function. Meanwhile, the TransparentUpgradeableProxy upgrade flow goes through the ProxyAdmin and calls upgradeAndCall from the Proxy contract.

Here are the call paths for both implementations:

Admin -> ProxyAdmin -> Proxy -> upgradeAndCall

Owner -> Proxy -> UUPS -> upgradeAndCall

This creates two separate entry points for upgrades, which could lead to confusion and potential security issues.

Recommendation

Use UUPSUpgradeable with ERC1967Proxy instead of mixing incompatible proxy patterns.

AFI-X: Fixed in commit f889989. Resolves incompatibility between UUPSUpgradeable and TransparentUpgradeableProxy by adjusting the upgrade pattern.

Cantina Managed: The issue was fixed by changing the Proxy to inherit from ERC1967Proxy

Description

The contracts afiToken, Yield and Manager inherit from UUPSUpgradeable, which sets roles and variables in the initializer. It's a best practice to call _disableInitializers in the constructor to prevent the implementation contract's variables from being initialized.

Recommendation

Add an empty constructor that calls _disableInitializers().

AFI-X: Fixed in commits f889989 and f56153e

Cantina Managed: Fix verified.

Description

The maxRedeemCap is set in the Manager contract but isn't validated in the afiToken contract when using withdraw, redeem or redeemFor. Additionally, simply checking the maxRedeemCap isn't fully effective, as users can easily bypass this limitation by using a multicall.

Recommendation

Implement a more robust check for maxRedeemCap that limits the maximum amount a user can redeem within the same block.

AFI-X: Fixed in commit f6320d9. Add validation for maxRedeemCap variable to enforce redemption limits

Cantina Managed: Fix verified.

Description

The function distributeYield in the Yield contract checks if the vault is registered in case of profit, but fails to perform this check in case of loss. This inconsistency allows losses to be accrued to unregistered vaults. Though there is no impact since the caller should be a trusted party, the code should maintain consistency.

Recommendation

Move the vaultTokens validation check to the beginning of the distributeYield function to ensure distribution only to registered vaults.

AFI-X: Fixed in commit 8553ab. Moved vaultTokens validation to start of distributeYield to ensure distribution only to registered vaults.

Cantina Managed: Fix verified.

Description

The system architecture creates circular dependencies between contracts. The afiToken contract references both manager and yield variables, the Yield contract references the manager variable, and the Manager contract references the yield contract. When any of these references is updated, corresponding updates must be made across all contracts. Failure to maintain proper synchronization could lead to issues such as sending tokens to incorrect addresses or inaccurate interest calculations.

Recommendation

Implement a dependency management. For example, use the Manager contract as the single source of truth for contract references. The afiToken contract could then access the yield contract through manager.yield() rather than storing its own reference. This pattern would eliminate the need to update multiple contracts when a dependency changes, reducing the risk of synchronization errors.

- function setManager(address _manager) external onlyRole(ADMIN_ROLE) {-    require(_manager != address(0), "Invalid manager address");-    manager = _manager;- }- function setYield(address _yield) external onlyRole(ADMIN_ROLE) {-    require(_yield != address(0), "Invalid yield address");-    yield = _yield;-    emit YieldSet(msg.sender, _yield);-}+ function setManagerAndYield(address _manager) external onlyRole(ADMIN_ROLE){+    require(_manager != address(0), "Invalid manager address");+    address _yield = Manager(_manager).yield();+    require(_yield != address(0), "Invalid yield address");+    manager = _manager;+    yield = _yield;+ }

AFI-X: Fixed in commit 8b6ae05 and 0e5d3da.

Cantina Managed: The latest commit fixes the finding by modifying the relationship between the Manager, Yield, and afiToken contracts. We now have a one-to-one-to-one relationship: one Yield to one Manager to one afiToken.

Description

In general, the permission structure of the project places an enormous amount of trust in the Manager roles. The DEFAULT_ADMIN can upgrade the contract to anything they want if they become malicious. The ADMIN can withdraw assets and assign other roles. The ADMIN and the OPERATOR can call execute which allows them to execute any arbitrary call against any arbitrary contract. The user who deposit in the Vault are completely trusting the addresses permissioned on the Manager with their funds.

However, even with this trust assumption, there is a way that some risk could be mitigated. Currently, the ADMIN can set whitelisted addresses and the treasury. The DEFAULT_ADMIN can assign roles like the ADMIN and the OPERATOR. There could be a clear line between administering the contract where the ADMIN gives permission for execute to interact with contracts and only the OPERATOR can decide how and when to interact. However, the permissions on execute allow both the ADMIN and the OPERATOR to call it, which means a compromised ADMIN would be able to add a contract to the execute whitelist and call execute in the same transaction.

This also allows the ADMIN to by pass the onlyRole(OPERATOR_ROLE) check on redeemFor since it allows the ADMIN to use execute to call the afiToken contract with the redeemFor(_user,_shares) data. The afiToken contract cannot distiguish a call from the manager that originates from execute or one that comes from Manager.redeemFor. This would allow the ADMIN to burn any arbitrary user's tokens even though the onlyRole(OPERATOR_ROLE) seems to be trying to limit this to the OPERATOR.

Recommendation (optional)

Only allow the OPERATOR to call execute. This separates the execute role from the permission setting role for whitelisting while preventing the admin from bypassing role checks in the Manager.

AFI-X: Fixed in commit 23ba61b. Reduce unnecessary trust risk on ADMIN by tightening permission scopes and applying principle of least privilege.

Cantina Managed: This change addresses this finding by not allowing the ADMIN to call the execute function.

Description

The afiToken uses strategies to earn yield, but these strategies expose it to potential complete loss of funds through defaults or other issues. If an afiToken experiences a total loss, users can still make deposits and receive an inflated number of shares due to the distorted share ratio. Meanwhile, existing users become unable to withdraw their tokens. This leaves the afiToken in an inconsistent state. Additionally, Due to the increase of the share ratio it could lead to other issues like overflow when calculating the new shares.

POC

This POC was created using the test-afiToken.t.sol file.

You can run it with: forge test --mt test_YieldDistribution_FullLoss -vv to see the results

function test_YieldDistribution_FullLoss() public {  vm.startPrank(user1);  vault.deposit(DEPOSIT_AMOUNT, user1);  vm.stopPrank();  uint256 initialAssets = vault.totalAssets();
  _distributeYieldWithAssets(DEPOSIT_AMOUNT, 0, 1, false);
  uint256 assetsAfterFullLoss = vault.totalAssets();  assertEq(assetsAfterFullLoss, 0, "Assets should be zero after full loss");    //@audit After a completely loss, user2 should be able to deposit and receive shares  vm.startPrank(user2);  vault.deposit(DEPOSIT_AMOUNT, user2);  vm.stopPrank();
  console2.log("Shares User1: ", vault.balanceOf(user1));  console2.log("Shares User2: ", vault.balanceOf(user2));
  vm.startPrank(user1);  vault.requestWithdrawal(vault.balanceOf(user1));  vm.stopPrank();
  vm.startPrank(user2);  vault.requestWithdrawal(vault.balanceOf(user2));  vm.stopPrank();
  vm.warp(block.timestamp + vault.cooldownPeriod() + 1);
  uint256 sharesUser1 = vault.balanceOf(user1);  uint256 sharesUser2 = vault.balanceOf(user2);
  //@audit It's not possible to user1 withdraw because the convertion of their assets is less than 0  uint256 assetBefore1 = asset.balanceOf(user1);  vm.startPrank(user1);  vm.expectRevert();  vault.redeem(sharesUser1, user1, user1);  vm.stopPrank();  uint256 assetAfter1 = asset.balanceOf(user1);  console2.log("AssetBefore1: ", assetBefore1);  console2.log("AssetAfter1: ", assetAfter1);
  //@audit But user2 still able to withdraw  uint256 assetBefore2 = asset.balanceOf(user2);  vm.startPrank(user2);  vault.redeem(sharesUser2, user2, user2);  vm.stopPrank();  uint256 assetAfter2 = asset.balanceOf(user2);  console2.log("AssetBefore2: ", assetBefore2);  console2.log("AssetAfter2: ", assetAfter2);}

Recommendation

It's recommended to implement functionality that can deactivate or close the afiToken in the event of a total loss of funds.

AFI-X: In case of such events, we will set the vault as deprecated in the user interface. Additionally, in the AFI Token, we can pause the deposit and withdrawal functions since both have whenNotPaused modifiers.

Cantina Managed: We acknowledge this finding, as it will be addressed off-chain by pausing the vault.

Description and Recommendation

The codebase contains inconsistent naming patterns for variables and contract names, reducing code readability and maintainability.

The specific naming issues are:

• The VaultTokens variable in the Yield contract should be renamed to afiToken or vaultToken
• The contract names afiToken and afiProxyAdmin don't follow the camel case naming convention

AFI-X: Fixed in commit 2960cce. Fixes standardize naming conventions to camelCase for variables and contract names refactored codebase to use camelCase consistently for improved readability and maintainability.

Cantina Managed: The commit partially fixes the issue. It still leaves the afiToken contract name without camel case.

Description

The Yield contract only implements the setVaultToken function, which sets a vaultToken address to true, registering a vault for yield distribution. However, the contract lacks functionality to remove vaults. In case of a complete loss, there should be a way to remove a vault to stop yield distribution and deactivate it.

Recommendation

Add a bool parameter to the setVaultToken function to enable or disable a vault.

AFI-X: Fixed in commit 8b6ae058 and 0e5d3da

Cantina Managed: The fix for this issue involved restructuring the relationship between the afiToken, Manager, and Yield contracts. Now, the Yield contract references a single Manager and afiToken.

Description

The distributeYield function should be called at a specific frequency, according to the documentation, the off-chain component should call it daily. However, this function lacks on-chain validation to ensure this timing is respected. A misconfigured off-chain rebalancer might call it at irregular intervals. While this has no impact other than more frequent updates, implementing time validation would ensure consistency between on-chain and off-chain integration.

Recommendation

Implement functionality to track and validate the timestamp of the last yield distribution call for each vault.

AFI-X: Fixed in commit 0e5d3da. Add on-chain time restriction to distributeYield for consistent invocation frequency.

Cantina Managed: Fix verified.

Description

withdrawalRequests currently occupies the storage slot right after yield. However, in the layout of the code, it is visually separated from the other storage variables by the struct definition. This increases the risk that a future implementation might add a new state variable right after yield since withdrawalRequests and overwrite the storage of withdrawalRequests.

Recommendation

Moving the struct above or into the IafiUSD contract would make the withdrawalRequests visually more part of the STATE VARIABLES section.

AFI-X: Fixed in commit 300f4a4. Reorganize storage layout to prevent risk of overwriting withdrawalRequests and adding a gap var.

Cantina Managed: Fix verified.

Description

While a delayed withdraw pattern is not envision in ERC-4626, withdraw interacts with assets and redeem uses shares. In the custom requestWithdrawal function this code uses shares with "withdrawal" possibly creating confusion.

Recommendation

Using requestRedeem with shares would be more inline with ERC-4626 naming conventions. Further ERC-7540 does specify a requestRedeem pattern that could be considered since the functionality is similar.

AFI-X: Fixed in commit 089fe2f. Update requestWithdrawal function to burn shares and use the struct to withdraw later, aligning with ERC-4626 naming patterns and increasing codebase consistency.

Cantina Managed: Fix verified.

Description

Generally it is preferred to use custom errors, they can reduce deployment cost and make debugging easier. However, whether the project uses custom errors or reverts with string error messages, it would be better to be consistent throughout the project.

AFI-X: Fixed in commit f3ccd4a.

Cantina Managed: Fix verified.

Description and Recommendation

1. The isNewVaultToken variable in Yield is unused. It seems to be related to a future upgrade, but should be removed if will not be used.

function distributeYield(address VaultToken, uint256 amount, uint256 feeAmount, uint256 nonce, bool isProfit)    external    onlyRole(REBALANCER_ROLE){    ...    if (isProfit) {        ...        ManageAssetAndShares memory manageAssetAndShares = ManageAssetAndShares({            VaultToken: VaultToken,            shares: feeShares,            assetAmount: feeAmount,            updateAsset: true,            isMint: true,            isNewVaultToken: false //@audit This variable is not used yet        });    }   ...}

2. The previewRedeem function in afiToken can return super.previewRedeem instead of setting it to assets variable

function previewRedeem(uint256 shares) public view override returns (uint256) {    ...-    uint256 assets = super.previewRedeem(remainingShares);-    return assets;+   return super.previewRedeem(remainingShares);}

3. The yield variable has undeclared visibility and should be explicitly set to public like other state variables.

4. Tracking the transaction hash in the distributeYield function is unnecessary. The hash already includes both the nonce and the VaultToken, and the existing check on the nonce prevents replay attacks using the same nonce + VaultToken combination.

5. Grouping the if(isProfit) in the distributeYield function to save gas:

function distributeYield(address VaultToken, uint256 amount, uint256 feeAmount, uint256 nonce, bool isProfit)  external  onlyRole(REBALANCER_ROLE){	...	    if (isProfit) {       profit[VaultToken] += amount;       IafiUSD(VaultToken).transferInRewards(amount, true);+      require(vaultTokens[VaultToken], "vault not registered");+      uint256 feeShares = IERC4626(VaultToken).convertToShares(feeAmount);+      ManageAssetAndShares memory manageAssetAndShares = ManageAssetAndShares({+           VaultToken: VaultToken,+           shares: feeShares,+           assetAmount: feeAmount,+           updateAsset: true,+           isMint: true,+           isNewVaultToken: false+       });+       IManager(manager).manageAssetAndShares(IManager(manager).treasury(), manageAssetAndShares);    } else {        ...    }  
-  if (isProfit) {-      require(vaultTokens[VaultToken], "vault not registered");-      uint256 feeShares = IERC4626(VaultToken).convertToShares(feeAmount);-      ManageAssetAndShares memory manageAssetAndShares = ManageAssetAndShares({-          VaultToken: VaultToken,-          shares: feeShares,-          assetAmount: feeAmount,-          updateAsset: true,-          isMint: true,-           isNewVaultToken: false-      });-      IManager(manager).manageAssetAndShares(IManager(manager).treasury(), manageAssetAndShares);-  }  emit DistributeYield(msg.sender, IERC4626(VaultToken).asset(), VaultToken, amount, isProfit);}

6. Lack of Event could cause integration/tracking issues for rewards. Only profitable rewards emit an event making tracking how many losses occur difficult.
7. Unclear comment for Manager.withdrawAssets function indicates it can only be called by admin or self; however, self is accessible through execute whose permissions include OPERATOR. It would be more clear to make note of this.
8. The code is using two types of implementing errors, revert with CustomError and require with string messages. For consistency, it's recommended to choose one approach throughout the codebase.
9. The function _updateWithdrawRequest in afiToken must be called before _withdraw. For better code organization, _updateWithdrawRequest should be moved inside _withdraw . it's used by the functions: withdraw, redeem, and redeemFor.

AFI-X: Fixed in commit 48553ab.Grouped if(isProfit) conditions in distributeYield to reduce gas consumption. Added transferToVault function to handle asset transfers to vaults during redemption. Moved _updateWithdrawRequest inside _withdraw for better logical cohesion; used by withdraw, redeem, and redeemFor functions. Few more optimizations.

Cantina Managed: The fix partially addresses the issues. The unsolved points are #4 and #6.