Platform: Code4rena
Start Date: 27/04/2022
Pot Size: $50,000 MIM
Total HM: 6
Participants: 59
Period: 5 days
Judge: 0xean
Id: 113
League: ETH
Rank: 6/59
Findings: 4
Award: $2,418.07
🌟 Selected for report: 1
🚀 Solo Findings: 0
https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L287 https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L321
The INFTOracle interface's NatSpec says that there are two return values, one of which indicates whether the rate returned can be trusted:
File: contracts/interfaces/INFTOracle.sol 8 /// @return success if no valid (recent) rate is available, return false else true. 9 /// @return rate The rate of the requested asset / pair / pool. 10 function get(address pair, uint256 tokenId) external returns (bool success, uint256 rate);
The NFTPairWithOracle ignores the success return code of calls to INFTOracle.get(), which may lead to the misspricing of assets and loss of funds/tokens
File: contracts/NFTPairWithOracle.sol #1 287 (, uint256 rate) = loanParams.oracle.get(address(this), tokenId);
File: contracts/NFTPairWithOracle.sol #1 321 (, uint256 rate) = params.oracle.get(address(this), tokenId);
Code inspection
Store the success return value and require(success) before using the rate
#0 - 0xm3rlin
2022-05-04T14:06:46Z
The borrower can lose his/her collateral immediately
The code that determines whether or not the collateral can be seized when it's underwater is as follows:
File: contracts/NFTPairWithOracle.sol 277 if (uint256(loan.startTime) + tokenLoanParams[tokenId].duration > block.timestamp) { 278 TokenLoanParams memory loanParams = tokenLoanParams[tokenId]; 279 // No underflow: loan.startTime is only ever set to a block timestamp 280 // Cast is safe: if this overflows, then all loans have expired anyway 281 uint256 interest = calculateInterest( 282 loanParams.valuation, 283 uint64(block.timestamp - loan.startTime), 284 loanParams.annualInterestBPS 285 ).to128(); 286 uint256 amount = loanParams.valuation + interest; 287 (, uint256 rate) = loanParams.oracle.get(address(this), tokenId); 288 require(rate.mul(loanParams.ltvBPS) / BPS < amount, "NFT is still valued"); 289 }
If loanParams.ltvBPS on line 288 is zero then the left side of the inequality will always be zero, the require() condition will pass, and the collateral will be seizable.
updateLoanParams() allows ltvBPS to be set to zero as long as the loan is still outstanding:
File: contracts/NFTPairWithOracle.sol 198 function updateLoanParams(uint256 tokenId, TokenLoanParams memory params) public { 199 TokenLoan memory loan = tokenLoan[tokenId]; 200 if (loan.status == LOAN_OUTSTANDING) { 201 // The lender can change terms so long as the changes are strictly 202 // the same or better for the borrower: 203 require(msg.sender == loan.lender, "NFTPair: not the lender"); 204 TokenLoanParams memory cur = tokenLoanParams[tokenId]; 205 require( 206 params.duration >= cur.duration && 207 params.valuation <= cur.valuation && 208 params.annualInterestBPS <= cur.annualInterestBPS && 209 params.ltvBPS <= cur.ltvBPS, 210 "NFTPair: worse params" 211 );
If an attacker wants to pay the loan amount minus the open fee plus the protocol fee, in a single transaction the attacker can call lend() with the right arguments, call updateLoanParams() with a ltvBPS of zero, and call removeCollateral().
Code inspection
In updateLoanParams() add the following requirement: require(rate.mul(loanParams.ltvBPS) / BPS >= amount)
#0 - cryptolyndon
2022-05-05T04:44:07Z
Duplicate of #51 but confirmed.
🌟 Selected for report: IllIllI
Also found by: 0x1337, 0x1f8b, 0xDjango, 0xf15ers, AuditsAreUS, BowTiedWardens, CertoraInc, Funen, GimelSec, MaratCerby, Ruhum, WatchPug, antonttc, berndartmueller, bobi, bobirichman, broccolirob, catchup, cccz, defsec, delfin454000, gs8nrv, gzeon, horsefacts, hubble, hyh, ilan, jah, joestakey, kebabsec, kenta, kenzo, m9800, mics, oyc_109, pauliax, reassor, robee, samruna, sikorico, simon135, throttle, unforgiven, z3s
708.3931 MIM - $708.39
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;
File: contracts/NFTPairWithOracle.sol #2 244 tokenLoanParams[tokenId] = params;
ERC721TokenReceiverAccording 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 {
File: contracts/NFTPairWithOracle.sol #2 69 contract NFTPairWithOracle is BoringOwnable, Domain, IMasterContract {
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
File: contracts/NFTPairWithOracle.sol #2 355 /// @param skim True if the funds have been transfered to the contract
require()sThe 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..
File: contracts/NFTPairWithOracle.sol #2 346 loan.startTime = uint64(block.timestamp); // Do not use in 12e10 years..
msg.valueThe 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`.
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`.
address(0x0) when assigning values to address state variablesFile: contracts/NFTPair.sol #1 729 feeTo = newFeeTo;
File: contracts/NFTPairWithOracle.sol #2 751 feeTo = newFeeTo;
ecrecover not checked for zero resultA 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");
File: contracts/NFTPair.sol #2 419 require(ecrecover(_getDigest(dataHash), v, r, s) == borrower, "NFTPair: signature invalid");
File: contracts/NFTPairWithOracle.sol #3 417 require(ecrecover(_getDigest(dataHash), signature.v, signature.r, signature.s) == lender, "NFTPair: signature invalid");
File: contracts/NFTPairWithOracle.sol #4 452 require(ecrecover(_getDigest(dataHash), signature.v, signature.r, signature.s) == borrower, "NFTPair: signature invalid");
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.
$ 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
File: contracts/NFTPairWithOracle.sol (various lines) #2
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 }
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 }
BoringMath.to128() are redundantAll 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();
File: contracts/NFTPairWithOracle.sol #2 552 uint256 interest = calculateInterest(principal, uint64(block.timestamp - loan.startTime), loanParams.annualInterestBPS).to128();
File: contracts/NFTPair.sol #3 519 uint256 interest = calculateInterest(principal, uint64(block.timestamp - loan.startTime), loanParams.annualInterestBPS).to128();
require()/revert() statements should have descriptive reason stringsFile: contracts/NFTPair.sol #1 501 revert();
File: contracts/NFTPairWithOracle.sol #2 534 revert();
public functions not called by the contract should be declared external insteadContracts 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 {
File: contracts/NFTPair.sol #2 713 function withdrawFees() public { 714 address to = masterContract.feeTo();
File: contracts/NFTPair.sol #3 728 function setFeeTo(address newFeeTo) public onlyOwner {
File: contracts/NFTPairWithOracle.sol #4 198 function updateLoanParams(uint256 tokenId, TokenLoanParams memory params) public {
File: contracts/NFTPairWithOracle.sol #5 735 function withdrawFees() public { 736 address to = masterContract.feeTo();
File: contracts/NFTPairWithOracle.sol #6 750 function setFeeTo(address newFeeTo) public onlyOwner {
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 {
File: contracts/NFTPairWithOracle.sol #2 54 interface INFTPair {
File: contracts/NFTPair.sol #3 37 interface ILendingClub {
File: contracts/NFTPair.sol #4 44 interface INFTPair {
2**<n> - 1 should be re-written as type(uint<n>).maxEarlier 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) {
File: contracts/NFTPairWithOracle.sol #2 533 if (interest >= 2**128) {
constants should be defined rather than using magic numbersFile: contracts/NFTPair.sol #1 500 if (interest >= 2**128) {
File: contracts/NFTPairWithOracle.sol #2 533 if (interest >= 2**128) {
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;
File: contracts/NFTPairWithOracle.sol #2 128 uint256 private constant YEAR_BPS = 3600 * 24 * 365 * 10_000;
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;
File: contracts/NFTPairWithOracle.sol #2 20 pragma solidity 0.6.12;
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
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 }
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 }
File: contracts/NFTPair.sol #1 90 // Track assets we own. Used to allow skimming the excesss.
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
File: contracts/NFTPair.sol #4 320 /// @param skim True if the funds have been transfered to the contract
File: contracts/NFTPair.sol #5 351 /// @param skimCollateral True if the collateral has already been transfered
File: contracts/NFTPair.sol #6 389 /// @notice Take collateral from a pre-commited borrower and lend against it
File: contracts/NFTPair.sol #7 394 /// @param skimFunds True if the funds have been transfered to the contract
File: contracts/NFTPair.sol #8 434 /// of the above inquality) fits in 128 bits, then the function is
File: contracts/NFTPair.sol #9 446 // (NOTE: n is hardcoded as COMPOUND_INTEREST_TERMS)
File: contracts/NFTPairWithOracle.sol #10 107 // Track assets we own. Used to allow skimming the excesss.
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
File: contracts/NFTPairWithOracle.sol #13 355 /// @param skim True if the funds have been transfered to the contract
File: contracts/NFTPairWithOracle.sol #14 386 /// @param skimCollateral True if the collateral has already been transfered
File: contracts/NFTPairWithOracle.sol #15 423 /// @notice Take collateral from a pre-commited borrower and lend against it
File: contracts/NFTPairWithOracle.sol #16 428 /// @param skimFunds True if the funds have been transfered to the contract
File: contracts/NFTPairWithOracle.sol #17 467 /// of the above inquality) fits in 128 bits, then the function is
File: contracts/NFTPairWithOracle.sol #18 479 // (NOTE: n is hardcoded as COMPOUND_INTEREST_TERMS)
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
indexed fieldsEach 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);
File: contracts/NFTPair.sol #2 66 event LogUpdateLoanParams(uint256 indexed tokenId, uint128 valuation, uint64 duration, uint16 annualInterestBPS);
File: contracts/NFTPair.sol #3 68 event LogRemoveCollateral(uint256 indexed tokenId, address recipient);
File: contracts/NFTPair.sol #4 73 event LogWithdrawFees(address indexed feeTo, uint256 feeShare);
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 );
File: contracts/NFTPairWithOracle.sol #6 83 event LogUpdateLoanParams(uint256 indexed tokenId, uint128 valuation, uint64 duration, uint16 annualInterestBPS, uint16 ltvBPS);
File: contracts/NFTPairWithOracle.sol #7 85 event LogRemoveCollateral(uint256 indexed tokenId, address recipient);
File: contracts/NFTPairWithOracle.sol #8 90 event LogWithdrawFees(address indexed feeTo, uint256 feeShare);
File: contracts/NFTPair.sol #1 383 require(ecrecover(_getDigest(dataHash), v, r, s) == lender, "NFTPair: signature invalid");
File: contracts/NFTPair.sol #2 419 require(ecrecover(_getDigest(dataHash), v, r, s) == borrower, "NFTPair: signature invalid");
File: contracts/NFTPairWithOracle.sol #3 417 require(ecrecover(_getDigest(dataHash), signature.v, signature.r, signature.s) == lender, "NFTPair: signature invalid");
File: contracts/NFTPairWithOracle.sol #4 452 require(ecrecover(_getDigest(dataHash), signature.v, signature.r, signature.s) == borrower, "NFTPair: signature invalid");
Pausable to have some protection against ongoing exploitsFile: contracts/NFTPair.sol #1 59 contract NFTPair is BoringOwnable, Domain, IMasterContract {
File: contracts/NFTPairWithOracle.sol #2 69 contract NFTPairWithOracle is BoringOwnable, Domain, IMasterContract {
Enums rather than separate constantsFile: 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;
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;
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;
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;
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)
#0 - cryptolyndon
2022-05-12T04:56:49Z
Low-risk issues:
Agreed; this does suggest ERC-20 transfers.
If you think this is going to be an issue, then think of all the gas wasted until then by even that single check! Time enough to write a V2.
Non-critical issues:
Nonstandard NFT types that are popular enough to use warrant their own contract type.
bentoBox is not a constant that will necessarily be invariable across different master contracts. Clones already work as suggested.
#1 - 0xean
2022-05-21T17:13:35Z
I believe this to be the most complete QA report. I agree with the severities outlined, with the exception of the Low Risk number 4, which should be non critical.
#2 - liveactionllama
2022-07-11T23:06:23Z
Per discussion with @cryptolyndon from the AbraNFT team, documenting the additional sponsor comments below for inclusion in the final audit report.<br>
Calls incorrectly allow non-zero msg.value:
"Simply requiring msg.value to be zero would break things when some, but not all, actions use it."
Missing checks for address(0x0) when assigning values to address state variables:
"The zero address is pretty much the ONLY wrong address we could enter where actual loss of funds is not possible."
Some compilers can't handle two different contracts with the same name:
"This is not some example project intended to be forked and used with a wide range of different compiler setups."
Use a more recent version of solidity:
"As time ticks on 0.8.x becomes increasingly safe to use, but the suggested reason here does not even apply to our contract."
Fee mechanics should be better described:
"The contract is not meant to serve as sole documentation of our fee schedule."
A best practice is to check for signature malleability:
"We use nonces to prevent replay attacks, rather than storing used signatures. A different, equally valid, signature of the same data would be of no use to an attacker."
🌟 Selected for report: BowTiedWardens
Also found by: 0x1f8b, 0xNazgul, 0xf15ers, 0xkatana, CertoraInc, Funen, GimelSec, Hawkeye, IllIllI, Kulk0, NoamYakov, Tadashi, Tomio, TrungOre, antonttc, catchup, defsec, delfin454000, fatherOfBlocks, gzeon, horsefacts, joestakey, kenta, oyc_109, pauliax, reassor, robee, samruna, simon135, slywaters, sorrynotsorry, z3s
130.1306 MIM - $130.13
The instances below point to the second or later access of a state variable within a function. Caching will replace each Gwarmaccess (100 gas) with a much cheaper stack read. Less obvious fixes/optimizations include having local storage variables of mappings within state variable mappings or mappings within state variable structs, having local storage variables of structs within mappings, or having local caches of state variable contracts/addresses.
File: contracts/NFTPair.sol #1 178 require(address(collateral) != address(0), "NFTPair: bad pair");
File: contracts/NFTPair.sol #2 297 bentoBox.balanceOf(asset, address(this)) >= (totalShare - openFeeShare + protocolFeeShare + feesEarnedShare),
File: contracts/NFTPair.sol #3 301 bentoBox.transfer(asset, lender, address(this), totalShare - openFeeShare + protocolFeeShare);
File: contracts/NFTPair.sol #4 305 bentoBox.transfer(asset, address(this), loan.borrower, borrowerShare);
File: contracts/NFTPair.sol #5 524 uint256 feeShare = bentoBox.toShare(asset, fee, false);
File: contracts/NFTPair.sol #6 528 require(bentoBox.balanceOf(asset, address(this)) >= (totalShare + feesEarnedShare), "NFTPair: skim too much");
File: contracts/NFTPair.sol #7 532 bentoBox.transfer(asset, msg.sender, address(this), feeShare);
File: contracts/NFTPair.sol #8 539 bentoBox.transfer(asset, from, loan.lender, totalShare - feeShare);
File: contracts/NFTPairWithOracle.sol #9 195 require(address(collateral) != address(0), "NFTPair: bad pair");
File: contracts/NFTPairWithOracle.sol #10 332 bentoBox.balanceOf(asset, address(this)) >= (totalShare - openFeeShare + protocolFeeShare + feesEarnedShare),
File: contracts/NFTPairWithOracle.sol #11 336 bentoBox.transfer(asset, lender, address(this), totalShare - openFeeShare + protocolFeeShare);
File: contracts/NFTPairWithOracle.sol #12 340 bentoBox.transfer(asset, address(this), loan.borrower, borrowerShare);
File: contracts/NFTPairWithOracle.sol #13 557 uint256 feeShare = bentoBox.toShare(asset, fee, false);
File: contracts/NFTPairWithOracle.sol #14 561 require(bentoBox.balanceOf(asset, address(this)) >= (totalShare + feesEarnedShare), "NFTPair: skim too much");
File: contracts/NFTPairWithOracle.sol #15 565 bentoBox.transfer(asset, msg.sender, address(this), feeShare);
File: contracts/NFTPairWithOracle.sol #16 572 bentoBox.transfer(asset, from, loan.lender, totalShare - feeShare);
File: contracts/NFTPairWithOracle.sol #17 278 TokenLoanParams memory loanParams = tokenLoanParams[tokenId];
tokenLoanParams https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L278
<x> += <y> costs more gas than <x> = <x> + <y> for state variablesFile: contracts/NFTPair.sol #1 307 feesEarnedShare += protocolFeeShare;
File: contracts/NFTPair.sol #2 536 feesEarnedShare += feeShare;
File: contracts/NFTPairWithOracle.sol #3 342 feesEarnedShare += protocolFeeShare;
File: contracts/NFTPairWithOracle.sol #4 569 feesEarnedShare += feeShare;
<array>.length should not be looked up in every loop of a for-loopThe overheads outlined below are PER LOOP, excluding the first loop
MLOAD (3 gas)CALLDATALOAD (3 gas)Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset
File: contracts/NFTPair.sol #1 641 for (uint256 i = 0; i < actions.length; i++) {
File: contracts/NFTPairWithOracle.sol #2 674 for (uint256 i = 0; i < actions.length; i++) {
require()/revert() strings longer than 32 bytes cost extra gasFile: contracts/NFTPair.sol #1 366 require(ILendingClub(lender).willLend(tokenId, params), "NFTPair: LendingClub does not like you");
File: contracts/NFTPairWithOracle.sol #2 398 require(ILendingClub(lender).willLend(tokenId, params), "NFTPair: LendingClub does not like you");
Use a solidity version of at least 0.8.0 to get overflow protection without SafeMath
Use a solidity version of at least 0.8.2 to get compiler automatic inlining
Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads
Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings
Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value
File: contracts/NFTPair.sol #1 20 pragma solidity 0.6.12;
File: contracts/NFTPairWithOracle.sol #2 20 pragma solidity 0.6.12;
File: contracts/NFTPair.sol #1 641 for (uint256 i = 0; i < actions.length; i++) {
File: contracts/NFTPairWithOracle.sol #2 674 for (uint256 i = 0; i < actions.length; i++) {
internal functions only called once can be inlined to save gasNot inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.
File: contracts/NFTPair.sol #1 578 function _bentoDeposit( 579 bytes memory data, 580 uint256 value, 581 uint256 value1, 582 uint256 value2 583 ) internal returns (uint256, uint256) {
File: contracts/NFTPair.sol #2 591 function _bentoWithdraw( 592 bytes memory data, 593 uint256 value1, 594 uint256 value2 595 ) internal returns (uint256, uint256) {
File: contracts/NFTPair.sol #3 603 function _call( 604 uint256 value, 605 bytes memory data, 606 uint256 value1, 607 uint256 value2 608 ) internal returns (bytes memory, uint8) {
File: contracts/NFTPairWithOracle.sol #4 611 function _bentoDeposit( 612 bytes memory data, 613 uint256 value, 614 uint256 value1, 615 uint256 value2 616 ) internal returns (uint256, uint256) {
File: contracts/NFTPairWithOracle.sol #5 624 function _bentoWithdraw( 625 bytes memory data, 626 uint256 value1, 627 uint256 value2 628 ) internal returns (uint256, uint256) {
File: contracts/NFTPairWithOracle.sol #6 636 function _call( 637 uint256 value, 638 bytes memory data, 639 uint256 value1, 640 uint256 value2 641 ) internal returns (bytes memory, uint8) {
calldata instead of memory for read-only arguments in external functions saves gasWhen a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length). Using calldata directly, obliviates the need for such a loop in the contract code and runtime execution. Structs have the same overhead as an array of length one
File: contracts/NFTPair.sol #1 39 function willLend(uint256 tokenId, TokenLoanParams memory params) external view returns (bool);
File: contracts/NFTPairWithOracle.sol #2 49 function willLend(uint256 tokenId, TokenLoanParams memory params) external view returns (bool);
++i costs less gas than ++i, especially when it's used in for-loops (--i/i-- too)File: contracts/NFTPair.sol #1 494 for (uint256 k = 2; k <= COMPOUND_INTEREST_TERMS; k++) {
File: contracts/NFTPair.sol #2 641 for (uint256 i = 0; i < actions.length; i++) {
File: contracts/NFTPairWithOracle.sol #3 527 for (uint256 k = 2; k <= COMPOUND_INTEREST_TERMS; k++) {
File: contracts/NFTPairWithOracle.sol #4 674 for (uint256 i = 0; i < actions.length; i++) {
require() statements that use && saves gasSee this issue for an example
File: contracts/NFTPair.sol #1 188 require( 189 params.duration >= cur.duration && params.valuation <= cur.valuation && params.annualInterestBPS <= cur.annualInterestBPS, 190 "NFTPair: worse params" 191 );
File: contracts/NFTPair.sol #2 283 require( 284 params.valuation == accepted.valuation && 285 params.duration <= accepted.duration && 286 params.annualInterestBPS >= accepted.annualInterestBPS, 287 "NFTPair: bad params" 288 );
File: contracts/NFTPair.sol #3 622 require(callee != address(bentoBox) && callee != address(collateral) && callee != address(this), "NFTPair: can't call");
File: contracts/NFTPairWithOracle.sol #4 205 require( 206 params.duration >= cur.duration && 207 params.valuation <= cur.valuation && 208 params.annualInterestBPS <= cur.annualInterestBPS && 209 params.ltvBPS <= cur.ltvBPS, 210 "NFTPair: worse params" 211 );
File: contracts/NFTPairWithOracle.sol #5 312 require( 313 params.valuation == accepted.valuation && 314 params.duration <= accepted.duration && 315 params.annualInterestBPS >= accepted.annualInterestBPS && 316 params.ltvBPS >= accepted.ltvBPS, 317 "NFTPair: bad params" 318 );
File: contracts/NFTPairWithOracle.sol #6 655 require(callee != address(bentoBox) && callee != address(collateral) && callee != address(this), "NFTPair: can't call");
uints/ints smaller than 32 bytes (256 bits) incurs overheadWhen using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.
https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size then downcast where needed
File: contracts/NFTPair.sol #1 32 uint128 valuation; // How much will you get? OK to owe until expiration.
File: contracts/NFTPair.sol #2 33 uint64 duration; // Length of loan in seconds
File: contracts/NFTPair.sol #3 34 uint16 annualInterestBPS; // Variable cost of taking out the loan
File: contracts/NFTPair.sol #4 65 event LogRequestLoan(address indexed borrower, uint256 indexed tokenId, uint128 valuation, uint64 duration, uint16 annualInterestBPS);
File: contracts/NFTPair.sol #5 65 event LogRequestLoan(address indexed borrower, uint256 indexed tokenId, uint128 valuation, uint64 duration, uint16 annualInterestBPS);
File: contracts/NFTPair.sol #6 65 event LogRequestLoan(address indexed borrower, uint256 indexed tokenId, uint128 valuation, uint64 duration, uint16 annualInterestBPS);
File: contracts/NFTPair.sol #7 66 event LogUpdateLoanParams(uint256 indexed tokenId, uint128 valuation, uint64 duration, uint16 annualInterestBPS);
File: contracts/NFTPair.sol #8 66 event LogUpdateLoanParams(uint256 indexed tokenId, uint128 valuation, uint64 duration, uint16 annualInterestBPS);
File: contracts/NFTPair.sol #9 66 event LogUpdateLoanParams(uint256 indexed tokenId, uint128 valuation, uint64 duration, uint16 annualInterestBPS);
File: contracts/NFTPair.sol #10 96 uint8 private constant LOAN_INITIAL = 0;
File: contracts/NFTPair.sol #11 97 uint8 private constant LOAN_REQUESTED = 1;
File: contracts/NFTPair.sol #12 98 uint8 private constant LOAN_OUTSTANDING = 2;
File: contracts/NFTPair.sol #13 102 uint64 startTime;
File: contracts/NFTPair.sol #14 103 uint8 status;
File: contracts/NFTPair.sol #15 162 uint8 private constant COMPOUND_INTEREST_TERMS = 6;
File: contracts/NFTPair.sol #16 361 uint8 v,
File: contracts/NFTPair.sol #17 401 uint8 v,
File: contracts/NFTPair.sol #18 443 uint64 t,
File: contracts/NFTPair.sol #19 444 uint16 aprBPS
File: contracts/NFTPair.sol #20 515 uint128 principal = loanParams.valuation;
File: contracts/NFTPair.sol #21 545 uint8 internal constant ACTION_REPAY = 2;
File: contracts/NFTPair.sol #22 546 uint8 internal constant ACTION_REMOVE_COLLATERAL = 4;
File: contracts/NFTPair.sol #23 548 uint8 internal constant ACTION_REQUEST_LOAN = 12;
File: contracts/NFTPair.sol #24 549 uint8 internal constant ACTION_LEND = 13;
File: contracts/NFTPair.sol #25 552 uint8 internal constant ACTION_BENTO_DEPOSIT = 20;
File: contracts/NFTPair.sol #26 553 uint8 internal constant ACTION_BENTO_WITHDRAW = 21;
File: contracts/NFTPair.sol #27 554 uint8 internal constant ACTION_BENTO_TRANSFER = 22;
File: contracts/NFTPair.sol #28 555 uint8 internal constant ACTION_BENTO_TRANSFER_MULTIPLE = 23;
File: contracts/NFTPair.sol #29 556 uint8 internal constant ACTION_BENTO_SETAPPROVAL = 24;
File: contracts/NFTPair.sol #30 559 uint8 internal constant ACTION_CALL = 30;
File: contracts/NFTPair.sol #31 562 uint8 internal constant ACTION_REQUEST_AND_BORROW = 40;
File: contracts/NFTPair.sol #32 563 uint8 internal constant ACTION_TAKE_COLLATERAL_AND_LEND = 41;
File: contracts/NFTPair.sol #33 608 ) internal returns (bytes memory, uint8) {
File: contracts/NFTPair.sol #34 609 (address callee, bytes memory callData, bool useValue1, bool useValue2, uint8 returnValues) = abi.decode(
File: contracts/NFTPair.sol #35 642 uint8 action = actions[i];
File: contracts/NFTPair.sol #36 659 (address user, address _masterContract, bool approved, uint8 v, bytes32 r, bytes32 s) = abi.decode(
File: contracts/NFTPair.sol #37 675 (bytes memory returnData, uint8 returnValues) = _call(values[i], datas[i], value1, value2);
File: contracts/NFTPair.sol #38 691 uint8 v,
File: contracts/NFTPair.sol #39 703 uint8 v,
File: contracts/NFTPairWithOracle.sol #40 33 uint128 valuation; // How much will you get? OK to owe until expiration.
File: contracts/NFTPairWithOracle.sol #41 34 uint64 duration; // Length of loan in seconds
File: contracts/NFTPairWithOracle.sol #42 35 uint16 annualInterestBPS; // Variable cost of taking out the loan
File: contracts/NFTPairWithOracle.sol #43 36 uint16 ltvBPS; // Required to avoid liquidation
File: contracts/NFTPairWithOracle.sol #44 42 uint8 v;
File: contracts/NFTPairWithOracle.sol #45 78 uint128 valuation,
File: contracts/NFTPairWithOracle.sol #46 79 uint64 duration,
File: contracts/NFTPairWithOracle.sol #47 80 uint16 annualInterestBPS,
File: contracts/NFTPairWithOracle.sol #48 81 uint16 ltvBPS
File: contracts/NFTPairWithOracle.sol #49 83 event LogUpdateLoanParams(uint256 indexed tokenId, uint128 valuation, uint64 duration, uint16 annualInterestBPS, uint16 ltvBPS);
File: contracts/NFTPairWithOracle.sol #50 83 event LogUpdateLoanParams(uint256 indexed tokenId, uint128 valuation, uint64 duration, uint16 annualInterestBPS, uint16 ltvBPS);
File: contracts/NFTPairWithOracle.sol #51 83 event LogUpdateLoanParams(uint256 indexed tokenId, uint128 valuation, uint64 duration, uint16 annualInterestBPS, uint16 ltvBPS);
File: contracts/NFTPairWithOracle.sol #52 83 event LogUpdateLoanParams(uint256 indexed tokenId, uint128 valuation, uint64 duration, uint16 annualInterestBPS, uint16 ltvBPS);
File: contracts/NFTPairWithOracle.sol #53 113 uint8 private constant LOAN_INITIAL = 0;
File: contracts/NFTPairWithOracle.sol #54 114 uint8 private constant LOAN_REQUESTED = 1;
File: contracts/NFTPairWithOracle.sol #55 115 uint8 private constant LOAN_OUTSTANDING = 2;
File: contracts/NFTPairWithOracle.sol #56 119 uint64 startTime;
File: contracts/NFTPairWithOracle.sol #57 120 uint8 status;
File: contracts/NFTPairWithOracle.sol #58 179 uint8 private constant COMPOUND_INTEREST_TERMS = 6;
File: contracts/NFTPairWithOracle.sol #59 476 uint64 t,
File: contracts/NFTPairWithOracle.sol #60 477 uint16 aprBPS
File: contracts/NFTPairWithOracle.sol #61 548 uint128 principal = loanParams.valuation;
File: contracts/NFTPairWithOracle.sol #62 578 uint8 internal constant ACTION_REPAY = 2;
File: contracts/NFTPairWithOracle.sol #63 579 uint8 internal constant ACTION_REMOVE_COLLATERAL = 4;
File: contracts/NFTPairWithOracle.sol #64 581 uint8 internal constant ACTION_REQUEST_LOAN = 12;
File: contracts/NFTPairWithOracle.sol #65 582 uint8 internal constant ACTION_LEND = 13;
File: contracts/NFTPairWithOracle.sol #66 585 uint8 internal constant ACTION_BENTO_DEPOSIT = 20;
File: contracts/NFTPairWithOracle.sol #67 586 uint8 internal constant ACTION_BENTO_WITHDRAW = 21;
File: contracts/NFTPairWithOracle.sol #68 587 uint8 internal constant ACTION_BENTO_TRANSFER = 22;
File: contracts/NFTPairWithOracle.sol #69 588 uint8 internal constant ACTION_BENTO_TRANSFER_MULTIPLE = 23;
File: contracts/NFTPairWithOracle.sol #70 589 uint8 internal constant ACTION_BENTO_SETAPPROVAL = 24;
File: contracts/NFTPairWithOracle.sol #71 592 uint8 internal constant ACTION_CALL = 30;
File: contracts/NFTPairWithOracle.sol #72 595 uint8 internal constant ACTION_REQUEST_AND_BORROW = 40;
File: contracts/NFTPairWithOracle.sol #73 596 uint8 internal constant ACTION_TAKE_COLLATERAL_AND_LEND = 41;
File: contracts/NFTPairWithOracle.sol #74 641 ) internal returns (bytes memory, uint8) {
File: contracts/NFTPairWithOracle.sol #75 642 (address callee, bytes memory callData, bool useValue1, bool useValue2, uint8 returnValues) = abi.decode(
File: contracts/NFTPairWithOracle.sol #76 675 uint8 action = actions[i];
File: contracts/NFTPairWithOracle.sol #77 692 (address user, address _masterContract, bool approved, uint8 v, bytes32 r, bytes32 s) = abi.decode(
File: contracts/NFTPairWithOracle.sol #78 708 (bytes memory returnData, uint8 returnValues) = _call(values[i], datas[i], value1, value2);
require()/revert() checks should be refactored to a modifier or functionSaves deployment costs
File: contracts/NFTPair.sol #1 251 require(msg.sender == loan.borrower, "NFTPair: not the borrower");
File: contracts/NFTPair.sol #2 405 require(block.timestamp <= deadline, "NFTPair: signature expired");
File: contracts/NFTPairWithOracle.sol #3 271 require(msg.sender == loan.borrower, "NFTPair: not the borrower");
File: contracts/NFTPairWithOracle.sol #4 436 require(block.timestamp <= signature.deadline, "NFTPair: signature expired");
payableIf a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are
CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost
File: contracts/NFTPair.sol #1 728 function setFeeTo(address newFeeTo) public onlyOwner {
File: contracts/NFTPairWithOracle.sol #2 750 function setFeeTo(address newFeeTo) public onlyOwner {
> uint128(-1) is cheaper than >= 2**128File: contracts/NFTPair.sol #1 500 if (interest >= 2**128) {
File: contracts/NFTPairWithOracle.sol #2 533 if (interest >= 2**128) {
#0 - cryptolyndon
2022-05-14T01:07:24Z
1 is valid.
11 probably contains a few opportunities, but suggesting that structs, one of which currently fits in 256 bits, use all 256-bit values casts suspicion over the rest of the entire report. Did that get in by accident?