Detectbox Audit Reports
Kunji Finance Security Audit Report
Findings

FINDINGS

Detailed Summary of Findings

Sl. No.NameSeverity
M-01Unsafe ERC20 Transfer Function UsageMedium
M-02Chainlink Sequencer Uptime Check Not Implemented ProperlyMedium
M-03Use call instead of Transfer for address payableMedium
M-04The owner is a single point Of failure and a centralization riskMedium
M-05Non-Upgradable OpenZeppelin Contracts Used in Upgradeable ContractsMedium
M-06Missing Access control in emergencyClose() functionMedium
L-01No logic is implemented to handle profit in rollover function.Low
L-02Natspec is missingLow
L-03Missing Zero-Address Check in setGmxObserver() FunctionLow
L-04No error or revert in claim function if claim == 0Low
L-05Avoid the use of Floating PragmaLow
L-06Consider implementing two-step procedure for updating protocol addressesLow
L-07Unused receive() function will lock Ether in contractLow
L-08Use Ownable2StepUpgradeable rather than OwnableUpgradeableLow
I-01Removal of Commented-Out Code for Better Code QualityInformational
I-02open TODOsInformational
I-03Variable names don't follow the Solidity style guideInformational
I-04Using unnamed mappingsInformational
I-05Consider adding emergency-stop functionalityInformational
I-06Imports could be organized more systematicallyInformational
I-07Event is not properly indexedInformational
I-08TyposInformational
G-01Explicitly initializing variables with their default values wastes gasGas
G-02Use assembly to check for address(0)Gas
G-03Caching the array length outside a loopGas
G-04Redundant zero-address check in the removeTrader() functionGas

Static Analysis

No major issues were found. Some false positive errors were reported by the tool. All the other issues have been categorized above according to their level of severity.

Manual Review

Medium Severity Issues

M-01. Unsafe ERC20 Transfer Function Usage

Description :

It is not safe to use the ERC20 transfer function without checking the results. In BaseVault.sol, emergencyWithdraw() function uses the transfer() method to transfer funds from users to the contract. However, due to some ERC20 tokens' non—compliance with the standards, if a non standard token like is used, the input will return nothing instead of a boolean value. As a result, the condition checking for if(!success) where is the return value of will always be triggered and revert the transaction.

Code Snippets :

 
function emergencyWithdraw(IERC20 token) external onlyOwner {
        if (address(token) == 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE) {
            uint256 ethBalance = address(this).balance;
            payable(owner()).transfer(ethBalance);
            return;
        }
        uint256 balance = token.balanceOf(address(this));
        token.transfer(owner(), balance); //@audit: Use safeTransfer instead of transfer
}
 
function collectFees(uint256 amount) external override onlyOwner {
        uint256 _kunjiFeesAssets = kunjiFeesAssets;
 
        if (amount > _kunjiFeesAssets) {
            revert TooBigAmount();
        }
 
        kunjiFeesAssets = _kunjiFeesAssets - amount;
 
        address feeReceiver = IContractsFactory(contractsFactoryAddress).feeReceiver();
        IERC20(underlyingTokenAddress).transfer(feeReceiver, amount); //@audit: se safe Transfer instead of transfer
}

Recommendations :

I recommend using OpenZeppelin's SafeERC20 versions with the safeTransfer and safeTransferFrom functions that handle the return value check as well as non-standard-compliant tokens.

M-02. Chainlink Sequencer Uptime Check Not Implemented Properly

Description :

In the DynamicValuation contract is that the _checkSequencerUptimeFeed() function is only implemented in the _getUSDValueOfAddress() function. This means that the sequencer uptime is not checked when fetching price feeds in the _getDataFeedAnswer() function. _getDataFeedAnswer function is called in getOraclePrice() to fetch price in different parts of contract. It leads to insufficient checks for if the sequencer is active. If the sequencer is down, then the price feeds returned by the _getDataFeedAnswer() function will be incorrect. This could lead to incorrect valuations of tokens and assets, which could have financial consequences for users of the DynamicValuation contract.

Code Snippets :

