###### tags: `Final Report` BendDAO Audit === > Copyright © 2022 by Verilog Solutions. All rights reserved. > May 24, 2022 > by **Verilog Solutions** <!-- <span style="position:fixed; top:200px; right:400px; opacity:0.5; font-size: 20px; z-index:99;">watermark</span> --> <!-- inser new picture here --> ![BendDAO-Audit-Cover](https://hackmd.io/_uploads/HJeDdzi85.png) 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 [TOC] --- ## 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](https://github.com/BendDAO/bend-protocol), specifically, commit hash [**087c284fda18e9d7fd3a07de384a2245d5eab6ea**](https://github.com/BendDAO/bend-protocol/tree/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](#Findings-amp-Improvement-Suggestions). --- ## BendDAO Architecture & Protocol Management <br /> <figure> <center> <img src="https://hackmd.io/_uploads/BJ1vLcZP5.png" width="600" alt="BendDAO Architecture"> <center><em><br />BendDAO Architecture</em></center></center> </figure> <br /> **BendDAO protocol contains three major parts:** - [Lending Protocol](#Lending-Protocol) - [NFT Oracle](#NFT-Oracle) - [Bound NFT](#BoundNFT-Protocol) ### 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. <br /> <figure> <center> <img src="https://hackmd.io/_uploads/rkj3NKnv9.png" width="600" alt="End-to-end encryption is enabled"> <center><em><br />End-to-end encryption is enabled </em></center></center> </figure> <br /> 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. <br /> <figure> <center> <img src="https://hackmd.io/_uploads/rkqxJXsU9.png" width="600" alt="6 hour twap"> <center><em><br />6h TWAP floor price comparing with the raw floor price </em></center></center> </figure> <br /> - 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](https://docs.benddao.xyz/portal/risk/security-and-audits). --- ## Findings & Improvement Suggestions #### <html></html> <style> .info { background-color:mediumseagreen; font-size: 12px; color: white; border-radius:4px; padding: 1px 4px; font-weight: 500; display: inline-block; margin: 2px; letter-spacing: 0.3px} </style><style> .minor { background-color: #698999; font-size: 12px; color: white; border-radius:4px; padding: 1px 4px; font-weight: 500; display: inline-block; margin: 2px; letter-spacing: 0.3px} </style><style> .medium { background-color: #FFCA0F; color: #121212; font-size: 12px; border-radius:4px; padding: 1px 4px; font-weight: 500; display: inline-block; margin: 2px; letter-spacing: 0.3px} </style><style> .major{ background-color: #FF6B4A; color: white; font-size: 12px; border-radius:4px; padding: 1px 4px; font-weight: 500; display: inline-block; margin: 2px; letter-spacing: 0.3px} </style><style> .critical{ background-color: #FF0000; color: white; font-size: 12px; border-radius:4px; padding: 1px 4px; font-weight: 500; display: inline-block; margin: 2px; letter-spacing: 0.3px} </style> <span class='info'>Informational</span><span class='minor'>Minor</span><span class='medium'>Medium</span><span class='major'>Major</span><span class='critical'>Critical</span> | | 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. <span class='medium'>Medium</span> **[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](https://github.com/BendDAO/bend-protocol/commit/64b7861e48cf0156d5f203e3a39166f22ae36dd0). ### Minor 1. `LendPoolLiquidator.sol`: unnecessary `require` in `redeem()` and redundant parameter `bidFine` <span class='minor'>minor</span> **[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). <span class='minor'>minor</span> **[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). <span class='minor'>minor</span> **[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.<span class='minor'>Minor</span> **[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](https://github.com/BoundNFT/boundnft-protocol/tree/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`) <span class='info'>Informational</span> **[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. <span class='info'>Informational</span> **[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` <span class='info'>Informational</span> **[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:** ```solidity= 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`: function`setAssetData()` variable incrementation <span class='info'>Informational</span> **[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:** ```solidity= 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 <span class='info'>Informational</span> **[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] <span class='info'>Informational</span> **[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. <span class='info'>Informational</span> **[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.