Backed Protocol contest - TerrierLover'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#L262-L266

Vulnerability details

Impact

Lender cannot seize the collateral as expected which ends up simply providing token to the malicious NFT holder.

Proof of Concept

The issue could happen in this scenario:

(1) An attacker prepares a malicious NFT whose transferFrom and safeTransferFrom functions do nothing:

contract MaliciousERC721 is ERC721 {
	constructor() ERC721("Malicious", "MALICIOUS") {}

    .....

    function transferFrom(
        address from,
        address to,
        uint256 tokenId
    ) public virtual override {
        // Do nothing
    }

    function safeTransferFrom(
        address from,
        address to,
        uint256 tokenId
    ) public virtual override {
        // Do nothing
    }

(2) The attacker provides the malicious NFT using createLoan function in NFTLoanFacilitator.sol

Specifically, the attacker sets the malicious NFT contract at collateralContractAddress variable at createLoan function. transferFrom is called at in createLoan function, but transferFrom does not transfer the NFT correctly to the NFTLoanFacilitator contract.

IERC721(collateralContractAddress).transferFrom(msg.sender, address(this), collateralTokenId);

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

(3) A victim lends the token to the attacker using lend function in NFTLoanFacilitator.sol without noticing the issue of malicious NFT. The attacker can receive the token without any issues.

(4) The attacker can just do nothing, and they do not have any worries of being their malicious NFT taken.

Tools Used

Static code analysis

In addition, I checked that the above mentioned scenario can happen using test script.

Recommended Mitigation Steps

As a temporary fix, the contract should ensure that the transfer truely happened using balanceOf function of the NFT. But even though this check exists, there are various ways that the attacker can take such as followings:

• Update the malicious NFT contract's balanceOf function so that it returns wrong value
• Use the upgradeablility contracts t the malicious NFT, and make it look that it does harmless first, but updates the contract to contain malicious codebase

Hence, it may be worth considering the whitelists of NFTs which can be used at createLoan function.

If lending money to the malicious NFT is all users' faults, this may not be a high pri issue.

Using custom errors can reduce the deployment gas cost

Target codebase

In these contracts, it uses require function with string message. https://github.com/code-423n4/2022-04-backed/blob/d34ddbdaf8d1bc1bf17446df830db629ee551308/contracts/BorrowTicket.sol https://github.com/code-423n4/2022-04-backed/blob/d34ddbdaf8d1bc1bf17446df830db629ee551308/contracts/LendTicket.sol https://github.com/code-423n4/2022-04-backed/blob/d34ddbdaf8d1bc1bf17446df830db629ee551308/contracts/NFTLoanFacilitator.sol https://github.com/code-423n4/2022-04-backed/blob/d34ddbdaf8d1bc1bf17446df830db629ee551308/contracts/NFTLoanTicket.sol

Using custom errors can reduce the gas deployment cost so the team can consider using it.

Potential improvement

Use custom errors instead of require function with string message.

This is an example of just using the pattern of custom errors and revert at notClosed modifier in NFTLoanFacilitator.sol, but it can reduce deployment gas cost and contract size.

error LoanClosed();

// ==== modifiers ====

modifier notClosed(uint256 loanId) { 
    if (loanInfo[loanId].closed) {
        revert LoanClosed();
    }
    _; 
}

Deployment gas improvement

Contract size change

Just using the pattern of custom errors and revert at notClosed modifier in NFTLoanFacilitator.sol, but the above mentioned improvements can be seen. If the all of the codebase uses this patten, it would reduce lots of gas deployment cost and contract size.

---

Using != 0 instead of > 0 can reduce the deployment gas a bit at NFTLoanFacilitator.sol

Target codebase

https://github.com/code-423n4/2022-04-backed/blob/d34ddbdaf8d1bc1bf17446df830db629ee551308/contracts/NFTLoanFacilitator.sol#L198 https://github.com/code-423n4/2022-04-backed/blob/d34ddbdaf8d1bc1bf17446df830db629ee551308/contracts/NFTLoanFacilitator.sol#L321

Potential improvement

Use use != 0 instead of > 0.

if (amountIncrease != 0) {

require(_improvementRate != 0, 'NFTLoanFacilitator: 0 improvement rate');

Deployment gas improvement

Confirmed that the deployment gas cost of NFTLoanFacilitator.sol slightly reduced

---

Using unchecked at updateOriginationFeeRate.sol can reduce the gas cost and

Target codebase

https://github.com/code-423n4/2022-04-backed/blob/d34ddbdaf8d1bc1bf17446df830db629ee551308/contracts/NFTLoanFacilitator.sol#L307

Potential improvement

Sinve INTEREST_RATE_DECIMALS is a fixed value, it is clear that the calculation will not overflow.

unchecked {
    require(_originationFeeRate <= 5 * (10 ** (INTEREST_RATE_DECIMALS - 2)), "NFTLoanFacilitator: max fee 5%");
}

Gas change

Confirmed that the gas cost is reduced for NFTLoanFacilitator.sol.

---

Target codebase

Potential improvement

Deployment gas improvement

Contract size change

---

facilitatorTake variable is not needed in NFTLoanFacilitator.sol

Target codebase

https://github.com/code-423n4/2022-04-backed/blob/d34ddbdaf8d1bc1bf17446df830db629ee551308/contracts/NFTLoanFacilitator.sol#L156

uint256 facilitatorTake = amount * originationFeeRate / SCALAR;
ERC20(loanAssetContractAddress).safeTransfer(
    IERC721(borrowTicketContract).ownerOf(loanId),
    amount - facilitatorTake
);

facilitatorTake is only used once, so this is no need to be defined.

Potential improvement

ERC20(loanAssetContractAddress).safeTransfer(
    IERC721(borrowTicketContract).ownerOf(loanId),
    amount - amount * originationFeeRate / SCALAR
);

Gas improvement

Both method and deployment gas cost decreased.

Contract size change

Slight contract size reduction is confirmed.

---