function _getDataFeedAnswer(
            OracleData memory oracleData,
            address token
        ) private view returns (uint2S6) {
            if (oracleData.dataFeed == address(O)) {
                revert leFocToken(token) ;
            }
            AggregatorV2V3Interface _dataFeed AggregatorV2V3Interface(
                oracleData. dataFeed
            );
            (, int answer, uint2S6 updatedAt, ) = _dataFeed.latestRoundData();
            if (answer <= 0) {
                revert BadPrice();
            }
            if(block.timeStamp - updatedAt > oracleData.heartBeat){
                revert TooOldPrice();
            }
            return uint256(answer);
        }

Recommendation :

Add a line to check sequencer uptime in -getDataFeedAnswer() function.

function _getDataFeedAnswer(
            OracleData memory oracleData,
            address token
        ) private view returns (uint2S6) {
            if (oracleData.dataFeed == address(O)) {
                revert leFocToken(token) ;
            }
            AggregatorV2V3Interface _dataFeed AggregatorV2V3Interface(
                oracleData. dataFeed
            );
            (, int answer, uint2S6 updatedAt, ) = _dataFeed.latestRoundData();
            if (answer <= 0) {
                revert BadPrice();
            }
            if(block.timeStamp - updatedAt > oracleData.heartBeat){
                revert TooOldPrice();
            }
            return uint256(answer);
        }

M-03. Use call instead of Transfer for address payable

Description :

The emergencyWithdraw() function in the BaseVault.sol contract uses the transfer() function to send ether to an address payable. The transfer() function forwards a fixed amount of 2300 gas. This means that the function may fail if the gas cost of EVM instructions changes significantly during a hard fork, or if the claimer smart contract does not implement a payable function or does implement a payable fallback function that uses more than 2300 gas units.

Code Snippets :

function emergencyWithdraw(IERC20 token) external {
        if (address(token) == 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE) {
            uint256 ethBalance address(this).balance;
            payable(owner()).transfer(ethBalance); //@audit-issue Use call instead of transfer for address payable
        return;
        }
        uint256 balance = token.balanceOf(address(this));
        token.transfer(owner(), balance);
}

Recommendations :

Instead use the .call() function to transfer ether and avoid some of the limitations of .transfer().

Gas units can also be passed to the .call() function as a variable to accommodate any uses edge cases. Gas could be a mutable state variable that can be set by the contract owner.

(bool success, ) = payable(owner()).call{value: ethBalance}("");
require(success, "Transfer failed.");

M-04. The owner is a single point of failure and a centralization risk

Description :

The owner of the BaseVault.sol contract has the ability to withdraw all funds from the contract. This creates a single point of failure and centralization risk. If the owner is malicious, they could steal all funds from the contract.

Code Snippets :

function emergencyWithdraw(IERC2Ø token) external onlyOwner { //Add a timelock function for it.
        if (address(token)  == 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE) {
            uint256 ethBalance = address(this).balance;
            payable(owner()).transfer(ethBalance)
            return;
        }
        uint256 balance = token.balanceOf(address(this));
        token.transfer(owner(), balance);
}

Recommendation :

A timelock should be implemented for the emergencyWithdraw() function. This would prevent the owner from withdrawing funds immediately.

M-05. Non-Upgradable OpenZeppelin Contracts Used in Upgradeable Contracts

Description :

The contracts ContractFactory.sol and TraderWallet.sol utilize an upgradable version of OpenZeppelin contracts such as OwnableUpgradeable and ReentrancyGuardUpgradeable, which ensures that the code is safe for upgradeability. However, these contracts import the non-upgradable versions of SafeERC20, IERC20Metadata and EnumerableSet from the OpenZeppelin library which is not a good practice. An upgradable version of the contract ensures that the code is safe for upgradeability. Check out here for more details.

Code Snippets :

    7: import {EnumerableSet} from "@openzeppelin/contracts/utiIs/structs/EnumerableSet.sol";
    8: import {IERC2ØMetadata} from "@openzeppe1in/contracts/token/ERC2Ø/extensions/IERC20Metadata.sol";
    6: import {SafeERC20} from "@penzeppelin/contracts/token/ERC20/utiIs/SafeERC20.sol" ;
    7: import {EnumerableSet} from "@openzeppelin/contracts/uti1s/structs/EnunerableSet.sol";

