Introduction

BlockApex (Auditor) was contracted by ZeroLiquid (Client) to conduct a Smart Contract Audit/ Code Review. This document presents the findings of our analysis, which started on 11th July ‘2023.

Scope

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

Project Overview

ZeroLiquid is a decentralized protocol that offers non-liquidation, interest-free, self-repaying loans. The protocol allows users to utilize Liquid Staking Derivative (LSD) assets to issue loans against their collateral. Upon depositing LSD tokens, users receive a synthetic token, zETH, which can be traded to provide immediate liquidity. The unique feature of ZeroLiquid is that loans are self-repaying, using the staking rewards generated by the collateral. Users have the option to early unstake by repaying the remaining loan amount.

• Users can borrow against their LSD tokens, with the maximum loan amount being equivalent to 12 months of yield. The borrowed amount is received in the form of zETH, ZeroLiquid's synthetic ETH, which is pegged to the price of ETH. This zETH can be traded in liquidity pools created and incentivized by ZeroLiquid.
• The Diffuser plays a crucial role in maintaining the zETH/ETH peg. Whenever there's a divergence in the peg, arbitrageurs can acquire zETH at a discount and exchange it 1:1 for ETH on the ZeroLiquid protocol. The yield generated from the deposited LSD tokens (after deducting a 10% protocol fee) is directed to the Diffuser at regular intervals. This yield is then distributed proportionally to all zETH submitted for swap. When users claim or swap their ETH from the Diffuser pool, an equivalent amount of zETH is burned. This process ensures peg stability and reduces the overall debt burden of all users proportionally. To further enhance peg stability, ZeroLiquid is implementing LP incentives and integrating zETH with various DeFi protocols to ensure robust liquidity.

ZeroLiquid employs a multi-pronged approach to safeguard the zETH/ETH peg:

• Arbitrage: Users can exploit price differences between zETH and ETH, creating an arbitrage opportunity. When zETH's price is lower than ETH's, users can acquire zETH and convert it 1:1 to ETH using the Diffuser, thus maintaining the peg.
• Incentivized Liquidity: ZeroLiquid incentivizes users to provide liquidity to zETH/ETH pools. Initially, these incentives are in the form of $ZERO tokens, but will later be sustained by protocol revenue. This ensures that the peg remains stable even during large transactions.
• Protocol-owned Liquidity: ZeroLiquid owns the $ZERO/ETH liquidity on Uniswap v3. The ETH generated from LP fees in this pool is used to add zETH/ETH liquidity. This secures the peg and ensures that community-generated fees are reinvested into the platform for the community's benefit.

System Architecture

The ZeroLiquid protocol is built on a robust and secure architecture that enables seamless asset management and loan issuance. The protocol periodically harvests staking rewards, credited proportionally to users, reducing their outstanding debt. This unique feature allows the loans to be self-repaying, providing a hassle-free experience for the users.

The architecture comprises several key components, including the ZeroLiquid, ZeroLiquidToken, and Steamer smart contracts.

ZeroLiquid

The ZeroLiquid contract is the core of the protocol, managing the deposit of LSD tokens, the issuance of loans, and the harvesting of staking rewards. It ensures the accurate tracking of assets and the proportional distribution of rewards to reduce the outstanding debt of users.

ZeroLiquidToken

The ZeroLiquidToken contract creates and manages zETH, the protocol's synthetic ETH. It includes functionality for minting and burning tokens and managing the protocol's flash loan feature. The ZeroLiquidToken contract interacts with a number of other contracts and libraries to provide its functionality.

Steamer

The Steamer contract is a crucial component of the ZeroLiquid Protocol, responsible for managing the protocol's native token. It includes functionality for minting and burning tokens and managing the protocol's flash loan feature.

These components work together to provide a robust and secure platform for creating and managing over-collateralized stablecoins. The architecture is designed to be modular and extensible, allowing for future upgrades and improvements. This ensures that the protocol can continue to evolve and adapt to the changing needs of the DeFi ecosystem.

In addition to these features, ZeroLiquid also allows users to unstake their collateral early by repaying the remaining loan amount. Users can do this by depositing either the LSD token they used as collateral or zETH. Furthermore, users can liquidate a portion or all of their collateral anytime, providing maximum flexibility and control over their assets.

Methodology & Scope

The codebase was audited using a filtered audit technique. A pair of two (2) security researchers scanned the codebase in an iterative process for eighteen (18) days.

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

Audit Report

Executive Summary

Our team performed a technique called Filtered Audit, where two individuals separately audited the Zero Liquid Protocol.

After a thorough and rigorous manual testing process involving line-by-line code review for bugs, an automated tool-based 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 Issues(s)
0	High Risk Issues(s)
2	Medium Risk Issues(s)
5	Low Risk Issues(s)
8	Informatory Risk Issues(s)

Key Findings

Detailed Overview

Medium-risk issues

1. Unchecked Delegate Call and Return Values

File: ZeroLiquid.sol

Status: Resolved

Function Name: -

Description: 

The sweepRewardTokens function uses delegateCall to call the claim function on the contract at the msg.sender address. However, the return value of the delegatecall is not checked, which could lead to unexpected behavior. The scenario arises due to two potential reasons as mentioned above;

• The function sweepRewardTokens can only be called by the keeper role, however, there is no contract found in the codebase shared that hints at what the keeper contract supposedly does. This leads to a state of indeterminacy where thefunctionality of claim() remains unclear and can affirm one of the two following assumptions;
  • The smart contract is assumed to be safe being outside the scope of the current audit.
  • The contract is unsafe and contains potential risks imposing undetermined threats.
• Return values of the delegate call, in each of the subcases above, are never checked, opening up windows of attack vectors related to such scenarios.

Code Affected:

msg.sender.delegatecall(
abi.encodeWithSignature("claim(address)", yieldToken)
)

Impact:

If the claim function fails due to some reasons which are unclear since the codebase is not shared, the sweepRewardTokens function will continue execution, potentially leading to incorrect state changes in the ZeroLiquid protocol smart contracts complex. This could lead to loss of funds or other unexpected behavior. Additionally, claim() can be used to reenter the smart contract in potential cases like the novel cross-functional reentrancy.

Recommendation:

It is recommended to confirm from the development team the following;

1. Ensure the contract address stored as a keeper role is always safe.
2. Check the return value of the delegatecall and revert if it is false. This will ensure that the sweepRewardTokens function fails if the claim function fails.

Developer's Response:

This function is no longer needed and will be removed.

Auditor's Response:

Acknowledged with no follow-up required.

Additional Comments:

The linked reports emphasize on severity of this issue as high (or medium, at minimum) in similar contexts. Fortifying the claim that the set of developer assumptions can be broken where either the smart contract in silo or the complete smart contract complex is led to an undetermined state - The remediations in all issues linked here focus on two aspects; 1) to document the intended functionality for successful target contract call even if no code executed AND 2) check for return values in any case. E.g. links 1 & 2 focus on unidentified state changes within the smart contracts, 3 & 4 focus on risks associated with such flow of execution, 5 & 6 emphasize on defining the severity of issue for impact and relevant likelihood - Link 1, Link 2, Link 3, Link 4, Link 5, Link 6

2. Missing notPaused Modifier in Several Functions

File: Streamer.sol

Status: Resolved

Description:

Several functions in the Steamer contract that should only be callable when the contract is not paused are missing the notPaused modifier. This includes the deposit, withdraw, claim, and exchange functions. These functions all modify the smart contract's state and should not be callable when the contract is paused. The contract will be entirely vulnerable in such cases, rendering the emergency mechanisms ineffectual.

Code Affected:

function deposit(
    uint256 amount,
    address owner
) external override nonReentrant {
    ...
}

function withdraw(
    uint256 amount,
    address recipient
) external override nonReentrant {
    ...
}
function claim(
    uint256 amount,
    address recipient
) external override nonReentrant {
    ...
}

function exchange(
    uint256 amount
) external override nonReentrant onlyBuffer {
    ...
}

Impact:

If these functions are called while the contract is paused, they could lead to unexpected behavior or potential vulnerabilities. For example, if the deposit function is called while the contract is paused, this could lead to an imbalance between the total amount of tokens deposited and the total amount of tokens that can be withdrawn.

Recommendation:

Add the notPaused modifier to all functions that should only be callable when the contract is not paused. This will ensure that these functions behave as expected and prevent potential vulnerabilities.

Developer Response:

deposit, withdraw & claim functions do not need to check pause state, only exchange function does. Even if user deposits debt token while paused the unconverted debt tokens can still be withdrawn. Anyother specific function you can name which should check the pause state?

Auditor’s Response:

Acknowledged. No other external functions perform critical updates to the smart contract's global state and hence there is no followup required in this regard.

Low-risk Issues

3. Insufficient Input Validation in setPendingAdmin, setSentinel, and setKeeper Functions

File: ZeroLiquid.sol

Status: Resolved

Description:

The setPendingAdmin, setSentinel, and setKeeper functions do not perform any checks on their parameters. This could allow the admin to set the pending admin, sentinel, or keeper to the zero address.

Code Affected:

function setPendingAdmin(address value) external override {
    _onlyAdmin();
    pendingAdmin = value;
    emit PendingAdminUpdated(value);
}
function setSentinel(address sentinel, bool flag) external override {
    _onlyAdmin();
    sentinels[sentinel] = flag;
    emit SentinelSet(sentinel, flag);
}
function setKeeper(address keeper, bool flag) external override {
    _onlyAdmin();
    keepers[keeper] = flag;
    emit KeeperSet(keeper, flag);
}

Impact:

If the setPendingAdmin, setSentinel, or setKeeper functions are called with the zero address, this could cause the contract to be in an incorrect state. This could lead to unexpected behavior and potential loss of control over the contract.

Recommendation:

Add validations to the setPendingAdmin, setSentinel, and setKeeper functions to ensure that the address parameters are not the zero address.

Developer Response:

Setters only enable or disable any specific address using boolean flag, doesn't need any unnecessary validation or concerete meaning of "insufficient" should be provided otherwise. Are you talking about checking if the address was previously enabled or disabled before repeating the same action again?

Auditor’s Response:

Acknowledged with no followup required.

4. Protocol Fee and flashMintFee Can Be Set to 99%

File: ZeroLiquid.sol

Status: Resolved

Description:

The setProtocolFee and the  setFlashFee functions allow the admin to set the protocol fee to any value less than BPS (10,000 basis points, representing 100%). This means the admin could set the fee to 99.99%, which would be extremely high and could deter users from using the contract’s functionality.

Code Affected:

function setProtocolFee(uint256 value) external override {
    _onlyAdmin();
    _checkArgument(value <= BPS);
    protocolFee = value;
    emit ProtocolFeeUpdated(value);
}
function setFlashFee(uint256 newFee) external onlyAdmin {
    if (newFee >= BPS) {
        revert IllegalArgument();
    }
    flashMintFee = newFee;
    emit SetFlashMintFee(flashMintFee);
}

Impact:

If the protocol fee or the flash loan minting fee is set to a very high value, this could deter users from using the contract. This could reduce the utility and adoption of the contract.

Recommendation:

Consider implementing a reasonable upper limit (and a lower limit for the flashMintFee state variable) for the protocol fee. This could be a fixed value or a value that governance can update

Developer Response:

These variables are purposefully dynamic so that they can be adjusted if needed, but the percentage is transparent on chain and not something hidden from users

Auditor’s Response:

Acknowledged with no follow-up required.

Additional Comments:

This is a vague intention by design since a common user of DeFi believes in the specifications and figures drawn over the publicly accessible interfaces like a protocol's website or the technical documentation. A security first design ensures that in no case a functionality be abused by an adversary, It is therefore suggested that the developer team specify a range and enforce it in the smart contract code so that even in case of compromise the adversary can never enforce an illegal fee for users.

Final update:

Fixed for both functions.

5. Insufficient Input Validations in Initialize Functions

File: Steamer.sol, ZeroLiquid.sol

Status: Resolved

Description:

The initialize function in the Steamer and the ZeroLiquid smart contracts lack sufficient input validation. This function is responsible for setting up the initial state of the contracts, including setting up roles and initializing various state variables. However, it performs no checks on the input parameters such as _syntheticToken, _underlyingToken, and _buffer in the Steamer contract and in the ZeroLiquid contract this missing validation could allow the admin to set invalid values for the protocolFee parameter.

Code Affected:

//Steamer.sol 
function initialize(
    address _syntheticToken,
    address _underlyingToken,
    address _buffer
) external initializer {
   // no validations in place
    ...
}

//ZeroLiquid.sol 
function initialize(
 InitializationParams memory params
) external initializer {
    _checkArgument(params.protocolFee <= BPS); //no ranges enforced
    // no validations in place
    debtToken = params.debtToken;
    admin = params.admin;
    steamer = params.steamer;
    minimumCollateralization = params.minimumCollateralization;
    protocolFee = params.protocolFee;
    protocolFeeReceiver = params.protocolFeeReceiver;
    …
}

Impact:

Without proper input validation, the initialize function could be called with invalid or malicious parameters, leading to unexpected behavior or potential vulnerabilities. For example, if the _syntheticToken or _underlyingToken parameters are set to the zero address, this could lead to loss of funds or other unexpected behavior. Similarly, if the _buffer parameter is set to an address that does not implement the expected interface, this could lead to reverts or other unexpected behavior.

Recommendation:

Add appropriate input validations to the initialize function in both smart contracts. Ensure that the provided addresses are not zero and that the tokens have the expected properties (e.g., decimals). This will prevent the function from being called with invalid or malicious parameters.Similarly, for the ZeroLiquid contract, add checks to the initialize function to ensure that all parameters are valid. For example, you could check that the admin and steamer addresses are not zero and that the protocolFee is within a reasonable range.

Developer Response:

Doesn't need any unnecessary validations or concrete meaning of "insufficient" should be provided otherwise.

Auditor’s Response:

Acknowledged with no followup required.

Additional Comments:

A faulty initialization in both cases will lead to redeployment of the smart contract complex incurring additional gas cost and addresses recalculation.

6. Potential Underflow in conversionFactor Calculation

File: Streamer.sol

Status: Resolved

Description:

The conversionFactor is calculated as 10 ** (debtTokenDecimals - underlyingTokenDecimals). If underlyingTokenDecimals is greater than debtTokenDecimals, this will result in a revert due to underflow.

Code Affected:

conversionFactor = 10 ** (debtTokenDecimals - underlyingTokenDecimals);

Impact:

An underflow in the conversionFactor calculation during the initialization could lead to the failure of contract deployment. Since the initialize function is a crucial part of contract setup, an error during this stage would prevent the contract from being set up correctly. This would render the contract unusable, as the conversionFactor is a critical parameter used in various functions within the contract.

Recommendation:

Add a check in the initialize function to ensure that debtTokenDecimals is greater than or equal to underlyingTokenDecimals before calculating conversionFactor. This will prevent an underflow during the initialization and ensure that the contract is set up correctly. If the check fails, the function should revert with an appropriate error message, indicating that the initialization parameters are invalid.

Developer Response:

What is the potential range of numbers in which underflow occurs?

Auditor’s Response:

For all cases; where, debtTokenDecimals < underlyingTokenDecimals the call to initialize will fail leading to failure in executing the deployment script. Since there exists no check to ensure the reason of failure, debugging will add to overall complexity.

Developer Response:

In our case, debtTokenDecimals (zETH) is 18 & underlyingTokenDecimals (WETH) is also 18 therefore the condition debtTokenDecimals < underlyingTokenDecimals will never be true, but still an edge case was identified therefore, we agree this is a low severity issue.

Auditor’s Response:

Acknowledged with no follow-up required.

7. flashLoan Function Does Not Check If amount Is Zero

File: ZeroLiquidToken.sol

Status: Resolved

Description:

The flashLoan function does not check if the amount parameter is zero. If the amount is zero, this would result in a no-operation and waste gas.

Code Affected:

function flashLoan(
    IERC3156FlashBorrower receiver,
    address token,
    uint256 amount,
    bytes calldata data
)
    external
    override
    nonReentrant
    returns (bool)
{
    if (token != address(this)) {
        revert IllegalArgument();
    }

    if (amount > maxFlashLoan(token)) {
        revert IllegalArgument();
    }

    uint256 fee = flashFee(token, amount);

    _mint(address(receiver), amount);

    if (receiver.onFlashLoan(msg.sender, token, amount, fee, data) != CALLBACK_SUCCESS) {
        revert IllegalState();
    }

    _burn(address(receiver), amount + fee); // Will throw error if not enough to burn

    return true;
}

Impact:

If the flashLoan function is called with the amount parameter set to zero, this would result in a no-op and waste gas.

Recommendation:

Add a check to the flashLoan function to ensure that the amount parameter is not zero.

Developer Response:

Transaction will revert if amount is zero.

Auditor’s Response:

Acknowledged with no followup required.

Additional Comments:

Since the ERC20 standard allows a zero (0) mint, burn and transfers, the Zero Liquid Token contract does not have any additional constraints in place to support the claim that transaction will revert if amount is zero.

Informational Issues

8. Insufficient Testing of the Zero Liquid Protocol

File: **

Status: Acknowledged

Description:

Throughout the codebase, the testing is insufficient to support the positive and negative test cases from the Zero Liquid protocol engineering team. The substandard testing of such a complex smart contract suite hints at a critical flaw in the Zero Liquid protocol as many cases remain unexplored.

Impact:

The lack of comprehensive testing for the ZeroLiquid protocol can have several potential impacts:

