Core Contracts
Regnum Aurum Acquisition Corp
HardhatReal World AssetsNFT
77,280 USDC
View results
Previous Next
Submission Details
Severity: high
Invalid

Borrowers collateral can be seized, due to failed repayments

dimah7

Summary

When a user interacts with the protocol (deposit, withdraw, borrow, repay) or he is getting liquidated the internal ReserveLibrary::updateInterestRatesAndLiquidity() is called to update the interest rates and the utilization rate:

function updateInterestRatesAndLiquidity(ReserveData storage reserve,ReserveRateData storage rateData,uint256 liquidityAdded,uint256 liquidityTaken) internal {
// Update total liquidity
if (liquidityAdded > 0) {
reserve.totalLiquidity = reserve.totalLiquidity + liquidityAdded.toUint128();
}
if (liquidityTaken > 0) {
if (reserve.totalLiquidity < liquidityTaken) revert InsufficientLiquidity();
reserve.totalLiquidity = reserve.totalLiquidity - liquidityTaken.toUint128();
}
​
uint256 totalLiquidity = reserve.totalLiquidity;
uint256 totalDebt = reserve.totalUsage;
​
uint256 computedDebt = getNormalizedDebt(reserve, rateData);
uint256 computedLiquidity = getNormalizedIncome(reserve, rateData);
​
// Calculate utilization rate
uint256 utilizationRate = calculateUtilizationRate(reserve.totalLiquidity, reserve.totalUsage);
​
// Update current usage rate (borrow rate)
rateData.currentUsageRate = calculateBorrowRate(
rateData.primeRate,
rateData.baseRate,
rateData.optimalRate,
rateData.maxRate,
rateData.optimalUtilizationRate,
utilizationRate
);
​
// Update current liquidity rate
rateData.currentLiquidityRate = calculateLiquidityRate(
utilizationRate,
rateData.currentUsageRate,
rateData.protocolFeeRate,
totalDebt
);

However this function can cause borrows or repayments to revert.

Vulnerability Details

Notice that reserve.totalLiquidity is updated everytime some tokens are deposited or withdrawn from the protocol. However this calculation doesn't account about the liquidity + earned yield in the Curve vault (which means can be withdrawn more than deposited) or when users are repaying their debt, because they will pay back the borrowed amount + any accrued interest on it. Thus liquidityTaken will be greater than reserve.totalLiquidity and the call will revert with InsufficientLiquidity error.

Consider the following scenario:

  • Case 1:

  1. Lender deposits 1000 tokens, -> totalLiquidity += 1000 (80% will be allocated to the Curve vault) (20% to RToken)

  2. The locked funds in the Curve vault earn yield over time (let's say 1 token)

  3. Bob deposits collateral worth 2000 tokens

  4. Bob wants to borrow some tokens, now if the selected amount is more than RToken's crvUSD reserves, the internal LendingPool::_ensureLiquidity() will be triggered to provide the required amount from the Curve vault

function _ensureLiquidity(uint256 amount) internal {
...
uint256 availableLiquidity = IERC20(reserve.reserveAssetAddress).balanceOf(reserve.reserveRTokenAddress);
​
if (availableLiquidity < amount) {
uint256 requiredAmount = amount - availableLiquidity;
// Withdraw required amount from the Curve vault
_withdrawFromVault(requiredAmount);
}
}

  1. So let's say Bob wants to borrow exactly 1001 tokens (800 + 1 (yield) from vault + 200 in RToken)

  2. The protocol should be able to lend this amount because it will be available

  3. But totalLiquidity will be 1000 and it will try to take out 1001 and it will revert

  • Case 2:

  1. Lender provides 1000 liquidity, -> totalLiquidity += 1000

  2. Bob deposits collateral worth 2000 tokens

  3. Bob borrows 1000 tokens

  4. After some time Bob decides to return them, but he will repay more due to accrued interest

LendingPool:
​
function _repay(uint256 amount, address onBehalfOf) internal {
...
// Burn DebtTokens from the user whose debt is being repaid (onBehalfOf)
// is not actualRepayAmount because we want to allow paying extra dust and we will then cap there
(uint256 amountScaled, uint256 newTotalSupply, uint256 amountBurned, uint256 balanceIncrease) =
IDebtToken(reserve.reserveDebtTokenAddress).burn(onBehalfOf, amount, reserve.usageIndex);
// Transfer reserve assets from the caller (msg.sender) to the reserve
@> IERC20(reserve.reserveAssetAddress).safeTransferFrom(msg.sender, reserve.reserveRTokenAddress, amountScaled);
reserve.totalUsage = newTotalSupply;
user.scaledDebtBalance -= amountBurned;
// Update liquidity and interest rates
@> ReserveLibrary.updateInterestRatesAndLiquidity(reserve, rateData, amountScaled, 0);
...
}

  1. Let's say that his has 1 token interest accured on his debt, that means he must repay 1001

  2. In updateInterestRatesAndLiquidity it will try to subtract totalLiquidity (1000) and liquidityTaken (1001) and will revert

Here is a coded POC, demonstrating case 1 (i'm using Foundry for tests)

  1. Install Foundry

  2. Run forge init --force in the terminal

  3. Paste the following file in the test folder and run forge test --mt testX

  4. Note: in order this test to be sucessful, there must be 3 fixes applied of issues reported in separate reports, one of them is explained and applied in the test. (that's why 800 tokens are transfered directly to the lending pool and that's why Bob borrows 1801 tokens, but not as described in the textual POC)

  5. The other ones are shown below in LendingPool::_withdrawFromVault:

function _withdrawFromVault(uint256 amount) internal {
- curveVault.withdraw(amount, address(this), msg.sender, 0, new address[](0));
+ curveVault.withdraw(amount, reserve.reserveRTokenAddress, msg.sender, 0, new address[](0));
- totalVaultDeposits -= amount;
}
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;
​
import {Test, console2} from "../lib/forge-std/src/Test.sol";
import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {LendingPool} from "../contracts/core/pools/LendingPool/LendingPool.sol";
import {ILendingPool} from "../contracts/interfaces/core/pools/LendingPool/ILendingPool.sol";
import {RToken} from "../contracts/core/tokens/RToken.sol";
import {DebtToken} from "../contracts/core/tokens/DebtToken.sol";
import {RAACHousePrices} from "../contracts/core/primitives/RAACHousePrices.sol";
import {RAACNFT} from "../contracts/core/tokens/RAACNFT.sol";
import {IRAACNFT} from "../contracts/interfaces/core/tokens/IRAACNFT.sol";
import {WadRayMath} from "../contracts/libraries/math/WadRayMath.sol";
import {ICurveCrvUSDVault} from "../contracts/interfaces/curve/ICurveCrvUSDVault.sol";
import {ReserveLibrary} from "../contracts/libraries/pools/ReserveLibrary.sol";
​
​
contract CrvUSD is IERC20, ERC20 {
constructor() ERC20("crvUSD", "CRVUSD") {}
​
function mint(address to, uint256 amount) public {
_mint(to, amount);
}
}
​
// Note: the mock vault is only transfering assets, without issuing shares, for simplicity, because the desbribed problem is not shares related
​
contract MockCurveVault is ICurveCrvUSDVault {
CrvUSD crvUSD;
​
constructor(CrvUSD _crvUSD) {
crvUSD = _crvUSD;
}
​
function deposit(uint256 assets, address receiver) external returns (uint256 shares) {
crvUSD.transferFrom(msg.sender, address(this), assets);
}
​
function withdraw(uint256 assets, address receiver, address owner, uint256 maxLoss, address[] calldata strategies) external returns (uint256 shares) {
crvUSD.transfer(receiver, assets);
}
​
function asset() external view returns (address) {}
function totalAssets() external view returns (uint256) {}
function pricePerShare() external view returns (uint256) {}
function totalIdle() external view returns (uint256) {}
function totalDebt() external view returns (uint256) {}
function isShutdown() external view returns (bool) {}
}
​
contract Tester is Test {
LendingPool lendingPool;
CrvUSD crvUSD;
RToken rToken;
DebtToken debtToken;
RAACHousePrices housePrices;
RAACNFT nft;
MockCurveVault vault;
​
uint256 initialPrimeRate = 1e26;
address owner = makeAddr("owner");
address bob = makeAddr("bob");
address lender = makeAddr("lender");
​
function setUp() external {
vm.startPrank(owner);
crvUSD = new CrvUSD();
vault = new MockCurveVault(crvUSD);
rToken = new RToken("RToken", "RT", owner, address(crvUSD));
debtToken = new DebtToken("DebtToken", "DT", owner);
housePrices = new RAACHousePrices(owner);
nft = new RAACNFT(address(crvUSD), address(housePrices), owner);
lendingPool = new LendingPool(
address(crvUSD),
address(rToken),
address(debtToken),
address(nft),
address(housePrices),
initialPrimeRate
);
housePrices.setOracle(owner); // owner is set as oracle for simplicity
housePrices.setHousePrice(1, 2000e18); // tokenID = 1 will cost 2000e18 crvUSD
debtToken.setReservePool(address(lendingPool));
rToken.setReservePool(address(lendingPool));
lendingPool.setCurveVault(address(vault));
vm.stopPrank();
​
// Lender provides liquidity
vm.startPrank(lender);
crvUSD.mint(lender, 8000e18);
crvUSD.transfer(address(lendingPool), 800e18); // directly transfering some amount so it can fix other issue mentioned in separate report
crvUSD.approve(address(lendingPool), type(uint256).max);
lendingPool.deposit(1000e18);
vm.stopPrank();
​
// Bob (borrower) setup
vm.startPrank(bob);
crvUSD.mint(bob, 2000e18);
crvUSD.approve(address(lendingPool), type(uint256).max);
crvUSD.approve(address(nft), type(uint256).max);
nft.mint(1, 2000e18);
nft.setApprovalForAll(address(lendingPool), true);
vm.stopPrank();
}
​
function testX() public {
// This replicates the earned yield, just transfers some amount for simplicity, to avoid additional code complexity
address yieldProvider = makeAddr("yp");
crvUSD.mint(yieldProvider, 1e18);
vm.prank(yieldProvider);
crvUSD.transfer(address(vault), 1e18); // Here the vault "earns" yield
​
vm.startPrank(bob);
lendingPool.depositNFT(1); // has deposited collateral worth of 2000 crvUSD
vm.expectRevert(ReserveLibrary.InsufficientLiquidity.selector);
lendingPool.borrow(1801e18);
vm.stopPrank();
}
}

Impact

Two impacts:

  1. DoS on borrows - Medium

  2. Revert on repayments, which means borrowers will be liquidated and their collateral will be seized - High

  • Overall: High

Tools Used

Manual Review

Recommendations

It's hard to give straightforward recommendation, but definitelty the logic of tracking liquidity should be refactored.

Updates

Lead Judging Commences

inallhonesty Lead Judge 6 months ago
Submission Judgement Published
Validated
Assigned finding tags:

LendingPool::_withdrawFromVault incorrectly uses msg.sender instead of address(this) as the owner parameter, causing vault withdrawals to fail

inallhonesty Lead Judge 6 months ago
Submission Judgement Published
Validated
Assigned finding tags:

LendingPool::_withdrawFromVault incorrectly uses msg.sender instead of address(this) as the owner parameter, causing vault withdrawals to fail

Appeal created

dimah7 Submitter
6 months ago
inallhonesty Lead Judge
6 months ago
inallhonesty Lead Judge 5 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.