Description
While not accepting partial repayments within LendingPool::closeLiquidation is certainly a design choice, it also causes an edge case in which a user who is subject into liquidation can still deposit additional NFTs into the protocol, which would then be liquidated as well, causing an unjustified liquidation on an NFT value way over anything justifiable.
Vulnerable Code & Details
LendingPool::closeLiquidation:
function closeLiquidation() external nonReentrant whenNotPaused {
address userAddress = msg.sender;
if (!isUnderLiquidation[userAddress]) revert NotUnderLiquidation();
ReserveLibrary.updateReserveState(reserve, rateData);
if (block.timestamp > liquidationStartTime[userAddress] + liquidationGracePeriod) {
revert GracePeriodExpired();
}
UserData storage user = userData[userAddress];
uint256 userDebt = user.scaledDebtBalance.rayMul(reserve.usageIndex);
if (userDebt > DUST_THRESHOLD) revert DebtNotZero();
isUnderLiquidation[userAddress] = false;
liquidationStartTime[userAddress] = 0;
emit LiquidationClosed(userAddress);
}
LendingPool::depositNFT:
function depositNFT(uint256 tokenId) external nonReentrant whenNotPaused {
ReserveLibrary.updateReserveState(reserve, rateData);
if (raacNFT.ownerOf(tokenId) != msg.sender) revert NotOwnerOfNFT();
UserData storage user = userData[msg.sender];
if (user.depositedNFTs[tokenId]) revert NFTAlreadyDeposited();
user.nftTokenIds.push(tokenId);
user.depositedNFTs[tokenId] = true;
raacNFT.safeTransferFrom(msg.sender, address(this), tokenId);
emit NFTDeposited(msg.sender, tokenId);
}
After looking at the code above consider the following scenario:
User Bob holds an NFT within RAAC worth 50k USD and loaned 40k USD against it within the protocol in anticipation of it's value increasing over time.
Now RAAC decides to drop some new Condos on the market for an early investment price of only 60k USD each, so bob decides to use his life savings to buy 4 of those units for this amazing deal, after all, he can deposit them back into the contract and borrow against it.
Now right before Bob deposits his newly bought assets RAACs price oracle updated the price on Bobs previous unit to 40k USD and initiates liquidation of Bob, since he is now undercollateralized.
Bob though deposits unknowingly his newly acquired NFTs into the contract to borrow against them only to realize that he is currently being liquidated. Having spent his remaining savings on those condos, Bob now does not have sufficient money to repay the old debt and will be liquidated with a Health Factor way above 1e18, losing all his investment.
PoC
Since the PoC is a foundry test I have added a Makefile at the end of this report to simplify installation for your convenience. Otherwise if console commands would be prefered:
First run: npm install --save-dev @nomicfoundation/hardhat-foundry
Second add: require("@nomicfoundation/hardhat-foundry"); on top of the Hardhat.Config file in the projects root directory.
Third run: npx hardhat init-foundry
And lastly, you will encounter one of the mock contracts throwing an error during compilation, this error can be circumvented by commenting out the code in entirety (ReserveLibraryMocks.sol).
And the test should be good to go:
To run the following PoC please apply a "fix" into StabilityPool as follows:
StabilityPool::liquidateBorrower
/// *** SNIP *** ///
+bool approveSuccess = crvUSDToken.approve(address(lendingPool), type(uint256).max);
-bool approveSuccess = crvUSDToken.approve(address(lendingPool), scaledUserDebt;
/// *** SNIP *** ///
As mentioned in a previous report, this approval issue makes liquidating users impossible.
./test/invariant/foundry/PoC.t.sol
pragma solidity ^0.8.0;
import {Test, console} from "forge-std/Test.sol";
import {StabilityPool} from "../../contracts/core/pools/StabilityPool/StabilityPool.sol";
import {LendingPool} from "../../contracts/core/pools/LendingPool/LendingPool.sol";
import {CrvUSDToken} from "../../contracts/mocks/core/tokens/crvUSDToken.sol";
import {RAACHousePrices} from "../../contracts/core/oracles/RAACHousePriceOracle.sol";
import {RAACNFT} from "../../contracts/core/tokens/RAACNFT.sol";
import {RToken} from "../../contracts/core/tokens/RToken.sol";
import {DebtToken} from "../../contracts/core/tokens/DebtToken.sol";
import {DEToken} from "../../contracts/core/tokens/DEToken.sol";
import {RAACToken} from "../../contracts/core/tokens/RAACToken.sol";
import {RAACMinter} from "../../contracts/core/minters/RAACMinter/RAACMinter.sol";
contract PoC is Test {
StabilityPool public stabilityPool;
LendingPool public lendingPool;
CrvUSDToken public crvusd;
RAACHousePrices public raacHousePrices;
RAACNFT public raacNFT;
RToken public rToken;
DebtToken public debtToken;
DEToken public deToken;
RAACToken public raacToken;
RAACMinter public raacMinter;
address owner;
address oracle;
address user1;
address user2;
address user3;
uint256 constant STARTING_TIME = 1641070800;
uint256 public currentBlockTimestamp;
uint256 constant WAD = 1e18;
uint256 constant RAY = 1e27;
function setUp() public {
vm.warp(STARTING_TIME);
currentBlockTimestamp = block.timestamp;
owner = address(this);
oracle = makeAddr("oracle");
user1 = makeAddr("user1");
user2 = makeAddr("user2");
user3 = makeAddr("user3");
uint256 initialPrimeRate = 0.1e27;
raacHousePrices = new RAACHousePrices(owner);
vm.prank(owner);
raacHousePrices.setOracle(oracle);
crvusd = new CrvUSDToken(owner);
raacNFT = new RAACNFT(address(crvusd), address(raacHousePrices), owner);
rToken = new RToken("RToken", "RToken", owner, address(crvusd));
debtToken = new DebtToken("DebtToken", "DT", owner);
deToken = new DEToken("DEToken", "DEToken", owner, address(rToken));
vm.prank(owner);
crvusd.setMinter(owner);
vm.prank(owner);
lendingPool = new LendingPool(
address(crvusd),
address(rToken),
address(debtToken),
address(raacNFT),
address(raacHousePrices),
initialPrimeRate
);
rToken.setReservePool(address(lendingPool));
debtToken.setReservePool(address(lendingPool));
rToken.transferOwnership(address(lendingPool));
debtToken.transferOwnership(address(lendingPool));
stabilityPool = new StabilityPool(address(owner));
deToken.setStabilityPool(address(stabilityPool));
raacToken = new RAACToken(owner, 0, 0);
raacMinter = new RAACMinter(address(raacToken), address(stabilityPool), address(lendingPool), owner);
stabilityPool.initialize(address(rToken), address(deToken), address(raacToken), address(raacMinter), address(crvusd), address(lendingPool));
vm.prank(owner);
raacToken.setMinter(address(raacMinter));
lendingPool.setStabilityPool(address(stabilityPool));
crvusd.mint(user1, type(uint128).max);
crvusd.mint(user2, type(uint128).max);
crvusd.mint(user3, type(uint128).max);
}
function test_userLosingAllNftsWithWrongTiming() public {
crvusd.mint(address(stabilityPool), 100e18);
vm.startPrank(user3);
crvusd.approve(address(lendingPool), 10e18);
lendingPool.deposit(10e18);
vm.stopPrank();
vm.prank(oracle);
raacHousePrices.setHousePrice(1, 5e18);
vm.startPrank(user1);
crvusd.approve(address(raacNFT), type(uint256).max);
raacNFT.mint(1, 5e18);
raacNFT.approve(address(lendingPool), 1);
lendingPool.depositNFT(1);
lendingPool.borrow(4e18);
vm.stopPrank();
vm.prank(oracle);
raacHousePrices.setHousePrice(1, 4e18);
vm.prank(user2);
lendingPool.initiateLiquidation(user1);
vm.startPrank(oracle);
raacHousePrices.setHousePrice(2, 5e18);
raacHousePrices.setHousePrice(3, 5e18);
raacHousePrices.setHousePrice(4, 5e18);
raacHousePrices.setHousePrice(5, 5e18);
vm.stopPrank();
vm.startPrank(user1);
raacNFT.mint(2, 5e18);
raacNFT.approve(address(lendingPool), 2);
lendingPool.depositNFT(2);
raacNFT.mint(3, 5e18);
raacNFT.approve(address(lendingPool), 3);
lendingPool.depositNFT(3);
raacNFT.mint(4, 5e18);
raacNFT.approve(address(lendingPool), 4);
lendingPool.depositNFT(4);
raacNFT.mint(5, 5e18);
raacNFT.approve(address(lendingPool), 5);
lendingPool.depositNFT(5);
vm.expectRevert();
lendingPool.closeLiquidation();
vm.stopPrank();
uint256 blockTime = block.timestamp;
vm.warp(blockTime + 4 * 1 days);
console.log("User Health Factor at time of Liquidation: ", lendingPool.calculateHealthFactor(user1) / 1e18);
assert(lendingPool.calculateHealthFactor(user1) > 1e18);
vm.prank(owner);
stabilityPool.liquidateBorrower(user1);
console.log("User Collateral Value after liquidation: ", debtToken.balanceOf(user1));
assertEq(0, lendingPool.getUserCollateralValue(user1));
}
After running it via forge test --mt test_userLosingAllNftsWithWrongTiming -vv we can see the following output:
Ran 1 test for test/invariant/Unit.t.sol:Unit
[PASS] test_userLosingAllNftsWithWrongTiming() (gas: 2051744)
Logs:
User Health Factor at time of Liquidation: 4
User Collateral Value after liquidation: 0
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 29.19ms (11.84ms CPU time)
Ran 1 test suite in 53.44ms (29.19ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
The output showcasing clearly, that the user lost NFT IDs 2-5 too, not only ID 1. Therefor the user lost in this scenario 20e18 tokens on top of the intended liquidation.
Impact
Since this issue can only arise under certain pre-conditions and has a very limited timeframe to occur the likelihood would be rated as a low - medium.
Losing NFT value beyond the intend of liquidation though, is a severe financial harm to the user, therefor the impact is to be rated high.
As result i conclude a severity of High in total is justified, simply because the impact can be devastating.
Tools Used
Manual Review & Foundry
Recommended Fix
Since other functions use already logic which makes usage of functions impossible during initiated liquidation, consider adding the same access control to deposit NFTs to avoid edge cases or rethink changing the closeLiquidation function into a health factor based functionality. Assuming the liquidation logic is intended the way it is I would though stick with access control as shown here:
+ error CanNotDepositNFTDuringLiquidation();
function depositNFT(uint256 tokenId) external nonReentrant whenNotPaused {
ReserveLibrary.updateReserveState(reserve, rateData);
+ if (isUnderLiquidation[userAddress]) revert CanNotDepositNFTDuringLiquidation();
if (raacNFT.ownerOf(tokenId) != msg.sender) revert NotOwnerOfNFT();
UserData storage user = userData[msg.sender];
if (user.depositedNFTs[tokenId]) revert NFTAlreadyDeposited();
user.nftTokenIds.push(tokenId);
user.depositedNFTs[tokenId] = true;
raacNFT.safeTransferFrom(msg.sender, address(this), tokenId);
emit NFTDeposited(msg.sender, tokenId);
}
Appendix
Copy the following import into your Hardhat.Config file in the projects root dir:
require("@nomicfoundation/hardhat-foundry");
Paste the following into a new file "Makefile" into the projects root directory:
.PHONY: install-foundry init-foundry all
install-foundry:
npm install --save-dev @nomicfoundation/hardhat-foundry
init-foundry: install-foundry
npx hardhat init-foundry
# Default target that runs everything in sequence
all: install-foundry init-foundry
And run make all
