BendDAO Blue-chip NFT Liquidity Audit Report

• Code Assessment
  • We evaluate the overall quality of the code and comments as well as the architecture of the repository.
  • We help the project dev team improve the overall quality of the repository by providing suggestions on refactorization to follow the best practice of Web3 software engineering.

• Code Logic Analysis
  • We dive into the data structures and algorithms in the repository and provide suggestions to improve the data structures and algorithms for the lower time and space complexities.
  • We analyze the hierarchy among multiple modules and the relations among the source code files in the repository and provide suggestions to improve the code architecture with better readability, reusability, and extensibility.

• Access Control Analysis
  • We perform a comprehensive assessment of the special roles of the project, including their authorities and privileges.
  • We provide suggestions regarding the best practice of privilege role management according to the standard operating procedures (SOP).

• Off-Chain Components Analysis
  • We analyze the off-chain modules that are interacting with the on-chain functionalities and provide suggestions according to the SOP.
  • We conduct a comprehensive investigation for potential risks and hacks that may happen on the off-chain components and provide suggestions for patches.

none ; )

1. Risk of reuse of signatures across forks due to lack of chain identifier (ID) validation.

   Severity	Medium
   Source	BendExchange.sol
   Status	Resolved in commit 6e5892eb3dcd9d9dfc100693267deb4cc6b82621

   • Description

     BendExchange.sol includes the EIP-712 standard into the smart contract to allow typed structured data hashing and signing. The domain separator is a crucial part of the EIP-712 standard, as it is designed to prevent the signature collision between different DApps. It usually includes a DApp’s unique information such as the DApp's name and the contract address, etc. In the event of a chain hard fork, the current smart contract design in BendExchange.sol does not include the chainID, as the result, the user’s signatures might be used for the replay attacks on different chains.

     Current smart contract hardcodes DOMAIN_SEPERATOR in the constructor:

     constructor(
         address _interceptorManager,  address _transferManager,
         address _currencyManager,
         address _executionManager,
         address _royaltyFeeManager,
         address _WETH,
         address _protocolFeeRecipient
       ) {
         // Calculate the domain separator
         DOMAIN_SEPARATOR = keccak256(
           abi.encode(
             0x8b73c3c69bb8fe3d512ecc4cf759cc79239f7b179b0ffacaa9a75d522b39400f, 
             // keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)")
             0xba0c660933e3f2279319fe2b72a6f829a2438d726bbe835523453fc0414c6020, 
             // keccak256("BendExchange")
             0xc89efdaa54c0f20c7adf612882df0950f5a951637e0307cdcb4c672f298b8bc6, 
             // keccak256(bytes("1")) for versionId = 1
             block.chainid,
             address(this)
           )
         );
         transferManager = ITransferManager(_transferManager);
         currencyManager = ICurrencyManager(_currencyManager);
         executionManager = IExecutionManager(_executionManager);
         royaltyFeeManager = IRoyaltyFeeManager(_royaltyFeeManager);
         interceptorManager = IInterceptorManager(_interceptorManager);
         WETH = _WETH;
       protocolFeeRecipient = _protocolFeeRecipient;
       }

     Due to the above design, DOMAIN_SEPERATOR is immutable after deployment. _validateOrders() function below is designed for checking the signature:

     // Verify the validity of the signature
       require(
         SignatureChecker.isValidSignatureNow(
           makerOrder.maker,
           ECDSA.toTypedDataHash(DOMAIN_SEPARATOR, makerOrderHash),
           abi.encodePacked(makerOrder.r, makerOrder.s, makerOrder.v)
         ),
         "Signature: invalid"
       );

     However, the above verification cannot detect the changing of the chainID after a hard fork.

   • Exploit Scenario

     • BendExchange.sol exists on Chain 1.

     • Later, a hard fork happens on Chain 1, and there are Chain 1 and Chain 12 now.

     • The block.chainid in DOMAIN_SEPARATOR is set to 1 when the contract is deployed. So, the DOMAIN_SEPARATOR will be the same on Chain 1 and Chain 12.

     • Alice signs a message on Chain 1. And it can be replayed on both Chain 1 and Chain 12
   • Recommendations

     Check and update chain id if it has been changed when constructing a new hash.

     Example: Openzeppelin EIP712 library

   • Results

     Resolved in commit 6e5892eb3dcd9d9dfc100693267deb4cc6b82621. When constructing a new hash, the chain id is checked and updated if it has been changed.

