Try   HackMD
tags: Final Report

BendDAO Audit

Copyright © 2022 by Verilog Solutions. All rights reserved.
May 24, 2022
by Verilog Solutions

Image Not Showing Possible Reasons
  • The image was uploaded to a note which you don't have access to
  • The note which the image was originally uploaded to has been deleted
Learn More →

This report presents our engineering engagement with the BendDAO team on their decentralized non-custodial NFT liquidity and lending protocol – the BendDAO Protocol.

Table of Content

Project Summary

BendDAO is a decentralized peer-to-pool-based NFT liquidity protocol. Depositors provide ETH liquidity to the lending pool to earn interest, while borrowers can borrow ETH from the lending pool using NFTs as collateral.

The BendDAO protocol enables NFT assets to be deposited and converted into ERC721 boundNFTs, which can be used to create NFT loans.

Service Scope

Our review focused on the main branch, specifically, commit hash 087c284fda18e9d7fd3a07de384a2245d5eab6ea.

Our auditing service with the BendDAO team includes the following two stages:

  • Architecture Consultancy Service
  • Smart Contract Auditing Service

  1. Architecture Consultancy Service

    • Price Oracle Design Discussion Meeting
      The Verilog Solutions team discussed with the BendDAO team regarding its collateral price oracle design. Throughout the discussion, the Verilog Solutions team conducted a review of the current oracle design and provided feedback and suggestions to the BendDAO team. It is acknowledged by both teams that the price oracle should be as simple as possible to ensure the reliability of data and reduce the attack surface.

    • Protocol Security & Design Discussion Meeting
      The Verilog Solutions team discussed with the BendDAO team about off-chain component executions and the security best practice for securing the off-chain components. Verilog Solutions team also provided feedback and suggestions on how to ensure reliability for mission-critical off-chain components including the price oracle service and monitoring.

  2. Smart Contract Auditing Service

    The Verilog Solutions team analyzed the entire project using a detailed-oriented approach to capture the fundamental logic and suggested improvements to the existing code. Details can be found under Findings & Improvement Suggestions.

BendDAO Architecture & Protocol Management


Image Not Showing Possible Reasons
  • The image was uploaded to a note which you don't have access to
  • The note which the image was originally uploaded to has been deleted
Learn More →

BendDAO Architecture

BendDAO protocol contains three major parts:

Lending Protocol

  • LendPool.sol: It is the main entry point into the BendDAO Protocol. Most interactions with the BendDAO Protocol happen via the LendPool.sol. Users can either deposit supported assets to earn interest or deposit supported NFTs to borrow reserve assets.

  • LendPoolLiquidator.sol: It contains the main logic of the auction and liquidation functions. LendPool.sol delegates calls to LendPoolLiquidator.sol for auction(), redeem() and liquidate().

  • LendPoolLoan.sol: It is the NFT loan manager of the protocol, which generates a unique loan when NFTs are used as collateral in borrowing, and maintains the relationship between the NFT and the loan.

  • BToken.sol: The BToken is an interest-bearing transferrable token minted and burned upon a deposit and withdrawal. The value of BToken is pegged to the value of the corresponding deposited asset at a 1:1 ratio.

  • DebtToken.sol: The Debt Token is an interest-accruing non-transferrable token minted and burned on borrow and repay. It represents debts owed by a token holder.

There are two major privileged roles for the lending pool:

  • poolAdmin:
    • configure the pool including upgrading contracts, setting essential parameters, adding/removing reserve assets and NFT assets.
  • emergencyAdmin:
    • pause and unpause the lending pool.

NFT Oracle

To provide lending protocol with a market valuation of the underlying asset, the BendDAO team designed an NFTOracle.sol smart contract. The BendDAO team designed a filter algorithm to fetch the floor price data from OpenSea and LooksRare.

  1. The NFT Oracle is currently maintained and operated by the BendDAO team. The data fetching and filtering algorithm scripts are running off-chain.
  2. In the future, the Bend governance mechanisms will manage the selection of data sources.

