Fractality
Findings & Analysis Report

2024-08-14

Table of contents

Overview

About C4

Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.

A C4 Pro League Audit is an event where elite tier Code4rena contributors, commonly referred to as wardens, reviews, audits and analyzes smart contract logic in exchange for a bounty provided by sponsoring projects.

During the Pro League audit outlined in this document, C4 conducted an analysis of the Fractality smart contract system written in Solidity. The audit took place between Aug 6 - Aug 8, 2024.

Wardens

3 Wardens contributed to Fractality:

  1. peakbolt
  2. cccz
  3. SpicyMeatball

Final report assembled by bytes032 and Sentinel

Summary

The C4 Pro League analysis yielded 6 MEDIUM and 2 Low-Risk severity vulnerabilities.

Additionally, C4 Pro League analysis included 1 finding with an informational rating.

Scope

The source code was delivered to Code4rena in a private Git repository.

Severity Criteria

C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.

High-level considerations for vulnerabilities span the following key areas when conducting assessments:

  • Malicious Input Handling
  • Escalation of privileges
  • Arithmetic
  • Gas use

For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.

Medium Risk Findings (6)

[M-1] The vault will not work with tokens that do not return values on transfer (e.g. USDT)

Context:

Description:

Some tokens doesn’t fully comply with ERC20 standard and do not return boolean values on transfer and transferFrom calls, one of these tokens is USDT. Since the vault contract checks for a successful transfer and expects to receive a return value from the asset, USDT cannot be used as an underlying token.

One example of such a transfer call:

        if (!asset.transferFrom(msg.sender, strategy.strategyAddress, assets)) {
            revert ERC20TransferFailed();
        }

Recommendation:

Consider using SafeTransferLib from Solmate to transfer assets.

Fractality:

Fixed in PR-1

C4 Pro League:

Issue has been resolved as per recommendation.

[M-2] Possible underflow in redeem() can prevent users from redeeming their assets

Context:

Description:

If the call order is requestRedeem() -> reportLosses() -> redeem(), vaultAssets may be less than request.redeemRequestAssetAmount, resulting in underflow. For example, Alice deposits 10000 tokens, vaultAssets is 10000. Alice call requestRedeem(), and redeemRequestAssetAmount is 10000. Then reportLosses() is called and vaultAssets reduce to 9000. When Alice call redeem(), since redeemRequestAssetAmount > vaultAssets, the subtraction vaultAssets -= request.redeemRequestAssetAmount will underflow and revert the transaction.

Recommendation:

It is recommended to reduce vaultAssets in requestRedeem(), and also burn shares in requestRedeem().

Fractality:

Issue has been resolved by burning shares (and related operations) on redeem request - PR-1.

C4 Pro League:

Issue has been resolved as per recommendation.

[M-3] Users who requested assets redemptions may be affected by the new claimableDelay and redeemFeeBasisPoints

Context:

Description:

Increasing claimableDelay will result in unfair extension of the redemption for users who submitted a request before the change was made.

    function setClaimableDelay(
        uint32 _newClaimableDelay
    ) external onlyRole(DEFAULT_ADMIN_ROLE) {
        claimableDelay = _newClaimableDelay;
        emit ClaimableDelaySet(_newClaimableDelay);
    }

And increasing redeemFeeBasisPoints will result in unfair charges of the redemption for users who submitted a request before the change was made.

    function setRedeemFee(
        uint16 _newRedeemFeeBasisPoints
    ) external onlyRole(DEFAULT_ADMIN_ROLE) {
        if (_newRedeemFeeBasisPoints > _MAX_BASIS_POINTS) {
            revert InvalidRedeemFee();
        }
        redeemFeeBasisPoints = _newRedeemFeeBasisPoints;
        emit RedeemFeeSet(_newRedeemFeeBasisPoints);
    }

Recommendation:

Calculate the redemption date and fee at the time of request creation:

-      request.redeemRequestCreationTime = uint96(block.timestamp);
+      request.redeemRequestFulfillTime = uint96(block.timestamp) + claimableDelay;
+      request.redeemRequestFee = _calculateWithdrawFee(assets);