1. Unintended Side Effect

   Severity	Low
   Source	BendExchange.sol#L179;
   Status	Resolved in commit a3dc493e15b0014c181b88da56269cfb01c4743b.

   • Description

     The matchAskWithTakerBidUsingETHAndWETH() function converts all exceeding ETH (msg.value) to WETH (wrapped native token). This is an unintended side-effect to matchAskWithTakerBidUsingETHAndWETH() use case and may confuse users.

   • Exploit Scenario

     • An NFT is listed for 10 ETH. Alice has 16 ETH in her account.

     • Alice calls the matchAskWithTakerBidUsingETHAndWETH() and accidentally set msg.value to 16 ETH instead of 10 ETH.

     • After the successful execution of the order, Alice has 6 WETH instead of 6 ETH in her account besides the NFT.
   • Recommendations

     Add a check on msg.value. The msg.value should be less or equal to the bidding price.

   • Results

     Resolved in commit a3dc493e15b0014c181b88da56269cfb01c4743b. A require statement has been added to make sure msg.value is less or equal to the bidding price.

2. Inconsistency in business logic and actual implementation

   Severity	Low
   Source	BendExchange.sol;
   Status	Resolved in commit aae6c9744a4ff2d7bc3e68314d17e91e830b9f1e.

   • Description

     The currency manager cannot reject accepting WETH as the bidding currency. Even if WETH is banned in the currency manager, the attacker can bypass the restriction by setting the currency field as address(0) which stands for the native token like ETH in BendDAO Liquidity.

     function _validateOrders(
         OrderTypes.MakerOrder calldata makerOrder,
         bytes32 makerOrderHash,
         OrderTypes.TakerOrder calldata takerOrder
       ) internal view {
         // skip unrelated lines
         // Verify whether the currency is whitelisted, address(0) means native ETH
         if (makerOrder.currency != address(0)) {
           require(currencyManager.isCurrencyWhitelisted(makerOrder.currency), "Currency: not whitelisted");
         }
         // skip unrelated lines
       }

     When validating currency, it only checks if the currency is address(0) or not. When an attacker set the currency to address(0), the attacker executes order through function matchAskWithTakerBidUsingETHAndWETH(). In this function, it simply converts the user's ETH to WETH and executes the order, and converts WETH to ETH for the order maker in the final step. It still interacts with the WETH contract.

     The current code in BendExchange.sol and CurrencyManager.sol cannot prevent the protocol’s interaction with WETH contract. When vulnerabilities are found in the WETH contract, BendDAO Liquidity is not able to refuse to interact with WETH by removing WETH from the whitelisted currencies or pausing the protocol. It can only rely on users themselves to cancel their orders.

   • Exploit Scenario

     N/A

   • Recommendations

     One approach to prevent this is to update the checking on the currency. If the BendDAO Liquidity protocol wants to ban WETH, it has to ban ETH as well.

   • Results

     Resolved in commit aae6c9744a4ff2d7bc3e68314d17e91e830b9f1e. The currency manager checks ETH as well.

3. Inconsistency in business logic and actual implementation

   Severity	Low
   Source	BendExchange.sol#L605;
   Status	Resolved in commit 1bfc92af416238541c6f22786ac77abfe0b22290.

   • Description

     The order interceptor validation logic is misplaced, and thus overly restricts the user’s input. The check makerOrder.isOrderAsk is placed in require(). Therefore, if the non-asker side enters interceptor by mistake, the transaction will be reverted.

     if (makerOrder.interceptor != address(0)) {
         require(
           makerOrder.isOrderAsk && interceptorManager.isInterceptorWhitelisted(makerOrder.interceptor),
           "Interceptor: maker interceptor not whitelisted"
         );
       }
       if (takerOrder.interceptor != address(0)) {
         require(
           takerOrder.isOrderAsk && interceptorManager.isInterceptorWhitelisted(takerOrder.interceptor),
           "Interceptor: taker interceptor not whitelisted"
         );
       }

     Interceptor shall only be checked on the ask order side. When the user makes a bid order with an interceptor, due to the current strict requirements, the transaction will revert when the order is executed on-chain.

   • Exploit Scenario

     N/A

   • Recommendations

     We recommend only validating interceptors for the ask order.

     Move the isOrderAsk check to the if statement. Check if the order is “ask” then validate the interceptor.

     if (makerOrder.isOrderAsk && makerOrder.interceptor != address(0)) {
         require(
           interceptorManager.isInterceptorWhitelisted(makerOrder.interceptor),
           "Interceptor: maker interceptor not whitelisted"
         );
       }
       if (takerOrder.isOrderAsk && takerOrder.interceptor != address(0)) {
         require(
           interceptorManager.isInterceptorWhitelisted(takerOrder.interceptor),
           "Interceptor: taker interceptor not whitelisted"
         );
       }

   • Results

     Resolved in commit 1bfc92af416238541c6f22786ac77abfe0b22290.

