NFT Lending Protocol

With the removeLendingDeskLoanConfig function, a lending desk owner is able to remove the lendingDeskLoanConfig for loans that are still active. As a result the lending owner is able to prevent ERC1155 loans from being closed as nftCollectionIsErc1155 would be false for that lending desk and collection.

Consequently, the makeLoanPayment function errantly attempts to treat ERC1155 tokens as ERC721 tokens and ultimately reverts.

The lending desk owner can then subsequently add the correct lendingDeskLoanConfig back with the setLendingDeskLoanConfigs function only after the loan has expired and the owner can now claim the borrower’s collateral.

Do not read from the lendingDeskLoanConfigs mapping in the makeLoanPayment function, instead add an additional nftCollectionIsErc1155 boolean on the Loan struct and rely on that cached value to determine how to handle the transferring of collateral.

Similarly, do not rely on the lendingDeskLoanConfigs mapping in the liquidateDefaultedLoan function, as the config may no longer be present. Instead rely on the new nftCollectionIsErc1155 boolean that will be stored on the Loan struct.

Magnify Team: The issue was resolved in PR#230.

Each lending desk has a LoanConfig per nftCollection address, which contains the details about the minimum and maximum interest charged to the borrower.

A malicious desk owner can front-run the initializeNewLoan call from the borrower, and change the loan configuration with a large interest rate and a small duration, making the user pay more interest than they expected to pay when the new loan transaction was originated.

Currently, there is no validation for maxInterest besides maxInterest>=minInterest. If the desk owner sets the interest the max allowed interest type(uint32).max = 4294967295 and configures the other Loan params to have constant interest, the borrower Loan interest will be set to minInterest chosen by desk owner. This means that the borrower will pay around 4900% per interest per hour and there is a minimum of 1 hour wait to repay the loan.

In summary, a malicious lender can set a small interest rate to honeypot borrowers, update the loan configuration before their transaction is initialized, and lock the borrower in at the max interest rate for at least an hour which they must pay.

Add an extra parameter to the initializedNewLoan function, where the borrower can set a maxInterestAllowed, which will act as a limit on what they are willing to pay.

Magnify Team: The issue was resolved in PR#238.

In the makeLoanPayment function, the lendingDesk.erc20 token is transferred directly to the lender address: IERC20(lendingDesk.erc20).safeTransferFrom(msg.sender, lender, _amount);

Therefore any lender that is blacklisted for the payment token will prevent the user from making loan payments. The lender will then force the user to be liquidated as they cannot pay back their loan in time.

Furthermore, in the case of tokens with hooks after transfers, like ERC777, the receiver can make the transfer revert, preventing the borrower to make any payments to the loan as well.

Do not push the lendingDesk.erc20 tokens directly to the lender address, instead increment a uint256 value in a mapping for an individual erc20 token and allow lenders to claim this amount with a separate function (pull-over-push pattern).

Magnify Team: The issue was resolved in PR#258.

When initializing a new loan, the user will pass both _duration and _amount parameters. If both amount and duration are variable, the interest should be calculated based on scaling both duration and amount.

The issue arises due to the inherent rounding down behavior in Solidity. When calculating the average amount and duration, they are rounded down to 0. After multiplying with (loanConfig.maxInterest - loanConfig.minInterest), the result is 0. Consequently, the interest is always set as loanConfig.minInterest, resulting in the lender missing out on potential additional interest due to rounding down.

• Example: If minAmount = 100, maxAmount = 200, and _amount=150
• (_amount - loanConfig.minAmount) / (loanConfig.maxAmount - loanConfig.minAmount)
• (150 - 100) / (200 - 100) = 50 / 100 = 0
• The same happens with duration.

Calculate in the following manner to prevent rounding down:

interest =
  loanConfig.minInterest +
  uint32(
      (// Take average of amount and duration factors
      (((_amount - loanConfig.minAmount) * (loanConfig.maxInterest - loanConfig.minInterest)) /
(loanConfig.maxAmount - loanConfig.minAmount)) +
       (((_duration - loanConfig.minDuration) * (loanConfig.maxInterest - loanConfig.minInterest)) /
(loanConfig.maxDuration - loanConfig.minDuration))
       ) / 2
  );

Magnify Team: The issue was resolved in PR#239.

Since the hoursElapsed is rounded down in the getLoanAmountDue function and the loan.duration check uses strictly greater than, loan payments are only disabled an entire hour after the end date of a loan.

Therefore during this time a borrower may still pay back their loans and may frontrun the liquidation tx to do so.

Alter the hoursElapsed > loan.duration check to be hoursElapsed >= loan.duration

Magnify Team: The issue was resolved in PR#232.

In the setLoanOriginationFee function the _loanOriginationFee basis points value is intended to be capped at a maximum of 10%, however the validation asserts that the _loanOriginationFee is less than 10_000, which represents 100% in basis points.

Validate that the _loanOriginationFee value is less than 1_000, rather than less than 10_000.

Magnify Team: The issue was resolved in PR#240.

.