There are two types of admin roles in the NFT Oracle:

  • _owner: a multi-sig wallet controlled by the BendDAO dev team
    1. setPriceFeedAdmin()
    set a new address for an admin account in the smart contracts.
    2. setAssets()
    add a list of addresses of NFT to the oracle
    3. addAsset()
    add a single address of NFT to the oracle
    4. removeAsset()
    remove a single address of NFT from the oracle
    5. setDataValidityParameters()
    set a list of validity checking & security-related parameters (e.g maximum price deviation, the shortest time interval for the price update .. )
    6. setPause()
    an emergency brake for a specific NFT of the oracle

  • _admin: a wallet imported to cloud server to feed price on-chain
    1. setAssetData()
    set the price of a specific NFT collection.

The admin addresses and private keys are managed and stored on a cloud server, and multiple measures were taken to enhance the security of the price oracle. There are two sets of oracle nodes with independent private keys. The duplicate node design, combined with an on-chain price filter, is to mitigate the price manipulation attack in case one of the private keys is leaked. In addition, the IP addresses of these oracle nodes are hidden to protect the nodes from DDOS attacks.


Image Not Showing Possible Reasons
  • The image was uploaded to a note which you don't have access to
  • The note which the image was originally uploaded to has been deleted
Learn More →

End-to-end encryption is enabled

In addition, the BendDAO team has acknowledged the risks and would actively monitor the oracle addresses. The team also manages a multi-sig wallet as the owner to replace the current EOA admin account of the NFT Oracle smart contract, if necessary.

To minimize the room for price manipulation, there are various constraints for the prices fed to the oracle and frequency of price update (e.g price feeding granularity settings and filters for abnormal prices and prices with volatility higher than accepted).

The operation of the BendDAO protocol is highly dependent on the NFT price oracle. To further enhance the stability and security of the protocol, discussions about NFT oracle design improvement were arranged between the BendDAO dev team and the Verilog Solutions audit team. Based on our discussion about the oracle structure and analysis of the historical data, we concluded that using a time-weighted moving average (TWAP) floor price (e.g., a 6h TWAP price can filter out abnormal floor price data) can be used as a valid market valuation.


Image Not Showing Possible Reasons
  • The image was uploaded to a note which you don't have access to
  • The note which the image was originally uploaded to has been deleted
Learn More →

6h TWAP floor price comparing with the raw floor price

  • Blue Line represents the frequently fetched market raw floor price.
  • Red Line represents the 6h TWAP floor price

BoundNFT

boundNFTs are promissory-note tokens that are minted and burned upon borrowing and repaying, representing the NFT used as collateral owed by the token holder, with the same token ID and metadata. It has a flash claim feature for users to claim airdrops for NFT holders.

There are two admin roles in BNFT contracts:

  • _owner: handles ownership and admin.

  • _claimAdmin: claims and executes ERC20/ERC721/ERC1155 airdrops.

Periphery Contracts

BendDAO protocol also has periphery contracts such as WETHGateway.sol and PunkGateway.sol:

  • WETHGateway.sol: LendPool only supports ERC20 tokens as underlying reserve assets. WETHGateway.sol provides an interface that allows users to deposit native tokens such as ETH directly without the need to swap between the native token and the wrapped native token themself.
  • PunkGateway.sol: Lending pool only supports interactions with the standard ERC721 token. NFT CryptoPunks is not ERC721 compatible. PunkGateway helps users to wrap and unwrap CryptoPunks and interact with lend pool.

Securing Admin Addresses

Lending Protocol, NFT Oracle, and BoundNFT have admin roles. The admin roles are secured via multi-sig and TimelockController mechanisms. The details of the multi-sig and TimelockController addresses can be found at BendDAO: Security and Audits Docs.

Findings & Improvement Suggestions

InformationalMinorMediumMajorCritical

Total Acknowledged Resolved
Critical 0 0 0
Major 0 0 0
Medium 1 1 1
Minor 4 4 1
Informational 7 7 1

Critical

none ; )

Major

none ; )

Medium

  1. Time-sensitive functions such as redeem and auction (LendPool.sol) do not count time elapsed in the paused state. Medium
    [Description]: If an auction starts and then the protocol enters into a pause state, the time elapsed for the pause is still counted in time-sensitive functions such as auction and redeem. As a result, any time elapsed in the pause state will erode into the 48-hour grace period of the auction state. Bidders will have a shorter time window to bid. The borrower will have a shorter time window to redeem.
    [Recommendation]: After unpause, the protocol needs to calculate the duration of the paused period and update auction timestamp to reflect the time elapsed in the paused state.
    [Result]: Pause/unpause is rarely executed unless there is an emergency situation. Resolved in commit 64b7861e48cf0156d5f203e3a39166f22ae36dd0.