1. Undiscovered Bugs and Vulnerabilities: Without thorough testing, there may be bugs or security vulnerabilities in the code that remain undiscovered. These could potentially be exploited by malicious actors, leading to financial loss for users or even the failure of the protocol.
2. Unpredictable Behavior: Insufficient testing can lead to unpredictable behavior in edge cases or under unusual market conditions. This could result in unexpected losses for users or other undesirable outcomes.
3. Maintenance and Upgrade Challenges: Without comprehensive tests, it can be more difficult to maintain the protocol or to make upgrades in the future. This could slow down the development process and make it harder to respond to changes in the market or the wider DeFi ecosystem.

Recommendation:

It is highly recommended to have a sufficient suite of test cases executed over the production-ready smart contract code to achieve the satisfaction of all functionality working as expected. Additionally, the testing can ensure edge cases are handled elegantly by the protocol ensuring a higher level of security.

Additional Comments:

The Zero Liquid Protocol contains a complex codebase and architecture with a lesser modularity associated as per the industry standard. Hence, sufficient testing refers to the fact that an exhaustive list of test cases be necessarily executed over the production ready code to, at the very least, have the happy test cases work as expected. Not having a test suite creates a lesser level of trust for security researchers as third party reviewers of the protocol's code.

9. Inefficient Mutexes and Insufficient Events

File: ZeroLiquid.sol

Status: Acknowledged

Description:

The setTransferAdapterAddress function in the ZeroLiquid contract uses a lock modifier commonly employed to prevent reentrancy attacks. However, given this function's behavior, the reentrancy risk seems minimal. Overusing the lock modifier can increase gas costs for the function calls. Additionally, the function does not emit an event after changing the state variable transferAdapter, a recommended practice for transparency and traceability.

Code Affected:

function setTransferAdapterAddress(
        address transferAdapterAddress
    ) external override lock {
        _onlyAdmin();
        transferAdapter = transferAdapterAddress;
    }

Recommendation:

• Remove the lock Modifier: Given that no ether is being sent or received in this function, and no external calls to untrusted contracts after a state change, the lock modifier might be unnecessary. Consider removing it to reduce gas costs.
• Emit an Event for State Changes: It's a good practice to emit events for significant state changes in a contract. Consider adding an event like TransferAdapterChanged and emit it when the transferAdapter address is updated.

Fixed-Code

event TransferAdapterChanged(address newAddress);

function setTransferAdapterAddress(
    address transferAdapterAddress
) external override { 
    _onlyAdmin();
    transferAdapter = transferAdapterAddress; 
    emit TransferAdapterChanged(transferAdapterAddress);
}

10. Insufficient Event Emission During Whitelist Update

File: ZeroLiquidToken.sol

Status: Acknowledged

Description:

The setWhitelist function updates the whitelist of addresses that are allowed to mint new tokens, but it does not emit an event when the whitelist is updated. This makes it harder to track changes to the whitelist.

Code Affected:

function setWhitelist(address minter, bool state) external onlyAdmin {
    whitelisted[minter] = state;
}

Impact:

Without an event being emitted when the whitelist is updated, it is harder to track changes to the whitelist. This could make it more difficult for users to understand who is allowed to mint new tokens, and it could make it harder to audit the contract.

Recommendation:

Consider emitting an event in the setWhitelist function when the whitelist is updated. This could include the address that was added or removed, and the new state of the whitelist.

11. Order of Layout Violated

File: ZeroLiquid.sol

Status: Acknowledged

Description:

The contract's layout order is violated from the Solidity docs. External functions that implement some functionality should be placed on top so that bytecode saves the code for interactive functions as soon as possible, incurring lesser gas costs to storage when it tries to fetch the code of the function.

Code Affected:

Various parts of the contract.

Impact:

The layout order in the contract does not affect its functionality, but it could increase the gas cost for storage and execution.

Recommendation:

Consider reordering the functions in the contract according to the Solidity docs. Place external functions that implement some functionality on top.

12. Structs Should Be Moved to Interface for Increased Modularity

File: Streamer.sol

Status: Acknowledged

Description:

Several structs are defined in the Steamer contract that could be moved to the ISteamer interface. This would increase the modularity, reusability, and readability of the code.

Code Affected:

struct Account {...}
struct UpdateAccountParams {...}
struct ExchangeCache {...}

Impact:

By defining these structs in the Steamer contract rather than the ISteamer interface, the code is less modular and reusable. Other contracts that interact with the Steamer contract might need to define their versions of these structs, leading to code duplication and potential inconsistencies.

Recommendation:

Move the definition of these structs to the ISteamer interface. This will make the code more modular and reusable and ensure that other contracts that interact with the Steamer contract can use the same definitions of these structs.

13. Typo in Variable Name

File: Streamer.sol

Status: Acknowledged

Description:

There is a typo in the variable name normaizedAmount in the exchange function. This could lead to confusion for developers reading or maintaining the code.

Code Affected:

uint256 normaizedAmount = _normalizeUnderlyingTokensToDebt(amount);

Recommendation:

Correct the typo in the variable name normalized Amount to normalizedAmount. This will improve the readability of the code.

14. Inefficient Initialization of isPaused Variable

File: Streamer.sol

Status: Acknowledged

Description:

The isPaused variable is initialized to false in the initialize function, which is unnecessary as boolean variables in Solidity are false by default.

Code Affected:

isPaused = false;

Impact:

This unnecessary initialization of isPaused to false results in additional gas costs during contract deployment. While the impact is low, it is a waste of gas and could be easily avoided.

Recommendation:

Remove the unnecessary initialization of isPaused to false in the initialize function. This will reduce the gas cost of contract deployment.

15. Missing Input Validation in setTransferAdapterAddress Function

File: ZeroLiquid.sol

Status: Acknowledged

Description:

The setTransferAdapterAddress function does not perform any checks on its parameter. This could allow the admin to set the transfer adapter address to zero.

Code Affected:

function setTransferAdapterAddress(
    address transferAdapterAddress
) external override lock {
    _onlyAdmin();
    transferAdapter = transferAdapterAddress;
}

Impact:

If the setTransferAdapterAddress function is called with the zero address, this could cause the contract to be in an incorrect state. This could lead to unexpected behavior and potential loss of control over the contract. Additionally, it will incur gas costs to reset the contract with the correct values to call the relevant functions again.

Recommendation:

Add a check to the setTransferAdapterAddress function to ensure that the address parameter is not the zero address.

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 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 another 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 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 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 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 the security of smart contracts.

Introduction

BlockApex (Auditor) was contracted by DeFiGeek Community (Client) for the purpose of conducting a Smart Contract Audit/ Code Review. This document presents the findings of our analysis, which started on 11th July ‘2023.

Scope

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

Project Overview

DeFiGeek Community (DFGC) is an open community developing DeFi DApps and middleware contributing to the Web3 era. DFGC develops and envisions multiple DApps. The first is the Yamato Protocol, a crypto-secured stablecoin generator DApp pegged to JPY. Yamato Protocol is a lending decentralized financial application (DeFi) that can generate the Japanese Yen stablecoin "CJPY". DeFiGeek Community Japan, a decentralized autonomous organization, is developing it.

Yamato Protocol is a crypto-asset overcollateralized stablecoin issuance protocol. V1 allows the issuance of CJPY (Japanese Yen equivalent coin) using ETH as collateral. It has the following features:

• No immediate forced liquidation
• No obligation to repay debt
• No interest rates (lump-sum fees)
• Low secured rates (minimum 130%)

NOTE: The current audit scope covers a smart contract consisting of Yamato
PriceFeedV3, which is a feature update from the previously audited version of the
Yamato protocol.

Yamato PriceFeedV3

Yamato PriceFeedV3 is a version update from its predecessor PriceFeedV2 where the main update is the removal of a secondary (backup) oracle to fetch the prices. The latest PriceFeed smart contract now relies on Chainlink as its sole provider of the latest prices to determine the flow of the DeFi protocol.

System Architecture

The idea for DeFiGeek Community’s Yamato Protocol is based on Maker and Liquidity protocols—crypto over-collateralized stablecoin render. Token Economics implements the Voting Escrow model with reference to Curve's ecosystem of CRV and veCRV (This will be implemented in a later version).

The initial model will be launched on Ethereum, and by depositing ETH, you can borrow a Japanese Yen stablecoin called CJPY (ERC-20).

As a P2C (Peer to Contract) Lender, it is an epoch-making design that eliminates the need for forced clearing. It is a basic application of web3 as a decentralized application that can borrow Japanese Yen equivalent coins (Convertible JPY (ticker: CJPY)) with ETH as collateral. In V2, DFGC will implement CUSD and CEUR and further develop as a global stablecoin render.

The smart contract feature update for PriceFeedV3 is dependent on a few interfaces that define the architecture of the smart contract. The main contract PriceFeedV3 offers Chainlink as its sole price oracle to keep track of the latest prices within the Blockchain ecosystem.

Yamato PriceFeedV3

The contract removes a set of functionalities along with the attached dependencies to ensure that the new architecture is based only on Chainlink. The contract PriceFeedV3 imports the following interfaces and manages the relevant data structures that support the feature update in the smart contract;

• ./Interfaces/IPriceFeedV3.sol
• ./Interfaces/IPriceFeedFlexV3.sol

Along with the above relevant interfaces, the feature update also covers a couple of other interfaces, namely IPriorityRegistryFlexV6.sol and IPriorityRegistryV6.sol that introduce a data structure to support the previous functionalities.

Methodology & Scope

The codebase was audited using a filtered audit technique. A band of two (2) auditors scanned the codebase in an iterative process for a time spanning six (6) days.

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 to find logical flaws in the codebase complemented with code optimizations, software, and security design patterns, code styles, best practices, and identifying false positives detected by automated analysis tools.

Audit Report

Executive Summary

Our team performed a technique called Filtered Audit, where two individuals separately audited the Yamato Protocol (Feature Update).

After a thorough and rigorous manual testing process involving line-by-line code review for bugs, an automated tool-based review was carried out using Slither for static analysis and Foundry for fuzzing invariants.

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 Issues(s)
0	High Risk Issues(s)
0	Medium Risk Issues(s)
1	Low Risk Issues(s)
4	Informatory Risk Issues(s)

Key Findings

Detailed Overview

Low-risk issues

1. Existence of Unused Function

File: contracts/PriceFeedV3.sol

Status: Fixed

Function Name: _changeStatus

Description: 

The function _changeStatus was identified in the codebase. This function, which was previously utilized in the fetchPrice function, has been replaced by a newly introduced function, _storePrice. The existence of unused functions can lead to unnecessary code complexity and potential security risks.

Code Affected:

function _changeStatus(Status _status) internal {
  if (lastSeen == block.number) return;
  if (status == _status) return;
  status = _status;
  emit PriceFeedStatusChanged(_status);
}

Impact:

Unused functions increase the complexity of the codebase, making it harder to read and maintain. They can also pose potential security risks if they contain overlooked vulnerabilities because the function is not in use.

Recommendation:

To ensure optimal code efficiency and security, removing the unused function _changeStatus from the codebase is advised.

Informatory Issues and Optimizations

2. Absence of Dev Comments in Interfaces

File: contracts/Interfaces/IPriceFeedV3.sol

Status: Fixed

Description:

The interfaces in the codebase lack developer comments. Proper NatSpec comments are considered best practice, providing valuable context and understanding for developers and auditors reviewing the code.

Code Affected:

pragma solidity 0.8.4;
interface IPriceFeedV3 {
   function fetchPrice() external returns (uint256);
   function getPrice() external view returns (uint256);
   function lastGoodPrice() external view returns (uint256);
}

Impact:

The absence of developer comments in interfaces can make the code harder to understand for other developers or auditors, increasing the likelihood of misunderstandings and errors. It can also make the codebase harder to maintain and extend in the future.

Recommendation:

To enhance code readability and maintainability, it is recommended to add appropriate NatSpec comments to the interfaces in the codebase.

3. Unnecessary Wrapper Function

File: contracts/PriceFeedV3.sol

Status: Fixed

Function Name: _chainlinkIsBroken

Description:

There is a function called _chainlinkIsBroken, which is a wrapper function. The visibility of this function is internal; this function calls another internal function _badChainlinkResponse, which seems unnecessary. Since, the feature is now updated to handle only one price oracle, i.e. Chainlink, the internal functions are not called in differing cases throughout the smart contract, and therefore, leaving the function hierarchy to more depth is not recommended.

Code Affected:

function _chainlinkIsBroken(
  ChainlinkResponse memory _currentResponse
) internal view returns (bool) {
  return _badChainlinkResponse(_currentResponse);
}

Impact:

Redundant wrapper functions increase the complexity of the codebase and can lead to confusion about the code's intended behavior. They can also increase the gas cost of transactions if they are called frequently.

Recommendation:

To streamline the code and reduce complexity, removing the _chainlinkIsBroken function and renaming _badChainlinkResponse to _chainlinkIsBroken is suggested.

4. Commented Code in fetchPrice Function

File: contracts/PriceFeedV3.sol

Status: Fixed

Function Name: fetchPrice

Description:

The fetchPrice function has been modified, and a new function, _storePrice, has been introduced. However, remnants of the previous code remain in the form of commented lines. This can lead to confusion and potential errors in future code modifications.

Code Affected:

function fetchPrice() external override returns (uint256) {
  uint256 _price = _simulatePrice();
  // _changeStatus(_status);
  _storePrice(_price);
  // if (_isAdjusted) {
  // lastAdjusted = block.number;
  // }
  return _price;
} //remove commented code

Impact:

Commented-out code in a function can lead to confusion about the function's intended behavior and the evolution of its implementation. This can make future modifications more error-prone and can lead to bugs if the commented code is accidentally uncommented or not properly updated during refactoring.

Recommendation:

Removing the commented code from the fetchPrice function is recommended to maintain code clarity and prevent potential misunderstandings.

5. Presence of Commented State Variable

File: contracts/PriceFeedV3.sol

Status: Fixed

Description:

Upon careful examination of the code, it has been observed that there exists a state variable that has been commented out. This is considered a code hygiene issue and can lead to confusion for future developers or auditors who may review this code.

Code Affected:

// uint256 public lastAdjusted; //remove this declaration

Impact:

The presence of commented-out code can lead to confusion and misinterpretation of the code's functionality. It can also make the codebase harder to maintain and increase the likelihood of errors during future development or refactoring.

Recommendation:

To maintain the highest standards of code cleanliness and readability, it is recommended that this commented state variable be removed from the codebase.

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 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 another 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 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 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 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 the security of smart contracts.

Introduction

BlockApex (Auditor) was contracted by ScriptTV (Client) for the purpose of conducting a Smart Contract Audit/ Code Review. This document presents the findings of our analysis which started on 30th Jan ‘2023.

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

Script.TV is a decentralized video delivery network that furnishes an expansive range of blockchain-enabled solutions to the problems related to the traditional video-streaming sector. The platform offers high-quality video streaming as well as multiple incentive mechanisms for decentralized bandwidth and content-sharing at a reduced cost as compared to conventional service providers. Script.TV offers an online TV platform in which both users and content publishers earn valuable tokens through video streaming. A user-first watch-2-earn solution that allows users to earn rewards on- and off-chain by upgrading their ScriptGlasses NFT.

System Architecture

The workflow of the protocol is that of a Watch to Earn. A user can mint three types of glasses (Common, Rare & SuperScript) by burning $SPAY where each type has two base attributes i.e. Durability and Gem tier. The durability of the glass increases as it levels up (via watching content) and the gem tier increases as gems are integrated with the glass rewarding users with $SPAY.

To onboard users, the protocol will follow a freemium model where 100,000 common glasses will be minted for free. These common glasses will have a fixed allowable watch time and will yield a fixed payout i.e., equivalent to a base common glass with no gems. The free mints phase will operate simultaneously with paid mints. To differentiate the free mint from the paid ones, there are certain milestones that are pegged to watch time enabling a user to modify their glasses and enhance their reward/ unit energy. Users can watch content on the platform for as long as they want, however, there are limits to the daily watch time to earn SPAY tokens.

Recharge vouchers add to the gamification element in the protocol and also serve as an incentivization mechanism for the users to upgrade their levels in order to avail of a discount on their recharge cost. To mint the recharge voucher, the user will have to burn their SPAY tokens. This is an optional burnable event as the user can choose not to purchase the recharge voucher after the eligibility criteria are met.

Methodology & Scope

The codebase was audited using a filtered audit technique. A band of three (3) auditors scanned the codebase in an iterative process for a time spanning one (1) week. Starting with the recon phase, a basic understanding was developed and the auditors worked on developing presumptions for the shared 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 & Mythril for static analysis and Foundry for fuzzing invariants. All the flags raised were manually reviewed and re-tested to identify the false positives.

Our team found

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

Key Findings

Detailed Overview

Critical-risk issues

1. Incomplete Voucher Release

Path: contracts/ScriptTV.sol

Description

The issue of ungraceful handling of voucher association against an unowned _glassID highlights a flaw in the voucher release mechanism. The associatedVouchers mapping is maintained using the msg.sender as the key and the glass ID as the value. When the releaseVoucher function is called, it sets the value in the associatedVouchers mapping for the key msg.sender and the glass ID to false, indicating that the voucher has been released. Since the protocol does not restrict users to associate multiple vouchers to a glass when multiple vouchers have been associated with the same glass and one voucher has been released, the associatedVouchers mapping for that glass will be set to false, leading to the code to revert an AssetNotOwned error when attempting to release another voucher for that glass. This results in an incomplete voucher release and the inability to release the rest of the vouchers for that glass.

