INTRODUCTION

BlockApex (Auditor) was contracted by VoirStudio (Client) for the purpose of conducting a Smart Contract Audit/Code Review. This document presents the findings of our analysis which started from 26th Jan 2022. 

Scope

The git-repository shared was checked for common code violations along with vulnerability-specific probing to detect major issues/vulnerabilities. Some specific checks are as follows:

Project Overview

Unipilot is an automated liquidity manager designed to maximize ”in-range” intervals for capital through an optimized rebalancing mechanism of liquidity pools. Unipilot V2 also detects the volatile behavior of the pools and pulls liquidity until the pool gets stable to save the pool from impairment loss.

System Architecture

The protocol is built to support multiple dexes (decentralized exchanges) for liquidity management. Currently it supports only Uniswap v3’s liquidity. In future, the protocol will support other decentralized exchanges like Sushiswap (Trident). The architecture is designed to keep in mind the future releases.

The protocol has 6 main smart contracts and their dependent libraries.

UnipilotActiveFactory.sol

The smart contract is the entry point in the protocol. It allows users to create a vault if it's not present on protocol. Nevertheless Active vaults can only be created by governance.

UnipilotPassiveFactory.sol

The smart contract is the entry point in the protocol. It allows users to create a vault if it's not present on protocol. However passive vaults can be created by anyone.

UnipilotActiveVault.sol

Vault contract allows users to deposit, withdraw, readjustLiquidity and collect fees on liquidity. It mints an LPs to its users representing their individual shares. It also has a pullLiquidity function if liquidity is needed to be pulled.

UnipilotPassiveVault.sol

PassiveVault contract allows users to deposit, withdraw, readjustLiquidity and collect fees on liquidity. It mints an LPs to its users representing their individual shares.

UnipilotStrategy.sol

The smart contract to fetch and process ticks’ data from Uniswap. It also decides the bandwidth of the ticks to supply liquidity.

UnipilotMigrator.sol

The smart contract aids to migrate users liquidity from other Uniswap V3 Liquidity Optimizer Protocols to Unipilot V2 Protocol.

Methodology & Scope

The codebase was audited in an iterative process. Fixes were applied on the way and updated contracts were examined for more bugs. We used a combination of static analysis tool (slither) and Automated testing tool (Foundry) which indicated some of the critical bugs in the code. We also did manual reviews of the code to find logical bugs, code optimizations, solidity design patterns, code style and the bugs/ issues detected by automated tools.

Privileged Roles

In a production environment, the unipilot protocol sets the address for a governance that exercises a privileged position over the factory and vault contracts in the system. The governor has the power to initiate a transfer of the governor role to a new address.

The governance address is capable of executing a set of actions including:

• Controlling the various whitelists for vaults in the factory contract
• Toggle a vault as whitelisted
• Set unipilot details to update addresses of Strategy and Index Fund contracts along with the Index Fund Fee Percentage 
• Set an operator address

The operator address has following activities it can be used for:

• Pull liquidity for a vault contract from its position on a Uniswap V3 Pool
• Call Readjust Liquidity to burn from and mint all liquidity back to a less volatile position on the Uniswap V3 Pool.

AUDIT REPORT

Executive Summary

The analysis indicates that some of the functionalities in the contracts audited are working properly.

Our team performed a technique called “Filtered Audit”, where the contract was separately audited by two individuals. After their thorough and rigorous process of manual testing, an automated review was carried out using Mythril, MythX, Surya and Slither. All the flags raised were manually reviewed and re-tested. 

Our team found: 

Findings

Critical risk issues

1. Deposit ether with zero value.

Description: 

If a deposit of tokens with ether as one is made, all the while the contract has pulled liquidity into the vault, the user making a deposit with 0 value can use the vault’s ether to execute a successful transaction.

Remedy:

Introduce a RefundETH() function that ensures a proper transfer of value either be in ethers or an ERC compatible version (WETH)

Status: 

Fixed

High risk issues

1. init() should have only once check

Description: 

Calling init() any time after the first time it has been called, can lead to permanent loss of position at uniswap V3.

function init() external onlyGovernance {
        int24 _tickSpacing = tickSpacing;
        int24 baseThreshold = _tickSpacing * getBaseThreshold();
        (, int24 currentTick, ) = pool.getSqrtRatioX96AndTick();

        int24 tickFloor = UniswapLiquidityManagement.floor(
            currentTick,
            _tickSpacing
        );

        ticksData.baseTickLower = tickFloor - baseThreshold;
        ticksData.baseTickUpper = tickFloor + baseThreshold;

        UniswapLiquidityManagement.checkRange(
            ticksData.baseTickLower,
            ticksData.baseTickUpper
        );
    }

Remedy:

There should be an onlyOnce modifier or a variable handling (as locks) that ensures init is never called again.

Status:

Fixed as per BlockApex recommendation.

2. Pulled liquidity rationale

Description: 

Considering the scenario where the vault has pulled Liquidity with the intention of depositing it back to Uniswap V3 when the pool is relatively less volatile; the smart contract code assumes a rational behavior to override checkDeviation modifier by manually modifying ticks through strategy using onlyGovernance functions. But the code does not guarantee any logic to push liquidity back to v3 in a safe manner

Remedy:

Debatable.

Status:

Acknowledge

Developer’s response:

Governance will manually update deviation in case of price volatility.

Medium risk issues

1. Unoptimized user liquidity is held in a vault.

Description: 

In Active vaults if a pool is created on 1-X ratio on Uniswap V3 and a user makes a deposit with 1-1 ratio through the vault, the vault is found to hold the remaining amount of the token initially at the price of X, giving users a complete share with proportion of the deposited amount while the remaining amount of user sits inactive within the vault.

Remedy: 

The vault must ensure that the amounts provided by a user are equal to the amounts of tokens actually deposited on a Uniswap pool.

Status:

Acknowledged

Developer’s response:

Added rearrange liquidity method.

2. Deposit should have non reentrant checks placed in both vaults

Description: 

The deposit() function in both vaults contract, that is, the UnipilotActiveVault and the UnipilotPassiveVault does not contain a non-reentrant modifier which is a standard practice to prevent any adversarial intent related to the reentrancy exploits.

Remedy:

A good industry practice requires that the deposit() function executes with a non-reentrant modifier; this modifier should be placed to ensure security as the deposit marks external calls through linked libraries to deposit values to the Uniswap V3.

Status:

Fixed as per BlockApex recommendation. 

Low-risk issue

1. Zero Address checks not placed in contract constructors()

Description: 

Constructor() does not contain checks for accepting params of address type whether an address is zero or not.

Remedy:

Since the constructor accepts an address from an argument, there should be a zero address check to ensure the functionality. These checks should be placed in almost every contract: Unipilot Factory , Unipilot Strategy , Unipilot Migration etc.

Status:

Acknowledged

2. Safecast in UnipilotPassiveVault.sol line 241 & 242 for `swapPercentage`

Description: 

In UnipilotPassiveVault.sol the readjustLiquidity() reads the swapPercentage variable in Line 238 of the contract to calculate the amountSpecified variable in Lines 241-242, this Math is unsafe as the calculation is executed with different types for each param. 

    if (amount0 == 0 || amount1 == 0) {
            bool zeroForOne = amount0 > 0 ? true : false;

            (, , , , uint8 swapPercentage) = getProtocolDetails();

            int256 amountSpecified = zeroForOne
                ? int256(FullMath.mulDiv(amount0, swapPercentage, 100))
                : int256(FullMath.mulDiv(amount1, swapPercentage, 100));

            pool.swapToken(address(this), zeroForOne, amountSpecified);
        }

Remedy:

Use safecasting for all type variables on lines 232-233 to ensure a seamless execution of the desired arithmetics.

Status:

Acknowledged

Developer’s response:

Percentage calculation is correct with this method. (Well tested)

3. Storage Layout is unoptimized.

Description:

Variable tight packing is strongly recommended for both vaults and factories in state variable declaration as the contracts are composed in order that is gas-consuming.

Remedy:

A solidity design pattern ‘Tight variable Packing’ ensures that the smart contract is optimized to execute efficiently within the EVM environment. 

Status:

Fixed as per BlockApex recommendation.

Status:

Slots are arranged bitwise now.

4. Check max cap for indexFundPercentage and swapPercentage.

Description: 

In setUnipilotDetails() the param indexFundPercentage is checked to receive a lowest value greater than zero.

  function setUnipilotDetails(
        address _strategy,
        address _indexFund,
        uint8 _indexFundPercentage
    ) external onlyGovernance {
        require(_strategy != address(0) && _indexFund != address(0));
        require(_indexFundPercentage > 0);
        strategy = _strategy;
        indexFund = _indexFund;
        indexFundPercentage = _indexFundPercentage;
    }

