Backed Protocol contest - berndartmueller's results

Lines of code

https://github.com/code-423n4/2022-04-backed/blob/main/contracts/NFTLoanFacilitator.sol#L88 https://github.com/code-423n4/2022-04-backed/blob/main/contracts/NFTLoanFacilitator.sol#L124

Vulnerability details

Impact

While the contracts use in most places safeTransferFrom() to transfer NFTs, there are a few cases where the unsafe counterpart transferFrom() is used.

safeTransferFrom checks that contract recipients are aware of the ERC721 protocol to prevent tokens from being forever locked.

Proof of Concept

NFTLoanFacilitator.createLoan() NFTLoanFacilitator.closeLoan()

Tools Used

Manual review

Recommended mitigation steps

Consider using safeTransferFrom() consistently.

As the NFTLoanFacilitator contract is receiving NFTs itself, the contract has to implement the interface ERC721TokenReceiver. To prevent reentrancy, consider adding a reentrancy guard to both functions as a safety precaution in case any other functionality is added later which might make this exploitable.

QA Report

Non-Critical Findings

None

Low Risk

Zero-address check is missing

Description

Zero-address checks are a best-practice for input validation of critical address parameters.

Impact: If _nftLoanFacilitator is set to address(0), it will break the contract and deployment of a new contract is necessary.

Findings

NFTLoanTicket.constructor()#_nftLoanFacilitator

Recommended mitigation steps

Add zero-address check:

require(address(_nftLoanFacilitator) != address(0), "Zero address");

Use _safeMint() instead of _mint()

Description

By using _mint() a minted NFT can get stuck in a recipients contract if the contract not designed to handle them. safeMint() checks whether the recipient contract can handle ERC721 tokens.

Findings

NFTLoanTicket.mint()

Recommended mitigation steps

Its is recommended to use _safeMint whenever possible.

_safeMint(to, tokenId);