Recommendation

Place a constraint in the associateVoucher function to always first check whether the _glassId being provided as a function argument does not have a voucher associated against the msg.sender.

High-risk issues

2. Overly Centralized Functionalities

Path: contracts/ScriptTV.sol

Description

The issue of signature malleability in the case of a compromised owner is a major security concern as it highlights the overly centralized nature of the system's functionalities. The script.TV owner is set during deployment, and a compromised owner could exploit this by equipping gems on the freemium token and removing unlimited SPAY from the earning pool reward and payout since these actions are unchecked. This level of centralization puts the whole protocol at risk, as the compromise of a single owner could bring down the entire system.

Recommendation

To mitigate this risk, it is necessary to implement proper checks and security measures and reduce the number of centralized functionalities in the system. Where a valid data structure is designed to handle.

Medium-risk Issues

3. Misconfigured Treasury Allocation

Path: contracts/ScriptTV.sol

Description: This issue of poorly sanitized treasury configurations highlights a potential flaw in the way funds are being managed within the protocol. An admin with the ability to set the fee can set it to 100, leading to the situation where all user funds are directed into the protocol fee. Furthermore, if the treasuryPercentage is set to 100, it is implied that 100% of the user’s payout/ reward amount passed to the earningPayout function will be sent to the treasury, and none will be sent to the sender. This misconfigured Treasury allocation could result in users not receiving any payouts, which could harm the reputation and sustainability of the project. It is important to thoroughly consider and properly configure the Treasury settings to align with the intended purpose and goals of the project, and to
ensure fairness to all parties involved.

Code:

/**
* @notice change the percentage of treasury cut
* @param _newPercentage new percentage for treasury cut
*/
function setTreasuryPercentage(uint256 _newPercentage) external onlyOwner {
 uint256 oldPercentage = treasuryPercentage;
 if (_newPercentage < 0 || _newPercentage > 100 ether) {
  revert PercentageOutOfRange();
 }
 if (_newPercentage == oldPercentage) {
  revert SameAsOld();
 }
 treasuryPercentage = _newPercentage;
 emit PercentageUpdated(oldPercentage, _newPercentage);
}

Recommendation: Place proper and sensible upper and lower limits for the percentage of the user’s total earnings which the treasury is eligible for in any and all cases.

4. Inapplicability of Checks-Effects-Interactions Pattern

Path: contracts/.sol, contracts/base/.sol

Description

The check-effects-interactions pattern being the first line of defense ensures that checks and validations are performed before any changes or interactions are made with external contracts, reducing the risk of reentrancy in case other contracts are compromised. Failing to properly enforce this pattern can result in unintended consequences, such as potential reentrancy and security breaches. The following of the smart contracts code base is vulnerable to all the attack surfaces caused due to missing implementation of the design pattern:

• ScriptVoucher
  • mint
• ScriptTV
  • mintFreemiumGlasses
  • mintGlasses
  • releaseVoucher
  • associateVoucher
• ScriptGlasses
  • safeMint

Recommendation

To avoid these risks, it is essential to properly implement and enforce the checks-effects-interactions pattern in the system to ensure the safe and secure handling of external calls.

Low-risk Issues

5. Redundant Duplicate Constraints

Path: contracts/ScriptTV.sol

Description

The implementation of the setTreasuryPercentage function includes a check to ensure that the provided uint value is never less than 0. This check is unnecessary, as the uint data type in solidity can only contain non-negative values.

The inclusion of this check adds unnecessary complexity to the code and can lead to confusion for developers. Additionally, setTreasury checks for function argument _newTreasury to never be a zero address in two different ways as attached below.

Code

