AbraNFT contest - pauliax's results

• Consider extracting these to an enum:

    uint8 private constant LOAN_INITIAL = 0;
    uint8 private constant LOAN_REQUESTED = 1;
    uint8 private constant LOAN_OUTSTANDING = 2;

• YEAR_BPS could be expressed with built-in time kewords:

    uint256 private constant YEAR_BPS = 3600 * 24 * 365 * 10_000;
    uint256 private constant YEAR_BPS = 365 days;

• I think all the functions that validate signatures, should also include DOMAIN_SEPARATOR(). Now this function is basically unused, and in the theoretical case if contract addresses are identical on different chains, the signature replay attack can be executed.

• Signature validations should check that ecrecover does not return an empty address (0x0) which is the case when the signature is invalid.

• Unprotected function init can be frontrunned by anyone. Even though the risk is small, consider applying onlyOwner to it.

• Function cook could validate that all the lengths of actions, values, and datas are the equal.

• Consider adding a re-entrancy protection or follow the Check-Effects-Interaction pattern to avoid unexpeced re-entrancy attacks even if you trust the callee, e.g. the order of lines should be swapped here:

  bentoBox.transfer(asset, address(this), to, _share);
  feesEarnedShare = 0;

also everywhere where NFT collateral or other tokens are transferred it should happend as the final action, e.g. function _requestLoan.

• Not sure why withdrawFees queries the receiver from the masterContract (itself):

  function withdrawFees() public {
      address to = masterContract.feeTo();

There is a variable named feeTo which is supposed to receive fees, so it should probably be cheapier to directly read it.

• Repeated calculations should be cached, e.g. totalShare - openFeeShare in function _lend could be extracted to a variable:

  bentoBox.balanceOf(asset, address(this)) >= (totalShare - openFeeShare + protocolFeeShare + feesEarnedShare)
        
  bentoBox.transfer(asset, lender, address(this), totalShare - openFeeShare + protocolFeeShare);

  uint256 borrowerShare = totalShare - openFeeShare;

• Using uint8 does not give any efficiency, actually, it is the opposite as EVM operates on default of 256-bit values so uint8 is more expensive in this case as it needs a conversion. It only gives improvements in cases where you can pack variables together, e.g. structs.

  uint8[] calldata actions,

  uint8 internal constant ACTION_REPAY = 2;
  uint8 internal constant ACTION_REMOVE_COLLATERAL = 4;

  uint8 internal constant ACTION_REQUEST_LOAN = 12;
  uint8 internal constant ACTION_LEND = 13;

  // Function on BentoBox
  uint8 internal constant ACTION_BENTO_DEPOSIT = 20;
  uint8 internal constant ACTION_BENTO_WITHDRAW = 21;
  uint8 internal constant ACTION_BENTO_TRANSFER = 22;
  uint8 internal constant ACTION_BENTO_TRANSFER_MULTIPLE = 23;
  uint8 internal constant ACTION_BENTO_SETAPPROVAL = 24;

  // Any external call (except to BentoBox)
  uint8 internal constant ACTION_CALL = 30;

  // Signed requests
  uint8 internal constant ACTION_REQUEST_AND_BORROW = 40;
  uint8 internal constant ACTION_TAKE_COLLATERAL_AND_LEND = 41;

• I don't think this library is used in any meaningful way:

  using RebaseLibrary for Rebase;