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: 14/47
Findings: 2
Award: $361.37
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: cmichel
Also found by: AuditsAreUS, IllIllI, Ruhum, csanuragjain, danb, joshie, t11s, tintin
293.89 USDC - $293.89
https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L129-L226 https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L230-L250
Attackers can listen for a borrower to call repayAndCloseLoan on a specific loanId, and frontrun their transaction with a call to lend, creating a new loan with an increased amount, causing the borrower's transaction to fail due to the new loanAmount being greater than initially anticipated.
This can cause many issues: a borrower may repeatedly find that they cannot repay and close their loan, and while the duration of the loan is extended on takeover, the borrower must keep retrying and/or try again later if they do not want to default on the loan.
If the underlying collateral is valuable enough (either originally in value, or it appreciated), an attacker would be motivated to continuously prevent the loan from being closed in an attempt to seize the collateral.
If the duration of the loan, minDurationSeconds, is low enough, the attacker can simply takeover the loan with a 10% more amount, and hope that the borrower will fail to repay and close the new loan that now has a new deadline so that they can call seizeCollateral.
A dedicated attacker can continue to deny the borrower from repaying their loan indefinitely. If the borrower is a regular user, they likely will not be able to keep up and end up defaulting.
A simple test suite can be written where the borrower calls repayAndCloseLoan on their outstanding loan, an attacker can see that txn in the mempool and submit a higher-priority one calling lend with 10% more amount, causing the call to repayAndCloseLoan to revert due to insufficient balance. If the borrower does not repay the loan before the new deadline, the attacker can call seizeCollateral and receive the collateralized NFT.
Manual code review, vscode
Front-running mitigation methods such as decay periods (durations after which certain txns are not allowed / conditions are changed) could be helpful.
#0 - wilsoncusack
2022-04-07T23:38:06Z
Don't think we plan to fix this, part of how the protocol works
#1 - gzeoneth
2022-04-15T11:56:07Z
Valid POC with fairy high probability of user losing collateral in the low duration scenario.
#2 - wilsoncusack
2022-04-16T01:19:53Z
Valid POC with fairy high probability of user losing collateral in the low duration scenario.
The loan can be repaid early, so the issue is to do with the 1% origination fee. Otherwise the improvement tx goes through and the borrower repays as normal, anyway. Only an issue when loan amount is increased. When they are trying to repay, attacker loans more. Borrower gets amount sub origination fee but owes total amount. Can be significant at large values.
IMO this is then the same as the issues for the loan amount being unbounded. There is nothing unique about the last minute nature of this attack? If someone wanted to prevent it via a large loan amount they could do so at any time? In general I doubt lenders will often being willing to risk the amounts required to make the origination fee difference so significant the borrower can't repay.
#3 - gzeoneth
2022-04-16T04:21:44Z
Valid POC with fairy high probability of user losing collateral in the low duration scenario.
The loan can be repaid early, so the issue is to do with the 1% origination fee. Otherwise the improvement tx goes through and the borrower repays as normal, anyway. Only an issue when loan amount is increased. When they are trying to repay, attacker loans more. Borrower gets amount sub origination fee but owes total amount. Can be significant at large values.
IMO this is then the same as the issues for the loan amount being unbounded. There is nothing unique about the last minute nature of this attack? If someone wanted to prevent it via a large loan amount they could do so at any time? In general I doubt lenders will often being willing to risk the amounts required to make the origination fee difference so significant the borrower can't repay.
The last minute variant significantly reduce the attacker's cost of attack. For example if a borrower repay 5 minutes before the deadline and can sign a tx every 1 minute, the attacker only need to risk (1+10%)^5-1 = 61% of the loan amount for the attack. Given the typical collateral ratio of NFT loan the risk is minimal. This issue is un-related to the loan amount bound because it is still possible to happen if it is bounded. I suggest preventing any buyout attempt X minutes before deadline.
#4 - wilsoncusack
2022-04-16T11:05:46Z
Every buyout restarts the loan duration? And buyouts do not prevent repayment? If a buyout without loan increases and a repayment are in the same block the repayment is unaffected: the amount required to repay is the exact same.
Am I missing something @gzeoneth?
#5 - gzeoneth
2022-04-16T11:19:45Z
Every buyout restarts the loan duration? And buyouts do not prevent repayment? If a buyout without loan increases and a repayment are in the same block the repayment is unaffected: the amount required to repay is the exact same.
Am I missing something @gzeoneth?
I am referring to a buyout that increase loan amount which will not restart the loan duration. When the loan amount increased, repayment can be affected. I believe having the loan amount unbounded is insufficient to mitigate the issue, it would need to be fixed. As I mentioned, the attacker only need 61% buffer to launch an attack if the borrower repay in the last 5 minutes.
#6 - gzeoneth
2022-04-16T11:45:05Z
I made a mistake during the initial triage which sponsor clarified loan duration will always increase upon buyout because lastAccumulatedTimestamp will reset in L192. Changing this to duplicate of #24.
🌟 Selected for report: IllIllI
Also found by: 0v3rf10w, 0x1f8b, 0xDjango, 0xkatana, BouSalman, CertoraInc, Dravee, FSchmoede, Hawkeye, Kenshin, Meta0xNull, PPrieditis, Ruhum, TerrierLover, VAD37, WatchPug, berndartmueller, csanuragjain, hake, horsefacts, hubble, m9800, rayn, reassor, robee, samruna, securerodd, shenwilly, sorrynotsorry, t11s, teryanarmen, tintin, z3s
67.476 USDC - $67.48
Remove require check for loanAssetContractAddress != address(0) in lend():144 and add the check against creating a loan with the 0 address in the createLoan() function instead. More efficient since the loanAssetContractAddress is being read from storage in lend().
The code on lines 167 to 179 of this contract do not need to be in brackets
There is no check that loanId exists when the lend() function is called. Requiring it to be <= _nonce could work. While a user could lend to a non-instantiated loan, the require below it checking the loanAssetContractAddress != address(0) would end up revert.
Casting block.timestamp to uint40 may increase gas costs unnecessarily, consider removing unless there is a specific reason for restricting its precision.
#0 - wilsoncusack
2022-04-07T00:31:20Z
loanAssetContractAddress != address(0). Less than _nonce is not good because id 0 is not used.