Platform: Code4rena
Start Date: 05/04/2022
Pot Size: $30,000 USDC
Total HM: 10
Participants: 47
Period: 3 days
Judge: gzeon
Total Solo HM: 4
Id: 106
League: ETH
Rank: 3/47
Findings: 3
Award: $2,966.75
🌟 Selected for report: 3
🚀 Solo Findings: 1
🌟 Selected for report: cmichel
Also found by: AuditsAreUS, IllIllI, Ruhum, csanuragjain, danb, joshie, t11s, tintin
293.89 USDC - $293.89
The loan amount is used as a min loan amount. It can be matched as high as possible (realistically up to the collateral NFT's worth to remain in profit) and the borrower has to pay interest on the entire amount instead of just on the desired loan amount when the loan was created.
lend with 350k USDC.The loan amount should not have min amount semantics. When someone wants to get a loan, they specify a certain amount they need, they don't want to receive and pay interest on more than that.
#0 - wilsoncusack
2022-04-06T12:09:43Z
The ability to increase the loan amount is seen as a feature of the protocol, not a bug
#1 - wilsoncusack
2022-04-06T20:02:38Z
#30
#2 - gzeoneth
2022-04-15T11:07:15Z
While a larger loan size is strictly beneficial to the borrower, the higher interest payment it entitled is not. The warden suggested a valid situation that may cost the user more than intended. Considering the amount lost is bounded because the lender carry more risk for a larger loan, downgrading this to Mid Risk for the sponsor to consider a maxLoanAmount parameter.
#3 - gzeoneth
2022-04-15T11:35:47Z
After considering #9 brining up the loan origination fee, I believe this is a high risk issue for the protocol to not have a maxLoanAmount parameter.
#4 - wilsoncusack
2022-04-16T01:43:31Z
After considering #9 brining up the loan origination fee, I believe this is a high risk issue for the protocol to not have a
maxLoanAmountparameter.
IMO it does not make sense to label this as high severity. This is not an exploit but is just the protocol working exactly as described in the README.
#5 - gzeoneth
2022-04-16T03:50:17Z
From README
Perpetual lender buyout: a lender can be boughtout at any time by a new lender who meets the existing terms and beats at least one term by at least 10%, e.g. 10% longer duration, 10% higher loan amount, 10% lower interest. The new lender pays the previous lender their principal + any interest owed. The loan duration restarts on buyout.
I don't agree that allowing higher loan amount necessarily mean the loan amount need to be unbounded. Given the increased interest and origination fee, a higher loan amount is not necessarily "beating existing terms" as described in the README.
#6 - wilsoncusack
2022-04-16T11:03:04Z
It certainly doesn't necessarily mean that but it is how we chose to implement it and I think the description is clear that there is no cap. We define "beating" as having one of those values changed by at least 10% and so I think it is beating as described by the readme.
Nonetheless, I appreciate your drawing focus again to this point (we discussed on twitter with our community during audit as this became a point of interest, and have of course considered this idea when designing the protocol at the outset). We will again consider adding a Boolean flag to each loan as to whether the borrower allows loan amount increases
#7 - wilsoncusack
2022-04-17T12:32:30Z
Respect judge to have final say, but since this is going public want to make sure our take on this is clear.
We believe the protocol design was clearly communicated in the README, including origination fee and the possibility for perpetually increasing loan amount. We think there is no "exploit" here, just people pointing out potential downsides to how the protocol is designed (as one might point out problems of impermanent loss with an AMM.) We view these as QA reports. We are interested in this feedback and listening to it in that we want to listen to potential users and make sure our protocol appeals to as many people as possible.
#8 - gzeoneth
2022-04-17T13:06:08Z
I consider this as an exploit because asset can be lost. Combining unbounded loan amount, interest rate and origination fee (max 5%) a malicious lender can grief borrower with limited risk and get a chance to seize the collateral as demonstrated in the POC.
The fact that the code is working as described in README is irrelevant if it is going to make user lose their asset. If this is going to stay as a protocol design, I recommend to clearly communicate the risk of unbounded loan amount which is lacking in the contest repo.
#9 - wilsoncusack
2022-04-21T17:31:17Z
Was resolved here https://github.com/wilsoncusack/backed-protocol/pull/75
🌟 Selected for report: cmichel
1843.3528 USDC - $1,843.35
The repayAndCloseLoan function does not revert if there has not been a lender for a loan (matched with lend).
Users should use closeLoan in this case but the contract should disallow calling repayAndCloseLoan because users can lose funds.
It performs a ERC20(loan.loanAssetContractAddress).safeTransferFrom(msg.sender, lender, interest + loan.loanAmount) call where interest will be a high value accumulated from timestamp 0 and the loan.loanAmount is the initially desired min loan amount minLoanAmount set in createLoan.
The user will lose these funds if they ever approved the contract (for example, for another loan).
Add a check that there actually is something to repay.
require(loan.lastAccumulatedTimestamp > 0, "loan was never matched by a lender. use closeLoan instead");
#0 - wilsoncusack
2022-04-06T12:06:14Z
ownerOf query here will fail if there is no lender https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L239
#1 - wilsoncusack
2022-04-06T15:03:00Z
actually this is wrong, we switched to solmate and this ownerOf will not fail. Is a legit issue
#2 - wilsoncusack
2022-04-06T18:09:53Z
not an attack, but funds can be loss some Medium probably makes sense?
#3 - wilsoncusack
2022-04-07T12:49:26Z
requires borrow to have approved the facilitator to move this erc20 and to call the wrong method
#4 - wilsoncusack
2022-04-14T16:29:58Z
Yooo just discovered solmate had not followed the ERC721 standard on this ownerOf and it should have reverted, updated here
https://github.com/Rari-Capital/solmate/commit/921a9ad4e22b995bd3d7b5464bcda294dd977209
#5 - gzeoneth
2022-04-15T13:07:57Z
Sponsor confirmed with fix
🌟 Selected for report: cmichel
Also found by: teryanarmen
829.5088 USDC - $829.51
Admins can update the origination fee by calling updateOriginationFeeRate.
Note that a borrower does not receive their minLoanAmount set in createLoan, they only receive (1 - originationFee) * minLoanAmount, see lend.
Therefore, they need to precalculate the minLoanAmount using the current origination fee to arrive at the post-fee amount that they actually receive.
If admins then increase the fee, the borrower receives fewer funds than required to cover their rent and might become homeless.
Reconsider how the min loan amount works. Imo, this minLoanAmount should be the post-fee amount, not the pre-fee amount. It's also more intuitive for the borrower when creating the loan.
#0 - wilsoncusack
2022-04-06T12:05:02Z
won't change, is just how it works
#1 - wilsoncusack
2022-04-08T00:26:39Z
from 111
The only true mitigation here would be to store originationFeeRate in the Loan struct at the time of origination to guarantee a borrower gets the fee rate that was present when they created the loan. But we do not plan to make this change
#2 - wilsoncusack
2022-04-13T01:52:03Z
#3 - wilsoncusack
2022-04-13T01:52:29Z
decided to fix because we could do so without too much gas
#4 - gzeoneth
2022-04-15T13:15:16Z
Sponsor confirmed with fix