Recommendation :

To ensure full compatibility with the upgradeable design pattern and to enhance the contract's upgradeability, it is recommended to use the upgradable versions of contracts consistently throughout the codebase.

 
    import {SafeERC20Upgradeable} from "@openzeppeIin/contracts-upgradeable/token/ERC2Ø/utiIs/SafeERC20Upgradeable.sol";
    import {EnumerableSetUpgradeable} from "&penzeppelin/contracts-upgradeable/uti1s/structs/Erunerab1eSetUpgradeable.sol" ;

M-06. Missing Access control in emergencyClose() function.

Description :

The emergencyClose() function in the UserVault.sol contract lacks proper access control. The function visibility is marked as external, which means that any external user can call this function. Additionally, there are no validation checks in the function to verify that the caller is authorized to close the user's position.

Code Snippet:

    function emergencyCl    ose() external { //@access control issue?
            address _traderWalletAddress =  traderWalletAddress;
            if (
                ITradeWallet(_traderWalletAddress).lastRolloverTimestamp() +
                emergencyPeriod >
                block.Timestamp &&
                @isEmergencyOpen
            ) revert TooEarly();
 
            bool isRequestFu1fi11ed = _closeUniswapPositions(_traderWalletAddress);
            _closeGmxPositions(_traderWalletAddress);
 
            if (!isRequestFu1fi11ed) {
                currentSlippage += slippageStepPercent;
                isEmergencyOpen = true;
            } else {
                currentSlippage = defaultSlippagePercent;
                isEmergencyOpen = false;
            }
    }

Recommendation :

Implement proper access control so that no unauthorized caller can access emergency functions. Additionally, The function should be modified to include validation checks to verify that the caller is authorized to close the user's position.

Low Severity Issues

L-01. No logic is implemented to handle profit in rollover function

Description :

The rollover() function in TraderWallet.sol calculates the profit and has an if check for overallProfit > O. However, the function does not implement any logic to handle the profit, and nothing is executed inside the if statement. There is a comment to do something with the profit, but no implementation is present inside the code block.

Code Snippets :

    // not sure if the execution is here. Don't think so
    function rollover() external override {
            if (lastRolloverTimestamp + rolloverPeriod > block.timestamp) {
                revert TooEarly();
            }
            uint256 _cumulativePendingDeposits = cumulativePendingDeposits;
            uint2S6 _cumulativePendingWithdrawals =  cumulativePendingWithdrawals;
 
            ....
            ....
 
            // get profits
            int2S6 overallProfit;
            if (_currentRound != O) {
                overallProfit =
                    int256(_newAfterRoundBalance) -
                    int256(afterRoundBalance);
            }
            if (overallProfit > O) { // @audit Profit calculated but no logic is
                // 00 SONE THING HERE WITH PROFIT
                // PROFIT IS CALCULATED IN ONE TOKEN
                // BUT PROFIT IS DISTRIBUTED OPEN POSITIONS
                // ANO TOKEN BALANCES
            }
    }

Recommendation :

The function does not have any fee handling logic based on profits. Therefore, we recommend to add logic accordingly or to remove the if statement as if it does not have any purpose.

L-02. Missing Natspec in many function

Description :

Solidity contracts can use a special form of comments to provide rich documentation for functions, return variables and more. This special form is named the Ethereum Natural Language Specification Format (NatSpec). There are multiple functions in few smart contracts in scope that lack NatSpec comments, which are essential for providing clear and comprehensive documentation for functions, return variables, and other contract elements.

Code Snippets :

The following smart contracts in scope lack NatSpec comments in

multiple functions: https://github.com/Kunji-Finance/KF-Contract/blob/develop/ (opens in a new tab)

contracts/ContractsFactory.sol https://github.com/Kunji-Finance/KF-Contract/blob/develop/ (opens in a new tab)