function setTreasuryPercentage(uint256 _newPercentage) external onlyOwner {

 if (_newPercentage < 0 || _newPercentage > 100 ether) {
  revert PercentageOutOfRange();
 }
 function setTreasury(address _newTreasury) external onlyOwner {
  address oldTreasury = treasury;
  require(_newTreasury != address(0), "New Treasury is the zero address");
  if (_newTreasury == address(0)) {
   revert ZeroAddress();
  }

Recommendation

To improve code readability and maintainability, it is recommended to safely remove the above mentioned redundant checks.

6. Emit events for important operations

Path: contracts/ScriptTV.sol

Description

In the current implementation of Script.TV smart contracts, significant blockchain write actions are not recorded as logged events which are imperative to the perpetuating history of blockchain transactions.

Recommendation

It is suggested to emit relevant and self explanatory events with appropriate arguments in all user-facing functionalities.

7. Insufficient and Inadequate testing

Path: contracts/ScriptTV.sol

Description

Throughout the codebase, the testing is insufficient to support the positive and negative test cases from the protocol engineering team. The low level of testing points to a critical lacking in the project as many cases are unexplored.

Recommendation

It is therefore recommended to have a sufficient suite of test cases executed over the production-ready smart contract code to ultimately achieve the satisfaction of all functionality working as expected.

Informational-risk Issues

8. Inconsistent Parameter Order in ScriptVoucher.mint

Path: contracts/base/ScriptVoucher.sol

Description

The function takes in parameters in a non-standard order, where the first parameter is the type of Voucher, and the second parameter is the address. This may cause confusion and increase the likelihood of errors, as it deviates from the standard parameter order used in similar functions.

Recommendation

It is recommended to change the order of the parameters to be (address, type) to align with industry standards and improve overall code clarity and maintainability.

9. Unstructured Enum Placement

Path: contracts/utils/ScriptNFTType.sol

Description

In the ScriptTV protocol, there is an incosistency with the enum data structures. The VoucherType enum is located in the file ScriptVoucher.sol instead of being in a dedicated file for enums. This could lead to overlapping and a lack of structure in the directory.

Recommendation

To avoid this, it is recommended to rename the file utils/ScriptNFTType.sol to utils/enums.sol and move the VoucherType enum to the new file. This will enforce a proper structure of the directory and prevent any potential issues in the future.

10. Optimized Enum Indexing for Glass and Voucher Types

Path: contracts/ScriptTV.sol

Description

To enhance the reliability of the code, it is recommended to utilize a freemium mode for glasses at the last index of the enum definition. This helps ensure that once the available options are exhausted, they will remain unused. The current code blocks make use of a relation between glassType and voucherType, where the glassType is used as +1 and -1 in relation to the voucherType.

Recommendation

To avoid potential arithmetic faults, it is suggested to use a 1-to-1 indexing system for common, rare, and superscript glass and voucher types. This ensures that the enumeration is optimized for improved performance and eliminates the risk of off-by-one issues.

11. Improper Event Emissions in Contract Functions

Path: contracts/ScriptTV.sol

Description

This improvement focuses on optimizing the error handling of stack too deep that occurs in events and improving the efficiency of the code by using scoping blocks. This will ensure that the correct data is emitted onto the blockchain in a clear and concise manner. The current code emits data in the format of RechargeGlasses with the variables msg.sender, discountedAmount, _glassId, and _nonce.

Recommendation

This design flaw can be changed to a more relevant set of variables that are emitted in a more user-friendly format, such as GlassesRecharged with the variables msg.sender, _glassId, and discountedAmount. This change will make the data emitted on the blockchain easier to understand and more readable for users.

12. Optimization of Events’ Naming Conventions

Path: contracts/ScriptTV.sol

Description

In order to improve the readability and transparency of actions performed on the blockchain, it is recommended to rename all events to be self-explanatory and have the parameters ordered in a logical manner. For example, instead of "RechargeGlasses" it could be renamed to "GlassesRecharged", and the parameters could be ordered as (msg.sender, _glassId, discountedAmount) instead of (msg.sender, discountedAmount, _glassId, _nonce).

This will improve the overall clarity and efficiency of the smart contract

• GlassesConfiscated(…)
• PayoutEarned(…)
• Rename GlassMinted(…) to GlassesMinted(…)

Recommendation

It is highly suggested to follow as above.

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 another 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 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 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 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 the security of smart contracts.

Introduction

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

Jump Defi infrastructure built on NEAR Protocol, a reliable and scalable L1 solution. Jump Defi is a one-stop solution for all core Defi needs on NEAR. Jump ecosystem has a diverse range of revenue-generating products which makes it sustainable. 

Jump Defi’s diverse range of protocols increases the visibility of other projects on NEAR which potentially increases the mass adoption of NEAR.

System Architecture

Jump Dex (Out of Scope)

is a fully permissionless automated market maker in which a user can trade or become a liquidity provider on the NEAR protocol to earn platform rewards.

Jump Dex AMM is powered by two constant product functions, inspired by Uniswapv2 and Curve Finance: 

• Swap function: x * y = k
• StableSwap function: χDn−1 * ∑ xi + ∏ xi = χDn + ( D / n )n 

Jump Pad

Provides a manner for cryptocurrency projects to raise liquidity via Initial DEX Offering (IDO) and private sales. 

Jump Pad IDO has two rounds, round one is for JUMP token holders and round two is open to the public to purchase any unsold tokens on a first come first serve basis. 1% of fees on IDO raises which uses to buy and deposit JUMP into the xJUMP pool

JUMP Pad private sales are for VC Investors only. The fees on private sales are decided on a project-by-project basis which also uses to buy and deposit JUMP into the xJUMP pool

Jump Token Laboratory 

Anyone can create a NEP-141 fungible token with the standard using Jump token laboratory with customizable code and intelligent tokenomics. 

Jump NFT Staking

It provides a staking-as-a-service infrastructure for NFT collection to efficiently structure fungible token rewards for their NFT holders. Approved NFT collection can have triple token rewards. 

Methodology & Scope

The codebase was audited using a filtered audit technique. A band of four (4) auditors scanned the codebase in an iterative process spanning over a time of two (2) weeks. 

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 cargo audit & cargo-tarpaulin for static analysis and cargo-fuzz for fuzzing invariants. All the flags raised were manually reviewed and re-tested to identify the false positives. 

Our Team Found

# of issues	Severity Of the Risk
5	Critical Risk Issues(s)
2	High Risk Issues(s)
4	Medium Risk Issues(s)
4	Low Risk Issues(s)
7	Informatory Risk Issues(s)

Key Findings

Detailed Overview

Critical-risk issues

1. Improper access controls leads to liquidity theft

File: launchpad/src/actions/dex_launch_actions.rs

Description:  
Users can add liquidity to partner_dex once the LOCK_PERIOD has expired. Although this ensures the public launch on a specified date, it introduces a sandwich attack where attackers can steal the liquidity.
To add liquidity, a user has to perform the following steps

1. Tx1: launch_on_dex → matches ListingStatus::SaleFinalized → cross-contract call to create pool → update the status to ListingStatus::PoolCreated 
2. Tx2: launch_on_dex → matches ListingStatus::PoolCreated → cross-contract call to send Project token → update the ListingStatus::PoolProjectTokenSent 
3. Tx3: launch_on_dex → matches ListingStatus::PoolProjectTokenSent → cross-contract call to send price token → update ListingStatus::PoolPriceTokenSent
4. Tx4: launch_on_dex → matches ListingStatus::PoolPriceTokenSent → cross-contract call to add liquidity → update ListingStatus::LiquidityPoolFinalized

Background:
We assume the attack scenario by building the underlying assumption of DEX like Uniswap V2. Uniswap V2 adds liquidity into the pool in three steps in a single transaction. 

1. It takes token A then & sends it to the pool.
2. It takes token B then & sends it to the pool.
3. It calls the `mint` function which actually mints the LP shares to the caller

These steps are necessary to be called in an atomic transaction, uniswap achieves atomicity through its periphery contract. See the code below from uniswapV2 periphery contract.

Code Reference: Uniswap V2 repository

function addLiquidity(address tokenA, address tokenB, uint amountADesired,
        uint amountBDesired, uint amountAMin, uint amountBMin,address to,
        uint deadline
    ) external virtual override ensure(deadline) returns (uint amountA, uint amountB, uint liquidity) {
        (amountA, amountB) = _addLiquidity(tokenA, tokenB, amountADesired, amountBDesired, amountAMin, amountBMin);
        address pair = UniswapV2Library.pairFor(factory, tokenA, tokenB);
		  // Below are the 3 steps in a single tx
        TransferHelper.safeTransferFrom(tokenA, msg.sender, pair, amountA);
        TransferHelper.safeTransferFrom(tokenB, msg.sender, pair, amountB);
        liquidity = IUniswapV2Pair(pair).mint(to);
    }

Since launch_on_dex needs 4 transactions to add liquidity, the attacker can create the following smart contract to steal the liquidity.

Code: 

pub fn steal_liquidity(victim: AccountId, listingId: u64, partner_dex:AccountId) -> Promise {
  ext_dex::ext(victim)
          .with_static_gas(DEX_INTERACTION_GAS)
          .with_attached_deposit(deposit)
          .launch_on_dex(
            listingId
          ); // call this 3 time, so that both tokens are transferred to dex, instead calling Tx:4 attacker directly adds liquidity to the dex
 ext_dex::ext(partner_dex)
          .with_static_gas(DEX_INTERACTION_GAS)
          .with_attached_deposit(deposit)
          .add_liquidity(
            listing.dex_id.unwrap(),
            vec![
              U128(listing.dex_project_tokens.unwrap()),
              U128(listing.dex_price_tokens.unwrap()),
            ],
            None,
          )
}

Recommendation: 
Ensure the atomicity in all 4 transactions by following the UnswapV2 periphery logic .

Dev’s Response #1:
Not Applicable -> alternative measures taken

The auditors claim that liquidity can be stolen in case the attacker does not complete the add liquidity lifecycle and that the transactions should be atomic in order to preserve security. However, these claims are based on the model followed by Uniswap V2 code on EVM based blockchains.
The Near Protocol has some properties that disallow such atomicity from being achieved, given that cross contract calls are performed across different shards through promises processed in different blocks, hence cross contract calls CANNOT preserve atomicity.
Moreover, the reference dex implementation, on which we are basing the code comes from ref.finance. In this model, it is necessary to perform 4 transactions in order to create a pool:

1. create pool
2. Add token 1 to launchpad's internal account in ref.finance
3. Add token 2 to launchpad's internal account in ref.finance
4. Take tokens from launchpad's internal balance and add them to liquidity in the pool created in (1)

In this model, there is no way to steal the liquidity deposited in steps 2 and 3 since it is locked in an internal account inside ref's contract that can only be accessed by its owner (the launchpad contract). If any other account tries to call add_liquidity their transaction will fail since they do not have the necessary internal balance deposited.
However, given that the JUMP DEX is currently not operational and that until it is the automatic generation of liquidity pools should not be used inside listings, we have changed the launch_on_dex method to always panic and made it impossible to set automatic IDOs for new sales created.
The code will be updated to revert the changes as soon as the JUMP DEX is launched and a full integration can be built.

Auditors’ Response #1:
After reviewing the ref.finance dex code, auditors agree with the devs. However, there is no functionality to remove liquidity from dex in Jump which might result in liquidity being permanently locked in the dex contract.

Dev’s Response #2:
This is precisely the idea, the project would be committing part of their capital permanently to providing liquidity on the DEX. This is equivalent to burning LP shares

Auditors’ Response #2:
The auditors agree with the devs.

2. Inexistent logic to unregister storage

File: Nft_Staking/src/actions/storage_impl.rs

Description: 

In Nft_Staking/src/actions/storage_impl.rs  the storage_unregister function will always fail at assertion forbidding investors to unregister storage.

Code:

#[payable]
  fn storage_unregister(&mut self, force: Option<bool>) -> bool {
    assert_one_yocto();
    let account_id = env::predecessor_account_id();
    if let Some(account_deposit) = self.internal_get_investor(&account_id) {
      // TODO: figure out force option logic.
      // assert!(account_deposit.allocation_count.is_empty(), "{}", ERR_203);
      assert!(false); // will always revert
      self.investors.remove(&account_id);
      Promise::new(account_id.clone()).transfer(account_deposit.storage_deposit);
      true
    } else {
      false
    }

 Recommendation: Remove assertion.
 Alleviation: This issue is fixed.

3. Inadequate implementation leads to loss of rewards

File: Staking/src/staking.rs

Description: 

Stakers can claim $JUMP as their staking rewards by calling the claim function. The function pulls the user's data and calculate the latest rewards as follows

user.unclaimed_rewards + ((user.balance * (contract_rps - user.user_rps)) / FRACTION_BASE)

Apparently at line#45, the function then updates the unclaimed amount as user.unclaimed_rewards = 0; before making the transfer call at line#51. Although setting unclaimed rewards to 0 is required, the current implementation leads to a critical flaw where the user gets nothing as the token_contract::ft_transfer function uses the latest amount for unclaimed rewards. See the code snippet below

 pub fn claim(&mut self) -> Promise {
   ...
    user.unclaimed_rewards = 0; //@audit 

    self.user_map.insert(&account_id, &user);

    token_contract::ft_transfer(
      account_id.to_string(),
      U128(user.unclaimed_rewards), //@audit sends 0
      "Claimed #{amount}".to_string(),
      &self.token_address,
      1,
      BASE_GAS,
    )
  }

Recommendation:
Use a temporary variable to store unclaim rewards

Dev’s Response:
The issue is acknowledged, however the staking contract will be discontinued as the team opted to go along only with the xJUMP model for token staking.
The staking contract has been excluded from the repository.

Alleviation:
The developers removed the staking contracts from the repository making it inapplicable.

4. Insufficient Access Controls on NFT Minting

File: locked_token/src/actions/user.rs

Description: 

The current implementation uses the internal_mint function to mint new NFTs by calling nft_mint function.  Internal functions generally do not implement validations of any kind and rely on public wrappers to ensure security controls. The following code snippet exhibits the lack of proper access controls.

pub fn nft_mint(&mut self, receiver_id: Option<AccountId>) -> Token {
    let account_id = receiver_id.unwrap_or(env::predecessor_account_id());

   ...

    let token = self
      .tokens
      .internal_mint(token_id, account_id, Some(metadata)); //@audit no-auth checks

    token
  }

Recommendation:
Use mint functionality from NEP-171 which has a vetted codebase containing appropriate access controls.

Dev’s Response:
Issue refers to testnet only contract, no need to implement

Alleviation:
Out-of-Scope as it is used for testing.

5. Users will not be able to withdraw their locked tokens even after they are unlocked

File: locked_token/src/actions/user.rs

Description:
Gas for `FT_TRANSFER_GAS` & `FT_TRANSFER_CALLBACK_GAS` is set to `zero` in `withdraw_locked_tokens` function. This will always result in cross-contract call failure.

Recommendation:
Set FT_TRANSFER_GAS & FT_TRANSFER_CALLBACK_GAS to standard cross-contract gas.

Alleviation:
The issue is fixed by allocating enough gas for a successful transaction.

High-risk issues

6. Improper Accumulation of Rewards

File: Nft_Staking/src/actions/staker.rs

Description: 

Users can earn rewards by staking their NFTs. It is worth noting that code documentation says about claiming rewards

The existence of these transactions is purely for a technical reason. When combined they allow a staker to withdraw their rewards. While inner_withdraw transfers a Staked NFT Balance to the respective Staker Balance, outer_withdraw allows the staker to withdraw a specific fungible token from their Staker Balance to their Staker Wallet.

As the Near API allows for batch calls, this won't be noticed by the user in the frontend. However CLI users need to be aware of these details

Although the above statements informs CLI users to be careful, in the current implementation rewards would be lost if users called the claim_reward function more than once before calling the withdraw_reward. 

The issue arises due to the way the balances are updated in claim_reward. withdraw _reward utilizes the StakingProgram::stakers_balances to get the claimable amount. StakingProgram::stakers_balances is updated by adding new_rewards and StakedNFT::balance. However when the claim function is called StakedNFT::balance is set. Therefore when a claim function is called again, StakingProgram::stakers_balances is updated only with new_rewards, losing the previous rewards

Code:

pub fn withdraw(&mut self) -> FungibleTokenBalance {
    let withdrawn_balance = self.balance.clone();
    for (_, v) in self.balance.iter_mut() {
      *v = 0; //@audit, balance set to zero
    }
    withdrawn_balance
  }

pub fn inner_withdraw(&mut self, token_id: &NonFungibleTokenID) -> FungibleTokenBalance {
    let mut staked_nft = self.claim_rewards(token_id);
    let withdrawn_balance = staked_nft.withdraw(); //@audit updates the states to zero
    let owner_id = &staked_nft.owner_id;
...   
    self.stakers_balances.insert(&owner_id, &balance); //@audit only updated with new balance
    self.staked_nfts.insert(&token_id, &staked_nft);

    balance
  }

Recommendation: 
We suggest to implement one of the following

1. Instead of replacing self.stakers_balances.insert(&owner_id, &balance), it should add the newer rewards to the old.
2. There should be a mutex here, which would not allow the inner_reward to be called again until outer_reward is called.

Alleviation:
The issue is fixed by following recommendation #1.

7. Inefficient NFT staking reward distribution

File: Nft_Staking/src/models/farm.rs

Description: 

The function for reward calculations for NFT Staking is flawed in many ways. 

1.When a user stakes NFT, Farm::nfts_rps should be set to updated value Farm::distribution.rps. So when the rewards are claimed, the user gets his fair share. But in current implementation, the Farm::nfts_rps is set to the older value of  Farm::distribution.rps. This implementation is flawed because this enables the user to claim extra rewards.  As we see in the code mentioned below Farm::nfts_rps is updated before updating  Farm::distribution.rps

CODE:

pub fn add_nft(&mut self, nft_id: &NonFungibleTokenID) {
    let mut balance = HashMap::new();

    for (ft_id, dist) in self.distributions.iter() {
      balance.insert(ft_id.clone(), dist.rps);
    }

    self.nfts_rps.insert(nft_id, &balance);

    self.distribute(); // TODO: confirm this is a bug, and should in fact happen before RPS assignment
  }

         Consider the following scenario for better understanding

// at round 2 rps was 10
round = 2
Farm::distribution.rps = 10

// at round 5, the user stakes his NFT
round = 5
Farm::distribution.rps = 10 + 3 rounds rewards which greater than 10
Farm::nfts_rps = 10 // set to older value


//at round 5, the user immediately claims his rewards
Farm::distribution.rps = 10 + 3 rounds rewards which is greater than 10
reward = Farm::distribution.rps - Farm::nfts_rps  = 10 + 3 rounds rewards - 10 > 0

expected_rewards = 0 //since claimed rewards in same transaction
actual_rewards > 0

Recommendation: 
There is already a TODO left. Changing the call order would fix the issue.

//current implementation 
self.nfts_rps.insert(nft_id, &balance);
self.distribute(); 
//fix
self.distribute(); 
self.nfts_rps.insert(nft_id, &balance);
Alleviation: The issue is fixed

Alleviation:
The issue is fixed

2.The staker can claim the reward for the entire round even staking for a very short duration discouraging users to stake for the entire round. This is due to the fact the reward is calculated on a per round basis.

pub fn claim(&self, token_rps: U256) -> (Self, u128) {
    let mut dist = self.clone();
    let claimed = denom_convert(self.rps - token_rps); // see below

    dist.unclaimed -= claimed;

    (dist, claimed)
  }

Consider the following scenario for better understanding.
Let suppose the following variables

round_interval = 10 mins
reward_per_round = 10 tokens 
Duration passed = 29 mins // from farm.start_at
NFTs staked (seed) = 3

AT t=29mins the user(attacker) stakes an NFT increasing the total_seeds to 4. The  global variables will be updated as follows

total_passed_rounds = delat_t/round_interval = 29/10 = 2 round
added_reward = tolata_passed_rounds * reward_per_round = 10 tokens
Prev_rsp = 2 * 10 / 3 = 6.66
Farm::distributions.rps = prev_rps + added_reward/total_seeds = 6.66 + 10/4 = 9.1
Farm::nfts_rps_<nft_id> = 9.1

At t=31 mins, the attacker claims the reward. Although only 2 mins have passed, the attacker will get the entire round’s reward. The claimable reward is calculated as

total_passed_rounds = delat_t/round_interval = 31/10 = 3 rounds
added_reward = tolata_passed_rounds * reward_per_round = 10 tokens //total_passed_round is 1, since the last action was at t = 29 mins

Farm::distributions.rps = 9.1 + 2.5 =11.6
claimableReward = 11.6 - 9.1 = 2.5

Recommendation:
We recommend implementing a linear reward schedule independent of rounds. Or making the round intervals short to avoid rounding errors

Dev’s Response #1:
The interval between rounds is determined when creating a Staking Program.
Note that the shorter the round interval, the more closely to linear on time the reward distribution function ends up as. It's recommended to set it to a couple of seconds at most.

Auditors’ Response #1:
We suggest introducing an upper cap for the round interval as described in the dev’s response. The documentation suggests short round intervals but this is not reflected in the smart contract.

Dev’s Response #2:
This has been included in the user documentation. We don't feel like there should be a hard limit since different projects might have different ideas on how to use the contract. For instance, a NFT project might want to reward all skaters that have stakes before a certain date. This would be done by implementing a large round interval and distributing all rewards in a single round.

Auditors’ Response #2:
The auditors agree with the devs.

Medium-risk issues

8. Improper evaluation of `dex_lock_time`

File: launchpad/src/actions/dex_launch_actions.rs

Description: 

When the lauch_on_dex function is called, it checks if dex_lock_time is less than current timestamp & sets it’s value to timestamp + LOCK_PERIOD but the callback always resets the value back to zero.

Code:

pub fn launch_on_dex(&mut self, listing_id: U64) -> Promise {
    let timestamp = env::block_timestamp();
    let mut listing = self.internal_get_listing(listing_id.0);
    assert!(listing.dex_lock_time <= timestamp, "{}", ERR_403); //here
    listing.update_treasury_after_sale();
    listing.dex_lock_time = timestamp + LOCK_PERIOD; //here
 ...
}

Calling this function updates the status of a listing & attaches a callback. Every callback always sets the value of dex_lock_time to zero. See the code snippet below.

Code:

pub fn callback_dex_launch_create_pool(
 ...
  ) -> PromiseOrValue<bool> {
    let mut listing = self.internal_get_listing(listing_id.0);
    listing.dex_lock_time = 0; //here    …

This will always allow the function to be called without LOCKED_PERIOD applied. This can be found at the following functions. i.e listing.dex_lock_time = 0;

1. In Callback_dex_add_liquidity, callback_actions.rs
2. In Callback_dex_deposit_project_token, callback_actions.rs
3. In Callback_dex_deposit_price_token, callback_actions.rs
4. In Callback_dex_add_liquidity, callback_actions.rs

Recommendation:
Introduce proper validations

Dev’s Response
Not Applicable
The idea behind it is that if the `launch_on_dex` method is called more than once in the same block, all calls but the first will revert. The lock will only be lifted after the execution of the callback to allow the users to proceed with the next phase of the launch.
The reason it is implemented as a timestamp instead of a boolean value is that in case some unintended bug affects the callback, funds do not get locked forever on the contract.
Therefore, no modifications were done to the contract on this issue's account.

Auditor’s response:
The auditors agree with the developers.

9. Collision of Guardians to steal contract tokens

File: nft_staking/src/funds/transfer.rs

As this is only exploitable by the guardian, and currently guardians are only created by the owner, but this could be changed in the future when DAO is implemented, as we have seen in the countless hacks in the DeFI ecosystem where the attacker was able to become a guardian, moreover in the specification of JUMPDeFi, it was written that only owner is able to interact with the contract_treasury but in the code implementation only the guardians are able to interact with it. The attack scenario is a bit complex but highly likely and very easy if a malicious user is able to become a guardian.

Description:

When the nft_staking is deployed the owner is set at the time of deployment, the nft_staking will have a single contract_treasury. Now the new staking-programs can be deployed and they will have their own collection_treasury. Each staking program will have a farm in which the tokens to be distributed will be set, although the contract_tokens and programs_tokens will have to be different,  contract_tokens can be added in the distribution which will eventually be distributed among the Stakers.

The other thing that we need to consider in it, is that inside the reallocate_funds function the guardian is able transfer contract_token from contract_treasury into collection_treasury, (the guardian should not be able to interact with contract_treasury only the owner should have done it).

After the contract_tokens are transferred into collection_treasury, it can be moved to distribution by using TransferOperation::CollectionToDistribution in reallocate_funds. Once the contract tokens are in distribution it would be distributed to the stakers.

Attack Scenario:

Now the attack pattern will look like, once the attacker is able to become a guardian.

1. Guardian will create a new staking_program and add the contract token as a distribution reward here in create_staking_program in guardian.rs.
2. Staking program is created with distribution tokens set similar to contract_token. Guardian will transfer the contract token from contract treasury into collection treasury. And then transfer them from the collection treasury into distribution. All in the reallocate_funds function.
3. Attacker will only stake himself in the staking_program, and set the staking time very short. In this way they will be able to take out all the contract tokens as reward because they will be the only staker and the way distribute function works.

Code:

fn reallocate_funds(
 ...
 )
match operation {
     TransferOperation::ContractToCollection => {
       let contract_treasury = self.contract_treasury.entry(token_id.clone()).or_insert(0);
       let amount = amount.unwrap_or(*contract_treasury);
       assert!(
         amount <= *contract_treasury,
         "{ERR_INSUFFICIENT_CONTRACT_TREASURY}"
       );
       *contract_treasury -= amount;
       *collection_treasury += amount;
     }
...
   }

Recommendation:
As per the specifications, only the owner should be able to transfer funds to the Collection Treasury instead of guardians. That is the quickest way to ensure safety here.

Alleviation:
The issue is fixed by placing an onlyOwner check.

10. Inadequate state roll back will lead to reward loss

File: Nft_Staking/src/actions/staker.rs

Description: 

Users can un-stake their NFT through the contract. During the call to transfer NFT it attaches the callback `compensate_unstake` function. If the promise fails it restakes the NFT which in turn calls `redistribute` function which will result in loss of reward for the user.

Code:

pub fn compensate_unstake(
   &mut self,
   token_id: NonFungibleTokenID,
   owner_id: AccountId,
   staked_timestamp: U64,
   balance: SerializableFungibleTokenBalance,
 ) {
   if is_promise_success() {
     events::unstake_nft(&token_id, balance);
   } else {
     let collection = token_id.0.clone();
     let staked_nft = StakedNFT::new(token_id, owner_id, staked_timestamp.0);
     let mut staking_program = self.staking_programs.get(&collection).unwrap();
     staking_program.insert_staked_nft(&staked_nft);
     self.staking_programs.insert(&collection, &staking_program);

Recommendation:
Introduce a mechanism to only revert the changes that were caused rather than making a new entry

Dev’s Response:
Essentially, the problem is that once you un-stake the total amount of staked NFTs gets reduced. This means rewards during the blocks that have the NFT unstaked are increased for every present NFT. In order for us to give rewards for the unstaked NFT in that period, we would need to recalculate the rewards given away, since we'd have to include 1 more NFT for the distribution. This cannot be done safely as other users might have claimed their rewards in the period

This would introduce a lot of race conditions in the code essentially. The callback is already a failsafe for cases where the NFT transfer fails, the user missing a couple blocks of rewards is a small loss considering that we already implemented a mechanism for him to not have to wait for the lock period again And in practice there is no reason for a nft_transfer to fail, since the contracts do not require storage deposits. In case this happens, it would probably be necessary to contact the technical team as the NFT's contract might have a bug. 

Auditor’s Response:
The auditors agree with the devs.

11.  Potential “million cheap data additions" attack

File: launchpad/src/actions/guardian_actions.rs

Description: 

Since the investors have to pay storage costs. The guardians can create fake project listings on the launchpad with the project owner being any investor (i.e victim) track_storage_usage will increase the storage_used and when investors call storage_withdraw he/she will get less amount. 

Since the guardians have to undergo strict KYC and other criteria, this is very unlikely that guardians will perform this attack. But we have to consider a case when the platform softens the policies for guardians, this attack makes more sense

Code:

pub fn create_new_listing(&mut self, listing_data: ListingData) -> u64 {
   self.assert_owner_or_guardian();
   let initial_storage = env::storage_usage();
   let project_owner_account_id = listing_data.project_owner.clone();
   let mut project_owner_account = self
     .internal_get_investor(&project_owner_account_id)
     .expect(ERR_010);
   let listing_id = self.internal_create_new_listing(listing_data);
   project_owner_account.track_storage_usage(initial_storage);
   self.internal_update_investor(&project_owner_account_id, project_owner_account);
   listing_id
 }

Recommendation: 

Implements a consent/permission mechanism for investors to be project owners. The following are the approaches.

• if the owner is EOA, the signature should be provided
• if the owner is a smart contract, then add a callback verify_permission_for_listing on SC while creating a listing

Dev’s Response:
The auditors found that a malicious guardian, either by malicious intent or by hacking of their private keys, might spam the contract with multiple unasked for listings and assign random investors as their owners, which would cause the investors to pay for the storage needed and would make them unable to recover their storage fees forever.

The suggested fix was to require a signature of the project_owner for a specific message (if address is a user wallet) or to do a callback to verify the intent (if address is a smart contract).
However, we found that such methods significantly deteriorate the ease of use of the contract, especially when setting up a new sale for a project.
As a solution, we implemented the boolean attribute authorized_listing_creation in the investor struct. It is default false and the user must call the new public method toggle_authorize_listing_creation to toggle its value to true.
Guardians can only call create_new_listing using the address of the project_owner if that address is registered as an investor with the authorized_listing_creation attribute set to true.
After creating a new listing, authorized_listing_creation is set back to false to prevent the creation of multiple undesired listings.

Alleviation:
The issue is fixed by introducing a boolean attribute `authorize_listing_creation`. The fix introduced by the developers is very efficient.

Low-risk issues

12. Inadequate function parameters

File: vesting_contract/src/investment.rs

Description: 

In vesting_contract/src/investment.rs  the new function the date_in parameter should not be optional.

Here when the user will go on to create an investment on a specific schema in the farm, the user will have the option to keep the initial_timestamp of the investment which is date_in parameter as optional. If it is optional the initial_timestamp of the investment will be the schema's initial_timestamp. Now this could cause a lot of issues. The main one is if the schema was created way back in time and investment was created recently, the user can easily increase their reward and could bypass the needed cliff period. As in the calculate_available_withdraw on these lines calculates whether the user has passed the cliff and vesting period and the rewards are calculated accordingly.

Only one schema is created for one category and there cannot be more than one schema for a single category, moreover the schema can only be created by the token contract means it is only callable by owner. So we can expect that new schemas will not be created everyday but new investments by user will be created all the time.
Code:

pub fn new(account: AccountId, total_value: u128, date_in: Option<u64>) -> Self {
      ...}

Recommendation: The parameter `date_in` shouldn’t be made optional.

Dev’s Response

This is the intended behavior, as the method can only be called by the owner, they must have the autonomy to actually select the date in which the investor entered the investment agreement.

The main reason for the behavior is that, in practice, many investors already put funds into Jump DeFi, even before the contracts were deployed as they were early investors. Therefore, their vesting schemas have already been started and must be faithfully reproduced, which might require the owner to set a previous start_date.

Auditors’ Response:
We agree with the dev’s response.

13. Insufficient logic for storage cost

File: token_launcher/src/lib.rs

Description: 

In the lib.rs contract of token_launcher, the user can register as many cratract and the storage cost for them will be borne by the contract instead of users paying for the storage. This would lead to a griefing attack where the attacker can mass register and try to put as much storage cost on the contract as possible.

Code:

pub fn register_contract(
   &mut self,
   contract_name: String,
   contract_hash: Base58CryptoHash,
   contract_cost: U128,
   init_fn_name: String,
   init_fn_params: String,
 ) {
   assert!(
  ...
   self.binaries.insert(&contract_name, &new_binary);//storage cost            
   event_new_contract_registered(contract_name.as_str(), contract_hash); 
 }

Recommendation:
A unregister_contract function should be created that would aid the user/contract owner in removing registration and freeing up the storage when necessary. Furthermore the storage cost should be paid by the user instead of the contract.

Alleviation:
The issue is fixed by introducing onlyOwner check.

14. Potential avoidance of launchpad fee

File: launchpad/src/actions/guardian_actions.rs

Description: 

The guardian can cancel the listing just before its ending period or phase 2 start time i.e an hour before. This will allow investors to withdraw the project tokens after vesting period. The project owners will be able to withdraw their project & price tokens without paying a fee to the launchpad.

Code:

#[payable]
 pub fn cancel_listing(&mut self, listing_id: U64) {
   self.assert_owner_or_guardian();
   self.internal_cancel_listing(listing_id.0);
 }
}

Recommendation: 
Add fees for launchpad regardless of listing cancellation or  not allow to cancel listing during the phase1 & phase2

Alleviation:
The issue is fixed.

15. Inadequate validation on AccountId

File: mintable_token_contract.rs

Description: 

All the functions in this contract that takes AccountId as a parameter does not validate that the AccountId value passed to it actually contains a valid AccountId following the NEAR’s account ID rules. As a result, an owner who wishes to burn or mint tokens to another user can mistakenly call the function with a string pointing to an invalid NEAR account ID, resulting in a complete and irreversible loss of control over the contract. 

Code:

#[near_bindgen]
impl Contract {
 #[payable]
 pub fn ft_mint(&mut self, quantity_to_mint: U128, recipient: AccountId) {
   assert_one_yocto();
   self.only_owner();
   self.token.internal_deposit(&recipient, quantity_to_mint.0);
   self.on_tokens_minted(recipient, quantity_to_mint.0);
 }
}

In the context of the mint function, the owner can mint tokens to an invalid account ID.

Note:

This vulnerability is present in all the functions of the following contracts which takes accountId as a parameter

• modified_contract_standards.rs
• staking.rs
• token_contract.rs
• vesting_contract.rs

Recommendation: 
The functions should check that the passing argument is in the form of correct AccountID. Update near-sdk version to 4.0.0+ or use env::is_valid_account_id from near-sdk for validating accountId.

Alleviation:
The issue is fixed by the auditors recommendation.

Informatory issues and Optimizations

16. Improper validation of deploy address

File: token_launcher/src/actions/deploy.rs

Description:
In deploy_new_contract function there is a check that deploy_address should not exceed the maximum size allowed by NEAR is 64 characters but the assertion is not implemented correctly because it limits the address to always be less than 64 characters. The correct logic should be less than and equal to 64 characters. 

Code:

#[payable]
 pub fn deploy_new_contract(
 ...
 ) {
  ...
   //verify if contract size does not exceed max allowed on near
   assert!(
     deploy_address.len() < 64, //here it should <=64
     "{}{}",
     ERR_102,
     env::current_account_id()
   );

Recommendation:
Verify if contract size does not exceed max allowed on near.

assert!(
     deploy_address.len() <= 64

Alleviation:
The issue is fixed by the auditors’ recommendation.

17.  Inadequate checks on unregister storage

File:  launchpad/src/actions/storage_impl.rs

Description:
Project owners can unregister_storage. There is a check !account_deposit.is_listing_owner. When a new Listing is created, investor's is_listing_owner is never set to true.

Code:

#[allow(unused_variables)]
 #[payable]
 fn storage_unregister(&mut self, force: Option<bool>) -> bool {
   assert_one_yocto();
   let account_id = env::predecessor_account_id();
   if let Some(account_deposit) = self.internal_get_investor(&account_id) {
     // TODO: figure out force option logic.
     assert!(account_deposit.allocation_count.is_empty(), "{}", ERR_203);
     assert!(!account_deposit.is_listing_owner, "{}", ERR_210);
     self.investors.remove(&account_id);
     Promise::new(account_id.clone()).transfer(account_deposit.storage_deposit);
     true
   } else {
     false

Recommendation: 
Update is_listing_owner field of investor

Alleviation:
The issue is fixed by updating the is_listing_owner value to true in create_new_listing function.

18. Optimization in withdraw

File: locked_token/src/actions/user.rs

Description:
In the withdraw_locked_token function there is a variable called  value_to_withdraw which saves the value the user can withdraw. But that value can be 0, which eventually leads to the loss of users' gas fees. 

Code:

#[payable]
 pub fn withdraw_locked_tokens(&mut self, vesting_id: U64) -> Promise {
...

   let value_to_withdraw = vesting.withdraw_available(env::block_timestamp());
   vesting_vector.replace(vesting_id, &vesting);

...

   ext_token_contract::ext(self.contract_config.get().unwrap().base_token)
     .with_static_gas(FT_TRANSFER_GAS)
     .with_attached_deposit(1)
     .ft_transfer(
       account_id.to_string(),
       U128(value_to_withdraw), //here it can be 0
       "locked token withdraw".to_string(),
     )
     ...
}

Recommendation:
Assertion on this variable can save users a lot of gas and avoid entire transaction execution.

Alleviation:
The optimization recommended by the auditors is implemented.

19. Standard Conformity and Inconsistencies in the requirements

Description:
In the documentation it is saying that program_token and contract_token can be the same tokens. But in the current implementation, when the collection owner will try to make one of the contract_token as program_token the contract will stop it and throw an error.

Documentation also says that the contract_token in contract_treasury can only be transferred by owner but in the code the guardian is able to transfer the contract_token into the collection treasury and so on.

Recommendation:
The code should be according to the requirements and documentation.

Alleviation:
The issue is Fixed by replacing the only guardian check with only owner check.

20. Incomplete Functions in the Contracts

Description: 
The contracts contain many incomplete functions and contracts, many of these contain core functionality of the protocol. These functions should be completed for the protocol to perform as intended.
Some of these incomplete functions are the following:

1. add_contract_token in nft_staking/src/actions/owner.rs
2. remove_contract_token in nft_staking/src/actions/owner.rs
3. storage_unregister in nft_staking/src/actions/storage_impl.rs
4. new in nft_staking/src/models/staking_program.rs

There are also some missing checks where anyone can deposit contract_tokens to contract_treasury, instead this should be restricted to partners.

Recommendation:
These functions should be completed as they are defined in the documentation.

Dev’s Response for add_contract_token:
This is an intentional technical debt. Currently there's no way to delete a staking program, so if we were to block the addition of a new contract token while it was being used as a program token, it would result in the impossibility of ever using any token which was at any point in time a program token, as a contract token.

It's likely this whole edge-case will remain irrelevant forever, as there's little reason to make a token a contract token, even more so a program token, as it likely isn't even owned by the contract owner.

Note that in the current contract behavior, what happens is that the collection owner authorization always has precedence over the guardians/owner. Meaning, if for some reason, in a given staking program, a program token is also a contract token, its role as a program token supersedes its role as a contract token, and only the collection owner may operate on it. We found this to be a reasonable trade-off with little downsides to an unlikely problem with a costly solution.

Alleviation:
The issue is partially fixed. The auditors agree with the devs’ explanation for not fixing 1 and 4.

21. Redundant code in calculate_vested_investor_withdraw function

File: launchpad/src/listing/mod.rs

Description:
In the calculate_vested_investor_withdraw function, the same logic is checked twice which raises redundancy in the code. As mentioned in the below code the condition timestamp >= self.cliff_timestamp is checked twice

Code:

pub fn calculate_vested_investor_withdraw(&self, allocations: u64, timestamp: u64) -> u128 {
   let allocations = allocations as u128;
   let initial_release =
     ((self.token_allocation_size * self.fraction_instant_release) / FRACTION_BASE) * allocations;
   let cliff_release =
     ((self.token_allocation_size * self.fraction_cliff_release) / FRACTION_BASE) * allocations;
   let final_release = self.token_allocation_size * allocations - initial_release - cliff_release;
   let mut total_release = initial_release;
   if timestamp >= self.cliff_timestamp // redundant
     && timestamp < self.end_cliff_timestamp
     && timestamp >= self.cliff_timestamp //here redundant
   {
...
}

Recommendation: 
It is redundant and should be removed.

Alleviation:
The developers implemented the auditor's recommendation.

22. Optimization in withdraw_rewards

File: Nft_Staking/src/actions/staker.rs

Description: 
The function withdraw_reward has a withdrawn_amount variable that can be zero. An assertion can be introduced to save user gas

Code:

#[payable]
 pub fn withdraw_reward(
   &mut self,
   collection: NFTCollection,
   token_id: FungibleTokenID,
   amount: Option<U128>,
 ) -> Promise {
...
     ext_fungible_token::ext(token_id.clone())
     .with_static_gas(FT_TRANSFER_GAS)
     .with_attached_deposit(1)
     .ft_transfer(caller_id.clone(), 
U128(withdrawn_amount), //here can be zero
 None)

Recommendation:
Revert early if amount is zero.

Alleviation:
The developers implemented the auditor's 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 DeFiGeek Community (DFGC) (Client) for the purpose of conducting a Smart Contract Audit/Code Review of Yamato Protocol. This document presents the
findings of our analysis which started on June 20th, 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

DeFiGeek Community (DFGC) is an open community that develops DeFi DApps and middlewares that contribute to the Web3 era. DFGC develops and envisions multiple DApps.
The first is the Yamato Protocol, a crypto-secured stablecoin generator DApp pegged to JPY. Yamato Protocol is a lending decentralized financial application (DeFi) that can generate Japanese Yen stablecoin "CJPY". It is being developed by DeFiGeek Community Japan, a decentralized autonomous organization.

Yamato Protocol is a crypto-asset overcollateralized stable coin issuance protocol. V1 allows the issuance of CJPY (Japanese Yen equivalent coin) using ETH as collateral. It has the following features:

• No immediate forced liquidation
• No obligation to repay debt
• No interest rates (lump-sum fees)
• Low secured rates (minimum 130%)

System Architecture

The idea for the Yamato Protocol is based on Maker and Liquity protocols. Crypto over-collateralized stablecoin render. Token Economics implements the Voting Escrow model with reference to Curve's ecosystem of CRV and veCRV (This will be implemented in a later version).

The initial model will be launched on Ethereum, and by depositing ETH, you can borrow a Japanese Yen stablecoin called CJPY (ERC-20).

As a P2C (Peer to Contract) Lender, it is an epoch-making design that eliminates the need for forced clearing. It is a basic application of web3 as a decentralized application that can borrow Japanese Yen equivalent coins (Convertible JPY (ticker: CJPY)) with ETH as collateral.
In V2, DFGC will implement CUSD and CEUR and further develop as a global stablecoin render.

Methodology & Scope

The codebase was audited using a filtered audit technique. A pair of auditors scanned the codebase in an iterative process spanning over a span of 2.5 weeks.

Starting with the recon phase, a basic understanding was developed and the auditors worked on establishing presumptions for the codebase and the relevant documentation/ whitepaper provided or found publicly.

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

The analysis indicates that the contracts under scope of audit are working properly.
Our team performed a technique called “Filtered Audit”, where the contract was separately audited by two individuals.

After a thorough and rigorous process of manual testing, an automated review was carried out using tools like slither for an extensive static analysis and foundry for property testing the
invariants of the system. All the flags raised
were manually reviewed in collaboration and
re-tested to identify the false positives.

Our Team Found

# of issues	Severity Of the Risk
1	Critical Risk Issues(s)
1	High Risk Issues(s)
3	Medium Risk Issues(s)
8	Low Risk Issues(s)
1	Informatory Risk Issues(s)

Key Findings

Detailed Overview

Critical-risk issues

Potential classic frontrunning attack

File: contracts/Currency.sol

Description: 

The race condition issue for the ERC20 approve() function is an exploitable code of the CJPY ERC20 token. The implementation does not follow the SWC-114 requirement properly and thus falls trap to the sandwich attack, commonly known as MEV.

function approve(address spender, uint256 amount)
       public
       override
       returns (bool)
   {
       require(
           _msgSender() != spender,
           "sender and spender shouldn't be the same."
       );
       require(amount > 0, "Amount is zero.");


       _approve(_msgSender(), spender, amount);
       return true;
   }

Remedy:

The issue is described in detail in the official SWC registry considered as the industry standard here. Alternatively, you could search on the internet for issue number SWC-114 which stands for Smart Contract Weakness Classification and Test Cases and is an industry-wide, accepted and maintained registry.

Status: 

Acknowledged

High-risk issues

Ineffectual Upgradeability

File: contracts/Dependencies/UUPSBase.sol

Description: 

In the light of our assumptions with the protocol and the understanding of the UUPS Proxy Pattern, the contract UUPSBase should not implement the initializer modifier on the constructor.
The audit team deems this modifier as an anti-pattern practice, according to the industry standards and best practices for the implementation of the Universal Upgradeable Proxy Standard. 

/// @custom:oz-upgrades-unsafe-allow constructor
   constructor() initializer {}

Remedy:

In case of such scenarios that follow any misassumed and authorized usage, the protocol may end up to an unreachable state and the funds locked up.

Note: For a to-the-point understanding and confirmation of the assumed protocol deployment, the audit team requires a walkthrough of the deployment procedure from the devs PoV which is not explicitly described in specs/tests or supported documentations.

Else, the priority remedy is to remove the initializer modifier along with the UUPSBase constructor.

/// @custom:oz-upgrades-unsafe-allow constructor
   constructor() {}

Status: 

Fixed

Medium-risk issues

Tractable balance accounting

File: contracts/PoolV2.sol

Description: 

Although the receive function expects a calling from the onlyYamato authorization, the funds will still be accepted by the contract through self destruct.
This can bring the contract to an unaccounted state for its ether balances, upon which the sendETH and refreshColl functions, that manages the user’s collateral and releasing mechanism, can lose the accounting of the total balance and always assume a wrong balance.

receive() external payable onlyYamato {
       emit ETHLocked(msg.sender, msg.value, address(this).balance);
   }

Remedy:

It is recommended that a minimal data structure be laid out in the Pool contract to manage the user’s balances and collaterals through proper mappings and always let the user pull their right amount of value from the protocol.

Status: 

Fixed

Inconsistent non-reentrant pattern

File: contracts/YamatoDepositorV2.sol

Description: 

The runDeposit() function implements an incorrect checks-effects-interactions pattern in the commented steps 1 and 2 of the codebase. Refer to the following code snippet for detail and the suggested remedy.

       /*
           1. Compose a pledge in memory
       */
       IYamato.Pledge memory pledge = IYamato(yamato()).getPledge(_sender);
       (uint256 totalColl, , , , , ) = IYamato(yamato()).getStates();


       pledge.coll += _ethAmount;
       if (!pledge.isCreated) {
           // new pledge
           pledge.isCreated = true;
           pledge.owner = _sender;
       }


       /*
           2. Validate lock
       */
       require(
           !IYamato(yamato()).checkFlashLock(_sender),
           "Those can't be called in the same block."
       );
       require(
           pledge.coll >= IYamatoV3(yamato()).collFloor(),
           "Deposit or Withdraw can't make pledge less than floor size."
       );

The collateral amount is added right after the pledge is created which leads to the violation of the infamous checks-effects-interactions pattern. The checks are placed further below in step 2 which leads to an inconsistent pattern for the codebase.

Remedy:

The remedy states that the checks be implemented before creating any effects in the storage so that lesser gas cost along with an optimized codebase is achieved.

       require(
           !IYamato(yamato()).checkFlashLock(_sender),
           "Those can't be called in the same block."
       );


       IYamato.Pledge memory pledge = IYamato(yamato()).getPledge(_sender);
       (uint256 totalColl, , , , , ) = IYamato(yamato()).getStates();


       pledge.coll += _ethAmount;


       require(
           pledge.coll >= IYamatoV3(yamato()).collFloor(),
           "Deposit or Withdraw can't make pledge less than floor size."
       );


       if (!pledge.isCreated) {
           // new pledge
           pledge.isCreated = true;
           pledge.owner = _sender;
       }

Status: 

Fixed

Incorrect version calling

File: contracts/YamatoDepositorV2.sol

Description: 

The runDeposit() function calls fetchPrice() from the V1 interface of PriceFeed whereas the code for the same in V2 has many modifications and changes. This leads to an inconsistent external calling of functions and ultimately the wrongly assumed outcome.

   function runDeposit(address _sender) public payable override onlyYamato { IPriceFeed(feed()).fetchPrice();

Remedy:

It is recommended to change the function such that it inherits and implements the correct IPriceFeed interface or in case this is the correct versioning then comment down the presumptions for the fetchPrice() function.

Status: 

Fixed

Low-risk issues

Ineffectual check for ICRPertenk

File: contracts/Dependencies/PledgeLib.sol

Description: 

The function FR() implements an unnecessary check in the if block [13000 <= _ICRpertenk] for the value of ICRPertenk variable which is already handled in the require statement at the start of the function.

  function FR(uint256 _ICRpertenk) public view returns (uint256 _FRpertenk) {

     function FR(uint256 _ICRpertenk) public view returns (uint256 _FRpertenk) {
       require(_ICRpertenk >= 13000, "ICR too low to get fee data.");
       // if (11000 <= _ICRpertenk && _ICRpertenk < 13000) {
       //     _FRpertenk = 2000 - ((_ICRpertenk - 11000) * 80) / 100;
       // } else
       if (13000 <= _ICRpertenk && _ICRpertenk < 15000) {
           _FRpertenk = 400 - ((_ICRpertenk - 13000) * 10) / 100;  

Remedy:

We recommend that this check be removed to save gas and optimize the function.

Status: 

Fixed

Ineffectual check for equality

File: contracts/YamatoWithdrawerV2.sol

Description: 

The function runWithdraw() implements a check using >= (greater equals) whereas the statement still fails even if the value of both sides is equal.
For instance, a scenario with the value for pledge.getICR(feed()) and uint256(IYamato(yamato()).MCR()) * 100 equal to 13000 will fail.

     require(
           pledge.getICR(feed()) >= uint256(IYamato(yamato()).MCR()) * 100,
           "Withdrawal failure: ICR is not more than MCR."
       );  

Remedy:

We recommend that checking equality be removed to save gas and optimize the function for edge cases.

Status: 

Fixed

Misplaced logic statement

File: contracts/YamatoRedeemerV4.sol

Description: 

The function runRedeem() stores the argument’s value at a point which is illogical in light of the require and conditional statements.

  _args.wantToRedeemCurrencyAmount = IPool(pool())
       .redemptionReserve();

Remedy:

The value is recommended to be stored outside the if block as follows.

       vars.ethPriceInCurrency = IPriceFeed(feed()).fetchPrice();
       _args.wantToRedeemCurrencyAmount = IPool(pool()).redemptionReserve();
       if (_args.isCoreRedemption) {
           require(
               _args.wantToRedeemCurrencyAmount > 0,
               "The redemption reserve is empty."
           );
       } else {
           require(
               _cjpy.balanceOf(_args.sender) >=
                   _args.wantToRedeemCurrencyAmount,
               "Insufficient currency balance to redeem."
           );
       }

Status: 

Fixed

Unused variable

File: contracts/YamatoRedeemerV4.sol

Description: 

The memory variable reminder in the runRedeem() function is redundant as it stores the value which is never used throughout the local scope of the function.

       vars._reminder = _args.wantToRedeemCurrencyAmount;

Remedy:

We recommend that the unnecessary variable be removed from the data structure and in the function for execution gas and storage optimization.

Status: 

Fixed

Inexistent Zero Address check

File: contracts/Currency.sol

Description: 

The setCurrencyOS(address _currencyOSAddr) function does not validate the input for the param _currencyOSAddr whether a user provides a non-zero address. 

   function setCurrencyOS(address _currencyOSAddr) public onlyGovernance {
       currencyOS = _currencyOSAddr;
   }

Remedy:

It is recommended that a require statement with correct input validations be included in the codebase. 

Status: 

Fixed

Unused and Unnecessary Functions

File: contracts/Dependencies/ [Ownable.sol && YamatoStore.sol]

Description: 

The _renounceOwnership() and __YamatoStore_init_unchained() functions are never called and contain empty code blocks respectively, hence, not performing any valuable activity.

function _renounceOwnership() internal {
       emit OwnershipTransferred(_owner, address(0));
       _owner = address(0);
   }

   function __YamatoStore_init_unchained() internal initializer {}

Remedy:

It is recommended to remove the functions from the codebase in favor of execution risks and storage optimizations.

Status: 

Fixed

Inconsistent safemath

File: contracts/YamatoWithdrawerV2.sol

Description: 

The pledge.coll attribute can be updated using the consistent pattern as in the Depositor and Borrower contracts.

       /*
           4. Update pledge
       */
       // Note: SafeMath unintentionally checks full withdrawal
       pledge.coll = pledge.coll - _ethAmount;

Remedy:

It is recommended to utilize a consistent coding practice throughout the Yamato Actions smart contracts.

        pledge.coll -= _ethAmount;

Status: 

Fixed

Additional informatory issues and optimizations

Note:
Increase test cases and code coverage for a satisfactory upgradeable contracts scenario. The current state of test cases do not engage any kind of upgradeability tests which poses a good deal of threat for a wide range of attack vectors.

This list contains additional coding style guides and best practices for a consistent codebase of the Yamato Protocol.

• General Codebase Recommendations:
  • Include thorough NatSpec documentation in the codebase.
  • Remove the usage of hardhat/console.sol as it increases the deployment and execution gas cost.
  • Convert all public functions to external if the contract itself or the inherited contracts do not use them.
  • Avoid using SafeMath for redundant operations as the codebase is already using solidity versions ^0.8.0.
• Mark Variable Visibility Explicitly:
  • In YamatoBase.sol: L20
  • In YamatoV3.sol: L69, L86
  • In PriorityRegistryV6.sol: L26, L30, L34, L35 (tight packing)
  • In PriceFeedV2.sol: L61, L62 (L94-L98: tight packing & wrap in struct)
• Mark Function Visibility Explicitly:
  • In YamatoV3.sol:
    • setDeps() as external
    • toggle() as external
    • setPriorityRegistry() as external
  • In PriorityRegistryV6.sol:
    • bulkUpsert() as internal
  • In CurrencyOSV2.sol:
    • setDeps() as external
    • setPriceFeed() as external
  • In UUPSBase.sol:
    • setGovernance() as external
    • acceptGovernance() as external
    • revokeGovernance() as external
• In PledgeLib.sol:
  • NatSpec documentation not found for multiple functions in the library file as follows;
    toMem(), clone(), addDebt(), sync(), assign(), nil(), toBeRedeemed(), cappedRedemptionAmount()
  • Though the contracts use solidity version 0.8.4 that comes prebuilt with SafeMath, the library implementation uses SafeMath and functions that check for overflow/underflow which is redundant and unnecessary.
• In YamatoStore.sol:
  • The contract contains an empty function with the initializer modifier without any actual value or processing inside, but just as part of the UUPS pattern.  Suggested to remove the modifier for redundant calling.
• In YamatoV3.sol:
  • The setFlashLock() function can be optimized to replace the redundant check (lock.blockHeight < block.number) and the redundant setting lock.blockHeight = block.number; to be implemented in an optimized way only once at the first if block instead of each conditional check.
  • The code block in setPledge() function can be optimized and made consistent by moving it to the top or below the remaining singular checks as follows;

    Pledge storage p = pledges[_owner];
       if (_p.debt == 0 && _p.coll == 0) {
           _p.owner = address(0);
           _p.isCreated = false;
           _p.priority = 0;
       }
       if (p.coll != _p.coll) {
           p.coll = _p.coll;
       }
       if (p.debt != _p.debt) {
           p.debt = _p.debt;
       }
       if (p.owner != _p.owner) {
           p.owner = _p.owner;
       }
       if (p.isCreated != _p.isCreated) {
           p.isCreated = _p.isCreated;
       }
       if (p.priority != _p.priority) {
           p.priority = _p.priority;
       }

• Move the onlyYamato modifier to the top of the contract to follow the code style guides as in the Solidity docs.
• Correct the typo on line 349; /// @dev Nood reentrancy guard.
• Correct the spelling for the method getIndivisualStates() to getIndividualStates() 

• In YamatorRepayerV2.sol:
  • The return variable _wantToRepayCurrencyAmount naming is misleading and can be corrected to _repayAmountInCurrency for consistency.
  • On L85 & L86, the counting for steps can be corrected to 6-1 and 6-2.
  • On L86, the comment should be corrected as the redeemer should be repayer
• In CurrencyOSV2.sol:
  • The getter method for feed() and the following instances of price feed should be replaced with priceFeed() for self explanatory use cases throughout the codebase.
  • Always emit an event when addYamato() is called.
  • Remove redundant import for PriceFeed.sol
  • Change the access modifier to view for the _permitMe() function.
  • Inherit the IYamatoV3.sol interface instead of IYamato.sol
• In Currency.sol:
  • The modifier onlyGovernance should move to the top of the contract to follow the Solidity docs code style.
  • Emit events for setCurrencyOS() and revokeGovernance() functions.
• In PriceFeedV2.sol:
  • The struct ChainlinkResponse and TellorResponse can be moved to the interface file.
  • The function _simulatePrice being internal can be moved to the bottom of the contract.
  • Use return variable _price in function signature declaration as
    • function getPrice() external view override returns (uint256 _price) {
    • function _scaleTellorPriceByDigits(uint256 _price) internal pure returns (uint256 _price) {
  • Remove this redundant check: else if (_answerDigits < TARGET_DIGITS)
  • Move all public functions to the top of the contract file to follow the solidity docs guideline.
• In UUPSBase.sol:
  • The modifier onlyTester is a redundant authorized role which should be removed from the production codebase. 

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 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 contacted by Phase Protocol (Client) for the purpose of conducting a Smart Contract Audit/Code Review. This document presents the findings of our analysis which started on  23st July 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

Phase Protocol, founded by DedMonke is an innovative escrow service that incentivizes credible projects teams for the community approved roadmaps and deliverables. They are a type of fundraising opportunity through the use of NFTs.

System Architecture

Fundraising Through NFTs
With the use of Phase protocol, development teams can easily raise funds for their projects by minting NFTs for their project deliverable. They are one of the finest examples of how NFTs can be used in different ways.

Treasury Control
All the funds are properly controlled by the treasury to assure the community about the transparency of the system and how much funds are currently allocated by the team. 

Fund Redistribution
In any event of failure the funds will be redistributed back to the current holders. There will also be a option to create a DAO in the near future.

Methodology & Scope

The codebase was audited using a filtered audit technique. A band of four (4) auditors scanned the codebase in an iterative process spanning over a time of two (2) weeks. 

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

The analysis indicates that the contracts under scope of audit are working properly excluding swap functionality which contains one recent issue.

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 cargo-audit & cargo-tarpaulin for static analysis and cargo-fuzz for fuzzing invariants. 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)
1	High Risk issue(s)
1	Medium Risk issue(s)
2	Low Risk issue(s)
2	Informatory issue(s)

Key Findings

Detailed Overview

Critical-risk issues

No issues were found.

High-risk issues

1. Lack Of Proper Checks Could Potentially Lead to Bypass When Changing Phases

File:  In the programs/phase-protocol/src/instructions/phase

Description: 

In the contracts for different phases under the given directory, when changing the state from one phase into another there is not any proper conditional verification and statements to check if the phase is even eligible to change the state. In the contract lock_phase_for_mint.rs only the phases that are approved could be locked, and the phases in the draft cannot be locked. Although the protection will not be bypassed in every scenario, there is a risk that this lack of checks could cause issues in the future. 

Remedy:

According to the documentation provided the phases usually go through the following phases

DRAFT —> APPROVED —> ACTIONED —> STAGED FOR COMPLETION —> COMPLETE.

Now before moving on to the next phase it should check whether the provided phase is even in the phase that is required. As stated above parameters type will usually stop these types of bypasses but adding a proper layer of checks is always good to keep everything properly secured, because optimization is not such an issue in Solana.

 assert!(
           availability.allow_place || availability.allow_cancel,
           ""
       );

Status: 
Fixed

Dev Response: 

Added in explicit state transition checks

Medium-risk issues

2. Too Much Usage Of The Helper Methods.

File: In the programs/phase-protocol/src

Description: 

There is a lot of usage of helper function unwrap(). inside every contract in the given directory. Although helper methods like unwrap(). are extremely helpful during the development and testing phase, making use of these functions in a production environment is an extremely bad practice that should be avoided because this usually causes the program to panic! and does not even show any helpful messages to the user to help solve or understand the problem. 

Remedy:

Some usage of unwrap(). is justified. Proper conditional statements or Some/ None should be made use of because they are more safe and secure.

Status: 

Fixed

Dev Response: 

Added Error handling and messages for client

Low-risk issues

3. Use of Insecure Math In Arithmetic Operations

File:  In the programs

Description: 

Overflow/ Underflow usually happens when the result of any arithmetic operation is outside the range of datatype. There is one example attached below, but it is recommended to use checked_mul(), checked_add() and checked_sub() in all the complex arithmetic operations because they make sure that the variables don’t overflow or underflow. The code below is from programs/phase-protocol/src/state/global_config.rs 

Remedy:

checked_mul(), checked_add() and checked_sub() in all arithmetic operations in the codebase.

Status: 

Fixed

Dev Response: 

Added in a max length constraint to any dynamic sized pda's. The rest are static sizes so there is not overflow issues from user input

4. Use Of Outdated And Vulnerable Crates

Description: 

We used the `cargo audit` to test and detect any outdated and vulnerable crates that are used by the phase protocol and we found out that the contract is utilizing the outdated version of `time` crate. The version in use is 0.1.44 while the latest version is 0.2.23.

Remedy:

We recommend that the given crate should be upgraded to the latest version in order to protect the contracts from any malicious actions. Moreover the old versions are usually less optimized when compared to newer releases.

Status: 

Fixed

Dev Response: 

Updated to latest version of solana-sdk to solve issues. But dep potential seg fault issues still exist in the latest versions

Informatory issues and Optimizations

5. Inexistent math overflows checks 

Description: 

In the overall code, there is a lack of underflow and overflow checks inside the cargo.toml.

Remedy:

In all the cargo.toml files, underflow and overflow checks should be defined. 

Status: 

Fixed

Dev Response: 

"overflow-checks = true" added to cargo.toml to ensure overflow checks are in release

6. Low Test Coverage

File:  In the programs/phase-protocol/tests

Description: 

The test coverage provided in the tests directory is low, although it covers most of the functionalities but it does not contain the negative test cases. Negative test cases are equally important as compared to positive test cases. The test coverage can be found via the command `cargo tarpaulin`. 

Moreover the test coverage provided contains the positive test cases and it does not have enough negative tests to fully test it out.

Remedy:

Writing proper test scenarios for each of the access points and functionality is among the best practices that must be included.

Status: 

Acknowledged

Dev Response: 

This will be worked on further in coming months but not currently able to be prioritized

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 Spin Finance (Client) for the purpose of conducting a Smart Contract Audit/Code Review. This document presents the findings of our analysis which started on  31st May 2022 . 

Scope

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

Project Overview

Spin is a DeFi derivative infrastructure built on NEAR Protocol, a reliable and scalable L1 solution. The on-chain order book solution offered by Spin provides a CEX-competitive experience to DeFi users.

Founded in June 2021, Spin was the first product to offer an on-chain order book solution on NEAR Protocol. The advantages of the order book model include better user experience compared to AMM, flexible liquidity, easy access for institutional traders, secure and transparent on-chain verification, opportunity to price different types of instruments, and trading robots interoperability.

System Architecture

Central Limit Order Book Model
Spin uses a single-asset pool, the liquidity of which is sent for market making to the order book. Thus, traders will always have enough liquidity for comfortable trading, and investors can profit from the sophisticated market-making mechanism developed by the Spin team.

Spot trading
The current version of the Spin spot DEX on NEAR is the first order book implementation that supports on-chain order matching and NEAR wallet connection. Currently, the Spin spot DEX is already live on mainnet: https://trade.spin.fi. Spin also provides users with an opportunity to make instant token swaps at the market price in a single click. Spin boasts lower fees compared to AMMs. 

On Spin, for example, on USN/USDC, the taker fee is 0.04% and the maker’s rebate is -0.02%. At the same time, NEAR Protocol’s largest AMM Ref Finance charges 0.3% from swappers.

Methodology & Scope

The codebase was audited using a filtered audit technique. A band of four (4) auditors scanned the codebase in an iterative process spanning over a time of two (2) weeks. 

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

The analysis indicates that the contracts under scope of audit are working properly excluding swap functionality which contains one recent issue.

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 cargo-audit & cargo-tarpaulin for static analysis and cargo-fuzz for fuzzing invariants. 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)
1	High Risk issue(s)
1	Medium Risk issue(s)
3	Low Risk issue(s)
7	Informatory issue(s)

Key Findings

Detailed Overview

Critical-risk issues

No issues were found.

High-risk issues

1. Logic Flaw in set_availability() will lead wrong state of the market been set

File:  In the marketplace/src/market.rs

Description: 

In the latest commit a mechanism for set_availability() function is changed and an assert!() statement is included to make sure that markets are not set when “placing is on and canceling is off”, the problem here is if both the placement and canceling is off then this assert!() statement will return true because of !availability.allow_place and will eventually set the market in the set_market_option() function of marketplace.rs contract. So because of lack of implementation of proper checks, the market will be set. 

   pub fn set_availability(&mut self, availability: MarketAvailability) {
       assert!(
           !availability.allow_place || availability.allow_cancel,
           "Market state when placing is on and cancelling is off is not possible."
       );
       self.availability = availability;
   }

Remedy:

There should be two checks, the first one will verify whether both the allow_place and allow_cancel are not false at the same thing. After that the current assert!() statement will be called. Adding a statement like above the already present assert!() statement will solve it.

       assert!(
           availability.allow_place || availability.allow_cancel,
           ""
       );

Status: 

Acknowledged.

Dev Response:

Spin consider "allow_place==false and allow_cancel==false" a valid market state. Moreover, that's required for a market to be created with such a configuration, so we can 

1. ensure market was created with correct base-quote pairs

2. ensure the fees and limits were set correctly

2. Order expiration time can be exploited 

File:  In the marketplace/src/marketplace.rs

Description: 

order ttl can be exploited by delaying or keeping users' transactions in the mempool. As the expiry time is calculated by current time_stamp + ttl.Current_time is calculated at the time the transaction is included in the block. Interested parties can keep the order in waiting queues or just do not include it in the block. When the prices are favorable they can execute the order. Although to pull this kind of attack requires high technical resources, it still can be exploited.

fn make_order(&mut self, order: PlaceOrder, meta: PlaceOrderMeta, signer: AccountId) -> Order {
       let expiration_time = if let Some(ttl) = order.ttl {
           // to nanoseconds
           meta.timestamp + (ttl * 1_000_000_000)
       } else {
           Timestamp(0)
       };
       self.order_id_sequence += 1;
 
       Order {
           id: self.order_id_sequence,
           acc: signer,
           price: order.price,
           average_price: U128(0),
           quantity: order.quantity,
           remaining: order.quantity,
           updated_at: Timestamp(0),
           created_at: meta.timestamp,
           expiration_time,
           o_type: order.order_type,
           client_order_id: order.client_order_id,
       }
   }

Remedy:

let expiration_time = ttl;

Status: 

Fixed

3. Potential MultiSig Failure/ Phishable Deployment 

File:  In the spot/src/lib.rs

Description: 

env::signer_account_id() is the account that originates a transaction on the NEAR protocol. In case of a multisig governance account management may not work and the deployer can be phished to interact with malicious contracts. Since the below contracts check on the transaction originator, this check can be bypassed. For further info please take a look at this SWC . 

pub fn create_market(&mut self, base: AccountId, quote: AccountId) -> MarketID {
       let _session = self.make_session();
       let signer = env::signer_account_id();
 
       self.marketplace
           .create_market(signer.as_str(), base.to_string(), quote.to_string())
   }

Remedy:

Use the predecessor’s account so that a call coming through a multisig governance contract could be handled appropriately.

let caller = env::predecessor_account_id().to_string();

Status: 

Fixed

4. Potentially dangerous drop_state() method

File:  In the spot/src/lib.rs

Description: 

The drop_state() function should be omitted as it is already a very unsafe method. Moreover it is calling the clear() function from currency.rs contract. The clear() function is just removing all the markets at once.

pub fn drop_state(&mut self, keys: Vec<String>, keep_balances: bool) {
       let _session = self.make_session();
       let signer = env::signer_account_id();
       self.marketplace
           .drop_state(signer.as_str(), keys, keep_balances)
   }
 
   /// Set contract root state.
   /// Used to change the state of the contract when updating the contract.
   pub fn set_root_state(&mut self, state: String) {
       let signer = env::signer_account_id();
       self.marketplace.ensure_root(signer.as_str());
       self.marketplace.ensure_markets_are_stopped();
 
       Promise::new(current_account_id()).function_call(
           "set_root_state_callback".to_string(),
           json!({ "state": state }).to_string().into_bytes(),
           0,
           SINGLE_CALL_GAS,
       );
   }

Remedy:

The drop_state() method should be discarded

Status: 

Fixed

5. Inessential parameter optional visibility

File:  In the spot/src/lib.rs

Description: 

The parameter ttl in the place_ask() and place_bid() function should not be optional and there must be a mandatory limit for how long a specific order exists. Not giving the order a time-to-live will open it for many different attack scenarios, and the order would be at risk of MEV attacks.

pub fn place_bid(
       &mut self,
       market_id: MarketID,
       price: Price,
       quantity: Quantity,
       ttl: Option<u64>,
       market_order: bool,
       client_order_id: Option<u32>,
   ) -> Option<OrderID> {
       self.place_order(
           PlaceOrder {
               price,
               quantity,
               ttl,
               market_order,
               order_type: OrderSide::Bid,
               client_order_id,
           },
           market_id,
       )
   }

Remedy:

It is our recommendation from the security perspective that each order should always have a time-to-live.  

Status: 

Fixed

Medium-risk issues

6. Unoptimized loop with redundant ops

File: In the marketplace/src/market.rs

Description: 

Inside the handle_execution_effects() function of market.rs contract, a continue; statement should be placed at the end of the first loop, currently it is also executing the whole function even if the particular order is empty. This is a waste of resources and gas, to make the code more optimized and secure from redundancy a continue statement should be placed.

fn handle_execution_effects(
       &mut self,
       balances: &mut Balances<P>,
       effects: &ExecutionEffects,
   ) -> Price {
       let execution_price = effects.applied_price();
       let mut taker_base_amount = U128(0);
       let mut taker_quote_amount = U128(0);
       for match_effect in &effects.apply_to {
           if match_effect.order.is_empty() {
               Accounts::<P>::remove_order(
                   &match_effect.order.acc,
                   self.id,
                   match_effect.order.id,
               );
           }

Remedy:

So it is advised to use a continue statement here and end the loop for the particular order.

Status: 

Acknowledged

Low-risk issues

7. Impossible ownership transfer

Description: 

There exists no function or implementation for transfer ownership or renounce ownership. These two functions are very necessary for any protocol so that a trust in potential multisig governance is built. 

Remedy:

We recommend that these functions should be added in the codebase to make it more usable and resistant in the long term.

Status: 

Acknowledged

8. Illegal Max Fee Possibility

File:  In the marketplace/src/market.rs 

Description: 

Inside the set_fees() function, there should be a maximum cap on how much the fees could go up. Or what is the max limit on how much the fees could be set. Because currently it is just checking whether or not the taker_fee is greater than or equal to zero, and if the maker_fee is less than or equal to taker_fee. In the current implementation a fee of more than 100% could be set up, which is an indicator of rug-pull for the users. 

Moreover there should also be a lower cap on the maker_fee. This will increase the credibility of your code. 

pub fn set_fees(&mut self, fees: MarketFeesInput) {
       assert!(
           fees.taker_fee >= 0,
           "Taker fee should not be less than zero."
       );
       assert!(
           fees.taker_fee >= fees.maker_fee.abs(),
           "Taker fee should be greater than or equal to maker fee."
       );
       self.fees = MarketFees::new(fees);
   }

Remedy:

There should be a proper check for setting the max upper and lower limit for the fees and if the provided fees exceed the limit then it should discard it.

Status: 

Fixed

9. Ineffectual availability of market

File:  In the marketplace/src/market.rs 

Description: 

In the market.rs contract of order book there is a function is_running(). It will check whether or not the market is running or not, and even if a single user is available then it will return true. The current condition that it is checking is self.availability.allow_place || self.availability.allow_cancel

There is an inconsistency here that needs to be addressed, if the allow_cancel for any market is false and allow_place is true then it would return true as the return value for is_running() function. 

pub fn is_running(&self) -> bool {
       self.availability.allow_place || self.availability.allow_cancel
   }

Remedy:

It is therefore recommended that setting the availability of the marketplace should be restricted so that whenever allow_cancel = false it should also be that allow_place = false , hence a consistent pattern of availability is achieved. In fact, it would be highly inconsistent if the canceling orders is turned off but placing orders is turned on. The allow_place could be set as false independently of allow_cancel, but the vice versa should not be true.

Status: 

Acknowledged

10. Inexistent math overflows checks 

Description: 

In the overall code, there is a lack of underflow and overflow checks inside the cargo.toml.

Remedy:

In all the cargo.toml files, underflow and overflow checks should be defined. 

Status: 

Acknowledged

Informatory issues and Optimizations

11. Inconsistent asserts and panics

File:  In the spot/src/lib.rs

Description: 

On multiple occasions e.g. as the one below an inconsistent checking pattern is observed where the function asserts for an invariant and also uses panics in if blocks to panic the code execution. This is an anti-pattern check in case of panics and can be replaced with using asserts. 

assert_eq!(sender_id.as_str(), signer.as_str(), "Invalid signer");
      if amount.0 == 0 {
          Near::panic("Attached deposit balance must be greater than 0.");
      };

Remedy:

Specifically, in the above mentioned case the statement amount.0 == 0 is purely anti pattern and can be replaced with the assertion assert_ne! following appropriate checks.

Status: 

Acknowledged

12. Unoptimized error message pattern 

File:  In the spot/src/lib.rs

Description: 

No standardized error codes were found in the codebase. The error handling is not upto the mark at this stage of implementation following any best practice or coding style.

Err(_) => Near::panic("Failed to get currency data."),

Remedy:

Create an error.rs file and define all the error messages as standardized error codes with explanatory messages, part of optimized best practices.

// Signatures and access
#[msg("Access denied")]
AccessDenied,

Status: 

Acknowledged

13. Inconsistent arguments type

File:  In the spot/src/ft.rs 

Description: 

In the ft.rs file, the functions storage_deposit() and ft_metadata() implement a mismatching argument type deviating from the code style and best practices. The arguments received by both functions consist of empty vectors but the ways are different in both.

/// Storage deposit.
pub fn storage_deposit(currency: AccountId) {
   Promise::new(currency).function_call(
       "storage_deposit".to_string(),
"{}".as_bytes().to_vec(),
       Decimal::new(125.into(), 5).scale(NEAR_DECIMALS).value().0,
       SINGLE_CALL_GAS * 2,
   );
}

/// Returns promise for ft_metadata.
pub fn ft_metadata(address: AccountId) -> Promise {
   Promise::new(address).function_call("ft_metadata".to_string(), vec![], 0, SINGLE_CALL_GAS)vec![]
}

Remedy:

It is therefore recommended that the functions should implement a singular pattern of sending arguments to a low level function call so that consistency is maintained and readability is improved.

Status: 

Acknowledged

14. Inexplicable balances variable

File:  In the spot/src/lib.rs 

Description: 

In the lib.rs file in the spot directory, the drop_state() function asks for the keep_balances boolean variable to confirm whether to keep balances before dropping and clearing a market state. This check is assumed to be an obvious pattern as setting it negative will cost in losing the user’s internal balance collections.

pub fn drop_state(&mut self, keys: Vec<String>, keep_balances: bool) {
       let _session = self.make_session();
       let signer = env::signer_account_id();
       self.marketplace
           .drop_state(signer.as_str(), keys, keep_balances)
   }

Remedy:

It is suggested that the param be removed from the function. If the need is necessary then the following check should be modified to only remove non-zero balances.

for key in keys {
           let key = base64::decode(key).expect("Invalid key");
           if keep_balances && key.starts_with(&balances_key) {
               continue;
           }
           P::remove(&key);
       }

Status: 

Fixed

15. Inconsistent functions to handle errors

File:  In the marketplace/src/marketplace.rs

Description: 

In the get_orders() function, a `if Some` and `else` statement should be added so that if none of the orders are found for users in a particular market, then it should throw a panic() statement. This will make the code more consistent with the error handling. The panic statement was implemented in the get_order(0 function, so it should also be implemented in get_order().  

pub fn get_order(
       &self,
       market_id: MarketID,
       account_id: AccountId,
       order_id: OrderID,
   ) -> Order {
       let market = self.markets.get(&market_id).expect("Market not found.");
       if let Some(order) = market.get_order(account_id, order_id) {
           order
       } else {
           P::panic("Order not found.");
       }
   }

/// Returns list of user [Order].
   pub fn get_orders(&self, market_id: MarketID, account_id: AccountId) -> Vec<Order> {
       let market = self.markets.get(&market_id).expect("Market not found.");
       market.get_user_orders(&account_id)
   }

Remedy:

The Panic statement should also be implemented in the get_orders() function just like it was implemented in get_order().

Status: 

Acknowledged

16. Unhandled whitelisting removal

File:  In the marketplace/src/marketplace.rs

Description: 

Inside this contract, there are functions to set the whitelist and fetch them but there is no function to remove the whitelist. In the long run this could prove to be worrisome as it will keep on accumulating the whitelist but there will be no way to remove the old and unnecessary whitelist. It will also be effective on the storage as data that is useless will take up resources. 

Remedy:

A method that would be used to remove the whitelist in the marketplace.rs contract should be created. 

Status: 

Acknowledged

17. Quality of Test Cases

Description: 

The test cases that are written are only negative test cases and there is no positive test coverage for the provided code. A full positive test coverage should be provided for the functionalities. 

Remedy:

Positive test cases should be properly defined.

Status: 

Acknowledged

18. Incomplete condition evaluation

File:   In the marketplace/src/order_book/order.rs 

Description: 

In the from(b: u8) function it will return OrderSide: Bid if b will be 0 and for all the other numbers it will return OrderSide: Ask. 

impl From<u8> for OrderSide {
   fn from(b: u8) -> Self {
       if b == 0 {
           OrderSide::Bid
       } else {
           OrderSide::Ask
       }
   }

Remedy:

Our recommendation is an `else if` statement should be created that will check whether b==1 then it will return OrderSide: Ask and for the else it will throw an error.

Status:
Acknowledged

19. Misidentified default order behavior 

File:  In the marketplace/src/order_book/order.rs 

Description: 

Inside the default() function it is returning the OrderSide: Ask. The default behavior should not be OrderSide: Ask.

impl Default for OrderSide {
   fn default() -> Self {
       OrderSide::Ask
   }
}

Remedy:

There should not be any default OrderSide, instead all the OrderSide should be defined accordingly at runtime.

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

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

Project Chrysus aims to be a fully decentralized ecosystem revolving around Chrysus Coin. Chrysus Coin (Chrysus) is an ERC20 token deployed on the Ethereum network, which is pegged to the price of gold (XAU/USD) using Decentralized Finance (DeFi) best practices. The ecosystem around Chrysus will involve a SWAP solution, a lending solution, and an eCommerce integration solution allowing for the use of Chrysus outside of the DeFi ecosystem. 

One of the main goals of Chrysus is not just closely to follow the price of gold but also to be a cash flow-generating token. This is achieved through the Chrysus Governance Token (CGT), which will serve both as a decentralization tool for the system and as a reward tool for Chrysus token minters. Fees collected through the different components of the Project Chrysus ecosystem will be re-distributed to CGT token holders who actively participate in the stability mechanisms of the platform.

System Architecture

Project Chrysus

Project Chrysus (PC) is a Decentralized Finance (DeFi) ecosystem, revolving around the Chrysus Coin (CHC: a synthetic asset tracking the price of gold) and the Chrysus Governance Token (CGT). Users can generate CHC by leveraging ETH and DAI as collateral, also known as Collateralized Debt Positions (CDPs)

Chrysus Coin

Chrysus (CHC) aims to become a de-facto stablecoin pegged to the price of gold. The pegging mechanism will be fully decentralized and will be a hybrid between the solutions developed by Maker and Synthetix.
It borrows the best aspect from both systems where it has: 

• Lower collateral requirement than Synthetix 
• Improved reward structure for opening of positions compared to Maker 
• Non-inflationary stability mechanisms for recapitalization of the system 
• 1:1 profit with the actual tracked asset

Governance Token (CGT)

Chrysus Governance Token (CGT) is the governance token issued as a reward for participants in the ecosystem. CGT will be a perpetually inflationary token, which will be rewarded for: 

• Minting Chrysus
• Providing liquidity to Chrysus 
• Chrysus Lending and Borrowing

The CGT token’s main purpose is to be used for governance via voting on the system’s parameters. An additional use of the CGT token will be the stability system, where people who stake CGT will receive a percentage allocation of all fees collected throughout the Chrysus Ecosystem. This ensures both the stability of Chrysus versus its peg and well providing multiple cash flow streams. 

For instance: A user deposits ETH & DAI to mint CHC, based on this they will be eligible for part of the daily CGT allocation. The user can use this Chrysus token in the Lending Borrowing module, enabling them to receive additional CGT. The user then stakes CGT in the Stability Module, enabling them to receive % of all fees collected on the platform.

Methodology & Scope

The codebase was audited using a filtered audit technique. A band of three (3) 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 like Slither.

AUDIT REPORT

Executive Summary

The analysis indicates that the contracts under scope of audit are working properly.
Our team performed a technique called “Filtered Audit”, where the contract was separately audited by three individuals. Followed by a rigorous process of manual testing and automated review using slither for static analysis and foundry for fuzzing invariants. All the flags raised were re-tested to identify the false positives.

Our team found

# of issues	Severity of the risk
4	Critical Risk issue(s)
2	High Risk issue(s)
2	Medium Risk issue(s)
3	Low Risk issue(s)
17	Informatory issue(s)

Key Findings

Detailed Overview

Critical-risk issues

Incorrect LP shares calculation

Source: contracts/Pair.sol

Description: 

In the Pair contract, the mint function mints liquidity shares, using a formula as follows;

} else {
     liquidity =
       (amount0 * _totalSupply) / _reserve0 - //difference calculated here
       ((amount1 * _totalSupply) / _reserve1);
   }

The concerning issue is that the function uses the difference of two values to calculate the liquidity share.

A simple edge case test confirms the vulnerability, that is, if a user deposits 100 and 100, no LP tokens are minted, completely forbidding the user to claim back the deposited amount of tokens entirely via the burn function, which fails with the UniswapV2 error message of insufficient burn liquidity.

In the meanwhile, Uniswap V2 uses the same formula as above, but selecting the minimum of the above two factors, the user share is calculated accordingly to mint an appropriate amount of LP tokens, and burning these LP tokens results in the claim of appropriate deposited tokens. 

This is a critical flaw in the Chrysus Pair contract tokenomics concerning the minting and burning functionalities and should be handled with immense care during restructuring and thorough testing.

Proof of Concept:
Consider a scenario, and a user deposits 100 and 80 amounts of tokens 0 and 1, respectively; they will receive 20LP tokens representing the user’s share of liquidity.

If the user then tries to burn these 20LP tokens, the burn formula (being indifferent from UniswapV2’s standard burn) miscalculates the reserves to give the user an irrational amount of the originally deposited tokens as shown in the below test case.

Hence, in all cases of subtracting the reserves, users won't be able to withdraw their actual share of reserves resulting in a loss of the original deposited tokens.

The following test verifies the values;

it("check liquidity share calculation formula", async function () {
 
   await dai.connect(daiHolder).approve(pair.address, BigInt(10000E20))
   await dai.connect(daiHolder).transferFrom(DAI_HOLDER, pair.address, BigInt(100E18))
   await chrysus.transfer(pair.address, BigInt(100E18))
   await pair.connect(team).mint(team.address)
 
   console.log("\n User1 has minted \n");
 
   await dai.connect(daiHolder).approve(pair.address, BigInt(10000E20))
   await dai.connect(daiHolder).transferFrom(DAI_HOLDER, pair.address, BigInt(100E18))
   await chrysus.transfer(pair.address, BigInt(80E18))
   await pair.mint(accounts[0].address)
 
   console.log("\n User2 has minted \n");
  
   await pair.transfer(pair.address, BigInt(await pair.balanceOf(accounts[0].address)));
   await pair.burn(accounts[0].address);
  
   console.log("\n User2 has burnt \n");
 
   console.log("dai received: ", await dai.balanceOf(accounts[0].address));
   console.log("chu received: ", await chrysus.balanceOf(accounts[0].address));
 });

Results from the tests:

 /** User2 amounts:
  * Deposited DAI: 100000000000000000000
  * Received DAI:  9850000000000000000000
  *
  * Deposited CHC: 80000000000000000000
  * Received CHC:  33333333333333333333
  */

Remedy: 

It is recommended to restructure the formula following the UniswapV2 logic, as above, to compute over the minimum value of the two reserves instead of the difference.
Alternatively, the burning functionality can also be redefined by following the minting formula in an off-by-one manner, i.e., the current minting logic could be mirrored in the burning function as well when the user tries to burn their deposited tokens. 

Status: Fixed

DoS attack stops minting completely

Source: contracts/Governance.sol

Description: 

In the Governance contract, the function mintDaily mints governance tokens (CGT) on a daily basis to multiple contracts. However, if it is called right before 24 hours have passed, the function still executes with a zero minting amount but also resets the variable lastMintTimestamp.

Consider a scenario, an adversary calls this function after 23 hours have passed since last calling, the function (having no checks) will calculate the number of days passed since last mint, that is, zero, will successfully mint zero amounts for the current day and reset the lastMintTimestamp to block.timestamp. This design leads to DoS attacks as the minting can be permanently stopped repetitively calling this function in the nick of time as it approaches a 24 hour cycle.

Proof of Concept:
The following code snippet exhibits the failing functionality;

function mintDaily() external {
       uint256 numDays = (block.timestamp - lastMintTimestamp) / 86400;
	...
   }

Remedy: 

Place a check to revert the execution if the value of variable numDays is not equal to one.

Status: Fixed

Incomplete functionality in the Chrysus contract

Source: contracts/Chrysus.sol

Description: 

In the Chrysus contract, the liquidate function enables self liquidation for users who initially deposit collateral to the contract and executes a swap functionality on the UniswapV2Pair while liquidating. This swap requires that the amounts to be swapped should be transferred before calling the swap function leading to a conflicting assumption that; 

1. msg.sender in this scenario shall always be an EOA, since it allows users to deposit and withdraw collateral,
2. meanwhile, assuming that the same user should interact with Chrysus contract (as it looks up to the mapping of user deposited collateral) expecting to transfer the amounts of tokens before swap occurs through a callback feature OR externally via atomic transactions.

This leads to two possible scenarios:

• Either the msg.sender is considered a smart contract in context of the liquidate function as it will require external actions to pre-transfer the amounts of tokens to UniswapV2Pair swap call using the callback feature implementation,
• Or all interactions with the Chrysus contract are done through a periphery layer that implements all the validatory checks and transfers the amounts on EOA user’s behalf during the liquidate call, leading to an invalid state of msg.sender being the periphery itself.

Thus if the swap callback is considered, the issue arises that only contracts implementing it will be allowed to liquidate themselves leading to the point that originally just these contracts should be allowed to make deposits and withdrawals.

Currently, only EOAs are assumed to make deposits and withdrawals, that is, users can directly interact with the token contract, this makes the liquidation mechanism confusing to call, rather test, and the functionality is inevitably rendered as incomplete.

Remedy:

It is highly recommended that the Chrysus contract liquidation mechanisms be restructured following the remedies in issue # 08 of this report and go through effective unit testing confirming all the positive and negative test cases work correctly.

Developer Response:
“At the early stages of the system you need to self-liquidate because you cannot rely on 3rd parties until the system becomes used enough. And even then, there is no good reason not to self-liqdudate… Sure, if the recommendation is to include external liquidations, we can do that. I suggest:

- 1% of the liquidation amount

- Hide the liquidation amounts in order to avoid cherry picking the liquidations”

Conclusion: 1% of the liquidation amount will be given as a reward to the liquidator though the percentage is subjected to change based on the average position size.

Auditor Response: The Client’s response elaborates on a new functionality that includes potential changes in the business layer. This iteration of audit is, henceforth considered as, completed from the Auditor.

Status: Fixed

Potential reentrancy risk in Chrysus Token

Source: contracts/Governance.sol

Description:
In the Chrysus contract, the function withdrawCollateral is prone to reentrancy; a particular attack vector with which a malicious contract can reenter a vulnerable contract in a nested manner. Specifically, it first calls a function in the vulnerable contract, but before the first instance of the function call is finished, a second call can be arranged to re-enter the vulnerable contract by invoking functions that should only be executed once. There are certain principles advised that mitigate such attack vectors like mutual exclusion (mutex sections), checks-effects-interactions (design pattern), pull over push (payment method) and whitelisted calls (trusted users only).

Proof of Concept:
The following code snippet exhibits the failing functionality;

function withdrawCollateral(address _collateralType, uint256 _amount)
   external { ... }

This function relies on msg.sender.call() to transfer the value to the caller. However, this design is vulnerable as fallback calls to msg.sender can be easily crafted to reenter the Chrysus contract. Apparently, the interaction with the external contract happens (line 375) when the msg.sender is sent value during the withdrawal. In this particular case, the external contract may hold certain hidden logic that can allow it to reenter the vulnerable contract.

Remedy:

It is highly recommended to use lock modifier with this function to ensure a mutex section is created and that the function is entirely executed in its atomicity.

Status: Fixed

High-risk issues

Potential manipulation of Governance voteCount

Source: contracts/Governance.sol

Description: 

In the Governance contract, the function executeVote makes an external call to the destination address with function name and bytes data as arguments. In all cases when the call to execute a vote at the destination address either passes or fails, the Vote.result is always saved as true. The return values from this external call are never checked or validated opening a window of false assumptions regarding execution of votes.

An instance of critical severity is the defected usage of the executed state variable inside the struct Vote, which is assumed to be updated during this execution but is never set throughout the codebase.

The design to execute votes in the governance contract hints at enforcing execution of arbitral contracts’ functions, only requiring a successful proposal. However, the assumption that reflects this is that the proposed vote is always benevolent and contains trusted contract calls only. In most cases of executing votes in smart contracts, this is a severe design flaw and is therefore recommended as requiring restructure.

Proof of Concept:
The following code snippet exhibits the failing functionality;

if (...) {
           v.result = true;
           address _destination = v.voteAddress;
           bool _succ;
           bytes memory _res;
           (_succ, _res) = _destination.call(
               abi.encodePacked(v.voteFunction, v.data)
           ); //When testing ...
       } else { ...

Consider a scenario where a vote is successfully proposed through proposeVote. Upon calling the executeVote function, if the external call to the destination address fails due to any inherent internal calls failing; the vote.result will be stated as true, but the variable voteCount will still be incremented. This shows that the governance contract aims to execute votes with failing results and does not handle the cases of reverting external calls.

Remedy:

Place checks on return values of destination.call for success to always be true and restructure the function to maintain the state of vote using the available variable executed.

Status: Fixed

Unchecked return values Dependance

Source: contracts/Chrysus.sol

Description:
In the Chrysus contract, there are multiple instances of value transfer functions like call, transfer and transferFrom that need return values to be checked before continuing execution. This ensures that all misassumptions are cleared during external calls and that the return value, specifically the boolean status of the call, is always checked according to the expected result ahead of remaining function execution.
 

Proof of Concept:
The following code snippets exhibit the failing functionality, in the withdrawCollateral function.

Chrysus.sol - Line # 408:

(success,) = address(swapSolution).call{ value: DSMath.div(_fees, 5) }("");

Chrysus.sol - Line # 412:

(success, ) = address(stabilityModule).call{ value: DSMath.wdiv( DSMath.wmul(_fees, 5000), 10000)}("");

Chrysus.sol - Line # 440:

IERC20(collateralType).transferFrom(address(this), pair, amount);

Chrysus.sol - Line # 442:

success = IERC20(collateralType).transfer(address (stabilityModule), DSMath.div(_fees, 2));

Status: Fixed 

Medium-risk issues

Rounding-off errors in collateralRatio

Source: contracts/Chrysus.sol

Description: 

In Chrysus contract, the view function collateralRatio returns a zero value for the collateralizationRatio variable when the totalCollateral may be less than the price of CHC, causing a proper fractional division. Since solidity by design does not handle floating points, the collateralizationRatio is returned after rounding-off to 0 in all cases where the division outputs a number between 0 and 1. For instance, if collateralizationRatio is supposed to be 0.99 it is instead stored as 0, negating all following calculations. 

Remedy:

It is highly recommended that the collateralizationRatio be restructured such that a factorial multiplier of the collateral is always returned instead of zero hence, all following arithmetic operations will be handled with precision.

Status: Fixed

Unorthodox options to liquidate

Source: contracts/Chrysus.sol

Description:
In the Chrysus contract, the current implementation of the liquidate function is bound for self liquidations only. Standalone self liquidation is an unorthodox practice, rare in industry standards of DeFi external liquidations. It is therefore suggested that the liquidate function be restructured such that:

• New functionality is added alongside the main liquidate function to accommodate self liquidations,
• liquidate shall accept arguments like userToLiquidate, +
• +collateralType and liquidationAmount with appropriate checks for these added inputs. 

Status: Fixed

Low-risk issues

Governance token contract does not emit relevant events

Description: 

It is necessary to emit relevant events on critical functions such as before proposing, executing votes or minting tokens.

Remedy: 

Emit relevant events on all user facing functions in the Governance contract.

Status: Fixed

Ensure contract is initialized

Description: 

In the Governance contract, a function named mintDaily mints tokens to multiple contracts on a daily basis, however the current implementation does not ensure the contract is initialized before it starts mining the governance voting powered tokens to different contracts in the protocol.

Remedy:

Place modifier on mintDaily function that checks whether the contract is initialized with an authorized role.

Status: Fixed 

Implement checks effects interactions pattern

Description:
The following instances in Chrysus contract do not comply with the checks-effects-interactions pattern which is a recommended design pattern for smart contracts.

• Move this line before the _mint call

userDeposits[msg.sender][_collateralType].minted += amountToMint;

• Move this line before the _burn call

userDeposits[msg.sender][_collateralType].minted -= amountToMint; 

Status: Fixed

Informatory Issues & Gas Optimizations

Improve functions layout

Description:
As stated in the Solidity style guide, the functions should be grouped according to their visibility and ordered:

• constructor
• receive function (if exists)
• fallback function (if exists)
• external
• public
• internal
• private

Status: Fixed 

Avoid using floating pragma solidity version

Description: 

Throughout the codebase, the current implementations of smart contracts have used floating pragma solidity versions. This practice leads to incompatible contracts during testing which is a matter of serious security because developers may have a different set of assumptions than auditors for testing of the contracts. It is therefore recommended to use a fixed solidity version across all smart contracts in the codebase.

Status: Fixed 

Inexistent arithmetic overflow checks

Description:
Throughout the codebase there are certain contracts that use pre 0.8.x solidity versions and are liable to arithmetic overflow and underflow exceptions. It is recommended that all the instances covering explicit mathematical operations be handled with safemath or using solidity versions greater than 0.8 otherwise. 

Status: Pending

Low test coverage & documentation

Description:
Throughout the codebase, the testing is insufficient to support the positive  and negative test cases from the protocol engineering team. The low level of testing points to a critical lacking in the project as many cases are 

Status: Pending

Use proper linting and prettier for codebase

Description:
Throughout the codebase, cosmetic tools like prettier can be utilized to execute linting and code styling as suggested by the SOlidity docs so that code readability is improved.

Status: Fixed

Use vetted codebase

Description:
Throughout the codebase, libraries have been written in-house, though the functions and instances use similar conventions yet this strongly suggests that audited and vetted contract/ libraries and dependencies be used for all helper and auxiliary operations. It is therefore recommended that;

• The instances of SafeERC20 and functions derived from it should be imported from the OpenZeppelin’s suite of smart contracts.
• Non-reentrant functionality should be imported from openzeppelin instead of writing an internal lock modifier.

Status: Fixed

Remove unused state variables

Description:
In Chrysus.sol, the two state variables at the top of the Chrysus contract named ethBalance and ethFees are never used inside the contract and consume unnecessary storage. It is advised to prepare a structured version of the contract that is pre-reviewed for all issues like these before moving to audit. 

Status: Fixed

Solidity docs: Coding Styles

Description:
For the codebase, in general, it is highly recommended to follow the coding styles and best guides for layout in the solidity docs. For instance, variables in Chrysus contract like; address public governance; address public treasury; address public auction; can be moved to the top and arranged considering the tight variable design pattern.

Status: Fixed

Mark as immutable

Description:
In Chrysus.sol, there are some instances for state variables that can be marked as immutable, such as; ISwap public swapSolution; IStabilityModule public stabilityModule; IUniswapV2Pair public pair;
Also ensure that variables have explicit visibility set as for the following; AggregatorV3Interface oracleCHC; AggregatorV3Interface oracleXAU;

Status: Fixed

Misleading naming

Description:
In Chrysus.sol, the variable collateralRequirement can be renamed for a more sensible and user readable convention such as minCollateral.
For another instance, the function collateralRatio the name getCollateralizationRatio can be used instead to increase readability.

Status: Fixed

Wrapped calling of variable

Description:
In Chrysus.sol, the constructor currently accepts the contract type argument for ISwapRouter _swapRouter. For ease of deployment, the argument type can be modified to accept the address for _swapRouter.

Status: Fixed

Constructor gas optimizations

Description:
In Chrysus.sol, the constructor currently checks against a list of provided addresses to not be zero address using require statement. It is suggested that all these checks be replaced with one accumulating if block or revert with an error message such as ZeroAddress.

Status: Fixed 

Improve constructor readability

Description:
In Chrysus.sol, the constructor readability can be increased by applying the standard coding practices and naming conventions;

• Make the function addCollateralType as public and call it for both DAI and ETH during the constructor execution OR restructure using unchained internal function structure as follows;
  • Place all necessary checks in the external functions and then call the internal function to add the collateral type,
  • E.g. the internal function _addCollateralType can be called inside the external addCollateralType function.

Status: Fixed 

Use modifiers for recurring checks

Description:
In the codebase for multiple instances some checks are used repetitively throughout different functions, for which it is suggested to use modifiers increasing the code readability and best style guides. For instance, the feeToSetter check in the swap contract and the check for governance role can be placed in modifiers.

Alternatively, for the sake of gas optimizations, it is suggested that all require statements be replaced with if statements accompanied by revert messages.

Status: Fixed 

Early return from function

Description:
In Chrysus.sol, the function collateralRatio can be modified to have an early return implemented such that the expensive for loop is avoided in certain practically possible cases. The statement if (valueCHC == 0) could be moved to top, following the declaration of variable uint256 valueCHC and return right after, to avoid the for loop.

Status: Fixed 

Introduce events in the contract Chrysus

Description:
In Chrysus.sol, to keep track of transactions and indexing through block explorers like etherscan it is suggested that all user facing functionalities have events implemented in them and emitted appropriately. Following instances are suggested:

- CollateralDeposited(user, amount)
- CollateralWithdrawn(user,amount)
- AddedCollateralType(address collateralToken)
- Liquidated(liquidator, user, amountLiquidated)
- FeesWithdrawn(treasuryFees, swapSolutionFees, stabilityModuleFees)

Status: Fixed 

Redundant checking for zero address

Description:
In Chrysus.sol, there is a need to restructure code for elegant code blocks in the function named depositCollateral function. Currently the function checks the argument _collateralType for zero address twice during its execution. This is a costly pattern and introduces redundancy in the code blocks. For instance,

- if (_collateralType != address(0)) {
	approvedCollateral[_collateralType].fees += tokenFee;
    }
- if (_collateralType != address(0)) {
      bool success = IERC20(_collateralType).transferFrom(
        msg.sender,
        address(this),
        _amount
      );
      require(success);
    }

Status: Fixed

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.