Review of Borrowing Feature Integration

[langnavina97]

Concerns:

1. Potential Out of Gas Error

• We currently iterate over each comptroller (which is a cluster by Venus where tokens can be borrowed, e.g. General, DeFi, etc.).
• For each comptroller, we then iterate over each borrowed token, if any.
• This double iteration could potentially lead to an Out of Gas error, especially if there are many comptrollers and borrowed tokens in each cluster.

2. Risk of User Loss Due to Manipulated Token List

• If an asset manager repays a loan without rebalancing or updating the token list, and doesn't rebalance the vault afterwards, or if a user withdraws in between, the user could potentially incur a loss.

Further concerns we should investigate (not sure if they're issues):

1. Risky Token Pulling During Flashloans: Danger of pulling tokens from the vault during flashloan interactions, potentially exposing the vault to exploitation.
2. Ineffective Dust Handling: Sending dust to the vault has no effect if tokens aren't in the token list; dust should be returned to users based on their share.
3. Calldata Manipulation: Risk of unauthorized access to vault funds if calldata in flashloan callbacks can be manipulated. ==> I think it is safe. The user balance is updated before the execution any calldata. The balances to pull from the vault are being calculated on-chain to make sure the user can only repay the amount for his own shares

[langnavina97]

Missing input verification:

• function executeVaultFlashLoan:

require(_receiver != address(0), "Invalid receiver address");
    require(repayData._factory != address(0) && repayData._token0 != address(0) && repayData._token1 != address(0), "Invalid address");
    require(repayData._flashLoanToken != address(0), "Invalid flash loan token");
    require(repayData._flashLoanAmount.length > 0 && repayData._debtRepayAmount.length > 0, "Empty amount array");
    require(repayData._flashLoanAmount.length == repayData._debtRepayAmount.length, "Mismatched amount arrays");
    require(repayData._debtToken.length == repayData._protocolToken.length, "Mismatched token arrays");
    require(repayData.firstSwapData.length == repayData.secondSwapData.length, "Mismatched swap data arrays");

• function loanProcessing:

require(vault != address(0) && executor != address(0) && controller != address(0) && receiver != address(0), "Invalid address");
    require(lendTokens.length > 0, "No lend tokens provided");
    require(totalCollateral > 0, "Invalid total collateral");
    require(fee <= MAX_FEE, "Fee exceeds maximum allowed"); // Define MAX_FEE constant

    require(flashData.flashLoanToken != address(0), "Invalid flash loan token");
    require(flashData.debtToken.length == flashData.protocolTokens.length, "Mismatched token arrays");
    require(flashData.flashLoanAmount.length == flashData.debtRepayAmount.length, "Mismatched amount arrays");
    require(flashData.firstSwapData.length == flashData.secondSwapData.length, "Mismatched swap data arrays");

• getCollateralAmountToSell:

require(_user != address(0) && _controller != address(0) && _protocolToken != address(0), "Invalid address");
    require(lendTokens.length > 0, "No lend tokens provided");
    require(_debtRepayAmount > 0, "Invalid debt repay amount");
    require(feeUnit > 0 && feeUnit <= MAX_FEE_UNIT, "Invalid fee unit"); // Define MAX_FEE_UNIT
    require(totalCollateral > 0, "Invalid total collateral");

• getUserAccountData
• getAllProtocolAssets

==> Suggestions from Cursor, might be suggesting duplicate checks, to be checked!!!

[langnavina97]

Naming conventions:

Functions:

• loanProcessing: Please rename the function and add proper comments, the function does not execute any function, it only builds transactions to be executed later
• repayBorrow: this function repays the borrowed token partially according to the withdrawal amounts, is used when a user withdraws funds
• repayVault: this function is used to completely repay the borrowed token, loan is completely redeemed
• withdrawTransactions: should start with _ (internal)

Other:

• Function parameters should start with underscore (not recommended in the solidity style guide but we already did it everywhere in our code)
• Make sure internal functions start with an underscore _
• Improve wording and comments in general to make it more readable (there are borrowed tokens, lending tokens, collateral - please try to make it more readable)
• uint256 amountOwed = totalFlashAmount + fee0; // Calculate the amount owed including the fee ==> improve comment, amount owed to? To owner, to be repaid? Please make sure the comments are clear
• //Need Dust Transfer ==> this comment should also be improved, need dust transfer is not a proper comment, explain why it has to be returned

[langnavina97]

• [ ] The exemptionIndex param is not being used in the added part, please move it down to where it's used to make it more readable

  uint256 exemptionIndex = 0;
  
  _borrowManager.repayBorrow(
    _portfolioTokenAmount,
    totalSupplyPortfolio,
    repayData
  );

• [ ] Inside the repayBorrow function, can we change the error "CallFailed" since we're already using it for Enso.. RepayFailed
• [ ] Variables are defined twice: underlying and tokenBalance

address[] memory underlying = new address[](borrowedLength); // Array to store underlying tokens of borrowed assets
        uint256[] memory tokenBalance = new uint256[](borrowedLength); // Array to store balances of borrowed tokens
        uint256 totalFlashAmount; // Variable to track total flash loan amount
        underlying = new address[](borrowedLength);
        tokenBalance = new uint256[](borrowedLength);

• [ ] address token = borrowedTokens[i]; in executeUserFlashLoan: call the variable borrowedToken
• [ ] underlying is collateral token? - rename
• [ ] Either pass it through in FlashloanData (already called before to find controller) or define it one time and reuse (function: algebraFlashCallback)

  address controller = _protocolConfig.marketControllers(
          flashData.protocolTokens[0]
      );