Description
In the event, in which a user has an outstanding debt with the Lending Pool and becomes liquidatable there is a scenario, in which the user tries to repay his debt after the grace period expires, but before the liquidation is finalized, which locks the users NFTs permanently in the Lending Pool.
Vulnerability Details
An inconsistency within allowed actions after the Grace Period expires can lead to NFTs locked within the Lending Pool, permanently.
Consider the following 2 functions within the Lending Pool:
LendingPool::_repay
function _repay(uint256 amount, address onBehalfOf) internal {
if (amount == 0) revert InvalidAmount();
if (onBehalfOf == address(0)) revert AddressCannotBeZero();
UserData storage user = userData[onBehalfOf];
ReserveLibrary.updateReserveState(reserve, rateData);
uint256 userDebt = IDebtToken(reserve.reserveDebtTokenAddress).balanceOf(onBehalfOf);
uint256 userScaledDebt = userDebt.rayDiv(reserve.usageIndex);
uint256 actualRepayAmount = amount > userScaledDebt ? userScaledDebt : amount;
uint256 scaledAmount = actualRepayAmount.rayDiv(reserve.usageIndex);
(uint256 amountScaled, uint256 newTotalSupply, uint256 amountBurned, uint256 balanceIncrease) =
IDebtToken(reserve.reserveDebtTokenAddress).burn(onBehalfOf, amount, reserve.usageIndex);
IERC20(reserve.reserveAssetAddress).safeTransferFrom(msg.sender, reserve.reserveRTokenAddress, amountScaled);
reserve.totalUsage = newTotalSupply;
user.scaledDebtBalance -= amountBurned;
ReserveLibrary.updateInterestRatesAndLiquidity(reserve, rateData, amountScaled, 0);
emit Repay(msg.sender, onBehalfOf, actualRepayAmount);
}
and 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);
}
As you can see in the closeLiquidation function, it protects from being called after the grace period expired, however, the _repay function does not.
Consider the following scenario:
Bob mints an NFT worth 10e18, deposits it into the LendingPool and takes a loan against it worth 8e18.
A price reduction (or accrued interest for that matter) would make Bob now liquidatable, the liquidation is initiated and rests for 3 days (it could be a pause too, due to unforseen problems).
After those 3 days, just before the admin calls finalizeLiquidation Bob repays his debt in full.
Now because of the following line within the StabilityPool::liquidateBorrower function:
if (userDebt == 0) revert InvalidAmount();
any attempt to liquidate Bobs NFT will fail, locking it permanently in the contract, since his DebtToken balance is 0, he can not be liquidated but he can not close the liquidation against him as well since the grace period expired.
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).
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.
And the test should be good to go:
After following above steps copy & paste the following code into ./test/invariant/PoC.t.sol and run forge test --mt test_tryToLiquidateAfterRepayWillFail -vv
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));
attacker = new Attacker(address(raacNFT));
crvusd.mint(address(attacker), type(uint128).max);
crvusd.mint(user1, type(uint128).max);
crvusd.mint(user2, type(uint128).max);
crvusd.mint(user3, type(uint128).max);
}
function test_tryToLiquidateAfterRepayWillFail() public {
vm.startPrank(user2);
crvusd.approve(address(lendingPool), type(uint256).max);
lendingPool.deposit(type(uint128).max);
vm.stopPrank();
crvusd.mint(user1, 12e18);
vm.startPrank(oracle);
raacHousePrices.setHousePrice(1, 10e18);
vm.stopPrank();
vm.startPrank(user1);
crvusd.approve(address(raacNFT), type(uint256).max);
raacNFT.mint(1, 10e18);
raacNFT.approve(address(lendingPool), 1);
lendingPool.depositNFT(1);
lendingPool.borrow(8e18);
vm.stopPrank();
vm.prank(oracle);
raacHousePrices.setHousePrice(1, 5e18);
vm.startPrank(user2);
lendingPool.initiateLiquidation(user1);
vm.stopPrank();
vm.startPrank(owner);
lendingPool.pause();
vm.warp(block.timestamp + 1 + 3 days);
lendingPool.unpause();
vm.stopPrank();
vm.startPrank(user1);
console.log(crvusd.balanceOf(user1));
crvusd.approve(address(lendingPool), 9e18);
lendingPool.repay(9e18);
vm.expectRevert();
lendingPool.closeLiquidation();
vm.stopPrank();
uint256 accruedInterest = 1_644_010_858_489_111;
console.log(crvusd.balanceOf(user1));
assertEq(crvusd.balanceOf(user1), 2e18 - accruedInterest);
crvusd.mint(address(stabilityPool), 100e18);
vm.prank(owner);
stabilityPool.liquidateBorrower(user1);
}
}
Running above test will produce the following log:
Ran 1 test for test/invariant/PoC.t.sol:PoC
[FAIL: InvalidAmount()] test_tryToRepayWillFail() (gas: 1197126)
Logs:
10000000000000000000
1998355989141510889
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 10.94ms (2.56ms CPU time)
Ran 1 test suite in 16.79ms (10.94ms CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)
Failing tests:
Encountered 1 failing test in test/invariant/PoC.t.sol:PoC
[FAIL: InvalidAmount()] test_tryToRepayWillFail() (gas: 1197126)
Encountered a total of 1 failing tests, 0 tests succeeded
As we can clearly see, the liquidation did not work out, due to the fact that Bob (user1) has simply no DebtToken balance, therefor locking the NFT permanently in the contract.
Impact
The NFTs minted and used by RAAC represent an integral part of the ecosystem and the solvency of the protocol is reliant on being able to sell assets due for liquidation. Rendering them useless by permanently locking them in the contract is directly affecting the solvency and economics of the protocol, but, this situation requires the user to have paid his bad debt already, mitigating the impact for the protocol but shifting it onto the user. However, this issue causes harm to either the user or the protocol and undermines the ownership principle for real estate, since this real estate will not be owned by anyone anymore really.
Furthermore the issue can arise accidentally or intentionally, the likelihood is though low.
My severity assessment in total would be a High, simply because the user would endure financial loss and there is no way of removing the NFT from the contract, also the user would lose any ability to participate in the contract without making a brand new wallet address.
Tools Used
Manual Review
Recommended Fix
Implement the same logic into the _repay function as it is in the closeLiquidation to avoid mentioned asymmetry:
function _repay(uint256 amount, address onBehalfOf) internal {
if (amount == 0) revert InvalidAmount();
if (onBehalfOf == address(0)) revert AddressCannotBeZero();
+ if (block.timestamp > liquidationStartTime[onBehalfOf] + liquidationGracePeriod) {
+ revert GracePeriodExpired();
+ }
UserData storage user = userData[onBehalfOf];
ReserveLibrary.updateReserveState(reserve, rateData);
uint256 userDebt = IDebtToken(reserve.reserveDebtTokenAddress).balanceOf(onBehalfOf);
uint256 userScaledDebt = userDebt.rayDiv(reserve.usageIndex);
uint256 actualRepayAmount = amount > userScaledDebt ? userScaledDebt : amount;
uint256 scaledAmount = actualRepayAmount.rayDiv(reserve.usageIndex);
(uint256 amountScaled, uint256 newTotalSupply, uint256 amountBurned, uint256 balanceIncrease) =
IDebtToken(reserve.reserveDebtTokenAddress).burn(onBehalfOf, amount, reserve.usageIndex);
IERC20(reserve.reserveAssetAddress).safeTransferFrom(msg.sender, reserve.reserveRTokenAddress, amountScaled);
reserve.totalUsage = newTotalSupply;
user.scaledDebtBalance -= amountBurned;
ReserveLibrary.updateInterestRatesAndLiquidity(reserve, rateData, amountScaled, 0);
emit Repay(msg.sender, onBehalfOf, actualRepayAmount);
}
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
