QA Report - Githubissues

Low Risk Issues

1. Should ensure loan collateral is not immediately seizable

For the Oracle version there are checks to make sure that the current valuation is above the amount loaned. There should be a similar check that the loan duration is not zero. Zero is not useful for flash loans because of the origination fees.

File: contracts/NFTPairWithOracle.sol   #1

224          tokenLoanParams[tokenId] = params;

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L224

File: contracts/NFTPairWithOracle.sol   #2

244          tokenLoanParams[tokenId] = params;

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L244

2. Pairs do not implement `ERC721TokenReceiver`

According to the README.md, NFTPairs specifically involve ERC721 tokens. Therefore the contract should implement ERC721TokenReceiver, or customer transfers involving safeTransferFrom() calls will revert

File: contracts/NFTPair.sol   #1

59  contract NFTPair is BoringOwnable, Domain, IMasterContract {

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L59

File: contracts/NFTPairWithOracle.sol   #2

69  contract NFTPairWithOracle is BoringOwnable, Domain, IMasterContract {

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L69

3. Incorrect comments

Skimming involves excess balances in the bentobox, not in the contract itself. This comment will lead to clients incorrectly passing tokens to the pair, rather than the bentobox. In addition, overall, there should be more comments devited to the interactions with the bentobox

File: contracts/NFTPair.sol   #1

320      /// @param skim True if the funds have been transfered to the contract

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L320

File: contracts/NFTPairWithOracle.sol   #2

355      /// @param skim True if the funds have been transfered to the contract

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L355

4. Comments should be enforced by `require()`s

The comments below should be enforced by require(block.timestamp < uint64(-1))

File: contracts/NFTPair.sol   #1

311          loan.startTime = uint64(block.timestamp); // Do not use in 12e10 years..

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L311

File: contracts/NFTPairWithOracle.sol   #2

346          loan.startTime = uint64(block.timestamp); // Do not use in 12e10 years..

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L346

5. Calls incorrectly allow non-zero `msg.value`

The comments below say that msg.value is "only applicable to" a subset of actions. All other actions should have a require(!msg.value). Allowing it anyway is incorrect state handling

File: contracts/NFTPair.sol   #1

631      /// @param values A one-to-one mapped array to `actions`. ETH amounts to send along with the actions.
632      /// Only applicable to `ACTION_CALL`, `ACTION_BENTO_DEPOSIT`.

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L631-L632

File: contracts/NFTPairWithOracle.sol   #2

664      /// @param values A one-to-one mapped array to `actions`. ETH amounts to send along with the actions.
665      /// Only applicable to `ACTION_CALL`, `ACTION_BENTO_DEPOSIT`.

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L664-L665

6. Missing checks for `address(0x0)` when assigning values to `address` state variables

File: contracts/NFTPair.sol   #1

729           feeTo = newFeeTo;

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L729

File: contracts/NFTPairWithOracle.sol   #2

751           feeTo = newFeeTo;

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L751

7. `ecrecover` not checked for zero result

A return value of zero indicates an invalid signature, so this is both invalid state-handling and an incorrect message

File: contracts/NFTPair.sol   #1

383              require(ecrecover(_getDigest(dataHash), v, r, s) == lender, "NFTPair: signature invalid");

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L383

File: contracts/NFTPair.sol   #2

419          require(ecrecover(_getDigest(dataHash), v, r, s) == borrower, "NFTPair: signature invalid");

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L419

File: contracts/NFTPairWithOracle.sol   #3

417              require(ecrecover(_getDigest(dataHash), signature.v, signature.r, signature.s) == lender, "NFTPair: signature invalid");

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L417

File: contracts/NFTPairWithOracle.sol   #4

452          require(ecrecover(_getDigest(dataHash), signature.v, signature.r, signature.s) == borrower, "NFTPair: signature invalid");

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L452

Non-critical Issues

1. Consider supporting CryptoPunks and EtherRocks

The project README.md says that NFTPairs are specifically ERC721 tokens, but not all NFTs are ERC721s. CryptoPunks and EtherRocks came before the standard and do not conform to it.

File: README.md   #1

58  - NFT Pair are a version of Cauldrons where the collateral isn't an ERC20 token but an ERC721 token, the deal OTC, the parameters of the loan themselves pre-defined.

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/README.md?plain=1#L58

2. Contracts should be refactored to extend a base contract with the common functionality

$ fgrep -xf NFTPair.sol NFTPairWithOracle.sol | wc -l
686
$ wc -l NFTPair.sol NFTPairWithOracle.sol
732 NFTPair.sol
754 NFTPairWithOracle.sol

686 / 732 = 93.7%
686 / 754 = 91.0%

About 92% of the lines in each file are exactly the same as the lines in the other file. At the very least the shared constants, the common state variables, and the pure functions should be moved to a common base contract.

File: contracts/NFTPair.sol (various lines)   #1

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol

File: contracts/NFTPairWithOracle.sol (various lines)   #2

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol

3. Some compilers can't handle two different contracts with the same name

Some compilers only compile the first one they encounter, ignoring the second one. If two contracts are different (e.g. different struct argument definitions) then they should have different names

File: contracts/NFTPair.sol   #1

37  interface ILendingClub {
38      // Per token settings.
39      function willLend(uint256 tokenId, TokenLoanParams memory params) external view returns (bool);
40  
41      function lendingConditions(address nftPair, uint256 tokenId) external view returns (TokenLoanParams memory);
42  }

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L37-L42

File: contracts/NFTPairWithOracle.sol   #2

47  interface ILendingClub {
48      // Per token settings.
49      function willLend(uint256 tokenId, TokenLoanParams memory params) external view returns (bool);
50  
51      function lendingConditions(address nftPair, uint256 tokenId) external view returns (TokenLoanParams memory);
52  }

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L47-L52

4. Calls to `BoringMath.to128()` are redundant

All calls to to128() occur on the result of calculateInterest(), which itself already checks that the value fits into a uint128

File: contracts/NFTPairWithOracle.sol   #1

285                  ).to128();

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L285

File: contracts/NFTPairWithOracle.sol   #2

552          uint256 interest = calculateInterest(principal, uint64(block.timestamp - loan.startTime), loanParams.annualInterestBPS).to128();

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L552

File: contracts/NFTPair.sol   #3

519          uint256 interest = calculateInterest(principal, uint64(block.timestamp - loan.startTime), loanParams.annualInterestBPS).to128();

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L519

5. `require()`/`revert()` statements should have descriptive reason strings

File: contracts/NFTPair.sol   #1

501               revert();

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L501

File: contracts/NFTPairWithOracle.sol   #2

534               revert();

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L534

6. `public` functions not called by the contract should be declared `external` instead

Contracts are allowed to override their parents' functions and change the visibility from external to public.

File: contracts/NFTPair.sol   #1

181       function updateLoanParams(uint256 tokenId, TokenLoanParams memory params) public {

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L181

File: contracts/NFTPair.sol   #2

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

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L713-L714

File: contracts/NFTPair.sol   #3

728       function setFeeTo(address newFeeTo) public onlyOwner {

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L728

File: contracts/NFTPairWithOracle.sol   #4

198       function updateLoanParams(uint256 tokenId, TokenLoanParams memory params) public {

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L198

File: contracts/NFTPairWithOracle.sol   #5

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

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L735-L736

File: contracts/NFTPairWithOracle.sol   #6

750       function setFeeTo(address newFeeTo) public onlyOwner {

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L750

7. Interfaces should be moved to separate files

Most of the other interfaces in this project are in their own file in the interfaces directory. The interfaces below do not follow this pattern

File: contracts/NFTPairWithOracle.sol   #1

47  interface ILendingClub {

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L47

File: contracts/NFTPairWithOracle.sol   #2

54  interface INFTPair {

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L54

File: contracts/NFTPair.sol   #3

37  interface ILendingClub {

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L37

File: contracts/NFTPair.sol   #4

44  interface INFTPair {

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L44

8. `2**<n> - 1` should be re-written as `type(uint<n>).max`

Earlier versions of solidity can use uint<n>(-1) instead. Expressions not including the - 1 can often be re-written to accomodate the change (e.g. by using a > rather than a >=, which will also save some gas)

File: contracts/NFTPair.sol   #1

500           if (interest >= 2**128) {

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L500

File: contracts/NFTPairWithOracle.sol   #2

533           if (interest >= 2**128) {

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L533

9. `constant`s should be defined rather than using magic numbers

File: contracts/NFTPair.sol   #1

500           if (interest >= 2**128) {

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L500

File: contracts/NFTPairWithOracle.sol   #2

533           if (interest >= 2**128) {

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L533

10. Numeric values having to do with time should use time units for readability

There are units for seconds, minutes, hours, days, and weeks

File: contracts/NFTPair.sol   #1

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

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L111

File: contracts/NFTPairWithOracle.sol   #2

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

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L128

11. Use a more recent version of solidity

Use a solidity version of at least 0.8.4 to get bytes.concat() instead of abi.encodePacked(<bytes>,<bytes>) Use a solidity version of at least 0.8.12 to get string.concat() instead of abi.encodePacked(<str>,<str>)

File: contracts/NFTPair.sol   #1

20   pragma solidity 0.6.12;

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L20

File: contracts/NFTPairWithOracle.sol   #2

20   pragma solidity 0.6.12;

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L20

12. Constant redefined elsewhere

Consider defining in only one contract so that values cannot become out of sync when only one location is updated. A cheap way to store constants in a single location is to create an internal constant in a library. If the variable is a local cache of another contract's value, consider making the cache variable internal or private, which will require external users to query the contract with the source of truth, so that callers don't get out of sync.

File: contracts/NFTPairWithOracle.sol   #1

93       IBentoBoxV1 public immutable bentoBox;

seen in contracts/NFTPair.sol https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L93

File: contracts/NFTPairWithOracle.sol   #2

94       NFTPairWithOracle public immutable masterContract;

seen in contracts/NFTPair.sol https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L94

13. Fee mechanics should be better described

When reading through the code the first time, it wasn't clear exactly what openFeeShare was for and why it's being subtracted from totalShare. Add to this the fact that the protocolFee is based on the openFeeShare and it seems like this area needs more comments, specifically that openFeeShare is the fee paid to the lender by the borrower during loan initiation, for the privilege of being given a loan.

File: contracts/NFTPair.sol   #1

295          if (skim) {
296              require(
297                  bentoBox.balanceOf(asset, address(this)) >= (totalShare - openFeeShare + protocolFeeShare + feesEarnedShare),
298                  "NFTPair: skim too much"
299              );
300          } else {
301              bentoBox.transfer(asset, lender, address(this), totalShare - openFeeShare + protocolFeeShare);
302          }

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L295-L302

File: contracts/NFTPairWithOracle.sol   #2

330          if (skim) {
331              require(
332                  bentoBox.balanceOf(asset, address(this)) >= (totalShare - openFeeShare + protocolFeeShare + feesEarnedShare),
333                  "NFTPair: skim too much"
334              );
335          } else {
336              bentoBox.transfer(asset, lender, address(this), totalShare - openFeeShare + protocolFeeShare);
337          }

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L330-L337

14. Typos

File: contracts/NFTPair.sol   #1

90       // Track assets we own. Used to allow skimming the excesss.

excesss https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L90

File: contracts/NFTPair.sol   #2

114       // `calculateIntest`.

calculateIntest https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L114

File: contracts/NFTPair.sol   #3

233       /// @param skim True if the token has already been transfered

transfered https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L233

File: contracts/NFTPair.sol   #4

320       /// @param skim True if the funds have been transfered to the contract

transfered https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L320

File: contracts/NFTPair.sol   #5

351       /// @param skimCollateral True if the collateral has already been transfered

transfered https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L351

File: contracts/NFTPair.sol   #6

389       /// @notice Take collateral from a pre-commited borrower and lend against it

commited https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L389

File: contracts/NFTPair.sol   #7

394       /// @param skimFunds True if the funds have been transfered to the contract

transfered https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L394

File: contracts/NFTPair.sol   #8

434       /// of the above inquality) fits in 128 bits, then the function is

inquality https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L434

File: contracts/NFTPair.sol   #9

446           // (NOTE: n is hardcoded as COMPOUND_INTEREST_TERMS)

hardcoded https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L446

File: contracts/NFTPairWithOracle.sol   #10

107       // Track assets we own. Used to allow skimming the excesss.

excesss https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L107

File: contracts/NFTPairWithOracle.sol   #11

131       // `calculateIntest`.

calculateIntest https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L131

File: contracts/NFTPairWithOracle.sol   #12

253       /// @param skim True if the token has already been transfered

transfered https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L253

File: contracts/NFTPairWithOracle.sol   #13

355       /// @param skim True if the funds have been transfered to the contract

transfered https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L355

File: contracts/NFTPairWithOracle.sol   #14

386       /// @param skimCollateral True if the collateral has already been transfered

transfered https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L386

File: contracts/NFTPairWithOracle.sol   #15

423       /// @notice Take collateral from a pre-commited borrower and lend against it

commited https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L423

File: contracts/NFTPairWithOracle.sol   #16

428       /// @param skimFunds True if the funds have been transfered to the contract

transfered https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L428

File: contracts/NFTPairWithOracle.sol   #17

467       /// of the above inquality) fits in 128 bits, then the function is

inquality https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L467

File: contracts/NFTPairWithOracle.sol   #18

479           // (NOTE: n is hardcoded as COMPOUND_INTEREST_TERMS)

hardcoded https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L479

15. NatSpec is incomplete

File: contracts/NFTPair.sol   #1

346       /// @notice Caller provides collateral; loan can go to a different address.
347       /// @param tokenId ID of the token that will function as collateral
348       /// @param lender Lender, whose BentoBox balance the funds will come from
349       /// @param recipient Address to receive the loan.
350       /// @param params Loan parameters requested, and signed by the lender
351       /// @param skimCollateral True if the collateral has already been transfered
352       /// @param anyTokenId Set if lender agreed to any token. Must have tokenId 0 in signature.
353       function requestAndBorrow(
354           uint256 tokenId,
355           address lender,
356           address recipient,
357           TokenLoanParams memory params,
358           bool skimCollateral,
359           bool anyTokenId,
360           uint256 deadline,
361           uint8 v,
362           bytes32 r,
363           bytes32 s

Missing: @param deadline https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L346-L363

File: contracts/NFTPair.sol   #2

346       /// @notice Caller provides collateral; loan can go to a different address.
347       /// @param tokenId ID of the token that will function as collateral
348       /// @param lender Lender, whose BentoBox balance the funds will come from
349       /// @param recipient Address to receive the loan.
350       /// @param params Loan parameters requested, and signed by the lender
351       /// @param skimCollateral True if the collateral has already been transfered
352       /// @param anyTokenId Set if lender agreed to any token. Must have tokenId 0 in signature.
353       function requestAndBorrow(
354           uint256 tokenId,
355           address lender,
356           address recipient,
357           TokenLoanParams memory params,
358           bool skimCollateral,
359           bool anyTokenId,
360           uint256 deadline,
361           uint8 v,
362           bytes32 r,
363           bytes32 s

Missing: @param v https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L346-L363

File: contracts/NFTPair.sol   #3

346       /// @notice Caller provides collateral; loan can go to a different address.
347       /// @param tokenId ID of the token that will function as collateral
348       /// @param lender Lender, whose BentoBox balance the funds will come from
349       /// @param recipient Address to receive the loan.
350       /// @param params Loan parameters requested, and signed by the lender
351       /// @param skimCollateral True if the collateral has already been transfered
352       /// @param anyTokenId Set if lender agreed to any token. Must have tokenId 0 in signature.
353       function requestAndBorrow(
354           uint256 tokenId,
355           address lender,
356           address recipient,
357           TokenLoanParams memory params,
358           bool skimCollateral,
359           bool anyTokenId,
360           uint256 deadline,
361           uint8 v,
362           bytes32 r,
363           bytes32 s

Missing: @param r https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L346-L363

File: contracts/NFTPair.sol   #4

346       /// @notice Caller provides collateral; loan can go to a different address.
347       /// @param tokenId ID of the token that will function as collateral
348       /// @param lender Lender, whose BentoBox balance the funds will come from
349       /// @param recipient Address to receive the loan.
350       /// @param params Loan parameters requested, and signed by the lender
351       /// @param skimCollateral True if the collateral has already been transfered
352       /// @param anyTokenId Set if lender agreed to any token. Must have tokenId 0 in signature.
353       function requestAndBorrow(
354           uint256 tokenId,
355           address lender,
356           address recipient,
357           TokenLoanParams memory params,
358           bool skimCollateral,
359           bool anyTokenId,
360           uint256 deadline,
361           uint8 v,
362           bytes32 r,
363           bytes32 s

Missing: @param s https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L346-L363

File: contracts/NFTPair.sol   #5

389       /// @notice Take collateral from a pre-commited borrower and lend against it
390       /// @notice Collateral must come from the borrower, not a third party.
391       /// @param tokenId ID of the token that will function as collateral
392       /// @param borrower Address that provides collateral and receives the loan
393       /// @param params Loan terms offered, and signed by the borrower
394       /// @param skimFunds True if the funds have been transfered to the contract
395       function takeCollateralAndLend(
396           uint256 tokenId,
397           address borrower,
398           TokenLoanParams memory params,
399           bool skimFunds,
400           uint256 deadline,
401           uint8 v,
402           bytes32 r,
403           bytes32 s

Missing: @param deadline https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L389-L403

File: contracts/NFTPair.sol   #6

389       /// @notice Take collateral from a pre-commited borrower and lend against it
390       /// @notice Collateral must come from the borrower, not a third party.
391       /// @param tokenId ID of the token that will function as collateral
392       /// @param borrower Address that provides collateral and receives the loan
393       /// @param params Loan terms offered, and signed by the borrower
394       /// @param skimFunds True if the funds have been transfered to the contract
395       function takeCollateralAndLend(
396           uint256 tokenId,
397           address borrower,
398           TokenLoanParams memory params,
399           bool skimFunds,
400           uint256 deadline,
401           uint8 v,
402           bytes32 r,
403           bytes32 s

Missing: @param v https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L389-L403

File: contracts/NFTPair.sol   #7

389       /// @notice Take collateral from a pre-commited borrower and lend against it
390       /// @notice Collateral must come from the borrower, not a third party.
391       /// @param tokenId ID of the token that will function as collateral
392       /// @param borrower Address that provides collateral and receives the loan
393       /// @param params Loan terms offered, and signed by the borrower
394       /// @param skimFunds True if the funds have been transfered to the contract
395       function takeCollateralAndLend(
396           uint256 tokenId,
397           address borrower,
398           TokenLoanParams memory params,
399           bool skimFunds,
400           uint256 deadline,
401           uint8 v,
402           bytes32 r,
403           bytes32 s

Missing: @param r https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L389-L403

File: contracts/NFTPair.sol   #8

389       /// @notice Take collateral from a pre-commited borrower and lend against it
390       /// @notice Collateral must come from the borrower, not a third party.
391       /// @param tokenId ID of the token that will function as collateral
392       /// @param borrower Address that provides collateral and receives the loan
393       /// @param params Loan terms offered, and signed by the borrower
394       /// @param skimFunds True if the funds have been transfered to the contract
395       function takeCollateralAndLend(
396           uint256 tokenId,
397           address borrower,
398           TokenLoanParams memory params,
399           bool skimFunds,
400           uint256 deadline,
401           uint8 v,
402           bytes32 r,
403           bytes32 s

Missing: @param s https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L389-L403

File: contracts/NFTPairWithOracle.sol   #9

381       /// @notice Caller provides collateral; loan can go to a different address.
382       /// @param tokenId ID of the token that will function as collateral
383       /// @param lender Lender, whose BentoBox balance the funds will come from
384       /// @param recipient Address to receive the loan.
385       /// @param params Loan parameters requested, and signed by the lender
386       /// @param skimCollateral True if the collateral has already been transfered
387       /// @param anyTokenId Set if lender agreed to any token. Must have tokenId 0 in signature.
388       function requestAndBorrow(
389           uint256 tokenId,
390           address lender,
391           address recipient,
392           TokenLoanParams memory params,
393           bool skimCollateral,
394           bool anyTokenId,
395           SignatureParams memory signature

Missing: @param signature https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L381-L395

File: contracts/NFTPairWithOracle.sol   #10

423       /// @notice Take collateral from a pre-commited borrower and lend against it
424       /// @notice Collateral must come from the borrower, not a third party.
425       /// @param tokenId ID of the token that will function as collateral
426       /// @param borrower Address that provides collateral and receives the loan
427       /// @param params Loan terms offered, and signed by the borrower
428       /// @param skimFunds True if the funds have been transfered to the contract
429       function takeCollateralAndLend(
430           uint256 tokenId,
431           address borrower,
432           TokenLoanParams memory params,
433           bool skimFunds,
434           SignatureParams memory signature

Missing: @param signature https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L423-L434

16. Event is missing `indexed` fields

Each event should use three indexed fields if there are three or more fields

File: contracts/NFTPair.sol   #1

65       event LogRequestLoan(address indexed borrower, uint256 indexed tokenId, uint128 valuation, uint64 duration, uint16 annualInterestBPS);

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L65

File: contracts/NFTPair.sol   #2

66       event LogUpdateLoanParams(uint256 indexed tokenId, uint128 valuation, uint64 duration, uint16 annualInterestBPS);

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L66

File: contracts/NFTPair.sol   #3

68       event LogRemoveCollateral(uint256 indexed tokenId, address recipient);

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L68

File: contracts/NFTPair.sol   #4

73       event LogWithdrawFees(address indexed feeTo, uint256 feeShare);

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L73

File: contracts/NFTPairWithOracle.sol   #5

75       event LogRequestLoan(
76           address indexed borrower,
77           uint256 indexed tokenId,
78           uint128 valuation,
79           uint64 duration,
80           uint16 annualInterestBPS,
81           uint16 ltvBPS
82       );

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L75-L82

File: contracts/NFTPairWithOracle.sol   #6

83       event LogUpdateLoanParams(uint256 indexed tokenId, uint128 valuation, uint64 duration, uint16 annualInterestBPS, uint16 ltvBPS);

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L83

File: contracts/NFTPairWithOracle.sol   #7

85       event LogRemoveCollateral(uint256 indexed tokenId, address recipient);

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L85

File: contracts/NFTPairWithOracle.sol   #8

90       event LogWithdrawFees(address indexed feeTo, uint256 feeShare);

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L90

17. A best practice is to check for signature malleability

File: contracts/NFTPair.sol   #1

383              require(ecrecover(_getDigest(dataHash), v, r, s) == lender, "NFTPair: signature invalid");

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L383

File: contracts/NFTPair.sol   #2

419          require(ecrecover(_getDigest(dataHash), v, r, s) == borrower, "NFTPair: signature invalid");

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L419

File: contracts/NFTPairWithOracle.sol   #3

417              require(ecrecover(_getDigest(dataHash), signature.v, signature.r, signature.s) == lender, "NFTPair: signature invalid");

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L417

File: contracts/NFTPairWithOracle.sol   #4

452          require(ecrecover(_getDigest(dataHash), signature.v, signature.r, signature.s) == borrower, "NFTPair: signature invalid");

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L452

18. Consider making contract `Pausable` to have some protection against ongoing exploits

File: contracts/NFTPair.sol   #1

59  contract NFTPair is BoringOwnable, Domain, IMasterContract {

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L59

File: contracts/NFTPairWithOracle.sol   #2

69  contract NFTPairWithOracle is BoringOwnable, Domain, IMasterContract {

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L69

19. States/flags should use `Enum`s rather than separate constants

File: contracts/NFTPair.sol   #1

96      uint8 private constant LOAN_INITIAL = 0;
97      uint8 private constant LOAN_REQUESTED = 1;
98      uint8 private constant LOAN_OUTSTANDING = 2;

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L96-L98

File: contracts/NFTPairWithOracle.sol   #2

113      uint8 private constant LOAN_INITIAL = 0;
114      uint8 private constant LOAN_REQUESTED = 1;
115      uint8 private constant LOAN_OUTSTANDING = 2;

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L113-L115

File: contracts/NFTPair.sol   #3

545      uint8 internal constant ACTION_REPAY = 2;
546      uint8 internal constant ACTION_REMOVE_COLLATERAL = 4;
547  
548      uint8 internal constant ACTION_REQUEST_LOAN = 12;
549      uint8 internal constant ACTION_LEND = 13;
550  
551      // Function on BentoBox
552      uint8 internal constant ACTION_BENTO_DEPOSIT = 20;
553      uint8 internal constant ACTION_BENTO_WITHDRAW = 21;
554      uint8 internal constant ACTION_BENTO_TRANSFER = 22;
555      uint8 internal constant ACTION_BENTO_TRANSFER_MULTIPLE = 23;
556      uint8 internal constant ACTION_BENTO_SETAPPROVAL = 24;
557  
558      // Any external call (except to BentoBox)
559      uint8 internal constant ACTION_CALL = 30;
560  
561      // Signed requests
562      uint8 internal constant ACTION_REQUEST_AND_BORROW = 40;
563      uint8 internal constant ACTION_TAKE_COLLATERAL_AND_LEND = 41;

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L545-L563

File: contracts/NFTPairWithOracle.sol   #4

578      uint8 internal constant ACTION_REPAY = 2;
579      uint8 internal constant ACTION_REMOVE_COLLATERAL = 4;
580  
581      uint8 internal constant ACTION_REQUEST_LOAN = 12;
582      uint8 internal constant ACTION_LEND = 13;
583  
584      // Function on BentoBox
585      uint8 internal constant ACTION_BENTO_DEPOSIT = 20;
586      uint8 internal constant ACTION_BENTO_WITHDRAW = 21;
587      uint8 internal constant ACTION_BENTO_TRANSFER = 22;
588      uint8 internal constant ACTION_BENTO_TRANSFER_MULTIPLE = 23;
589      uint8 internal constant ACTION_BENTO_SETAPPROVAL = 24;
590  
591      // Any external call (except to BentoBox)
592      uint8 internal constant ACTION_CALL = 30;
593  
594      // Signed requests
595      uint8 internal constant ACTION_REQUEST_AND_BORROW = 40;
596      uint8 internal constant ACTION_TAKE_COLLATERAL_AND_LEND = 41;

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L578-L596

20. Non-exploitable re-entrancies

Code should follow the best-practice of check-effects-interaction

Reentrancy in NFTPair._lend(address,uint256,TokenLoanParams,bool) (contracts/NFTPair.sol#271-315):  #1
    External calls:
    - bentoBox.transfer(asset,lender,address(this),totalShare - openFeeShare + protocolFeeShare) (contracts/NFTPair.sol#301)
    - bentoBox.transfer(asset,address(this),loan.borrower,borrowerShare) (contracts/NFTPair.sol#305)
    State variables written after the call(s):
    - feesEarnedShare += protocolFeeShare (contracts/NFTPair.sol#307)
    - tokenLoan[tokenId] = loan (contracts/NFTPair.sol#312)

Reentrancy in NFTPairWithOracle._lend(address,uint256,TokenLoanParams,bool) (contracts/NFTPairWithOracle.sol#300-350):  #2
    External calls:
    - (rate) = params.oracle.get(address(this),tokenId) (contracts/NFTPairWithOracle.sol#321)
    - bentoBox.transfer(asset,lender,address(this),totalShare - openFeeShare + protocolFeeShare) (contracts/NFTPairWithOracle.sol#336)
    - bentoBox.transfer(asset,address(this),loan.borrower,borrowerShare) (contracts/NFTPairWithOracle.sol#340)
    State variables written after the call(s):
    - feesEarnedShare += protocolFeeShare (contracts/NFTPairWithOracle.sol#342)
    - tokenLoan[tokenId] = loan (contracts/NFTPairWithOracle.sol#347)

Reentrancy in NFTPair._requestLoan(address,uint256,TokenLoanParams,address,bool) (contracts/NFTPair.sol#205-227):  #3
    External calls:
    - collateral.transferFrom(collateralProvider,address(this),tokenId) (contracts/NFTPair.sol#218)
    State variables written after the call(s):
    - tokenLoan[tokenId] = loan (contracts/NFTPair.sol#223)

Reentrancy in NFTPairWithOracle._requestLoan(address,uint256,TokenLoanParams,address,bool) (contracts/NFTPairWithOracle.sol#225-247):  #4
    External calls:
    - collateral.transferFrom(collateralProvider,address(this),tokenId) (contracts/NFTPairWithOracle.sol#238)
    State variables written after the call(s):
    - tokenLoan[tokenId] = loan (contracts/NFTPairWithOracle.sol#243)

Reentrancy in NFTPairWithOracle.removeCollateral(uint256,address) (contracts/NFTPairWithOracle.sol#267-297):  #5
    External calls:
    - (rate) = loanParams.oracle.get(address(this),tokenId) (contracts/NFTPairWithOracle.sol#287)
    State variables written after the call(s):
    - delete tokenLoan[tokenId] (contracts/NFTPairWithOracle.sol#294)

Reentrancy in NFTPair.repay(uint256,bool) (contracts/NFTPair.sol#505-543):  #6
    External calls:
    - bentoBox.transfer(asset,msg.sender,address(this),feeShare) (contracts/NFTPair.sol#532)
    State variables written after the call(s):
    - delete tokenLoan[tokenId] (contracts/NFTPair.sol#537)

Reentrancy in NFTPairWithOracle.repay(uint256,bool) (contracts/NFTPairWithOracle.sol#538-576):  #7
    External calls:
    - bentoBox.transfer(asset,msg.sender,address(this),feeShare) (contracts/NFTPairWithOracle.sol#565)
    State variables written after the call(s):
    - delete tokenLoan[tokenId] (contracts/NFTPairWithOracle.sol#570)

Reentrancy in NFTPair.requestAndBorrow(uint256,address,address,TokenLoanParams,bool,bool,uint256,uint8,bytes32,bytes32) (contracts/NFTPair.sol#353-387):  #8
    External calls:
    - _requestLoan(msg.sender,tokenId,params,recipient,skimCollateral) (contracts/NFTPair.sol#385)
        - collateral.transferFrom(collateralProvider,address(this),tokenId) (contracts/NFTPair.sol#218)
    - _lend(lender,tokenId,params,false) (contracts/NFTPair.sol#386)
        - bentoBox.transfer(asset,lender,address(this),totalShare - openFeeShare + protocolFeeShare) (contracts/NFTPair.sol#301)
        - bentoBox.transfer(asset,address(this),loan.borrower,borrowerShare) (contracts/NFTPair.sol#305)
    State variables written after the call(s):
    - _lend(lender,tokenId,params,false) (contracts/NFTPair.sol#386)
        - tokenLoan[tokenId] = loan (contracts/NFTPair.sol#312)

Reentrancy in NFTPairWithOracle.requestAndBorrow(uint256,address,address,TokenLoanParams,bool,bool,SignatureParams) (contracts/NFTPairWithOracle.sol#388-421):  #9
    External calls:
    - _requestLoan(msg.sender,tokenId,params,recipient,skimCollateral) (contracts/NFTPairWithOracle.sol#419)
        - collateral.transferFrom(collateralProvider,address(this),tokenId) (contracts/NFTPairWithOracle.sol#238)
    - _lend(lender,tokenId,params,false) (contracts/NFTPairWithOracle.sol#420)
        - (rate) = params.oracle.get(address(this),tokenId) (contracts/NFTPairWithOracle.sol#321)
        - bentoBox.transfer(asset,lender,address(this),totalShare - openFeeShare + protocolFeeShare) (contracts/NFTPairWithOracle.sol#336)
        - bentoBox.transfer(asset,address(this),loan.borrower,borrowerShare) (contracts/NFTPairWithOracle.sol#340)
    State variables written after the call(s):
    - _lend(lender,tokenId,params,false) (contracts/NFTPairWithOracle.sol#420)
        - tokenLoan[tokenId] = loan (contracts/NFTPairWithOracle.sol#347)

Reentrancy in NFTPair.takeCollateralAndLend(uint256,address,TokenLoanParams,bool,uint256,uint8,bytes32,bytes32) (contracts/NFTPair.sol#395-422):  #10
    External calls:
    - _requestLoan(borrower,tokenId,params,borrower,false) (contracts/NFTPair.sol#420)
        - collateral.transferFrom(collateralProvider,address(this),tokenId) (contracts/NFTPair.sol#218)
    - _lend(msg.sender,tokenId,params,skimFunds) (contracts/NFTPair.sol#421)
        - bentoBox.transfer(asset,lender,address(this),totalShare - openFeeShare + protocolFeeShare) (contracts/NFTPair.sol#301)
        - bentoBox.transfer(asset,address(this),loan.borrower,borrowerShare) (contracts/NFTPair.sol#305)
    State variables written after the call(s):
    - _lend(msg.sender,tokenId,params,skimFunds) (contracts/NFTPair.sol#421)
        - tokenLoan[tokenId] = loan (contracts/NFTPair.sol#312)

Reentrancy in NFTPairWithOracle.takeCollateralAndLend(uint256,address,TokenLoanParams,bool,SignatureParams) (contracts/NFTPairWithOracle.sol#429-455):  #11
    External calls:
    - _requestLoan(borrower,tokenId,params,borrower,false) (contracts/NFTPairWithOracle.sol#453)
        - collateral.transferFrom(collateralProvider,address(this),tokenId) (contracts/NFTPairWithOracle.sol#238)
    - _lend(msg.sender,tokenId,params,skimFunds) (contracts/NFTPairWithOracle.sol#454)
        - (rate) = params.oracle.get(address(this),tokenId) (contracts/NFTPairWithOracle.sol#321)
        - bentoBox.transfer(asset,lender,address(this),totalShare - openFeeShare + protocolFeeShare) (contracts/NFTPairWithOracle.sol#336)
        - bentoBox.transfer(asset,address(this),loan.borrower,borrowerShare) (contracts/NFTPairWithOracle.sol#340)
    State variables written after the call(s):
    - _lend(msg.sender,tokenId,params,skimFunds) (contracts/NFTPairWithOracle.sol#454)
        - tokenLoan[tokenId] = loan (contracts/NFTPairWithOracle.sol#347)

Reentrancy in NFTPair.withdrawFees() (contracts/NFTPair.sol#713-723):  #12
    External calls:
    - bentoBox.transfer(asset,address(this),to,_share) (contracts/NFTPair.sol#718)
    State variables written after the call(s):
    - feesEarnedShare = 0 (contracts/NFTPair.sol#719)

Reentrancy in NFTPairWithOracle.withdrawFees() (contracts/NFTPairWithOracle.sol#735-745):  #13
    External calls:
    - bentoBox.transfer(asset,address(this),to,_share) (contracts/NFTPairWithOracle.sol#740)
    State variables written after the call(s):
    - feesEarnedShare = 0 (contracts/NFTPairWithOracle.sol#741)

Reentrancy in NFTPair._requestLoan(address,uint256,TokenLoanParams,address,bool) (contracts/NFTPair.sol#205-227):  #14
    External calls:
    - collateral.transferFrom(collateralProvider,address(this),tokenId) (contracts/NFTPair.sol#218)
    State variables written after the call(s):
    - tokenLoanParams[tokenId] = params (contracts/NFTPair.sol#224)

Reentrancy in NFTPairWithOracle._requestLoan(address,uint256,TokenLoanParams,address,bool) (contracts/NFTPairWithOracle.sol#225-247):  #15
    External calls:
    - collateral.transferFrom(collateralProvider,address(this),tokenId) (contracts/NFTPairWithOracle.sol#238)
    State variables written after the call(s):
    - tokenLoanParams[tokenId] = params (contracts/NFTPairWithOracle.sol#244)

Reentrancy in NFTPair.repay(uint256,bool) (contracts/NFTPair.sol#505-543):  #16
    External calls:
    - bentoBox.transfer(asset,msg.sender,address(this),feeShare) (contracts/NFTPair.sol#532)
    State variables written after the call(s):
    - feesEarnedShare += feeShare (contracts/NFTPair.sol#536)

Reentrancy in NFTPairWithOracle.repay(uint256,bool) (contracts/NFTPairWithOracle.sol#538-576):  #17
    External calls:
    - bentoBox.transfer(asset,msg.sender,address(this),feeShare) (contracts/NFTPairWithOracle.sol#565)
    State variables written after the call(s):
    - feesEarnedShare += feeShare (contracts/NFTPairWithOracle.sol#569)

Reentrancy in NFTPair._lend(address,uint256,TokenLoanParams,bool) (contracts/NFTPair.sol#271-315):  #18
    External calls:
    - bentoBox.transfer(asset,lender,address(this),totalShare - openFeeShare + protocolFeeShare) (contracts/NFTPair.sol#301)
    - bentoBox.transfer(asset,address(this),loan.borrower,borrowerShare) (contracts/NFTPair.sol#305)
    Event emitted after the call(s):
    - LogLend(lender,tokenId) (contracts/NFTPair.sol#314)

Reentrancy in NFTPairWithOracle._lend(address,uint256,TokenLoanParams,bool) (contracts/NFTPairWithOracle.sol#300-350):  #19
    External calls:
    - (rate) = params.oracle.get(address(this),tokenId) (contracts/NFTPairWithOracle.sol#321)
    - bentoBox.transfer(asset,lender,address(this),totalShare - openFeeShare + protocolFeeShare) (contracts/NFTPairWithOracle.sol#336)
    - bentoBox.transfer(asset,address(this),loan.borrower,borrowerShare) (contracts/NFTPairWithOracle.sol#340)
    Event emitted after the call(s):
    - LogLend(lender,tokenId) (contracts/NFTPairWithOracle.sol#349)

Reentrancy in NFTPair._requestLoan(address,uint256,TokenLoanParams,address,bool) (contracts/NFTPair.sol#205-227):  #20
    External calls:
    - collateral.transferFrom(collateralProvider,address(this),tokenId) (contracts/NFTPair.sol#218)
    Event emitted after the call(s):
    - LogRequestLoan(to,tokenId,params.valuation,params.duration,params.annualInterestBPS) (contracts/NFTPair.sol#226)

Reentrancy in NFTPairWithOracle._requestLoan(address,uint256,TokenLoanParams,address,bool) (contracts/NFTPairWithOracle.sol#225-247):  #21
    External calls:
    - collateral.transferFrom(collateralProvider,address(this),tokenId) (contracts/NFTPairWithOracle.sol#238)
    Event emitted after the call(s):
    - LogRequestLoan(to,tokenId,params.valuation,params.duration,params.annualInterestBPS,params.ltvBPS) (contracts/NFTPairWithOracle.sol#246)

Reentrancy in NFTPair.removeCollateral(uint256,address) (contracts/NFTPair.sol#247-268):  #22
    External calls:
    - collateral.transferFrom(address(this),to,tokenId) (contracts/NFTPair.sol#266)
    Event emitted after the call(s):
    - LogRemoveCollateral(tokenId,to) (contracts/NFTPair.sol#267)

Reentrancy in NFTPairWithOracle.removeCollateral(uint256,address) (contracts/NFTPairWithOracle.sol#267-297):  #23
    External calls:
    - (rate) = loanParams.oracle.get(address(this),tokenId) (contracts/NFTPairWithOracle.sol#287)
    - collateral.transferFrom(address(this),to,tokenId) (contracts/NFTPairWithOracle.sol#295)
    Event emitted after the call(s):
    - LogRemoveCollateral(tokenId,to) (contracts/NFTPairWithOracle.sol#296)

Reentrancy in NFTPair.repay(uint256,bool) (contracts/NFTPair.sol#505-543):  #24
    External calls:
    - bentoBox.transfer(asset,msg.sender,address(this),feeShare) (contracts/NFTPair.sol#532)
    - bentoBox.transfer(asset,from,loan.lender,totalShare - feeShare) (contracts/NFTPair.sol#539)
    - collateral.transferFrom(address(this),loan.borrower,tokenId) (contracts/NFTPair.sol#540)
    Event emitted after the call(s):
    - LogRepay(from,tokenId) (contracts/NFTPair.sol#542)

Reentrancy in NFTPairWithOracle.repay(uint256,bool) (contracts/NFTPairWithOracle.sol#538-576):  #25
    External calls:
    - bentoBox.transfer(asset,msg.sender,address(this),feeShare) (contracts/NFTPairWithOracle.sol#565)
    - bentoBox.transfer(asset,from,loan.lender,totalShare - feeShare) (contracts/NFTPairWithOracle.sol#572)
    - collateral.transferFrom(address(this),loan.borrower,tokenId) (contracts/NFTPairWithOracle.sol#573)
    Event emitted after the call(s):
    - LogRepay(from,tokenId) (contracts/NFTPairWithOracle.sol#575)

Reentrancy in NFTPair.requestAndBorrow(uint256,address,address,TokenLoanParams,bool,bool,uint256,uint8,bytes32,bytes32) (contracts/NFTPair.sol#353-387):  #26
    External calls:
    - _requestLoan(msg.sender,tokenId,params,recipient,skimCollateral) (contracts/NFTPair.sol#385)
        - collateral.transferFrom(collateralProvider,address(this),tokenId) (contracts/NFTPair.sol#218)
    - _lend(lender,tokenId,params,false) (contracts/NFTPair.sol#386)
        - bentoBox.transfer(asset,lender,address(this),totalShare - openFeeShare + protocolFeeShare) (contracts/NFTPair.sol#301)
        - bentoBox.transfer(asset,address(this),loan.borrower,borrowerShare) (contracts/NFTPair.sol#305)
    Event emitted after the call(s):
    - LogLend(lender,tokenId) (contracts/NFTPair.sol#314)
        - _lend(lender,tokenId,params,false) (contracts/NFTPair.sol#386)

Reentrancy in NFTPairWithOracle.requestAndBorrow(uint256,address,address,TokenLoanParams,bool,bool,SignatureParams) (contracts/NFTPairWithOracle.sol#388-421):  #27
    External calls:
    - _requestLoan(msg.sender,tokenId,params,recipient,skimCollateral) (contracts/NFTPairWithOracle.sol#419)
        - collateral.transferFrom(collateralProvider,address(this),tokenId) (contracts/NFTPairWithOracle.sol#238)
    - _lend(lender,tokenId,params,false) (contracts/NFTPairWithOracle.sol#420)
        - (rate) = params.oracle.get(address(this),tokenId) (contracts/NFTPairWithOracle.sol#321)
        - bentoBox.transfer(asset,lender,address(this),totalShare - openFeeShare + protocolFeeShare) (contracts/NFTPairWithOracle.sol#336)
        - bentoBox.transfer(asset,address(this),loan.borrower,borrowerShare) (contracts/NFTPairWithOracle.sol#340)
    Event emitted after the call(s):
    - LogLend(lender,tokenId) (contracts/NFTPairWithOracle.sol#349)
        - _lend(lender,tokenId,params,false) (contracts/NFTPairWithOracle.sol#420)

Reentrancy in NFTPair.takeCollateralAndLend(uint256,address,TokenLoanParams,bool,uint256,uint8,bytes32,bytes32) (contracts/NFTPair.sol#395-422):  #28
    External calls:
    - _requestLoan(borrower,tokenId,params,borrower,false) (contracts/NFTPair.sol#420)
        - collateral.transferFrom(collateralProvider,address(this),tokenId) (contracts/NFTPair.sol#218)
    - _lend(msg.sender,tokenId,params,skimFunds) (contracts/NFTPair.sol#421)
        - bentoBox.transfer(asset,lender,address(this),totalShare - openFeeShare + protocolFeeShare) (contracts/NFTPair.sol#301)
        - bentoBox.transfer(asset,address(this),loan.borrower,borrowerShare) (contracts/NFTPair.sol#305)
    Event emitted after the call(s):
    - LogLend(lender,tokenId) (contracts/NFTPair.sol#314)
        - _lend(msg.sender,tokenId,params,skimFunds) (contracts/NFTPair.sol#421)

Reentrancy in NFTPairWithOracle.takeCollateralAndLend(uint256,address,TokenLoanParams,bool,SignatureParams) (contracts/NFTPairWithOracle.sol#429-455):  #29
    External calls:
    - _requestLoan(borrower,tokenId,params,borrower,false) (contracts/NFTPairWithOracle.sol#453)
        - collateral.transferFrom(collateralProvider,address(this),tokenId) (contracts/NFTPairWithOracle.sol#238)
    - _lend(msg.sender,tokenId,params,skimFunds) (contracts/NFTPairWithOracle.sol#454)
        - (rate) = params.oracle.get(address(this),tokenId) (contracts/NFTPairWithOracle.sol#321)
        - bentoBox.transfer(asset,lender,address(this),totalShare - openFeeShare + protocolFeeShare) (contracts/NFTPairWithOracle.sol#336)
        - bentoBox.transfer(asset,address(this),loan.borrower,borrowerShare) (contracts/NFTPairWithOracle.sol#340)
    Event emitted after the call(s):
    - LogLend(lender,tokenId) (contracts/NFTPairWithOracle.sol#349)
        - _lend(msg.sender,tokenId,params,skimFunds) (contracts/NFTPairWithOracle.sol#454)

Reentrancy in NFTPair.withdrawFees() (contracts/NFTPair.sol#713-723):  #30
    External calls:
    - bentoBox.transfer(asset,address(this),to,_share) (contracts/NFTPair.sol#718)
    Event emitted after the call(s):
    - LogWithdrawFees(to,_share) (contracts/NFTPair.sol#722)

Reentrancy in NFTPairWithOracle.withdrawFees() (contracts/NFTPairWithOracle.sol#735-745):  #31
    External calls:
    - bentoBox.transfer(asset,address(this),to,_share) (contracts/NFTPairWithOracle.sol#740)
    Event emitted after the call(s):
    - LogWithdrawFees(to,_share) (contracts/NFTPairWithOracle.sol#744)

code-423n4 / 2022-04-abranft-findings

QA Report #75

Low Risk Issues

1. Should ensure loan collateral is not immediately seizable

2. Pairs do not implement `ERC721TokenReceiver`

3. Incorrect comments

4. Comments should be enforced by `require()`s

5. Calls incorrectly allow non-zero `msg.value`

6. Missing checks for `address(0x0)` when assigning values to `address` state variables

7. `ecrecover` not checked for zero result

Non-critical Issues

1. Consider supporting CryptoPunks and EtherRocks

2. Contracts should be refactored to extend a base contract with the common functionality

3. Some compilers can't handle two different contracts with the same name

4. Calls to `BoringMath.to128()` are redundant

5. `require()`/`revert()` statements should have descriptive reason strings

6. `public` functions not called by the contract should be declared `external` instead

7. Interfaces should be moved to separate files

8. `2**<n> - 1` should be re-written as `type(uint<n>).max`

9. `constant`s should be defined rather than using magic numbers

10. Numeric values having to do with time should use time units for readability

11. Use a more recent version of solidity

12. Constant redefined elsewhere

13. Fee mechanics should be better described

14. Typos

15. NatSpec is incomplete

16. Event is missing `indexed` fields

17. A best practice is to check for signature malleability

18. Consider making contract `Pausable` to have some protection against ongoing exploits

19. States/flags should use `Enum`s rather than separate constants

20. Non-exploitable re-entrancies

code-423n4 / 2022-04-abranft-findings

QA Report #75

Low Risk Issues

1. Should ensure loan collateral is not immediately seizable

2. Pairs do not implement ERC721TokenReceiver

3. Incorrect comments

4. Comments should be enforced by require()s

5. Calls incorrectly allow non-zero msg.value

6. Missing checks for address(0x0) when assigning values to address state variables

7. ecrecover not checked for zero result

Non-critical Issues

1. Consider supporting CryptoPunks and EtherRocks

2. Contracts should be refactored to extend a base contract with the common functionality

3. Some compilers can't handle two different contracts with the same name

4. Calls to BoringMath.to128() are redundant

5. require()/revert() statements should have descriptive reason strings

6. public functions not called by the contract should be declared external instead

7. Interfaces should be moved to separate files

8. 2**<n> - 1 should be re-written as type(uint<n>).max

9. constants should be defined rather than using magic numbers

10. Numeric values having to do with time should use time units for readability

11. Use a more recent version of solidity

12. Constant redefined elsewhere

13. Fee mechanics should be better described

14. Typos

15. NatSpec is incomplete

16. Event is missing indexed fields

17. A best practice is to check for signature malleability

18. Consider making contract Pausable to have some protection against ongoing exploits

19. States/flags should use Enums rather than separate constants

20. Non-exploitable re-entrancies

2. Pairs do not implement `ERC721TokenReceiver`

4. Comments should be enforced by `require()`s

5. Calls incorrectly allow non-zero `msg.value`

6. Missing checks for `address(0x0)` when assigning values to `address` state variables

7. `ecrecover` not checked for zero result

4. Calls to `BoringMath.to128()` are redundant

5. `require()`/`revert()` statements should have descriptive reason strings

6. `public` functions not called by the contract should be declared `external` instead

8. `2**<n> - 1` should be re-written as `type(uint<n>).max`

9. `constant`s should be defined rather than using magic numbers

16. Event is missing `indexed` fields

18. Consider making contract `Pausable` to have some protection against ongoing exploits

19. States/flags should use `Enum`s rather than separate constants