Minor

  1. LendPoolLiquidator.sol: unnecessary require in redeem() and redundant parameter bidFine minor
    [Description]:

    • require(vars.bidFine <= bidFine, Errors.LPL_BID_INVALID_BID_FINE); (L218)
      bidFine is an input supplied by the user, while vars.bidFine is a variable calculated from the contract's internal states. vars.bidFine is actually referenced in other places, while bidFine is unused anywhere except this requirement check.
      Therefore, this requirement check is unnecessary, as the input variable being checked bidFine is not used anywhere in other places. The consequence of having this unnecessary requirement check is that if the user inputs a bidFine that is smaller than vars.bidFine, the function call will revert, meanwhile the incorrect bidFine would not have effect anywhere, as bidFine is not used anywhere else except this require check.
    • require(vars.repayAmount <= vars.maxRepayAmount, Errors.LP_AMOUNT_GREATER_THAN_MAX_REPAY); (L227)
      The current design will cause revert in the cases where user input amount that is bigger than the maxRepayAmount. Thus the front-end is responsible for calculating the correct amount used in the function call. If the users are constructing the transaction themselves (i.e. in case of front-end downtime), the users must calculate the correct amount, otherwise the transaction will fail.
      As an alternative design that can improve user experience, the function can just assign the repayAmount with the maxRepayAmount if the amount that user inputs is bigger than the maxRepayAmount.

    [Recommendation]: Remove these two require and assign the repayAmount with the maxRepayAmount if the amount user inputs are bigger than the maxRepayAmount.
    [Result]: Acknowledged. BendDAO team decided to keep this design.

  2. DataTypes.sol: unused LoanState(L64). minor
    [Description]: LoanState.Created is never used. LoanState is changed to Active after createLoan().
    [Recommendation]: Remove the unused state.
    [Result]: It's from the previous code design. BendDAO team decided to keep this design.

  3. Redundant parameter amount in function liquidate() (LendPool.liquidate(): L457, LendPoolLiquidator.liquidate(): L286). minor
    [Description]: function parameter amount is only used in the require statement. User can just pass in any value to bypass the require check.
    [Recommendation]: parameter amount in function liquidate() can be removed.
    [Result]: This parameter is mainly for the WETHGateWay.sol contract, checking the msg.value passing from the gateway contract to the main lend pool contract. BendDAO team decided to keep this design.

  4. AirdropFlashLoanReceiver.sol: function executeOperation() uses method from non-standard EIP721.Minor
    [Description]: Function executeOperation() uses IERC721Enumerable(token).tokenOfOwnerByIndex(), which is not supported by standard EIP721. This function call will fail when interacting with airdrop tokens that do not support this method.
    [Recommendation]:

    • In the short term, the team needs to be careful when supporting new airdrop ERC721 tokens. The token must support ERC721Enumerable.
    • In the long term, we suggest modifying the function and only using methods from standard ERC721, which will greatly improve the compatibility of the contract.

    [Result]: Fixed at commit 6f2a316faf05bc17ac4c19438a22091424766deb and already been tested in the latest Doodles airdrop. BendDAO team will pay extra attention when adding support for new NFT assets.

Informational

  1. Move batchBorrow() and batchRepay() functions to periphery/helper contracts. (LendPool.sol, PunkGateway.sol, WETHGateway.sol) Informational
    [Description]: batchBorrow() and batchRepay() make it possible to execute multiple borrows/repays in one function call. These two functions are essentially multicall to the original borrow and repay functions. Calls to external functions were made inside the loops, which might increase the attack surface of the contract.
    [Recommendation]: We suggest keeping the main contracts simple and elegant, only with necessary functionalities. Less code, fewer attack surfaces. Helper functions like batch functions can be moved to a periphery contract/helper contract and done by a multicall to the function of the main contract. Those periphery contracts can easily be replaced without updating/changing the main contract if any vulnerabilities/issues found in those helper functions.
    [Result]: BendDAO decides to keep this batch feature in the future version of deployments.

  2. Caution when adding new reserve assets. Informational
    [Description]: Tokens with before and after transfer hooks and tokens with double/multiple entry-point bring more security risk. Be cautious when adding new tokens as reserve assets.
    [Recommendation]: Be cautious when adding new tokens as reserve assets.
    [Result]: Acknowledged.

  3. NFTOracle.sol: function removeAsset() code optimization by Using enumerate mapping Informational
    [Description]:
    Generally speaking, it is a better practice to keep the complexity within O(1) for the contracts deployed on Ethereum mainnet.
    [Recommendation]:
    Here are the improved logic:

    1. when addNFT(), save the NFT address to the array and record its index
    2. when removeNFT(), copy the address of the last element to the target removed NFT index and then pop the last one.

    checking the below enumerate mapping example:

    

























    ​​​​address[] public activeNFTs;