contracts/DynamicValuation.sol https://github.com/Kunji-Finance/KF-Contract/blob/develop/ (opens in a new tab)

contracts/TraderWallet.sol https://github.com/Kunji-Finance/KF-Contract/blob/develop/ (opens in a new tab) contracts/UsersVault.sol

Recommendation :

To enhance the quality and usability of the smart contracts, it is strongly recommended to add NatSpec comments to various parts of the code.

L-03. Missing Zero-Address Check in setGmxObserver() Function

Description

The setGmxObserver() function in the DynamicValuation.sol contract lacks a check for a zero address (OXO) when setting the gmxObserver value. However, many other setter functions in the contract properly include a check for a zero address.

Code Snippets :

    function setGmxObserver(address newVa1ue) external override onlyOwner {
            gmxObserver = newVaIue; //@audit no check for 0-address.
 
            emit SetGmxObserver(newVa1ue) ;
    }

Recommendation :

The recommended mitigation steps are to add a check for 0- addresses to the setGmxObserver() function.

    function setGnxObserver(address newVa1ue) external override onlyOwner {
            _checkZeroAddress (newVa1ue, "_GnxObserver") ;
            gmxObserver = newVa1ue;
            emit SetGmxObserver(newVa1ue);
    }

L-04. No error or revert in claim function if claim ==0.

Description :

The claim() function in UserVault.sol does not check if the unclaimedDepositShares or unclaimedWithdrawAssets variables are equal to 0. If these variables are equal to 0, the function will silently execute without any error or revert.

Code Snippets :

    function claim() external override {
            UserData memory data = _updateUserData(mse. sender);
            if(data.unclaimedDepositShares > O) {
                super._transfer(
                    address(this),
                    msg.sender,
                    data.unclaimedDepositShares
                );
                delete _ userData(msg.sender).unclaimedDepositShares;
                emit SharesClaimed(
                    data.round,
                    data.unclaimedDepositShares,
                    msg.sender,
                    msg.sender
                );
            }
            if (data.unclaimedWithdrawAssets > 0) {
                uint256 underlyingBalance = IERC20(underlyingTokenAddress)
                // LOGIC HERE
                emit AssetClaimed(
                    data.round ,
                    transferAmount,
                    msg.sender,
                    msg.sender
                );
            }
    }

Recommendation :

To mitigate this vulnerability, the claim() function should be updated to check if the unclaimedDepositShares and unclaimedWithdrawAssets variables are equal to 0. If they are equal to 0, the function should revert with an error.

L-05. Floating Pragma

Description :

Contracts should be deployed with the same compiler version and flags used during development and testing. Locking the pragma helps to ensure that contracts do not accidentally get deployed using another pragma.

Code Snippets :

    contracts/adapters/gnx/interfaces/IGnxOrderBook.sol:
        3: pragma solidity
    contracts/adapters/enx/interfaces/IGmxPositionManager.sol:
        3: pragma solidity
    contracts/adapters/gnx/interfaces/IGnxReader.sol:
        3: pragma solidity
    contracts/adapters/enx/interfaces/ICnxRouter.sol:
        3: pragma solidity
    contracts/adapters/gnx/interfaces/IGnxVault.sol:
        3: pragma solidity
    contracts/adapters/gnx/interfaces/IVauItPriceFeed.sol
        3: pragma solidity
    contracts/adapters/uni swap/interfaces/INonFungiblePositionManager.sol:
        3: pragma solidity
    contracts/adapters/uni swap/interfaces/IQuoterV2.sol:
        3: pragma solidity
    contracts/adapters/uniswap/interfaces/IUniswapV3Factory.sol:
        3: pragma Solidity >= 0.8.0;
    contracts/adapters/uni swap/interfaces/IUniswapV3Pool.sol:
        3: pragma solidity >= 0.8.0;
    contracts/adapters/uniswap/interfaces/IUniswapV3Router.sol:
        3: pragma solidity >= 0.8.0;
    contracts/adapters/uniswap/libraries/BytesLib.sol:
        3: pragma solidity >= 0.8.0 < 0.9.0;
    contracts/interfaces/IAdapter. sol:
        3: pragma solidity >= 0.8.0;
    contracts/ interfaces/IAdaptersRegistry.sol:
        3: pragma solidity >= 0.8.0;
    contracts /interfaces/IBaseVauIt. sol:
        3: pragma solidity >= 0.8.0;
    contracts/ interfaces/ IContractsFactory.sol:
        3: pragma solidity >= 0.8.0;
    contracts/interfaces/ IDynamicValuation.sol:
        3: pragma solidity >= 0.8.0;
    contracts/ interfaces/ILens.sol:
        3: pragma solidity>= 0.8.0;
    contracts/interfaces/IObserver.sol:
        3: pragma solidity >= 0.8.0;
    contract s / interfaces/ IPlatformAdapter.sol:
        3: pragma solidity >= 0.8.0;
    contracts/ interfaces/ I Trader*al let - sol:
        3: pragma solidity >= 0.8.0;
    contracts/ interfaces/IUsersVau1t . sol:
        3: pragma solidity >= 0.8.0;