Remedy:

Ensure a check placed to bound the maximum value for the indexFundPercentage

Status:

Fixed

5. Deposit() function optimization.

Description: 

In UnipilotActiveVault.sol and UnipilotPassiveVault.sol, Users can call deposit() with zero amounts of both tokens and the function executes until the end.

function deposit(
        uint256 amount0Desired,
        uint256 amount1Desired,
        address recipient
    )
        external
        payable
        override
        returns (
            uint256 lpShares,
            uint256 amount0,
            uint256 amount1
        )
    {
        address sender = _msgSender();

        (lpShares, amount0, amount1) = pool.computeLpShares(
            true,
            amount0Desired,
            amount1Desired,
            _balance0(),
            _balance1(),
            totalSupply(),
            ticksData
        );

Remedy:

Function should check for zero value for both input args in the deposit() function in vaults contract.

Status:

Fixed

6. toggleWhitelistAccount() redundant.

Description:

toggleWhitelistAccount() can toggle the gov off in a redundant call of the same function to whitelist itself back. 

 function toggleWhitelistAccount(address _address) external onlyGovernance {
        require(_address != address(0));
        isWhitelist[_address] = !isWhitelist[_address];
    }

Remedy:

Ensure the address is checked to not allow governance to be toggled for whitelist.

Status:

Acknowledged.

7. _SortWeth() optimization

 function _sortWethAmount(
        address _token0,
        address _token1,
        uint256 _amount0,
        uint256 _amount1
    )
        private
        pure
        returns (
            address tokenAlt,
            uint256 altAmount,
            address tokenWeth,
            uint256 wethAmount
        )
    {
        // (
        //     address tokenA,
        //     address tokenB,
        //     uint256 amountA,
        //     uint256 amountB
        // ) = _token0 == WETH
        //         ? (_token0, _token1, _amount0, _amount1)
        //         : (_token0, _token1, _amount1, _amount0);

        (tokenAlt, altAmount, tokenWeth, wethAmount) = _token0 == WETH
            ? (_token1, _amount1, _token0, _amount0)
            : (_token0, _amount0, _token1, _amount1);
    }

Description: 

This function’s logic can be concise. The remedy, tested against the required logic, is mentioned as a code snippet in the screenshot above.

Status:

Acknowledged

Informatory issues

1. No NatSpec documentation

Description: 

NatSpec documentation is an essential part of smart contract readability; it is therefore advised that all contracts and following files contain proper explanatory commenting;

• UnipilotActiveVault.sol
• UnipilotPassiveVault.sol
• UnipilotMigrator.sol

Status:

Acknowledged

Developer’s response:

Completed netspec for all contracts.

2. Mark token0 and token1 as immutable in UnipilotActivevault.sol and UnipilotPassiveVault.sol

Description: 

State variables containing the address of tokens should be marked as immutable as the constructor locks the values for each after deployment.

Status:

Partial Fixed.

Developer’s response:

PassiveVault used immutables however active vaults don’t due to size issues.

3. onlyGovernance() modifier in passive vault contract

Description: 

The onlyGovernance modifier in the Passive Vault contract remains unused within the contract.

Status:

Fixed as per BlockApex recommendation.

4. _WETH address should be hardcoded before production wherever necessary

Description: 

Address of the WETH token contract is passed as a constructor param in both Factories which can be optimized by hardcoding the actual address of _WETH in the final deployment of the production environment.

constructor(
        address _pool,
        address _unipilotFactory,
        address _WETH,
        address governance,
        string memory _name,
        string memory _symbol
    ) ERC20Permit(_name) ERC20(_name, _symbol) {
        WETH = _WETH;
        unipilotFactory = IUnipilotFactory(_unipilotFactory);
        pool = IUniswapV3Pool(_pool);
        token0 = IERC20(pool.token0());
        token1 = IERC20(pool.token1());
        fee = pool.fee();
        tickSpacing = pool.tickSpacing();
        _operatorApproved[governance] = true;
    }

Status:

Acknowledged

5. Factory function createVault() optimized version

Description: 

createVault() is found to be optimized if it executes in the following recommended pattern: 

• First check pool on v3 - if returns true then check vault exists - if returns true then return from function
• If pool returns false - create & initialize pool then create vault

Current Implementation:

function createVault(
        address _tokenA,
        address _tokenB,
        uint24 _fee,
        uint160 _sqrtPriceX96,
        string memory _name,
        string memory _symbol
    ) external override onlyGovernance returns (address _vault) {
        require(_tokenA != _tokenB);
        (address token0, address token1) = _tokenA < _tokenB
            ? (_tokenA, _tokenB)
            : (_tokenB, _tokenA);
        require(vaults[token0][token1][_fee] == address(0));
        address pool = uniswapFactory.getPool(token0, token1, _fee);

        if (pool == address(0)) {
            pool = uniswapFactory.createPool(token0, token1, _fee);
            IUniswapV3Pool(pool).initialize(_sqrtPriceX96);
        }

        _vault = address(
            new UnipilotActiveVault{
                salt: keccak256(abi.encodePacked(_tokenA, _tokenB, _fee))
            }(pool, address(this), WETH, governance, _name, _symbol)
        );

        isWhitelist[_vault] = true;
        vaults[token0][token1][_fee] = _vault;
        vaults[token1][token0][_fee] = _vault; // populate mapping in the reverse direction
        emit VaultCreated(token0, token1, _fee, _vault);
    }

Status:

Acknowledged

6. Assignment of Params in order to receive the function signature.

Description: 

In all four contracts of vault and factory the constructor receives arguments in order which is out-of-sync to the one being assigned, reducing the code readability. Ensure param values and actual assignments are in sync for better code readability.

constructor(
        address _pool,
        address _unipilotFactory,
        address _WETH,
        address governance,
        string memory _name,
        string memory _symbol
    ) ERC20Permit(_name) ERC20(_name, _symbol) {
        WETH = _WETH;
        unipilotFactory = IUnipilotFactory(_unipilotFactory);
        pool = IUniswapV3Pool(_pool);
        token0 = IERC20(pool.token0());
        token1 = IERC20(pool.token1());
        fee = pool.fee();
        tickSpacing = pool.tickSpacing();
        _operatorApproved[governance] = true;
    }

Status:

Acknowledged

7. Order of functions as in solidity Style Guide

Description: 

Receive() and Fallback() should be moved on top, below constructor; following the solidity design patterns

Status:

Fixed

8. uint256 can be cheaper than uint8

Description: 

Uint8 is proved to be more costly than uint256 variables in a number of scenarios, where a better and optimized variable packing for uint8 variables is recommended or replaced with uint256/ uint64/ uint24 type vars.

Status:

Fixed

9. pullLiquidity() is vulnerable in its current execution

Description: 

The pullLiquidity(address _recipient) method is vulnerable to some extent, holding potential for mal-intent or permanent loss of value. Checking for the address argument as not another whitelisted vault can ensure no accidental and permanent loss of tokens happen.

Status:

Pending

Developer’s response:

Vaults will be whitelisted only for the execution of pull liquidity (when needed) soon after execution that vault will be blacklisted in order to avoid accidentally sending tokens to other active vaults.

10. Spelling mistakes in function signatures

Description: 

In the UnipilotMigrator.sol file,
migrateUnipilotLiquididty() and  _refundRemainingLiquidiy() are spelled wrong, causing readability issues as well as creating the wrong function signature.

Status:

Fixed 

11. Mark private functions as internal

Description: 

In the UnipilotMigrator.sol file,
_sortWethAmount() and _addLiquidityUnipilot() are private, which are gas costly.

Status:

Fixed as per BlockApex recommendation.

DISCLAIMER 

The smart contracts provided by the client for audit purposes have been thoroughly analyzed in compliance with the global best practices till date w.r.t cybersecurity vulnerabilities and issues in smart contract code, the details of which are enclosed in this report. 

This report is not an endorsement or indictment of the project or team, and they do not in any way guarantee the security of the particular object in context. This report is not considered, and should not be interpreted as an influence, on the potential economics of the token, its sale or any other aspect of the project. 

Crypto assets/tokens are results of the emerging blockchain technology in the domain of decentralized finance and they carry with them high levels of technical risk and uncertainty. No report provides any warranty or representation to any third-Party in any respect, including regarding the bug-free nature of code, the business model or proprietors of any such business model, and the legal compliance of any such business. No third-party should rely on the reports in any way, including for the purpose of making any decisions to buy or sell any token, product, service or other asset. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project.

Smart contracts are deployed and executed on a blockchain. The platform, its programming language, and other software related to the smart contract can have its vulnerabilities that can lead to hacks. The scope of our review is limited to a review of the Solidity code and only the Solidity code we note as being within the scope of our review within this report. The Solidity language itself remains under development and is subject to unknown risks and flaws. The review does not extend to the compiler layer, or any other areas beyond Solidity that could present security risks.

This audit cannot be considered as a sufficient assessment regarding the utility and safety of the code, bug-free status or any other statements of the contract. While we have done our best in conducting the analysis and producing this report, it is important to note that you should not rely on this report only - we recommend proceeding with several independent audits and a public bug bounty program to ensure security of smart contracts.

INTRODUCTION

BlockApex (Auditor) was contracted by Dafi Protocol (Client) for the purpose of conducting a Smart Contract Audit/Code Review. This document presents the findings of our analysis which took place from 16th Dec 2021 to 14th Jan 2022. 

Scope

The git-repository shared was checked for common code violations along with vulnerability-specific probing to detect major issues/ vulnerabilities. Some specific checks are as follows:

Project Overview

A comprehensive explanation of Staking module V2 for the Dafi Protocol is present in the official documentation and can be viewed at: https://docs.dafiprotocol.io/super-staking/super-staking-v2

Dafi Protocol Super Staking V2 is an update to the V1 module released earlier this year (2021) in the month of July. Super Staking V2 claims to offer a more stable APY rate and enhanced distribution of dDAFI rewards by modifying the math behind reward calculation to rather depend only on accumulated amounts of reward every time demand factor changes to calculating the rewards of users as the sum of all rewards in the past adjusted to the latest demand factor. 

Aside from the new reward formula, V2 also holds a couple of security improvements where demand factor is now enumerated using the latest price and is fortified by introduction of a delay i.e. a variance tolerance mechanism to ultimately prevent a sudden change in price. This new demand factor is supported by a TWAP calculation to make the price curve less steeper. Although strict monitoring is required, in cases of hackable oracle feeds, the protocol is able to recover from any kind of exploits by updating to an entirely new oracle service.

System Architecture

Codebase:

The system consists of 5 main smart contracts (namely: Staking Manager V2, Staking Database, Rebase Engine, Network Demand, Token Pool) and supplied with external data through 2 more contracts of Price Feeds and TVL Feeds.

Note: All of the contracts mentioned above contain onlyOwner modifiers to add, set and/or update configurations.

SHA-256 fingerprints of Contracts included:

• Staking Manager V2 : ./StakingManagerV2.sol
  759bb0b2eb3f3d18b1d835f1b1b89733613ff6d1  
• Staking Database : ./StakingDatabase.sol
  e4bb62c5ac3cb28c6f875af06158c4bff448d7410f84e147e8de08fcd72667d0  
• Rebase Engine : ./rebase engine/RebaseEngine.sol
  61ab614f761737a42134cdaef991a0f7d5dd7ca1244993b2ba8bb277217432c4  
• TokenPool : ./TokenPool.sol
  62c376c91256035f1601be81f9ce07d069acd1ccae3c00f330507f8618aac986  
• Network Demand : ./network demand/NetworkDemand.sol
  bf71534344959ba076b03b0ac83f34a0ac20fa2ed2986bd7d85889e2b56fbb30  
• Price Feeds : ./network demand/PriceFeeds.sol
  75b13b5e14cd516aaf65ac82526e7a1c317eebb57d306d6a0bbff7033317c5c7  
• TVL Feeds : ./network demand/TVLFeeds.sol
  119130bd33ef19f8e901e7c81f6d3089d7c8df8f98586c2c1881c71021dcded2  

Static-Analysis summary

Methodology & Scope

Audit log

Manual Review: The audit launched with a recon phase where a manual code review was conducted to clarify the layers of understanding of the complexities and the general flow of the program. We started by reviewing the two main contracts against common solidity flaws. After the reconnaissance phase we wrote unit-test cases to ensure that the functions are performing their intended behavior. Then we began with the line-by-line manual code review.

Property Testing: From the reconnaissance, a handful of properties were also extracted and labeled as Invariants. In the following days of the audit procedure, the invariants were thoroughly tested against a setup flow of Dafi Staking V2 to ensure each of them held its proper definition.

AUDIT REPORT

Executive Summary

The analysis indicates that some of the functionalities in the contracts audited are poorly-secured. 

Our team performed a technique called “Filtered Audit”, where the contract was separately audited by two individuals. After their thorough and rigorous process of manual testing, an automated review was carried out using Mythril, MythX, and Slither. All the flags raised were manually reviewed and re-tested. 

Our team found: 

Findings

Below is the summary of our findings from the complete audit. This includes any flags raised from the manual/automated code review, behavioral/scenario testing and the properties tested for formal verification. 

Critical-risk

CR-1. Oracle Integrity

Status: Passed

Description

In recent events, Oracles and external data sources have been manipulated to the adversary's benefit. We started off with our focus on the integrity of Oracles (Chainlink in this case) in order to make sure that the data imported into the contracts are not compromised.

The Chainlink oracle library method was tested against the following parameters of a standard audit technique.

• Third-party (data source) function calls
• Data type intact for functions’ return values
• Access modifiers and state variables (if any)

function getThePrice() public view override returns (uint) {
        (
        ,
        int price,
        ,
        ,
        ) = priceFeed.latestRoundData();
        return uint(price);
    }

Suggestion: It is advised that a more tested and bonafide data source of TWAPs be considered to make Chainlink’s oracle integrity and reliability as the most optimal.

High-risk

HR-1. MAX_DAFI is always greater than totaldDafiDistributed

Status: Failed

Description

If a program is marked as “ended” before the preset ending duration, that is to say, the elapsed time of the program is less than the program duration, users get revert transactions for staking into the ended program (as seen in the testnet event and confirmed over a fuzzing test scenario with the reason of MAX_DAFI being calculated less than total DAFI distributed till the ending timestamp). This property does not hold at fuzzed inputs;

function test_ConfirmMaxDafiOverflowFuzz(
        uint256 md,
        uint32 pd,
        uint32 bn
    ) public {
        SetupContracts caller = dapping.users(0).setupContract();

        caller.wrapSEtStakingParams(1, md + 1, pd + 1);

        hevm.warp(block.number + bn);

        caller.wrapRebasePool();
        caller.database().getPool();

        uint256 MaxDafi = caller.rebaseEngine().MAX_DAFI();
        uint256 TdDafidist = caller.rebaseEngine().totaldDAFIDistributed();
        
        assertGt(MaxDafi, TdDafidist, 'maxDafi < tdDafiDist; program ended; dDafi overflows');
        
        emit log_named_uint('value of MaxDafi', MaxDafi);
        emit log_named_uint('value of TdDafiDist', TdDafiDist);

    }

Suggestion: It is advised that program duration should always be maintained greater than the time elapsed by supplying checks that assure it.

        if (database.isprogramEnded() && pool.lastUpdatedon > database.getProgramEndedAt()) {
            return;
        } else if (database.isProgramEnded() && pool.lastUpdatedon < database.getProgramEndedAt()) {
            maxTimestampForcalc = database.getProgramEndedAt();
        } else {
            maxTimestampForCalc = block.timestamp;
        }

HR-2. Reward Pool should not have balance if the program has ended and all users have unstaked

Status: Failed

Description

This property claims that the reward pool should be empty after the program has ended and that all the users have unstaked. 

HR-2(a). “markProgramEnded()” by owner’s mistake can lock funds of the reward pool

In case of human error if the program is marked ended the reward pool retains an amount of tokens unclaimed against the percentage of tokens staked in first place.

function test_RewardpoolUnemptyAfterProgramEnded public {
        SetupContracts caller = dapping.users().setupContract();
        hevm.warp (block. timestamp + 5000);
        
        
        caller.wrapSetStakingParams (1, 1000 ether, 12);
        
        uint256 distPoolBalanceBefore = caller.stakingManager().distributionPool(). balance();
        
        hevm.warp(block.timestamp + 99000);
        caller.wrapstake ( 100);
        
        caller.wrapMarkProgramEnded();
        
        hevm warp (block timestamp + 99000);
        caller.wrapunstake(100);
        
        uint256 distPool BalanceAfter = caller.stakingManager().distributionPool(). balance();
        
        assertGt(distPool BalanceBefore, distPoolBalanceAfter);
        
        emit Log named uint('pool Before', distPoolBalanceBefore);
        emit Log named uint('poolAfter', distPoolBalanceAfter);
    }

Suggestion: It is advised to have a refund mechanism if program duration is ended. The refund amount of tokens should be equal to MAX_DAFI - (MDI*totalElapsedTime).

HR-3. If a program has ended, users should not be able to stake

Status: Failed

Description

In a simple scenario test, it was confirmed that Staking was allowed even if the program duration is completed and the program is marked as ended which in the understanding of the Auditor team is an (incomprehensible feature) and can be proved to be a loophole of the system in some scenarios.

Suggestion: The modifier stakeCheck should be supplied with some additional check to ensure monitoring of programs marked as ended not to allow staking.

function test_StakeAfterProgramEnded() public {
        SetupContracts caller = dapping.users().setup Contract();
        hevm.warp(block. timestamp + 500);
        
        // _ms, _md, _pd
        uint256 minStakeDays = 1;
        uint256 maxDafi = 1000 ether;
        uint32 progDuration = 12;
        
        caller.wrapsetStakingParams (minStakeDays, maxDafi, progDuration);
        
        // stakin/ unstaing multiple times for seeding
        for (uint256 i = 0; i < 3; i++) {
            hevm warp (block. timestamp + 90000);
            caller.wrapStake ( 1000000000);
            hevm warp(block. timestamp + 90000);
            caller.wrapunstake ( 1000000000);
        }
        
        caller.wrapMarkProgramEnded();
        
        // stakin/ unstaing multiple times for seeding
        for (uint256 i = 0; i < 3; i++)
            hevm.warp (block. timestamp + 90000);
            caller.wrapStake ( 1000000000);
            hevm.warp(block. timestamp + 90000);
            caller.wrapUnstake ( 1000000000);
        }
    
        assertTrue(false);
    }