Claimable check:

        if (
-          block.timestamp < request.redeemRequestCreationTime + claimableDelay
+          block.timestamp < request.redeemRequestFulfillTime
        ) {

Fee transfer:

        uint256 netAssetRedeemAmount = request.redeemRequestAssetAmount -
            request.redeemRequestFee;

        if (
            !asset.transfer(redeemFeeCollector, request.redeemRequestFee) ||
            !asset.transfer(receiver, netAssetRedeemAmount)
        ) {
            revert ERC20TransferFailed();
        }

Fractality:

Fixed in PR-1

C4 Pro League:

The issue with fees has been resolved per recommendation and the delay issue is acknowledged.

[M-4] Users may experience losses due to slippage issues

Context:

Description:

When a redemption request is made, the vault will calculate the amount of assets the user will receive for his shares:

    function requestRedeem(
        uint256 shares,
        address controller,
        address owner
    )
        external
        onlyWhenNotHalted
        operatorCheck(owner)
        nonReentrant
        returns (uint8)
    {
        if (controller == address(0) || owner == address(0)) {
            revert ZeroAddress();
        }
>>      uint256 assets = convertToAssets(shares);
        if (assets == 0) {
            revert ZeroAssets();
        }

        RedeemRequestData storage request = redeemRequests[controller];

        if (request.redeemRequestCreationTime > 0) {
            revert ExistingRedeemRequest();
        }

        request.redeemRequestShareAmount = shares;
>>      request.redeemRequestAssetAmount = assets;

The problem may occur if there is a significant delay between sending requestRedeem transaction and it’s execution. For example the share price may go down and user will receive fewer assets than expected.

Consider the following scenario:

  • Bob is the only shareholder with 1000 shares, and there are 1000 USDC in the vault
  • he calls requestRedeem and expects to receive 1000 USDC in exchange for 1000 shares
  • his transaction was delayed for some reason (low gas amount, arbitrum sequencer down) and reportLosses was executed first, leaving only 500 USDC in the vault
  • when Bob’s transaction is finally executed, he will have only 500 USDC in the request.

Recommendation:

To fully comply with ERC7540 it is recommended to create an overloaded version of the requestRedeem function in which user can specify minimum amount of assets he wants to receive:

    function requestRedeem(
        uint256 shares,
        address controller,
        address owner
    )
        external
        returns (uint8)
    {
         return _requestRedeem(shares, controller, owner);
    }

    function requestRedeem(
        uint256 shares,
        uint256 minAssetsOut,
        address controller,
        address owner
    )
        external
        returns (uint8)
    {
         if(convertToAssets(shares) < minAssetsOut) revert MinAmountFail();
         return _requestRedeem(shares, controller, owner);
    }

Fractality:

Fixed in PR-1

C4 Pro League:

Issue has been resolved as per recommendation.

[M-5] Pending withdrawals dilute the exchange rate

Context:

Description:

When users call requestRedeem(), their shares are sent to the contract. And when users call redeem(), these shares are burned. These shares in the contract will dilute the exchange rate, and only when users call redeem() to redeem the pending withdrawals, the exchange rate will be correct.

  1. Say total assets are 1000 and total shares are 1000.
  2. Alice request redeem for 500 shares.
  3. Then reportProfits increases total assets to 2000.
  4. The exchange rate will be 2000/1000 = 2, but since the exchange rate for the withdrawn 500 shares is locked, the correct exchange rate should be (2000-500)/(1000-500) = 3.

Recommendation:

It is recommended to reduce vaultAssets and burn shares in requestRedeem().

Fractality:

Issue has been resolved by burning shares (and related operations) on redeem request - PR-1.

C4 Pro League:

Issue has been resolved as per recommendation.

[M-6] requestRedeem() can be DoS for specific controller

Context:

Description:

requestRedeem() can be requested for a specific controller address by anyone, and only one request can be made for each controller at any time.

This allows an attacker to request redeem for a victim controller address with dust shares and then not call redeem() to leave it as pending indefinitely. This will block legitimate redeem request for that controller as new redeem request cannot be created till the pending request is redeemed.

    function requestRedeem(
        uint256 shares,
        address controller,
        address owner
    )
        external
        onlyWhenNotHalted
        operatorCheck(owner)
        nonReentrant
        returns (uint8)
    {
        if (controller == address(0) || owner == address(0)) {
            revert ZeroAddress();
        }
        uint256 assets = convertToAssets(shares);
        if (assets == 0) {
            revert ZeroAssets();
        }

           RedeemRequestData storage request = redeemRequests[controller];

        if (request.redeemRequestCreationTime > 0) {
            revert ExistingRedeemRequest();
        }

Recommendation:

This can be fixed by introducing multiple redeem requests for each controller, allowing more than one redeem request for each controller address. Each request shall be uniquely identified by both controller and requestId.

  • It is important that each redeem request have a different requestId, as ERC-7540 requires request with the same requestId to be fungible (i.e. requests with same requestId must be claimable at the same exchange rate).
  • As per ERC-7540, requests with different requestId do not have the fungible requirements and can be claimable at different times and at different exchange rate. Hence, this is recommended.

As discussed, we do not recommend adding to existing redeem request as there is no good solution on updating the redeemRequestCreationTime in a fair manner. That is because if we allow subsequent request to update the creation time (e.g to the latest creation time), it will allow one to indefinitely delay the redeem time. Another issue of bypass the redeem delay could also occur if we try to take an average of the creation time, as that will allow part of the redeem request to be claimable at an earlier time.

Fractality:

This issue has been resolved with PR-1

C4 Pro League:

Issue has been resolved as per recommendation.

Low Risk Findings (1)

[L-1] maxDeposit() and maxMint() should return 0 when halted

Context:

Description:

maxDeposit() and maxMint() should return 0 when halted, similiar to maxRedeem(). This is aligned with ERC4626 specification where both functions are required to return 0 when deposits/mints are disabled.

    function maxDeposit(
        address /*receiver*/
    ) public view override returns (uint256) {
        uint256 remainingCapacity = maxVaultCapacity - vaultAssets;
        return
            remainingCapacity < maxDepositPerTransaction
                ? remainingCapacity
                : maxDepositPerTransaction;
    }

    function maxMint(address receiver) public view override returns (uint256) {
        return convertToShares(maxDeposit(receiver));
    }

Recommendation:

Update maxDeposit() and maxMint() to return 0 when halted.

    function maxDeposit(
        address /*receiver*/
    ) public view override returns (uint256) {

+        if (halted) {
+            return 0; //cannot deposit when halted
+        }

        uint256 remainingCapacity = maxVaultCapacity - vaultAssets;
        return
            remainingCapacity < maxDepositPerTransaction
                ? remainingCapacity
                : maxDepositPerTransaction;
    }

    function maxMint(address receiver) public view override returns (uint256) {
        return convertToShares(maxDeposit(receiver));
    }

Fractality:

Fixed as recommended, under the following commit

C4 Pro League:

Issue has been resolved as per recommendation.

Informational Findings (2)

[I-1] Vault does not support rebasing token

Context:

Description:

The vault asset accounting is synthetic. totalAssets() uses vaultAssets that is updated based on deposit/mint and not based on balanceOf(). That means the vault will not be able to support rebasing tokens.

   function totalAssets() public view override returns (uint256) {
        return vaultAssets;
    }

Recommendation:

Consider acknowledging this and add documentation to only use vault asset that are non-rebasing token or wrapper for re-basing token.

Fractality:

Documented to make sure a rebasing token is not used as the asset in the following commit

C4 Pro League:

Issue has been resolved as per recommendation.

[I-2] Withdraw() event should use after-fees asset amount

Context:

Description:

According to ERC-4626, previewRedeem() and previewWithdraw() must be inclusive of fees so that integrators are aware of the withdrawal fees. This implies that Withdraw event should be based on actual asset amount received by the receiver, after accounting for fees. This is also explained in the OZ doc for [Custom behavior: Adding fees to the vault].(https://docs.openzeppelin.com/contracts/5.x/erc4626#fees)

However, the protocol emits Withdraw() event based on request.redeemRequestAssetAmount, which is not inclusive of fees.

        emit Withdraw(
            msg.sender,
            receiver,
            request.originalSharesOwner,
            request.redeemRequestAssetAmount,
            shares
        );

Recommendation:

Consider using netAssetRedeemAmount instead of request.redeemRequestAssetAmount as follows:

        emit Withdraw(
            msg.sender,
            receiver,
            request.originalSharesOwner,
-            request.redeemRequestAssetAmount,
+            netAssetRedeemAmount
            shares
        );

Fractality:

Fixed as recommended, under the following commit

C4 Pro League:

Issue has been resolved as per recommendation.

Disclosures

C4 is an open organization governed by participants in the community.

C4 audits incentivize the discovery of exploits, vulnerabilities, and bugs in smart contracts. Security researchers are rewarded at an increasing rate for finding higher-risk issues. Audit submissions are judged by a knowledgeable security researcher and solidity developer and disclosed to sponsoring developers. C4 does not conduct formal verification regarding the provided code but instead provides final verification.

C4 does not provide any guarantee or warranty regarding the security of this project. All smart contract software should be used at the sole risk and responsibility of users.