Recommendation :

Consider locking the pragma in all the contract.

L-06. Consider implementing two-step procedure for updating protocol addresses

Description :

Certain functions, such as setGmxObserver and setFeeReceiver, allow an address to be set in a single step. This approach can be error-prone. The critical procedures should be a two-step process.

Code Snippets :

    function setGmxObserver(address newVatue) external override ontyOwner
    {
            gmxObserver = newValue;
 
            emit SetGmxObserver(newValue) ;
    }
    function setFeeReceiver(address newFeeReceiver) external override onlyOwner {
 
                _checkZeroAddress(newFeeReceiver, "newFeeReceiver");
 
                feeReceiver = newFeeReceiver;
 
                emit FeeReceiverSet(newFeeReceiver) ;
    }

Recommendation :

The lack of a two-step procedure for critical operations leaves them error-prone. Consider adding two-step procedure on the critical functions.

L-07. Unused receive() function will lock Ether in contract

Description :

The BaseVault.sol contract contains an unused receive() function. This function is payable, meaning that it can receive Ether. However, the function does not do anything with the Ether that it receives. This means that if an user sends Ether to the contract using the receive() function, the Ether will be locked in the contract and cannot be recovered.

Code Snippets :

    receive() external payable {}

Recommendation :

The following mitigation steps can be taken to address this vulnerability:

  1. Remove the receive() function from the contract.
  2. If the receive() function is needed, then modify the function to do something with the Ether that it receives.

L-08. Use Ownable2StepUpgradeable rather than OwnableUpgradeable

Description :

The BaseVault.sol, DynamicValuation.sol, and ContractsFactory.sol contracts all use the Ownable contract from OpenZeppelin. However, the Ownable contract does not prevent the contract ownership from mistakenly being transferred to an address that cannot handle it (e.g. due to a typo in the address). This could result in the contract being taken over by an attacker.

Code Snippets :

    BaseVault.sol:5:import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradable/access/OwnableUpgradeable.sol";
    DynamicValuation.sol:7:import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradable/access/OwnableUpgradeable.sol";
    ContractsFactory.sol:5:import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradable/access/OwnableUpgradeable.sol";

Recommendation :

Use the Ownable2Step or Ownable2StepUpgradeable contract instead of the Ownable contract. These contracts prevent the contract ownership from being transferred to an address that cannot handle it

I-01. Removal of Commented-Out Code for Better Code Quality

Description :

The contracts TraderWallet.sol and Lens.sol contains lines of code that are commented out but serve no purpose. Commented-out code segments do not contribute to the functionality of the contracts but can clutter the codebase, making it harder for developers to understand the actual logic of the contracts. These lines of code should be removed to improve the overall code quality and clarity of the contracts.