Low-risk

LR-1. Reward is claimable even after program is ended

Status: Passed

Description

Regardless of the program being ended, users should be able to claim their rewards for their staked dafi tokens. This claim should not be bound by any type of time-related constraints. We checked whether this property would fail in any circumstance but it passed on all fuzzed inputs.

LR-2. Unit tests in the testing and main branch of repo at /test/v2.ts should pass

Status: Failed

Description

Unit tests are critical in proving the developer's expected intention and behavior of the working code hence the set of tests not passing entirely and partially in the testing repository code is slightly questionable.

Informatory issues and Optimizations

IR-1. Memory optimization by using uint8 in place of true/false

Description

Following the best practices and the Solidity design patterns guide and since the client code uses a good number of state variables to manage the switches for staking, unstaking and alike. It is therefore suggested by the auditing team that uint8 type variables can be replaced by the developer in place of true/false on multiple occasions to minimize the memory usage and reduce the code size as an optimization.

DISCLAIMER

The smart contracts provided by the client for audit purposes have been thoroughly analyzed in compliance with the global best practices till date w.r.t cybersecurity vulnerabilities and issues in smart contract code, the details of which are enclosed in this report. 

This report is not an endorsement or indictment of the project or team, and they do not in any way guarantee the security of the particular object in context. This report is not considered, and should not be interpreted as an influence, on the potential economics of the token, its sale or any other aspect of the project. 