​​​​mapping(address => uint256) public indexOfNFT;

​​​​function addNFT(address nftAddress) public {
​​​​    require(nftAddress != address(0), "AO: nftAddress is zero address");
​​​​    require(indexOfNFT[nftAddress] == 0, "AO: already existed");
​​​​    activeNFTs.push(nftAddress);
​​​​    indexOfNFT[nftAddress] = activeNFTs.length;
​​​​}

​​​​function removeNFT(address nftAddress) public {
​​​​    require(nftAddress != address(0), "AO: nftAddress is zero address");
​​​​    uint256 valueIndex = indexOfNFT[nftAddress];
​​​​    require(valueIndex != 0, "AO: does not existed");
​​​​    uint256 toDeteleIndex = indexOfNFT[nftAddress] - 1;
​​​​    uint256 lastIndex = activeNFTs.length - 1;

​​​​    if (lastIndex != toDeteleIndex) {
​​​​        address lastValue = activeNFTs[lastIndex];
​​​​        activeNFTs[toDeteleIndex] = lastValue;
​​​​        indexOfNFT[lastValue] = valueIndex;
​​​​    }

​​​​    activeNFTs.pop();
​​​​    detele indexOfNFT[nftAddress];
​​​​}

    [Result]: This operation is rarely executed and only executed by admin, not users, Hence it's not urgent for the BendDAO team to update the contract code for this suggestion.

  4. NFTOracle.sol: functionsetAssetData() variable incrementation Informational
    [Description]:
    _roundId is currently treated as an input. It would be better if there is an incrementation logic in setAssetData() to ensure the _roundId will not collide with the ones before.
    [Recommendation]:
    Add an incrementation logic inside the function to make sure the _roundId will never collide with the previous ones.

    checking the below example:

    



















    ​​​​  function setAssetData(
​​​​    address _nftContract,
​​​​    uint256 _price,
​​​​    uint256 /*_timestamp*/
​​​​  ) external override onlyAdmin whenNotPaused(_nftContract) {
​​​​    requireKeyExisted(_nftContract, true);
​​​​    uint256 _timestamp = _blockTimestamp();
​​​​    require(_timestamp > getLatestTimestamp(_nftContract), "NFTOracle: incorrect timestamp");
​​​​    require(_price > 0, "NFTOracle: price can not be 0");
​​​​    bool dataValidity = checkValidityOfPrice(_nftContract, _price, _timestamp);
​​​​    require(dataValidity, "NFTOracle: invalid price data");
​​​​    
​​​​    // NOTE: read latest _roundID and increment it by 1
​​​​    uint256 newRoundId = getLatestRoundId(_nftContract) + 1;
​​​​    // NOTE: assign the newRoundID to data
​​​​    NFTPriceData memory data = NFTPriceData({price: _price, timestamp: _timestamp, roundId: newRoundId});
​​​​    nftPriceFeedMap[_nftContract].nftPriceData.push(data);

​​​​    emit SetAssetData(_nftContract, _price, _timestamp, _roundId);
​​​​  }

    [Result]: Acknowledged.

  5. NFTOracle.sol: suggest integrating Chainlink Any API Informational
    [Description]:
    The BendDAO protocol uses OpenSea & LooksRare as the data source. Currently, all data queries & sorting are managed off-chain. Bringing data queries & sorting on-chain can improve the transparency and reliability of the data source.
    [Recommendation]:
    Integrating with Chainlink product Any API. Chainlink enables smart contracts to access any external data sources through its decentralized oracle network.
    [Result]: BendDAO team is considering this suggestion in future development.

  6. NFTOracle.sol: comment mistake [L192] Informational
    [Description]:
    _interval variable no longer existed in the contract
    [Recommendation]:
    change _interval in the comment to twapInterval
    [Result]: Resolved.

  7. NFTOracle.sol: getAssetPrice() returns twap price only. Informational
    [Description]:
    In branch Oracle/roundid's commit, the updated getAssetPrice() does not return the raw price data.
    [Recommendation]:
    Consider returning a (twapPrice, rawPrice) tuple, or adding an extra getAssetRawPrice() function to fetch the raw price data.
    [Result]: BendDAO team decided to keep the original design.