Code Snippets :

  //     address tokenOut
  // ) external view returns (uint256 amountIn) {
  //      return iGmxReader(gmxReader).getMaxAmountIn(address(gmxVault), tokenIn, tokenOut)
  //}
 
    // @notice Returns amount out after fees and the fee arnount
    // @param tokenln The address of input token
    // @param tokenOut The address of output token
    // @param amount In The amount of tokenln to be swapped
    // @return amountOutAfterFees The amount out after fees,
    // @return feeAmount The fee amount in terns of tokenOut
    // function getAmuntOut(
    //       tokenln,
    //       token()ut) ;
    // address tokenln,
    // address tokenOut,
    // uint256 amount In
    // external view returns (uint256 amountOutAfterFees, uint256 feeAmount) {
    // return
    //    IGmxReader (gmxReader).getAmountOut (
    //      address (gmxVault),
    //      uint256 feeAmount){

Recommendation :

To enhance the code quality and maintainability of the contracts, it is recommended to remove the commented-out code segment

I-02. open TODOs

Description :

The BaseVault, IGmxAdapter and few other contracts contain TODO comments. These comments indicate that there are open questions or issues that need to be resolved before the contracts can be deployed.

Code Snippets :

contracts/BaseVault.sol :
    169
    170: // todo consider adding TimeLock for this function?
    171  /// @notice Withdrawing tokens in the emergency case from the contract
 
 
    contracts/adapters/gmx/interfaces/IGnxAdapter.sol:
    62  /// @notice Returns data for open position
    63:  // todo
    64  function getPosition(uint256) external view returns (uint256[] memory);
 
tests/ adapters/uniswapV3- adapter.ts :
    959
    960: // todo add tests with ratio < le18
    961

Recommendation :

Remove the TODO comments or resolve them.

I-03. Variable names don't follow the Solidity style guide

Description :

The Solidity style guide specifies that constant variable names should be in CONSTANT_CASE, which means that each word should be capitalized and underscores should be used to separate the words. However, there are multiple instances in the contract where this pattern is not followed.

Code Snippets :

    DynamicValuation.sol:28: uint8 public constant override decimals = 30;
    Observers/GMXObserver.sol:14:   IGmxVault public constant gmxVault =
    Observers/GMXObserver.sol:16: IGmxReader public constant gaxReader =
    Observers/GMXObserver.sol:18:   IGmxOrderBook public constant gxOrderBook =
    Observers/GMXObserver.sol:22: uint8 public constant override decimals = 30;
    adapters/uniswap/UniswapV3Adapter.sol:35: IUniswapV3Router public constant uniswapV3Router
    adapters/uniswap/UniswapV3Adapter.sol:37:   IQuoterV2 public constant quoter =
    adapters/uniswap/UniswapV3Adapter.sol:40:   uint256 public constant ratioDenominator = 1e18;
    adapters/uniswap/UniswapV3Adapter.sol:43: uint128 public constant slippageAllowanceMax = 3e17
    // 30%
 
    adapters/uniswap/UniswapV3Adapter.sol:46: uint128 public constant slippageAllowanceMin = 1e15;
    // 0.1%
    adapters/gmx/GMXAdapter.sol:21: address internal constant gmxRouter =
    adapters/gmx/GMXAdapter.sol:23: address internal constant gmxPositionRouter =
    adapters/gmx/GMXAdapter.sol:25: IGmxVault internal constant gxVault =
    adapters/gmx/GMXAdapter.sol:27: address internal constant gmxOrderBook =
    adapters/gmx/GMXAdapter.sol:29: address internal constant gmxOrderBookReader =
    adapters/gmx/GMXAdapter.sol:31: address internal constant gmxReader =
    adapters/gmx/GMXAdapter.sol:35: uint256 private constant ratioDenominator = 1e18;
    adapters/gmx/GMXAdapter.sol:38: uint256 public constant slippage = 1e17; // 10%

Recommendation :

Rename the variables to follow the Solidity style guide.

I-04. Using unnamed mappings

Description :

The contracts use unnamed mappings, which can make it difficult to understand the purpose of each mapping. Consider using named mappings to make it easier to understand the purpose of each mapping. This can make the code less readable and maintainable.

Code Snippets :

