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: 22/59
Findings: 2
Award: $232.66
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 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
170.9951 MIM - $171.00
Low
Function NFTPair.cook allows executing external calls and can be trigger by anyone. For security reasons it is forbidden to invoke calls to addresses of bentoBox, collateral and the NFTPair contract itself.
require(callee != address(bentoBox) && callee != address(collateral) && callee != address(this), "NFTPair: can't call");
Analyzing other contracts that could be called by NFTPair leads to LendingClubMock that has in some of its functions logic that checks if the msg.sender is NFTPair contract.
LendingClubMock.willLendif (msg.sender != address(nftPair)) { return false; }
LendingClubMOck.lendingConditionsif (_nftPair != address(nftPair)) { TokenLoanParams memory empty; return empty; } else {
Functions willLend and lendingConditions are view functions so in current implementation there is direct security risk in bypassing implemented checks but that will depend on the future development of LendingClub contracts.
Manual Review / VSCode
It is recommended to add documentation that will clearly describe mechanism that anyone can trigger external calls as NFTPair contract and its address should not be used for any checks.
Low
Function NFTPair.removeCollateral allows deleting a loan and removing collateral by borrower if the loan was not received or by lender as a part of liquidation process. It is possible to pass as an argument address of to account that will receive the removed collateral. If the value of to is an address of contract that does not support handling ERC721 tokens the NFT will be lost and it will be not possible to recover it.
Manual Review / VSCode
It is recommended to use safeTransferFrom that checks if the address that should receive NFT is able to handle ERC721 tokens.
Low
Multiple functions of NFTPair contract do not check for zero addresses which might lead to loss of funds, failed transactions and can break the protocol functionality.
to address - https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L209bentoBox_ address - https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L169asset address - https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L177-L178newFeeTo address - https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L728Manual Review / VSCode
It is recommended to add zero address checks for listed parameters.
Low
Contract NFTPair in multiple functions does not follow checks-effects-interactions pattern which might lead to reentrancy attacks and potential loss of funds.
Vulnerable functions:
_requestLoan triggered through requestLoan_lend triggered through lendwithdrawFeesManual Review / VSCode
It is recommended to follow checks-effects-interactions pattern or add openzeppelin's reentrancy guard to protect against reentrancy attacks.
Non-Critical
Contract NFTPair uses old solidity version 0.6.12. Using the latest versions might make contracts susceptible to undiscovered compiler bugs.
Manual Review / VSCode
It is recommended to use newer solidity version such as 0.8.10.
Non-Critical
Event LogRemoveCollateral is missing indexing for parameter address recipient.
Manual Review / VSCode
It is recommended to add indexed into address recipient parameter.
Non-Critical
Function NFTPair.withdrawFees allows withdrawing fees. Even if the number of _shares is 0 and there is no transfer still event LogWithdrawFees is emitted which might confuse off-chain monitoring applications.
It is recommended not to emit LogWithdrawFees if the value of _shares is 0.
Non-Critical
Function NFTPair.calculateInterest checks if final interest fits into 128bit value. Following if statement is implemented:
if (interest >= 2**128) {
It is recommended to use type(int256).max for comparison instead of magic number 2*128.
🌟 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
61.6553 MIM - $61.66
Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.
Manual Review / VSCode
It is recommended to cache array length outside of loop
uint256 length = actions.length;
When dealing with unsigned integer types, comparisons with != 0 are cheaper that with > 0.
Manual Review / VSCode
It is recommended to use != 0 instead of > 0:
if(_share != 0) { (..) }
Shortening revert strings to fit in 32 bytes will decrease gas costs for deployment and gas costs when the revert condition has been met.
Manual Review / VSCode
It is recommended to decrease revert messages to maximum 32 bytes.
++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration).
Manual Review / VSCode
It is recommended to use ++i instead of i++ to increment value of an unsigned integer variable.
Function NFTPair.calculateInterest checks if final interest fits into 128bit value. Following if statement is implemented:
if (interest >= 2**128) {
Statement performs unnecessary calculations 2**128 which can be simply replaced with type(int256).max.
Manual Review / VSCode
It is recommended to use type(int256).max for comparison instead of magic number 2*128.
If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for addresses). Explicitly initializing it with its default value is an anti-pattern and waste of gas.
As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with for (uint256 i; i < numIterations; ++i) {.
Manual Review / VSCode
It is recommended to remove explicit initializations with default values.
#0 - cryptolyndon
2022-05-14T01:15:50Z
Seen, thanks