Crypto assets/tokens are results of the emerging blockchain technology in the domain of decentralized finance and they carry with them high levels of technical risk and uncertainty. No report provides any warranty or representation to any third-Party in any respect, including regarding the bug-free nature of code, the business model or proprietors of any such business model, and the legal compliance of any such business. No third-party should rely on the reports in any way, including for the purpose of making any decisions to buy or sell any token, product, service or other asset. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project.

Smart contracts are deployed and executed on a blockchain. The platform, its programming language, and other software related to the smart contract can have its vulnerabilities that can lead to hacks. The scope of our review is limited to a review of the Solidity code and only the Solidity code we note as being within the scope of our review within this report. The Solidity language itself remains under development and is subject to unknown risks and flaws. The review does not extend to the compiler layer, or any other areas beyond Solidity that could present security risks.

This audit cannot be considered as a sufficient assessment regarding the utility and safety of the code, bug-free status or any other statements of the contract. While we have done our best in conducting the analysis and producing this report, it is important to note that you should not rely on this report only - we recommend proceeding with several independent audits and a public bug bounty program to ensure security of smart contracts.

INTRODUCTION

BlockApex (Auditor) was contracted by  VoirStudio  (Client) for the purpose of conducting a Smart Contract Audit/ Code Review. This document presents the findings of our analysis which started from  25th Feb 2022. 

Scope

The git-repository shared was checked for common code violations along with vulnerability-specific probing to detect major issues/vulnerabilities. Some specific checks are as follows:

Project Overview

Unipilot yield farming incentivizes Liquidity Providers to earn $PILOT tokens by staking their Unipilot LP tokens on whitelisted pools.

System Architecture

Unipilot yield farming has 1 main smart contract; UnipilotFarm.sol: which allows Liquidity Providers to earn $PILOT token, an $ALT token, or both by staking their Unipilot LP tokens. UnipilotFarm linearly distributes the $PILOT according to rewardPerBlock and rewardMultiplier.

Methodology & Scope

The codebase was audited in an iterative process. Fixes were applied on the way and updated contracts were examined for more bugs. We used a combination of static analysis tool (slither) and testing framework (hardhat) which indicated some of the critical bugs like reentrancy in the code. We also did manual reviews of the code to find logical bugs, code optimizations, solidity design patterns, code style and the bugs/ issues detected by automated tools.

AUDIT REPORT

Executive Summary

The analysis indicates that the contracts audited are working properly.

Our team performed a technique called “Filtered Audit”, where the contract was separately audited by two individuals. After their thorough and rigorous process of manual testing, an automated review was carried out using Mythril, MythX, and Slither. All the flags raised were manually reviewed and re-tested. 

Our team found: 

Findings

Critical-risk issues

1. First block reward is wrong in Only ALT and Dual Case.

Description: 

If the reward type is changed from only Pilot to only ALT or only Pilot to Dual, the code updates the lastBlockReward variable first for Pilot then for ALT, calculating block difference as 2 instead of 0. Hence, the user gets triple reward for just the first block.

else {
            if (_rewardType == RewardType.Alt) {
                altState.lastRewardBlock = blockNumber;
                updateVaultState(_vault);
            } else {
                altState.lastRewardBlock = blockNumber;
                }
            }
        }
        emit RewardStatus(
            _vault,
            vaultInfo[_vault].reward,
            vaultInfo[_vault].reward = _rewardType,
            _altToken
        );

Remedy:

When updating the reward type lastRewardBlock should be set to the current block number.