DynamicValuation.sol:35:    mapping (address => OracleData) private _chainlinkOracles;
TraderWallet.sol:39:    mapping(address => mapping(address => bool)) public override gmxShortPairs;
ContractsFactory.so1:28:    mapping(address => bool) public override isTraderWallet;
ContractsFactory.sol:31:    mapping(address =>  bool) public override isUsersVault;
ContractsFactory.sol:33:    mapping(address => bool) public override allowed Investors;
ContractsFactory.sol:34:    mapping(address => bool) public override allowedTraders;
UsersVault.sol:47:  mapping (uint256 => uint256) public assets PerShareXRound;
UsersVault.sol:49:  mapping(address => UserData) private userData;
UsersVault.sol:50:  mapping (uint256 => uint256) private underlyingPriceXRound;

Recommendation :

Rename the mappings to make it clear what the purpose of each mapping is.

l-05. Consider adding emergency-stop functionality

Description :

The contracts do not have a global emergency halt mechanism. This means that if there is a hack or other emergency, the only way to stop the protocol is to pause individual contracts one-by- one. This can be time-consuming and stressful, and it may not be possible to pause all of the contracts before the hack is complete. Similar Issue can be found here.

Recommendation :

• Add a global emergency halt mechanism to the contracts. • Document the emergency halt mechanism so that users know how to use it.

l-06. Imports could be organized more systematically

Description :

The imports in the contracts are not organized in a systematic way. The contract's interface should be imported first, followed by each of the interfaces it uses, followed by all other files. However, the imports in the contract are not in this order. Similar findings in a Code4rena Contest.

Code Snippets:

import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
import {ReentranceGuardUpgradeable} from "@openzeppelin/contracts-upgradeable/security/ReentranceGuardUpgradeable.sol";
import {IERC20Metadata} from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol";
import (GMXAdapter) from "./adapters/gmx/GMXAdapter.sol";
import {Events} from "./interfaces/Events.sol";
import {Errors} from "./interfaces/Errors.sol";
import {IAdapters Registry} from "./interfaces/IAdaptersRegistry.sol";
import {IContractsFactory} from "./interfaces/IContractsFactory.sol";
import {IDynamicValuation} from "./interfaces/IDynamicValuation.sol";
import {IAdapter} from "./interfaces/IAdapter.sol";
import {IBaseVault} from "./interfaces/IBaseVault.sol";
import {16mxVault} from "./adapters/gmx/interfaces/IGmxVault.sol";
import {IERC20} from "@openzeppelin/contracts/interfaces/IERC20.sol";

Recommendation :

• Use namespaces, import groups, and aliases to help organize the imports. • Organize the imports in a systematic way.

I-07. Event is not properly indexed

Description :

Multiple events in the Events.sol contract are not indexed efficiently. Each event only indexes one field, when it could index three fields. This means that off-chain tools that parse events will have to scan more data to find the information they need.

Code Snippets :

event SharesClaimed(
    uint256 round,
    uint256 shares,
    address caller,
    address receiver
);
event AssetsClaimed(
    uint256 round,
    uint256 assets,
    address owner,
    address receiver
);
event UsersVaultRolloverExecuted(
    uint256 round,
    uint256 underlyingTokenPerShare,
    uint256 sharesToMint,
    uint256 sharesToBurn,
    int256 overallProfit,
    uint256 unusedFunds
);
event OperationExecuted(
    uint256 protocolId,
    uint256 timestamp,
    bool replicate,
    string target,
    uint256 walletRatio
);
event TraderWallet RolloverExecuted(
    uint256 timestamp,
    uint256 round,
    int256 traderProfit,
    uint256 unusedFunds
);

Recommendation :

It can be mitigated by adding indexing to the remaining fields in the above events.

I-08. Typos

Description :

The comment on line 10 of the IAdapter.sol contract contains a typo. The word "signature" is misspelled as "signatura".

Code Snippets :

// signatura of the funcion

Gas Optimizations

G-01. Explicitly initializing variables with their default values

wastes gas.

Description :

The DynamicValuation.sol, GMXAdapter.sol, Lens.soli ContractsFactory.soI, and UsersVauIt.soI contracts all contain variables that are initialized with their default values. For example, the UsersVault.sol contract contains collateralDelta variable, which is initialized to O.

