Core Contracts

Summary

In the LendingPool.sol the users can deposits NFTs and can get a loan or borrow crvUsd for that NFT. If the value of the NFTs decrease or say their worth goes down to the limit of the liquidationThreshold i.e 80% initally, meaning that the user gets below a healthFactor that he was supposed to maintain. In that case the user who deposited the NFTS as collateral will get liquidated and his NFTs will be taken in return for not maintaing the required healthFactor.

Vulnerability Details

Suppose their is a user who deposits NFTs worth 1000e18 usd collectively and he borrows CrvUsd for that given collateral(NFTs) and after some time the value of his deposited NFTs go down below the required healthFactor and now someone calls initiateLiquidation to start the liquidation process. Now the user who is being set for liquidation will have a gracePeriod in which he can repay his borrowed amount and clear the debt that he had. if the user does this he can call the closeLiquidation function and if doesnt, the finalizeLiquidation function will be called and the user's NFT will be taken by stabilityPool.

However, A malicious user can get away or avoid being liquidated, if not that, he can definitely exploit the caller of the finalalizeLiquidation in paying for a huge amount of gas for no reason. Lets see how this can take place and because of what. Example- Suppose this user who deposited NFTs as collat and his got the below the helathFactorThreshold the liquidation will be initiated via initiateLiquidation function and the gracePeriod which his given passed away. Now the finalizeLiquidation will be called and what this function does is validates all the required fields like if the user is under liquidation or not and if the gracePeriod has passed away etc. and after that whats done in the function is the main cause for this issue as what the function does after the input validation is it loops through all the NFTs that the user has deposited as collateral and then transfers them to the msg.sender i.e the stabilityPool.sol contract. The issue is that the function loops through those arrays and mapping of via storage of the LendingPool as can be seen here. Now due to this the user can exploit or perform a gas grief attack, what he will do is that he will see the transaction of the finalizeLiquidation bieng called in the mempool and will front-run that tx by calling depositNFT and in that function he can deposit a huge like a massive amount of NFTs which then are stored in this struct of the LendingPool contract's storage that finalizeLiquidation function uses to loop through the NFTs Note that these can any NFTs regardless of their worth, maybe just useless for the user, and this will be possible because there is no maximum cap that the user is restricted with, so anyone can deposit any amount of NFTs whether it be totally worthless. Now as it is obvious that the function loops through the storage and for every storage read the amount of gas that is consumed is always expensive(the minimum is 100) for every SLOAD and when the finalizeLiquidation will execute, it will have to loop through and make storage reads for perse a lot of trash that the attacker deposited via front running and it might throw an out gas revert too resulting in user not getting liquidated.

Impact

The impact here will be pretty decent as the user can get avoid liquidation if the out gas revert is thrown and secondly even if the transation to finalizeLiquidation passes the amount of gas that the StabilityPool will end up paying will be very expensive and noone would wanna pay that much amount of money for some insanely huge for loop

Tools Used

Manual Review

Recommendations

Two things should be and can be done here: 1. In the finalizeLiquidation function instead of reading/loop through the storage, create a memory copy of that struct and then perform the for loop to avoid the out of gas revert. Secondly, The protocol can enforce a maximum cap to the amount of NFTs a user can deposit for eg: 20-30

Code Snippets