else {
            if (_rewardType == RewardType.Alt) {
                altState.lastRewardBlock = blockNumber;
                vaultState.startBlock = blockNumber;
                updateVaultState(_vault);
            } else {
                altState.lastRewardBlock = blockNumber;
                }

Status: 

Fixed as per BlockApex recommendation. 

2. Consecutive change in reward type will disable the stakeLp functionality.

Description: 

If the governance changes reward type from only Pilot to only Alt and then changes it again from only Alt to Dual, no user can stake their LPs.

Scenario: Before staking starts, if the last reward type is changed twice and is finally set to Dual, it causes the last reward block check, inside the stakeLp() function, to break, i.e., to not equal to start block, causing the getGlobalReward() function to be invoked, which in turn causes the mulDiv() error as the value of totalLpLocked never changed from zero.

 function getGlobalReward(
        address _vault,
        uint256 _blockDiff,
        uint256 _multiplier,
        uint256 _lastGlobalReward
    ) private view returns (uint256 _globalReward) {
        if (vaultInfo[_vault].totalLpLocked > 0) {
            _globalReward = FullMath.mulDiv(
                rewardPerBlock,
                _multiplier,
                1e18
            );

            _globalReward = FullMath
                .mulDiv(
                    _blockDiff.mul(_globalReward),
                    1e18,
                    vaultInfo[_vault].totalLpLocked
                )
                .add(_lastGlobalReward);
        } else {
            _globalReward = vaultInfo[_vault].globalReward;
        }
    }

Remedy:

If the totalLpLocked is zero,  the previous state of global reward should return the value of its previous state.  In case zero returns, the function should not calculate the new global reward.

function getGlobalReward(
        address _vault,
        uint256 _blockDiff,
        uint256 _multiplier,
        uint256 _lastGlobalReward
    ) private view returns (uint256 _globalReward) {
        if (vaultWhitelist[_vault]) {
            if (vaultInfo[_vault].totalLpLocked > 0) {
                _globalReward = FullMath.mulDiv(
                    rewardPerBlock,
                    _multiplier,
                    1e18
                );

                _globalReward = FullMath
                    .mulDiv(
                        _blockDiff.mul(_globalReward),
                        1e18,
                        vaultInfo[_vault].totalLpLocked
                    )
                    .add(_lastGlobalReward);
            } else {
                _globalReward = vaultInfo[_vault].globalReward;
            }
        } else {
            _globalReward = vaultInfo[_vault].globalReward;
        }
    }

Status:

Fixed as per BlockApex recommendation.

3. Miscalculation in pilot reward.

Description: 

In an empty farm, if the reward type is changed from only Pilot to only ALT or only Pilot to Dual, the first user who stakes gets a reward of all the empty blocks since the reward type has been updated.

Remedy:

Modify the updateRewardType() function to check if there are no staked LPs in the farm, only then should the start block be updated to set as the current block number.

Status: 

Fixed as per BlockApex recommendation. 

High-risk issues

No issues were found

Medium-risk issues

1. RewardToken validity should be checked.

Description: 

• While calling the initializer() function, if the user sends invalid reward token there is no check to validate whether that token exists in the contract or not. 
• Same problem is confirmed in the UpdateRewardType() function, which also requires a token validity check.

if (!vaultWhitelist[_vaultt[i]] && vaultState.totalLpLocked == 0) {
  insertVault(_vaultt[i], _multipliert [i]);
} else {
  require(!vaultWhitelist[_vault t [i]], "AI");
  if (vaultstate reward == RewardType.Dual) {
    vaultState. LastRewardBlock = blockNum;
    vaultaltState. lastRewardBlock = blockNum;
  } else if (vaultstate reward = RewardType.Alt) {
      vaultAltState. lastRewardBlock = blockNum;
  } else {
      vaultState. LastRewardBlock = blockNum;
  }
}

Remedy:

initializer() and UpdateRewardType() functions should check in the vault contract if the token actually exists by calling the balanceOf() function.

if (!vaultWhitelist[_vault [i]] && vaultstate.totalLpLocked == 0) {
if (
  _rewardType 1 (i) = RewardType. Alt |||
  _rewardType 1 [i] == RewardType.Dual
{
require(
  IERC20(_rewardToken ^ [i]).balanceof(address(this)) > 0,
  "NEB"
);
  vaultAltstate, multiplier = _multipliert[i];
  vaultAltstate.startBlock = blockNum;
  vaultAltState. LastRewardBlock = blockNum;
  vaultAltState. rewardToken = _rewardTokent [i];
}

Status:

Fixed as per BlockApex recommendation.

2. The UpdateRewardPerBlock function will manipulate the last reward for every user.

Description: 

The event in updateRewardPerBlock() function emits the old reward value as well as updating and sending the new reward value, the problem is that this event emits at the start of the function which manipulates the last block reward for every user in the vault.

function updateRewardPerBlock(uint256 _value)
        external
        override
        onlyGovernance
    {
        emit RewardPerBlock(rewardPerBlock, rewardPerBlock = _value);
        require(_value > 0, "IV");

Remedy:

Event should be fired after the calculation at the end of the function. 

function updateRewardPerBlock(uint256 _value)
        external
        override
        onlyGovernance
    {
        require(_value > 0, "IV");
        address[] memory vaults = vaultListed();
        for (uint256 i = 0; i < vaults.length; i++) {
            if (vaultWhitelist[vaults[i]]) {
                if (vaultInfo[vaults[i]].totalLpLocked != 0) {
                    if (vaultInfo[vaults[i]].reward == RewardType.Dual) {
                        updateVaultState(vaults[i]);
                        updateAltState(vaults[i]);
                    } else if (vaultInfo[vaults[i]].reward == RewardType.Alt) {
                        updateAltState(vaults[i]);
                    } else {
                        updateVaultState(vaults[i]);
                    }
                }
            }
        }
        emit RewardPerBlock(rewardPerBlock, rewardPerBlock = _value);
    }

Status:

Fixed as per BlockApex recommendation

3. Should allow Parameters to send Zero to reopen farming

Description: 

There is a check in updateFarmingLimit()  function which does not allow sending zero value in parameter, but it contradicts with the functionality. If the gov sets the farming limit to a specific block and they want to reopen or update the limit they have to send zero value to the updateFarmingLimit() function which will not work if zero value check is placed.

function updateFarmingLimit(uint256 _blockNumber)
        external
        override
        onlyGovernance
    {
       require(_blockNumber > 0, "IV");
        emit UpdateFarmingLimit(
            farmingGrowthBlockLimit,
            farmingGrowthBlockLimit = _blockNumber
        );
        updateLastBlock();
    }

Remedy:

Zero value check should be removed from the function.

function updateFarmingLimit(uint256 _blockNumber)
        external
        override
        onlyGovernance
    {
        emit UpdateFarmingLimit(
            farmingGrowthBlockLimit,
            farmingGrowthBlockLimit = _blockNumber
        );
        updateLastBlock();
    }

Status:

Fixed as per BlockApex recommendation

Low-risk issues

1. RewardToken validity check

Description: 

No zero address check placed for RewardToken while calling the initializer() function.

 function initializer(
        address[] calldata _vault,
        uint256[] calldata _multiplier,
        RewardType[] calldata _rewardType,
        address[] calldata _rewardToken
    ) external override onlyGovernance {
        require(_vault.length == _multiplier.length,"LNS");
        uint256 blockNum = block.number;
        for (uint256 i = 0; i < _vault.length; i++) {

Remedy:

Zero address check should be Placed.

Status:

Fixed as per BlockApex recommendation

2. Necessary checks in updateFarmingLimit() function.

Description: 

If the updateFarmingLimit() function is called with the same value of the block number in which it is going to be executed (or the past block number), the tx will be mined but it will not limit the farming as expected.

function updateFarmingLimit(uint256 _blockNumber)
        external
        override
        onlyGovernance
    {
        emit UpdateFarmingLimit(
            farmingGrowthBlockLimit,
            farmingGrowthBlockLimit = _blockNumber
        );
        updateLastBlock();
    }

Remedy:

A check should be placed in the updateFarmingLimit() function to ensure that block number never equals to current block or past block.

Status:

Fixed as per BlockApex recommendation

3. Emergency exit for users.

Description: 

In event of any mishap with $ALT or $PILOT reward, a user won't be able to withdraw their LP funds. A user calls the unstakeLp()  function, the contract will throw the “Insufficient balance” error.

Remedy:

Contract should have emergency withdraw()  function to withdraw user’s staked LPs 

Status:

Fixed as per BlockApex recommendation

Informatory issues and Optimization

1. Should update Multiplier and RewardType while calling init  function.

Description: 

It's extra work for the governance, for the first time if they want to set a vault for only ALT reward they have to call 3 different functions, this work can be done by calling only one function. 

function initializer(
        address[] calldata _vault,
        uint256[] calldata _multiplier
    ) external override onlyGovernance {
        require(_vault.length == _multiplier.length,"LNS");
        uint256 blockNum = block.number;
        for (uint256 i = 0; i < _vault.length; i++) {
            VaultInfo storage vaultState = vaultInfo[_vault[i]];
            AltInfo storage vaultAltState = vaultAltInfo[_vault[i]];

Remedy:

RewardType and RewardToken should be set by Calling Initializer() function and later it can be handled by individual functions.

function initializer(
        address[] calldata _vault,
        uint256[] calldata _multiplier,
        RewardType[] calldata _rewardType,
        address[] calldata _rewardToken
    ) external override onlyGovernance {

Status:

Fixed as per BlockApex recommendation

DISCLAIMER

The smart contracts provided by the client for audit purposes have been thoroughly analyzed in compliance with the global best practices till date w.r.t cybersecurity vulnerabilities and issues in smart contract code, the details of which are enclosed in this report. 

This report is not an endorsement or indictment of the project or team, and they do not in any way guarantee the security of the particular object in context. This report is not considered, and should not be interpreted as an influence, on the potential economics of the token, its sale or any other aspect of the project. 

Crypto assets/tokens are results of the emerging blockchain technology in the domain of decentralized finance and they carry with them high levels of technical risk and uncertainty. No report provides any warranty or representation to any third-Party in any respect, including regarding the bug-free nature of code, the business model or proprietors of any such business model, and the legal compliance of any such business. No third-party should rely on the reports in any way, including for the purpose of making any decisions to buy or sell any token, product, service or other asset. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project.

Smart contracts are deployed and executed on a blockchain. The platform, its programming language, and other software related to the smart contract can have its vulnerabilities that can lead to hacks. The scope of our review is limited to a review of the Solidity code and only the Solidity code we note as being within the scope of our review within this report. The Solidity language itself remains under development and is subject to unknown risks and flaws. The review does not extend to the compiler layer, or any other areas beyond Solidity that could present security risks.

This audit cannot be considered as a sufficient assessment regarding the utility and safety of the code, bug-free status or any other statements of the contract. While we have done our best in conducting the analysis and producing this report, it is important to note that you should not rely on this report only - we recommend proceeding with several independent audits and a public bug bounty program to ensure security of smart contracts.

INTRODUCTION

BlockApex (Auditor) was contracted by Voirstudio (Client) for the purpose of conducting a Smart Contract Audit/Code Review. This document presents the findings of our analysis which took place on   _9th November 2021___ . 

Scope

The git-repository shared was checked for common code violations along with vulnerability-specific probing to detect major issues/vulnerabilities. Some specific checks are as follows:

Project Overview

Unipilot yield farming incentivizes LPs to earn $PILOT by staking their Unipilot NFT of whitelisted pools.

System Architecture

Unipilot yield farming has 1 main smart contract.

UnipilotFarm.sol: Smart contract which allows LPs to earn $PILOT by staking their Unipilot NFTs. UnipilotFarm linearly distributes the $PILOT according to rewardPerBlock and rewardMultiplier

Methodology & Scope

The codebase was audited in an incremental way. Fixes were applied on the way and were re-audited. We used a static analysis tool (slither) which indicated the reentrancy bug in the code. We did manual reviews on the code to find logical bugs and the bugs reported by the automated tools.

AUDIT REPORT

Executive Summary

The analysis indicates that the contracts audited are working properly.

Our team performed a technique called “Filtered Audit”, where the contract was separately audited by two individuals. After their thorough and rigorous process of manual testing, an automated review was carried out using Mythril, MythX and Slither. All the flags raised were manually reviewed and re-tested. 

Our team found

FINDINGS

Critical-risk issues

1. DOS in deposit

Description
The smart contract maintains a global reward per LP share as the pool's globalReward. It is calculated as the following formula:

globalReward = FullMath.mulDiv(blockDifference.mul(temp), 
1e18,poolInfo[pool].totalLockedLiquidity).add(_globalReward)

When the last user in the pool withdraws his Unipilot NFT, totalLockedLiquidity is
set to 0. When the next user tries to deposit in the same pool, the contract throws an error (div by 0).

Remedy:

This edge case should be handled.

Dev’s Response:

They acknowledged and fixed by resetting the pool’s variables when the last users removes NFT

Status:

Fixed and Verified

High-risk issues

1. Reentrancy when distributing Alt token Rewards

Description:
Unipilot also helps other tokens (specially new tokens) to gain traction by rewarding LPs with this Alt token. So the LPs will be rewarded $PILOT and $ALT_TOKEN. Now when the user claims his reward, Unipilot transfers the shares of $ALT_TOKEN but updates the user reward debt after transferring. Now if the $ALT_TOKEN is ERC777, reentrancy is possible. Although it uses IERC20, but it will work with ERC777 tokens

IERC20(poolAltState.altToken).safeTransfer(userInfo[_tokenId].user, altReward);
poolAltState.lastRewardBlock = block.number;

Remedy:

Update all state variables before transfer. And also use ReentrancyGuard.

Dev’s Response:

We already have used nonReentrant modifier in the new commit

Status

Fixed and verified

Note:
It is good to communicate the changes earlier, so the auditor know beforehand the commit he is auditing

Medium-risk issues

1. Wrong assumption in require statement

Description:
In depositNft the smart contract assumes that the user shares of liquidity should be less than totalLiquidity in the pool and shares of liquidity should be greater than zero. But in the following line it uses or( || ) instead of and (&&) operator

 require(totalLiquidity >= liquidity || liquidity > 0, "IL");

Remedy:

AND (&&) should be used.

Dev’s Response

Acknowledged

Status:

Not Fixed yet

2. Unfair Reward distribution when pilotPerBlock is changed

Description:

There is a global variable pilotPerBlock which is used to calculate the $PILOT reward and act as a multiplier. This variable can be updated via governance. But When this variable is updated, it multiplies with the whole duration of staking.

Remedy:

Implement a mechanism like updating the rewardMultiplier of the pool by looping on all pools. Or just remove pilotPerBlock and just use rewardMultiplier to adjust pool rewards

Dev’s Response:

Acknowledged and will loop on all pools

Status:

Fixed and Verified

Low-risk issues

1. Missing Event in migrateFunds

2. Wrong event in updateUnipilot

3. Remove unused imports like LiquidityAmount.sol

Informatory issues and Optimization

No issues were found

DISCLAIMER

The smart contracts provided by the client for audit purposes have been thoroughly analyzed in compliance with the global best practices till date w.r.t cybersecurity vulnerabilities and issues in smart contract code, the details of which are enclosed in this report. 

This report is not an endorsement or indictment of the project or team, and they do not in any way guarantee the security of the particular object in context. This report is not considered, and should not be interpreted as an influence, on the potential economics of the token, its sale or any other aspect of the project. 

Crypto assets/tokens are results of the emerging blockchain technology in the domain of decentralized finance and they carry with them high levels of technical risk and uncertainty. No report provides any warranty or representation to any third-Party in any respect, including regarding the bug-free nature of code, the business model or proprietors of any such business model, and the legal compliance of any such business. No third-party should rely on the reports in any way, including for the purpose of making any decisions to buy or sell any token, product, service or other asset. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project.

Smart contracts are deployed and executed on a blockchain. The platform, its programming language, and other software related to the smart contract can have its vulnerabilities that can lead to hacks. The scope of our review is limited to a review of the Solidity code and only the Solidity code we note as being within the scope of our review within this report. The Solidity language itself remains under development and is subject to unknown risks and flaws. The review does not extend to the compiler layer, or any other areas beyond Solidity that could present security risks.

This audit cannot be considered as a sufficient assessment regarding the utility and safety of the code, bug-free status or any other statements of the contract. While we have done our best in conducting the analysis and producing this report, it is important to note that you should not rely on this report only - we recommend proceeding with several independent audits and a public bug bounty program to ensure security of smart contracts.

INTRODUCTION

BlockApex (Auditor) was contracted by KaliCo LLC_ (Client) for the purpose of conducting a Smart Contract Audit/Code Review. This document presents the findings of our analysis which took place from 20th of December 2021 . 

Scope 

The git-repository shared was checked for common code violations along with vulnerability-specific probing to detect major issues/vulnerabilities. Some specific checks are as follows: 

Project Overview 

Kali is a protocol for on-chain organizations inspired by Compound and Moloch DAO Governance. Kali proposals are broken into a variety of types, such that each variance can have their own Governance settings, such as simple/super majority and Quorum requirements. 

System Architecture 

KaliDAO.sol 

KaliDAO is a Comp-style governance into a single contract, it supports extensions to add contracts as apps, for example crowdsale and redemption contracts. 

Kali supports hashing and amending docs from deployment and through proposals, providing a hook to wrap organizations into legal templates to rationalize membership rules and liabilities. 

KaliDAOtoken.sol 

KaliDAOtoken represent voting stakes, and can be launched as transferable or non-transferable, with such settings being updateable via PAUSE proposal. Voting weight can also be delegated, and such weight automatically updates upon token transfers from delegators, incorporating functionality from Comp-style tokens. 

Methodology & Scope 

Audit log:
In the first two days, we developed a deeper understanding of the DAO and its workings. We started by reviewing the two main contracts against common solidity flaws. After the reconnaissance phase we wrote unit-test cases to ensure that the functions are performing their intended behavior. Then we began with the line-by-line ma

AUDIT REPORT

Executive Summary

The analysis indicates that the contracts audited are working properly.

After the initial audit, the client was provided with the initial audit report, and the issues reported were discussed. After the fixes had been made, our team performed a re-audit of the codebase. No further issues were found. The contracts were separately reviewed by two individuals. After their thorough and rigorous process of manual testing, an automated review was carried out using Mythril, MythX and Slither. All the flags raised were manually reviewed and re-tested.

Our team found:

INITIAL FINDINGS

Update on initial findings

Critical-risk issues

CR-1. “Arbitrary call” proposals can lead to unauthorized transfers 

Contract : KaliDAO.sol 

it("proposal type call , using arbitrary calls to call transfer and transferFrom function of kalidao token ", async function () {

    // hex data for payload
    const tranferFromCall = lexdao.interface.encodeFunctionData("transferFrom", [owner.address,addr3.address, 25]);
    
    // any approved users balance for lexDAO
    await lexdao.approve(lexdao.address , 1000000000);

    const ownerPreBal = await lexdao.balanceOf(owner.address);

    // proposalType.CALL, accounts = [lexdaoAddress], value = 0 ether, call = transferFromCall data
    await lexdao.propose(2, "TEST", [lexdao.address] , [0], [tranferFromCall]);
    await lexdao.vote(0,true); // votecount check
    await forwordTime(40); // vote ended, ignore typo :)
    await lexdao.processProposal(0); // process proposal
    
    // after balances
    const add3AftBal = await lexdao.balanceOf(addr3.address);
    const ownerAftBal = await lexdao.balanceOf(owner.address);

    // balances tranferred
    expect(add3AftBal.toNumber()).not.eq(0); // tranferFrom call executed
    expect(ownerAftBal.toNumber()).lessThanOrEqual(ownerPreBal.toNumber());
    
});

})

Exploit Scenario 

Any malicious user can submit the proposal type call, vote type simple majority and payload has a transferFrom transactions as we created payload on above test. Attackers can transfer any amount from any user if the user has approved his funds to the LexDao contract. 

Remedy 

1. BlackList specific account addresses and calls. 

2. Implement validation of transaction data. (this might create centralization) 

Developer's Response 

“We believe this issue is avoided by discouraging users from approving token pulls to the `KaliDAO.sol` contract. Instead, payments to DAOs will be through direct transfers or by the use of extension contracts which are authorized to make such token pulls. It is our preference to avoid limiting the kinds of calls that can be made through these kinds of proposals or adding centralization factors” 

Auditor's Response 

Status : Acknowledged. 

CR-2. New “Proposals” should not be processed before processing previous Proposals 

Contract : KaliDAO.sol 

Function : processProposal() 

// skip previous proposal processing requirement in case of escape hatch
        if (prop.proposalType != ProposalType.ESCAPE) {
            // allow underflow in this case to permit first proposal
            unchecked {
                require(proposals[proposal - 1].creationTime == 0, 'PREV_NOT_PROCESSED');
            }
        }

Exploit Scenario 

As stated in the piece of code mentioned above, it is very clear that if any previous proposal is pending, no new proposal will be processed. However, this check can be easily by-passed if a non-member adds a proposal and then sponsorProposal is called after that. This way, a new proposal will be processed. 

it("Process proposal without processing previous proposals", async function () {

    //proposal 0
    await lexdao.propose(0, "TEST", [owner.address] , [1], [0x00]);
    //proposal 1
    await lexdao.propose(0, "TEST", [owner.address] , [1], [0x00]);
    //proposal 2 by non-menber
    await lexdao.connect(addr3).propose(0, "TEST", [owner.address] , [1], [0x00]);
    //sponsor by member
    await lexdao.sponsorProposal(2);
    await forwordTime(40);
    try {
        //process Proposal without process previous proposals
        await lexdao.processProposal(3);
    } catch (error) {
        console.log(error);
    }
});

Remedy 

Do not create a “new” proposal when sponsorProposal() is called. 

Developer's Response 

“We have made a fix by tracking a new global state variable, currentSponsoredProposal and updating it upon every sponsored proposal. This value gets appended to the Proposal struct and is checked on processing, like so: 

if (proposals[prop.prevProposal].creationTime != 0) revert PrevNotProcessed(); 

Auditor's Response 

Status : Resolved. 

High-risk issues

HR-1. A “SUPERMAJORITY” proposal can be triggered without casting any votes 

Contract : KaliDAO.sol 

Function : _countVotes() 

function _countVotes(
        VoteType voteType,
        uint256 yesVotes,
        uint256 noVotes
    ) internal view virtual returns (bool didProposalPass) {
        // rule out any failed quorums
        if (voteType == VoteType.SIMPLE_MAJORITY_QUORUM_REQUIRED || voteType == VoteType.SUPERMAJORITY_QUORUM_REQUIRED) {
            uint256 minVotes = (totalSupply * quorum) / 100;
            
            // this is safe from overflow because `yesVotes` and `noVotes` are capped by `totalSupply`
            // which is checked for overflow in `KaliDAOtoken` contract
            unchecked {
                uint256 votes = yesVotes + noVotes;

                if (votes < minVotes) return false;
            }
        }
        
        // simple majority
        if (voteType == VoteType.SIMPLE_MAJORITY || voteType == VoteType.SIMPLE_MAJORITY_QUORUM_REQUIRED) {
            if (yesVotes > noVotes) return true;
        // super majority
        } else {
            // example: 7 yes, 2 no, supermajority = 66
            // ((7+2) * 66) / 100 = 5.94; 7 yes will pass
            uint256 minYes = ((yesVotes + noVotes) * supermajority) / 100;

            if (yesVotes >= minYes) return true;
        }
    }

Exploit Scenario 

Let's consider the code above; The else() statement : if yesVotes and noVotes are zero and supermajority is set to 66, the resulting minYes will ultimately be zero as well. Now let’s see the last if() inside the else( ) condition. Both yesVotes and minYes are zero, so the condition will return true. If any malicious user adds a proposal of mint/burn, they just have to wait to process the proposal because that proposal does not need any votes. 

Remedy 

Remove the equality sign and ensure that the voting period is long enough so that the users are able to vote on every proposal. This will make sure that the yesVotes required to process a proposal is greater than 0 and can get votes from other users as well. 

Developer's Response 

We have a made a fix to this issue by putting in a return check at the top of the “_countVotes” internal function that fails a proposal if nobody participated, like so: “if (yesVotes == 0 && noVotes == 0)” 
return false;   

Auditor's Response 

Status : Resolved. 

HR-2. Whitelisting can be bypassed in extension 

Contract : KaliDAOcrowdsale.sol 

         if (sale.listId != 0) require(whitelistManager.whitelistedAccounts(sale.listId, account), 
            'NOT_WHITELISTED');

Exploit Scenario 

User can bypass the whitelisting check by providing listId “0”. 

(Note: This might be a false assumption because we didn't properly recon the latest codebase at this point) 

Remedy 

A contract should have proper checks that restrict users to set extension with “0” listId. Developer's Response 

In the crowdsale extension contract, we purposefully allow users to set a null value for whitelist ID in order to allow for unrestricted token sales. 

Auditor's Response 

Status : Acknowledged. 

HR-3. Any malicious user can withdraw “purchaseToken” and drain the whole contract 

Contract : KaliDAOcrowdsale.sol 

it("Bypassing whitelisting can lead to unautherized transfers " , async function() {
    // Data for crowdsale Extension
    const data = abiCoder.encode(
        ["uint256" , "address" , "uint8" , "uint96" , "uint32"] , 
        [0 , token.address , 1, BigNumber("100000").toString() , BigNumber("1640838044").toString()]);
    //transfering funds to address 3
    await token.transfer(addr3.address , BigNumber("100").toString());
    //checking balance before
    console.log(await token.balanceOf(addr3.address));
    console.log(await token.balanceOf(addr4.address));
    //approving funds by address 3 to crowdsale contract
    await token.connect(addr3).approve(crowdsale.address , BigNumber("100").toString());
    //set extension using address 4
    await crowdsale.connect(addr4).setExtension(data);
    //call extension using address 4
    await crowdsale.connect(addr4).callExtension(addr3.address, BigNumber("100").toString());
    //checking balance after
    console.log(await token.balanceOf(addr3.address));
    console.log(await token.balanceOf(addr4.address));

})

Exploit Scenario 

Consider that a user can set an extension using the listId “0” (the check which can be easily bypassed). Now if any user has approved their funds to the crowdsale contract, any malicious user can simply call the setExtension() using the token address in his data and then call callExtension() using address 3 (used in the example above) in the parameters which will ultimately withdraw all the funds user have approved to the crowdsale contract. 

Remedy 

Whitelisting should be handled properly like funds should be transferred to a whitelisted address instead of msg.sender. 

Developer's Response 

Resolved the issue by limiting users to call the extension directly from crowdsale and deducting amounts form msg.sender instead of any address. 

Auditor's Response 

Status : Resolved. 

Informatory issues and Optimization 

IR-1. Upper and lower bound on Governance params 

Contract : KaliDAO.sol 

Description 

init function has proper upper and lower bounds but if the user set values by Governance, it has no lower and upper bound check. Unchecked math assumes these bounds. 

Remedy 

A proper upper and lower bounds check should be placed while changing values by Governance. 

Developer's Response 

We provide reversion checks on governance params in the “propose()” function to ensure that proposals to amend “votingPeriod”, “quorum”, “supermajority” and proposal types are kept within expected bounds. 

Auditor's Response 

Status : Acknowledged. 

IR-2. “Pause” proposal should escape previous proposals 

Contract : KaliDAO.sol 

Description 

To pause the contract, a proposal is submitted. Most of the time a pause is needed in emergencies. To timely execute a pause proposal it should not wait for previous proposals. (We have seen mishaps with $COMP in the past, where their proposal to fallback takes days). 

Remedy 

There should be a check in processProposal() that if the proposal type is “pause” it can execute right away. 

Developer's Response 

It is our preference to maintain a voting period for PAUSE proposal types in order to accommodate the use case of gradual decentralization by some DAOs. For example, there are non-emergency situations where a DAO might deploy with a closed founder group, grow its membership through proposals, but then want to vote and reach consensus on making membership tokens transferable, and therefore, open to the public. 

Auditor's Response 

Status : Acknowledged. 

IR-3. Bad error on proposal: “PROCESSED” for non-existent proposals Contract : KaliDAO.sol 

 function processProposal(uint256 proposal) public nonReentrant virtual returns (
        bool didProposalPass, bytes[] memory results
    ) {
        Proposal storage prop = proposals[proposal];

        require(prop.creationTime != 0, 'PROCESSED');

Description 

If a user tries to process a non-existent proposal, the above code won’t let him process because proposal creation time is 0 but the user will get the “PROCESSED” error. This should be handled properly by separately checking if the proposal actually exists or not. 

Remedy 

“Proposal does not exist” should be displayed by adding a check using require. Developer's Response 

We have provided a clarified reversion message, “Processed()” -> “NotCurrentProposal()”. We otherwise would like to avoid additional checks to optimize for gas efficiency. 

Auditor's Response 

Status : Acknowledged. 

IR-4. Bad error on voting: “VOTING_ENDED” for non-existent proposals Contract :

   // this is safe from overflow because `votingPeriod` is capped so it will not combine
        // with unix time to exceed 'type(uint256).max'
        unchecked {
            require(block.timestamp <= prop.creationTime + votingPeriod, 'VOTING_ENDED');
        }

KaliDAO.sol

Description 

If a user tries to vote on a non-existent proposal, the above code won't let him vote because proposal creationTime + votingPeriod will be less than block.timestamp but the user will get “VOTING_ENDED” error. This should be handled properly by separately checking if the proposal actually exists or not. 

Remedy 

“Proposal does not exist” should be displayed by adding a check using require. 

Developer's Response 

We have provided a clarified reversion message, “VotingEnded()” -> “NotVoteable()”. We otherwise would like to avoid additional checks to optimize for gas efficiency. 

Auditor's Response 

Status : Acknowledged. 

DISCLAIMER

The smart contracts provided by the client for audit purposes have been thoroughly analyzed in compliance with the global best practices till date w.r.t cybersecurity vulnerabilities and issues in smart contract code, the details of which are enclosed in this report. 

This report is not an endorsement or indictment of the project or team, and they do not in any way guarantee the security of the particular object in context. This report is not considered, and should not be interpreted as an influence, on the potential economics of the token, its sale or any other aspect of the project. 

Crypto assets/tokens are results of the emerging blockchain technology in the domain of decentralized finance and they carry with them high levels of technical risk and uncertainty. No report provides any warranty or representation to any third-Party in any respect, including regarding the bug-free nature of code, the business model or proprietors of any such business model, and the legal compliance of any such business. No third-party should rely on the reports in any way, including for the purpose of making any decisions to buy or sell any token, product, service or other asset. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project. 

Smart contracts are deployed and executed on a blockchain. The platform, its programming language, and other software related to the smart contract can have its vulnerabilities that can lead to hacks. The scope of our review is limited to a review of the Solidity code and only the Solidity code we note as being within the scope of our review within this report. The Solidity language itself remains under development and is subject to unknown risks and flaws. The review does not extend to the compiler layer, or any other areas beyond Solidity that could present security risks. 

This audit cannot be considered as a sufficient assessment regarding the utility and safety of the code, bug-free status or any other statements of the contract. While we have done our best in conducting the analysis and producing this report, it is important to note that you should not rely on this report only - we recommend proceeding with several independent audits and a public bug bounty program to ensure security of smart contracts.

INTRODUCTION

Scope

The git-repository shared was checked for common code violations along with vulnerability-specific probing to detect major issues/vulnerabilities. Some specific checks are as follows:

Project Overview

A detailed overview of the project can be found here:
https://blog.dafiprotocol.io/introducing-the-dbridge-testnet-c564f5b4eea2

Dafi’s “dbridge” enables users to bring their ERC-20 $DAFI tokens across from the Ethereum network to Binance Smart Chain, and vice versa, with aims of making $DAFI available on multiple high-speed and low-cost networks. As Dafi’s goal is to support every token on most chains, to launch their own dToken, it’s important that their protocol is cross-chain.

Dafi Bridge is an implementation of a generic POA Bridge. Authority is distributed among validators. Validators sign the proof-of-burn message and the user submits (to avoid gas griefing attacks) the signature on the alternate chain to claim tokens.

The codebase:

The system consists of 3 smart contracts (i.e ETH Bridge, BSC Bridge & a burnable/mintable ERC20 token representing Dafi on alternate chains)

Bridge contracts have onlyOwner modifier to set configurations (i.e adding/removing validators, minimum signers required(threshold)).

AUDIT REPORT

Executive Summary

The analysis indicates that the contracts audited are working properly.

Our team performed a technique called “Filtered Audit”, where the contract was separately audited by two individuals. After their thorough and rigorous process of manual testing, no potential flags were raised.

Our team found:

# of issues	Severity of the risk
0	Critical Risk issue(s)
0	High Risk issue(s)
1	Medium Risk issue(s)
1	Low Risk issue(s)
3	Informatory issue(s)

FINDINGS

Critical-risk issues

No critical-risk issues were found in the review.

High-risk issues

No high-risk issues were found in the review.

Medium-risk issues

1. If ETHToken is changed, tokens will be locked forever in the contract

File: ETHBridgeOptimized.sol

Description:

Owner(multisig) can change the underlying ETHToken address. If there are tokens locked in the smart contract and changeToken() is called, tokens will be locked forever in the contract.

Remedy:

Remove changeToken() method from the contract. If there is a need to change the token address, a new contract can be deployed.

Status: 

Fixed.

Low-risk issues

1. Unnecessary allowance check in burn/lock tokens

File: ETHBridgeOptimized.sol, BinanceBridgeOptimized.sol

Description:

 if (IERC20(BSCTOKEN).allowance(msg.sender, address(this)) < amount)
  revert AllowanceInsufficient( 
  IERC20(BSCTOKEN).allowance(msg.sender, address(this)), amount); 

When burning/locking tokens, the contract checks if the msg.sender has allowed the smart contract (address(this)) to burn/lock. This check will cost extra gas. Normally these checks are used with burnFrom. There is already check placed in the burn method of dafiToken.sol

Remedy:

Remove allowance checks

Status: 

Fixed.

Informatory issues and Optimizations

1. Misleading variable/function names

File: ETHBridgeOptimized.sol, BinanceBridgeOptimized.sol, dafiTokenBSC.sol
 function burnTokens(uint256 amount, address targetChain) external { 
 function lockTokens(uint256 amount, address targetChain) external { 
 function viewOwners(address checkAddress) external view returns (bool) //viewValidators 
 function burn(uint256 _value, address _beneficiary) external onlyBridge { // burnFrom 

Remedy:

Our suggestion is that
1. targetChain should be recipient
2. viewOwners should be ViewValidators
2. burn should be burnFrom in dafiToken

2. Gas cost optimization while incrementing nonce

File: ETHBridgeOptimized.sol, BinanceBridgeOptimized.sol

Description:

nonceIncrement() method is used to increment a state variable nonce. Calling a function instead of directly updating the state variable will save the gas cost. Calling a function introduce JUMP opcode which has a higher gas cost

Status: 

Fixed.

3. Centralization risk on minting/burning on BSC Token

Description:

onlyBridge can burn/mint tokens. Bridge address can be changed by the owner (MultiSig). There are no potential risks as long as the signers of the multisig are honest.

DISCLAIMER

The smart contracts provided by the client for audit purposes have been thoroughly analyzed in compliance with the global best practices till date w.r.t cybersecurity vulnerabilities and issues in smart contract code, the details of which are enclosed in this report. 

This report is not an endorsement or indictment of the project or team, and they do not in any way guarantee the security of the particular object in context. This report is not considered, and should not be interpreted as an influence, on the potential economics of the token, its sale or any other aspect of the project. 

Crypto assets/tokens are results of the emerging blockchain technology in the domain of decentralized finance and they carry with them high levels of technical risk and uncertainty. No report provides any warranty or representation to any third-Party in any respect, including regarding the bug-free nature of code, the business model or proprietors of any such business model, and the legal compliance of any such business. No third-party should rely on the reports in any way, including for the purpose of making any decisions to buy or sell any token, product, service or other asset. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project.

Smart contracts are deployed and executed on a blockchain. The platform, its programming language, and other software related to the smart contract can have its vulnerabilities that can lead to hacks. The scope of our review is limited to a review of the Solidity code and only the Solidity code we note as being within the scope of our review within this report. The Solidity language itself remains under development and is subject to unknown risks and flaws. The review does not extend to the compiler layer, or any other areas beyond Solidity that could present security risks.

This audit cannot be considered as a sufficient assessment regarding the utility and safety of the code, bug-free status or any other statements of the contract. While we have done our best in conducting the analysis and producing this report, it is important to note that you should not rely on this report only - we recommend proceeding with several independent audits and a public bug bounty program to ensure security of smart contracts.