INTRODUCTION

BlockApex (Auditor) was contracted by BullionFX (Client) for the purpose of conducting a Smart Contract Audit/Code Review. This document presents the findings of our analysis which started on 20th Nov 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

Rain Protocol is a set of building blocks that enable your token economy including staking, vesting, emissions, escrow, order book, verification, sale, and membership. Rain VM uses EVM making it fully compatible with all the EVM chains. DEMI has integrated Rain protocol’s Staking contracts.

System Architecture

Staking Factory Contract:

The system uses a design pattern called the ‘factory method’ to deploy homogenous contracts from the same parent & keeps a record of the child contracts that have been deployed by the factory. In this particular case, the system deploys a staking contract.

Staking Contract:

Staking contract is a modified version of a vault contract tokenized. It inherits all of the properties of the ERC-4626 contract which standardizes the interface for easily managing deposited tokens & their shares within the system. The contract also introduces a custom logic for maintaining its own internal ledger for share calculation through the checkpointing mechanism.

Methodology & Scope

The codebase was audited using a filtered audit technique. A pair of two (2) auditors scanned the codebase in an iterative process spanning over a time of One (1) week.

Starting with the recon phase, a basic understanding was developed and the auditors worked on developing presumptions for the developed codebase and the relevant documentation/whitepaper. Furthermore, the audit moved on with the manual code reviews with the motive to find logical flaws in the codebase complemented with code optimizations, software, and security design patterns, code styles, best practices, and identifying false positives that were detected by automated analysis tools.

AUDIT REPORT

Executive Summary

Our team performed a technique called “Filtered Audit”, where the contract was separately audited by four individuals. After a thorough and rigorous process of manual testing, an automated review was carried out using slither for static analysis. All the flags raised were manually reviewed and re-tested to identify the false positives.

Our team found:

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

KEY FINDINGS

DETAILED OVERVIEW

Critical-risk issues

No critical issues were found.

High-risk issues

No High-risk issues were found.

Medium-risk issues

No Medium-risk issues were found.

Low-risk issues

Potential Loss of Precision

File: stake/Stake.sol#L15

Description: 

In contracts/math/FixedPointMath.sol the function scaleN will lead to precision loss if a number is scaled down to 18.
There may arise a scenario in future development where a deposited token having a higher decimal (i.e > 18) may need to scale down to 18 decimals for logic consistency. The conversion either from or to 18 will lead to a loss in precision which may result in the dust amount being locked in the contract.

However, this function is not used in the current implementation of the staking contract to either scale up or down.

Recommendation:

Introduce a mechanism to manage the difference that was lost during the conversion. One way could be by storing the difference & using it to convert back.

DEMI Staking Audit - Report

Dev’s Response:

As noted staking contract does not use scaleN. Worth also noting that ERC4626 that the staking contract is based on dedicates a lot of the spec to rounding/dust handling, so if scaleN would be hypothetically used in the future it would still need to be 4626 compliant (which means always leaving dust from the underlying asset in the vault in the case of rounding issues) that's largely what the mulDiv is handling in openzeppelin's implementation and we wrap in the other fixed point math functions

As scaleN is a library contract it has no storage of its own so there's nowhere for it to save information about the lost precision directly, the best it could do is return two values, one representing the scaled value and one representing the lost precision. Currently, scaleN is only used in expressions in the interpreter as a provided opcode, so I'm not sure if it's something on the average expression author's radar to be worrying about or ready to handle (juggling 2 values on the stack to avoid some dust).

it's probably worth documenting all this though as it's worth pointing out as something to consider for anyone who does care about it.

DEMI Staking Audit - Report

Auditor’s Response

The auditors agree with the devs.

Informatory issues and Optimizations

1. Inexplicable inclusion of unused library

File: stake/Stake.sol#L15

Description:

The staking contract imports a library called FixedPointMath.sol that is not used within the scope of the contract.

Recommendation: Remove the unused import along with its using statement.

Alleviation: This issue is fixed.

DISCLAIMER

The smart contracts provided by the client for audit purposes have been thoroughly analyzed in compliance with the global best practices to 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 the 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 assets. 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 sucient 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 the security of smart contracts.

Also see other Audit reports.

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.