Backed Protocol contest - t11s's results

Lines of code

https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L162-L224

Vulnerability details

Impact

Lenders are allowed to "buy out" another lender on a position via the loan function.

https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L162-L224

This is supposed to be a purely positive sum action for the borrower, as the new lender must provide "better" terms than their predecessor, as checked here:

https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L171-L179

However, in practice, this mechanic appears fundamentally flawed for a number of reasons:

Extending a loan's duration is not necessarily positive sum

By increasing the duration of a loan, the lender is also increasing the amount of interest the borrower must pay to keep their NFT at the end of the loan. If a borrower created a loan only expecting to have to pay $100 in interest over 12 months, if another lend came and extended the loan by 100 years, they'd now have to pay $10100 to keep their NFT. Even if the borrower noticed this and went to close their loan, this is a potential griefing vector, as the lender can keep extending their loan and force the borrower to pay gas each time to close it. There is a low cost for the attacker if the loan amount is low, as with a long enough duration the interest paid can outpace the principal.

Increasing a loan's amount is not necessarily positive sum

For most of the same reasons above, the ability to increase the amount being loaned to a user at any time is dangerous. A malicious lender could 10x a user's loan amount, forcing them to pay more interest to keep their NFT or risk having it seized. Sure the borrower could just default on purpose and keep the tokens, but if the NFT is precious for non monetary reasons or they are simply unaware the borrower could unintentionally wind up paying far more interest then they expected when they created their loan. Once again, even if the borrower is aware of the increased amount and goes to close their loan, this opens up a griefing vector as described above.

Recommended Mitigation Steps

Either:

• Remove the ability to extend a loan's duration or amount entirely (allowing a new lender to propose a lower borrowing rate appears safe)
• Allow the borrower to set limits on the max duration and amount of the loan
• Require the lender's approval before their loan can be bought out on new terms

Or a implement a combination of two or more of these options.

Lines of code

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

Vulnerability details

Impact

Backed uses solmate's SafeTransferLib to safely transfer non-standard ERC20 tokens:

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

Notably, SafeTransferLib does not revert if the token being transferred has no code at all:

https://github.com/Rari-Capital/solmate/blob/4eaf6b68202e36f67cab379768ac6be304c8ebde/src/utils/SafeTransferLib.sol#L9

This means transferring address(0) as a token will succeed, which could confuse users. As a result, Backed checks that the token is not address(0) on this line:

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

However, this check is insufficient to protect users from accidentally creating loans with invalid tokens, as any address without code will be accepted as a valid token. One particularly consequential example of how this could go wrong for users would be inputting the address of a common token like DAI or USDC for another network on accident, but having the loan creation succeed as if it was the correct token. See the proof of concept for more details:

Proof of Concept

• Alice wants to create a DAI loan on Ethereum
• Alice googles the DAI address and mistakenly copies the address of DAI on Optimism
• Alice creates a the loan using Optimistic DAI and it succeeds
• Alice is annoyed and confused why no one is lending to her
• Alice closes the loan in anger and does not return to use Backed protocol

Recommended Mitigation Steps

Rewrite the require like so:

   require(loanAssetContractAddress.code.length != 0, "NFTLoanFacilitator: invalid loan");

Inefficient safe cast

A safe cast check is done here:

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

The check uses <=, which compiles down to 2 opcodes (ISZERO, GT). This check can be done in a single opcode like so:

require(accumulatedInterest < 1 << 128, "NFTLoanFacilitator: accumulated interest exceeds uint128");

because 1 << 128 is equivalent to 2 ** 128 which is equal to type(uin128).max + 1

Unnecessary ownerOf check

A check that the from address equals ownerOf[id] is performed here:

https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/LendTicket.sol#L32

This is unnecessary as the only place where the internal _transfer is ever called (via the access controlled public version: loanFacilitatorTransfer) will only ever pass the owner of the token as ensured by this line:

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

Therefore the check can be removed to save an SLOAD.