When the owner pauses the NFTYFinanceV1 contract, borrowers cannot repay their loans and therefore may be forced to default and lose their NFT collateral.

Consider allowing the makeLoanPayment function to be called when the protocol is paused.

Magnify Team: The issue was resolved in PR#257.

In the setLoanOriginationFee function there is not validation that the _loanOriginationFee is not 0, therefore the platformFee that is taken from new loans can be 0 when the loanOriginationFee is set to 0.

Some ERC20 tokens choose to revert upon transferring a 0 amount, however there is no check that the platformFee is nonzero before attempting to transfer this amount to the platformWallet.

In the initializeNewLoan function, only attempt to transfer the platformFee to the platformWallet if the platformFee is nonzero.

Magnify Team: The issue was resolved in PR#241.

In the initializeNewLoan function a promissoryNote is minted to the lender using the INFTYERC721V1.mint function, which relies on safeMint. As a result if the lender address is home to a contract, the onERC721Received function will be invoked at that address.

The contract at the lender address may have malicious logic implemented for the onERC721Received function to waste the borrower’s gas, causing a loss of native tokens for the user.

Consider using _mint rather than _safeMint for the mint implementation in the NFTYERC721V1 contract so that borrowers cannot be exposed to gas griefing.

Magnify Team: The issue was resolved in PR#253.

If the block.timestamp is a few seconds before the beginning of a new hour and User A sends a tx to pay back their full loan amount, the lender may stuff blocks on the network until the next hour begins. As a result, the borrowers debt to be repaid will increase and the borrowers tx will no longer close the loan.

Immediately the borrower will have to pay an additional period of interest. More insidiously, the borrower may not realize that the loan remains open and as a result may be unexpectedly liquidated.

Consider allowing users to pass a boolean indicating whether they would like to repay the full amount, rather than always specifying a particular amount to pay back. This way the transaction will close the loan regardless of when the transaction is recorded.

Magnify Team: The issue was resolved in PR#242.

In the liquidateDefaultedLoan function the loan status is assigned to Defaulted upon liquidation, however the comment on line 843 suggests that the loan state is assigned to resolved.

Update the comment to indicate that the loan.status will be assigned to LoanStatus.Defaulted rather than LoanStatus.Resolved.

Magnify Team: The issue was resolved in PR#229.

The function burn has the same NatSpec from the mint function.

Update the documentation to reflect the burn function accurately.

Magnify Team: The issue was resolved in PR#243.

The function setLendingDeskLoanConfigs is missing a check for empty loan config. Therefore it is possible to pass an empty _loanConfigs array but the LendingDeskLoanConfigsSet event will still be emitted.

Add a check to ensure _loanConfigs.length > 0

Magnify Team: Acknowledged.

The NFTYERC721V1 contract, which is the base contract for NFTYLendingKeysV1, NFTYObligationNotesV1 and NFTYPromissoryNotesV1, has a function setNftyFinance which allows the owner to update the nftyFinance address.

The main goal is to have the correct nftyFinance address set is to prevent unauthorized access to mint and burn function, meaning, only that contract is authorized to call them. In the scenario where the owner updates this address, existing loan desks and loanIds in the NFTFinanceV1 contract will be stuck, as the every call to burn will now fail, due to the onlyNftyFinance modifier.

Avoid updating nftyFinance when there are active loans.

Magnify Team: Acknowledged.

In the initializeNewLoan function the interest calculations include casting a uint256 to a uint32. These calculations should always be safe and avoid overflow, however as a best practice it would be prudent to use OpenZeppelin’s SafeCast library to perform these casts.

Consider implementing SafeCast for these uint32 casts.

Magnify Team: Resolved.

Interest on loans is charged on the original loan amount even if some of the loan principle has been repaid.

Be sure to clearly document this behavior to users.

Magnify Team: Acknowledged.

Throughout the NFTYFinanceV1 contract external calls are made without regard to state updates, the following rules ought to be followed:

• safeTransferFrom should occur first in functions to avoid making an external call via callback

tokens after accounting has been updated but before funds have been received.

• Other external calls should occur after all state updates have occurred.

Implemented the above suggestions, as seen in this PR:

a Guardian proof of concept

Magnify Team: The issue was resolved in PR#259.

The Magnify Cash contracts are configured to user solidity 0.8.22 and higher, these versions of the EVM compiler make use of the PUSH0 opcode which is not supported by all EVM compatible chains.

The immediate deployment target of Ethereum Mainnet is safe as this network supports the PUSH0 opcode, however the team should be wary of PUSH0 support as they deploy to new EVM compatible networks.

Before deploying to a new target chain, be sure to check whether the chain supports the PUSH0 opcode, and if it does not consider reducing the compiler version to < 0.8.20.

Magnify Team: Acknowledged.

Throughout the NFTYFinanceV1 contract the token transfer accounting assumes that the transferred amount is received, however this may not be the case for fee-on-transfer or rebase tokens.

Be sure to clearly document that the system is not compatible with fee-on-transfer tokens. Otherwise if they are intended to be supported then the amount of tokens actually received should be measured by a before and after balance check.

Magnify Team: Acknowledged.