4. Orders can only be safely invalidated via on-chain operations

   Severity	Low
   Source	BendExchange.sol;
   Status	Acknowledged.

   • Description

     The nonce of an order is set off-chain and it can be any number. Orders can only be safely canceled or replaced via on-chain canceling.

     When a user signs an order off-chain and then signs new order with the same nonce to replace or cancel the previous order, it might fail because there is no guarantee that the new order must override the previous order. It all depends on which order the taker fills first.

     To safely invalidate an order, the on-chain operation is a must. Users can either invalidate an order by calling the function cancelAllOrdersForSender() or cancelMultipleMakerOrders() or just execute the order themselves.

   • Exploit Scenario

     N/A

   • Recommendations

     inform users only on-chain executions can safely cancel or replace an order.

   • Results

     Acknowledged.

     Reply from BendDAO team: Users can adjust their orders and must cancel them before re-listing on the front end

5. Inconsistency in business logic and actual implementation

   Severity	Low
   Source	interceptors/RedeemNFT.sol#L49;
   Status	Resolved in commit aae6c9744a4ff2d7bc3e68314d17e91e830b9f1e.

   • Description

     The repaid token balance of the user proxy should be greater or equal to the sum of total debt and bid fine. The strict requirement for balance to be greater than the sum of total debt and bid fine could result in an unnecessary revert.

     According to the code and business logic, the user who bids for an NFT on auction in Bend lend pool just needs to have enough tokens to pay for debt and bid fine. However, the code strictly requires the token balance to be greater than the debt and fine. Users with the repaid token balance equal to debt and fine will fail on bidding the NFT.

     require(
         vars.totalDebt + vars.bidFine < IERC20(vars.tokenRepaid).balanceOf(address(this)),
         "Interceptor: insufficent to repay debt"
       );

   • Exploit Scenario

     N/A

   • Recommendations

     Change < to <=

     require(
         vars.totalDebt + vars.bidFine <= IERC20(vars.tokenRepaid).balanceOf(address(this)),
         "Interceptor: insufficent to repay debt"
       );

   • Results

     Resolved in commit aae6c9744a4ff2d7bc3e68314d17e91e830b9f1e.

1. Missing comments

   Severity	Informational
   Source	BendExchange.sol#L118;
   Status	Resolved in commit aae6c9744a4ff2d7bc3e68314d17e91e830b9f1e.

   • Description

     Some comments for function parameters are missing. The constructor of BendExchange.sol has the following comments:

     /**
        * @notice Constructor
        * @param _currencyManager currency manager address
        * @param _executionManager execution manager address
        * @param _royaltyFeeManager royalty fee manager address
        * @param _WETH wrapped ether address (for other chains, use wrapped native asset)
        * @param _protocolFeeRecipient protocol fee recipient
       */

     While the argument list of the constructor is as follows:

     address _interceptorManager,
       address _transferManager,
       address _currencyManager,
       address _executionManager,
       address _royaltyFeeManager,
       address _WETH,
       address _protocolFeeRecipient

     Comments for _interceptorManager and _transferManager are missing.

   • Exploit Scenario

     N/A

   • Recommendations

     Add comments for parameters _interceptorManager and _transferManager.

   • Results

     Resolved in commit aae6c9744a4ff2d7bc3e68314d17e91e830b9f1e.

2. Inconsistent comments

   Severity	Informational
   Source	BendExchange.sol#L248;
   Status	Resolved in commit aae6c9744a4ff2d7bc3e68314d17e91e830b9f1e.

   • Description

     There is an error in the comments.

     @notice Match a takerBid with a matchAsk

     should be replaced by

     @notice Match a takerBid with a makerAsk

   • Exploit Scenario

     N/A

   • Recommendations

     Change matchAsk to makerAsk.

   • Results

     Resolved in commit aae6c9744a4ff2d7bc3e68314d17e91e830b9f1e.

     Resolved in commit aae6c9744a4ff2d7bc3e68314d17e91e830b9f1e.

• The owner of BendExchange.sol is able to first set and then update the contract addresses of all the manager contracts.

• The owner of the AuthorizationManager.sol is able to set the revoked variable to true. If this variable is set to true, only owners of registered proxies will be able to use most of its functionalities.

• The owner of the contracts RoyalteeFeeSetter.sol and RoyaltyFeeRegistry.sol can update the NFT collection’s royalty information regardless of changes made by NFT collection owners via the RoyalteeFeeSetter.sol.

• The owner of the CurrencyManager.sol contract can add and remove currencies for trading on BendDAO Liquidity.

• The owner of the ExecutionManager.sol contract is able to add and remove execution strategies for trading on the BendDAO Liquidity.

• The owner of the TransferManager.sol contract can add specific transfer rules for collection addresses that could be ERC721, ERC1155, and non-compliant ERC721 tokens.

• The owner of the InterceptorManager.sol contract is able to add and remove the interceptor for trading NFT locked in the bend lend pool.