The default value of a uint256 variable is O, so explicitly initializing collateralDeIta with O is unnecessary. This wastes gas, as the Solidity compiler has to store the value O in memory even though it is not actually used.

Code Snippets :

DynamicValuation.sol:138:  for(uint256 i = 0 ; i < token.length; i++) {
adapters/gmx/GMXAdapter.sol:83: if (uint256(traderOperation.operationId) == 0) {
adapters/Lens.sol:236:     for (uint256 i = 0; i < lengthShorts; ++i) {
adapters/Lens.sol:259:  for (uint256 i = 0; i < totallength; ++i) {
ContractsFactory.sol:75:    for (uint256 i = 0; i < length; ++i) {
ContractsFactory.sol:108:   for (uint256 i = 0; i < length; ++i) {
UsersVault.sol:753: for (uint256 i = 0; i < tokens.length; ++i) {
UsersVault.sol:756: for (uint256 i = 0; i < positions.length; ++i) { uint256 collateralDelta = 0;

Recommendation :

Declare variables without initializing them. number; instead of uint number = 0;

G-02. Use assembly to check for address(0).

Description :

We can use uint The BaseVault.sol contract contains the -checkZeroAddress() function, which checks if the supplied address is not a 0-address. However, the current implementation of the function is not the most gas efficient. By using assembly, the function can be made more gas efficient.

Code Snippets :

function _checkZeroAddress(
    address _variable,
    string memory _message
) internal pure {
    if (_variable == address (0)) revert ZeroAddress({target: message});
}

G-03. Caching the array length outside a loop

Description :

The DynamicValuation.sol, TraderWallet.sol, Observers/ GMXObserver.sol, adapters/gmx/GMXAdapter.sol, adapters/ Lens.sol, ContractsFactory.sol, and UsersVault.sol contracts all contain loops that iterate over arrays. In each loop, the length of the array is read. However, the length of the array does not change during the loop, so it can be cached outside the loop. This would save 3 gas per iteration, or a total of 27 gas per loop.

Code Snippets :

    DynamicValuation.sol:138:   for (uint256 i 0; i < tokens.length; ++i) { =
    TraderWallet.sol:130:   for (uint256 i; i < collateralTokens.length; ) {
    TraderWallet.sol:163:   for (uint256 i; i <_tokens.length; ) {
    Observers/GMXObserver.sol:91:   for (uint256 i; i<istong.length; ) {
    adapters/gmx/GMXAdapter.sol:815:    for (uint256 i; i < allowedTradeTokens.length; ) {
    adapters/Lens. sol:236: for (uint256 i 0; i < lengthShorts; ++i)
    adapters/Lens.sol:243:  for (uint256 i = lengthshorts; i < totallength; ++i) {
    adapters/Lens.sol:259:  for (uint256 i 0; i < totallength; ++i) {
    ContractsFactory.sol:75:    for (uint256 i = 0; i < length; ++i) {
    ContractsFactory.sol:108:   for (uint256 i = 0; i < length; ++i) {
    ContractsFactory.sol:199:   for (uint256 i; i<_tokens.length; ) {
    UsersVault.sol:672: for (uint256 i = 1; i < tokens.length; ++i) {
    UsersVault.sol:753: for (uint256 i = 0; i < positions.length; ++i) {

Recommendation :

The following code snippet shows how to cache the array length outside a loop:

    uint256 length = tokens. length;
    for (uint256 i z O; i < length; ++i) {
        // do something with the token at index i

G-04. Redundant zero-address check in the removeTrader()

function

Description :

The removeTrader() function in the UsersVauIt.soI contract contains a zero-address check. This check ensures that the address passed to the function is not a zero address. However, this check is redundant, as the add Trader() function already performs a zero-address check before adding an address to the allowed Traders array.

Code Snippets :

https://qithub.com/Kunji-Finance/KF-Contract/blob/29e4fb07cb3a4eOeba477b8a7504846c2a600adf/contracts/ContractsFactory.sol#L124 (opens in a new tab)

Recommendation :

Remove the zero-address check from the removeTrader() function.

OverviewPost Audit Conclusion