Business Logic Discussion

  1. The liquidation rule only allows collateral to be liquidated when the proceeds can fully recover the amount borrowed(LendPool.sol).

    [Description]: To liquidate collateral, the bid must be high enough to recover the amount borrowed. If the prevailing market price of the collateral drops sharply below the amount borrowed, there might be a lack of bidders to bid at a price that can fully recover the amount borrowed, as bidders might not wish to pay a premium on the prevailing market price. Hence the borrowing becomes undercollateralized.

    [Recommendation]: Discussion needed with the BendDAO team.
    [Result]: As discussed with the BendDAO team, the current design of requiring full payment is a feature to ensure that the lenders are made whole whenever possible.

Disclaimer

Verilog Solutions receives compensation from one or more clients for performing the smart contract and auditing analysis contained in these reports. The report created is solely for Clients and published with their consent. As such, the scope of our audit is limited to a review of code, and only the code we note as being within the scope of our audit detailed in this report. It is important to note that the Solidity code itself presents unique and unquantifiable risks since the Solidity language itself remains under current development and is subject to unknown risks and flaws. Our sole goal is to help reduce the attack vectors and the high level of variance associated with utilizing new and consistently changing technologies. Thus, Verilog Solutions in no way claims any guarantee of security or functionality of the technology we agree to analyze.

In addition, Verilog Solutions reports do not provide any indication of the technologies proprietors, business, business model, or legal compliance. As such, reports do not provide investment advice and should not be used to make decisions about investment or involvement with any particular project. Verilog Solutions has the right to distribute the Report through other means, including via Verilog Solutions publications and other distributions. Verilog Solutions makes the reports available to parties other than the Clients (i.e., “third parties”) – on its website in hopes that it can help the blockchain ecosystem develop technical best practices in this rapidly evolving area of innovation.

Last changed by 
 
Verilog Solutions
VERILOG LTD.
0
3
1961

Read more

Vesta Finance Audit

Copyright &#xA9; 2022 by Verilog Solutions. All rights reserved. January 30, 2022 by Verilog Solutions <!-- watermark --> Project Summary Vesta Finance is an Arbitrum-based lending protocol. Users can collateralize ETH and other supported assets to borrow $VST, which is Vesta Finance&apos;s stablecoin. Vesta will support other collateral after the launch. A portion of Vesta Finance&apos;s code is based on Liquity. Service Scope

May 28, 2022
YuzuSwap Audit

Copyright &#xA9; 2022 by Verilog Solutions. All rights reserved. January 4, 2022 by Verilog Solutions <!-- watermark --> This report presents our engineering engagement with YuzuSwap, one of the first DEX projects for the Emerald paratime on the Oasis Network. YuzuSwap is an AMM DEX with innovative trading incentive designs, such as the trading pool share token (TPST). YuzuSwap&apos;s TPST design keeps track of DEX users&apos; swap operations and provides rewards for these users. Table of Content [TOC]

May 28, 2022
Fountain Protocol Audit

Copyright &#xA9; 2022 by Verilog Solutions. All rights reserved. February 18, 2022 by Verilog Solutions <!-- watermark --> This report presents Verilog&apos;s smart contract auditing engagement with Fountain Protocol. Fountain Protocol is one of the first Lending protocols on the Emerald Paratime of Oasis Network. Table of Content [TOC]

May 28, 2022
TGT Finance Audit

Copyright &#xA9; 2022 by Verilog Solutions. All rights reserved. February 24, 2022 by Verilog Solutions <!-- watermark --> This report presents Verilog&apos;s smart contract auditing engagement with TGT Protocol. TGT Protocol is one of the first lending protocols and margin trading platforms on the Emerald Paratime of Oasis Network. Table of Content [TOC]

May 28, 2022
Read more from Verilog Solutions
Published on HackMD