Backed Protocol contest - Ruhum's results

Lines of code

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

Vulnerability details

Impact

A borrower specifies a minimum loan amount. The lender can provide a loan as high as they want. As long as it's higher than the minimum value. A value too high might cause the borrower to not be able to pay it back.

A higher loan means a higher facilitator fee and higher interest payments. Meaning there is a larger amount of tokens (in absolute terms) they have to pay back at the end of the loan.

This might cause the borrower to lose their collateral.

Proof of Concept

Let's say Alice wants to borrow 5 ETH. Bob finalizes the loan with 50 ETH.

If there's a 1% facilitator fee, Alice receives 5 ETH * 0.99 = 4.95 ETH or 50 ETH * 0.99 = 49.5

Alice wants to repay the loan 6 months later. The interest should be:

perAnumInterestRate = 50 (5%)

5e18 * 262800 * (50 * 1e18 / 525600) / 1e21 = 1.25e17

or

50e18 * 262800 * (50 * 1e18 / 525600) / 1e21 = 1.25e18

So if she got a 50 ETH loan, she actually only received 49.5 ETH. When she has to repay it, she owes in total 51.25 ETH. That's 1.75 ETH out of her own pocket.

Now, if Alice is not able to get the 1.75 ETH she needs to repay the loan, she loses her collateral.

If she got the original 5 ETH she wanted, she only has to pay in total 5.075 ETH. That's only 0.125 ETH out of her own pocket. Way less.

By not being able to influence the final loan amount she risks not having enough liquidity to pay the final loan. Obviously, the lender would only lend up to a given amount (depending on the collateral) so this is a rather extreme example. But depending on the collateral it might happen.

Tools Used

none

Recommended Mitigation Steps

I'd argue that the loan amount should be fixed. The borrower should specify it. The lenders can then outbid each other on the duration and the interest.

Lines of code

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

Vulnerability details

Impact

When the lend asset is a token with fees on transfer, the facilitator fee is higher than it should be. The contract doesn't use the actual token amount it received but the one where the fees aren't subtracted yet. This causes the borrower to receive fewer tokens than they should.

Proof of Concept

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

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

If the token has fees on transfer the contract receives less than amount. By computing facilitatorTake with amount it results in a higher number than it really should.

Tools Used

Recommended Mitigation Steps

use the actual token amount:

uint prevBalance = ERC20(loanAssetContractAddress).balanceOf(address(this));
ERC20(loanAssetContractAddress).safeTransferFrom(msg.sender, address(this), amount);
uint newBalance = ERC20(loanAssetContractAddress).balanceOf(address(this));
uint actualBalance = newBalance - prevBalance;

Report

Use a two-step process for ownership transfer

Consider using a two-step process for transferring the ownership of a contract. While it costs a little more gas, it's safer than transferring directly.

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

Here's an example from the Compound Timelock contract: https://github.com/compound-finance/compound-protocol/blob/master/contracts/Timelock.sol#L45-L58

Use ERC721.safeTransferFrom consistently

When closing a lone, the contract doesn't use safeTransferFrom. In all the other places where an ERC721 token is transferred to a user-provided address, you do use it.

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