A2-security - Liquidators can manipulate RepaidDebt calculation to seize excess collateral from users being liquidated

[sherlock-admin4]

A2-security

High

Liquidators can manipulate RepaidDebt calculation to seize excess collateral from users being liquidated

Summary

• A vulnerability in the liquidation process allows malicious liquidators to seize all collateral. The issue stems from a discrepancy between debt calculation in RiskModule.sol's _validateSeizedAssetValue function and actual repayment in PositionManager.sol. By exploiting duplicate pool entries and type(uint256).max in the debtData array, attackers can trick the system into allowing seizure of more collateral than justified by the repaid debt.

Vulnerability Detail

• Sentiment protocol's liquidation process involves two key steps: validating the liquidation parameters and executing the actual repayment. The protocol aims to ensure that liquidators cannot seize more collateral than they're entitled to based on the debt they repay.

Let's examine the relevant parts of the code in detail:

1. Liquidation Validation:

In the RiskModule contract, the _validateSeizedAssetValue function calculates the expected debt repayment :

function _validateSeizedAssetValue(address position, DebtData[] calldata debtData, AssetData[] calldata assetData, uint256 discount) internal view {
    uint256 debtRepaidValue;
    uint256 debtLength = debtData.length;
    for (uint256 i; i < debtLength; ++i) {
        uint256 poolId = debtData[i].poolId;
        uint256 amt = debtData[i].amt;
   >>   if (amt == type(uint256).max) amt = pool.getBorrowsOf(poolId, position);
        address poolAsset = pool.getPoolAssetFor(poolId);
        IOracle oracle = IOracle(riskEngine.getOracleFor(poolAsset));
        debtRepaidValue += oracle.getValueInEth(poolAsset, amt);
    }
    // ... rest of the function
}

• Notice that this function allows the use of type(uint256).max as a special value to indicate repayment of the full borrowed amount from a pool. When amt == type(uint256).max, it fetches the full borrowed amount for that Position using pool.getBorrowsOf(poolId, position) which always return the full borrowed amount of this position.

    function getBorrowsOf(uint256 poolId, address position) public view returns (uint256) {
        PoolData storage pool = poolDataFor[poolId];
        (uint256 accruedInterest,) = simulateAccrue(pool);
        // [ROUND] round up to enable enable complete debt repayment
        return _convertToAssets(borrowSharesOf[poolId][position], pool.totalBorrowAssets + accruedInterest, pool.totalBorrowShares, Math.Rounding.Up);
    }

2. Actual Debt Repayment:
   The actual repayment occurs in the PositionManager contract:

function _repayPositionDebt(address position, DebtData[] memory debtData) internal {
    uint256 debtLength = debtData.length;
    for (uint256 i; i < debtLength; ++i) {
        uint256 poolId = debtData[i].poolId;
        uint256 amt = debtData[i].amt;
        if (amt == type(uint256).max) amt = pool.getBorrowsOf(poolId, position);
        pool.repayBorrow(poolId, position, amt);
    }
}

• notice that we use the same approach in case of amt == type(uint256).max to repay the full borrowed amount. the key here is that if we repay the borrowed amount will be lower next time we fetch it.

the issue arises because of these two key points:

1. Allowing duplicate poolIds in the debtData array
2. Using type(uint256).max to fetch the full borrowed amount

• combining these two an attacker can trick the system to believe that he is repaying way more than he's actually repaying which allows him to seize all collateral of the user being liquidated

• An attacker can exploit this by crafting a debtData array with duplicate pool entries, causing the validation function to overestimate the debt being repaid.

Example Poc:

Here's how an attacker can trick the system:

Consider a position with the following characteristics:

• Debt: 1000 USD in pool1
• Average LTV: 50%
• Collateral: 1 wETH (valued at 1950 USD)
• Liquidation bonus: 10%

This position is eligible for liquidation due to its current LTV exceeding the average LTV.

The attacker provides this data for liquidation:

debtData = [
    {poolId: pool1, amt: 950},
    {poolId: pool1, amt: type(uint256).max}
]
assetData = [
    AssetData { asset: weth , amt : 1} // 1950 Usd == 1 WETH
]

In _validateSeizedAssetValue function :

• First iteration: debtRepaidValue += 950

• Second iteration: debtRepaidValue += 1000 (amt = pool.getBorrowsOf(pool1, position) = 1000)

• total debtRepaidValue calculated: 1950 USD

• this check will pass since assetSeizedValue == maxSeizedAssetValue(1950usdc == usdValue(1 weth)) :

    function _validateSeizedAssetValue(address position, DebtData[] calldata debtData, AssetData[] calldata assetData, uint256 discount) internal view {
       // some code ...
        uint256 maxSeizedAssetValue = debtRepaidValue.mulDiv(1e18, (1e18 - discount));
   >>     if (assetSeizedValue > maxSeizedAssetValue) revert RiskModule_SeizedTooMuch(assetSeizedValue, maxSeizedAssetValue);
    }

• However, in _repayPositionDebt function the attacker will be repaying :

  • First repayment: 950 USD
  • Second repayment: 50 USD
    that because : pool.repayBorrow(pool1, position) = 50 , since 950 USD is already repaid.

• Total debt actually repaid: 1000 USD

This discrepancy allowed the attacker to seize collateral based on a 1950 USD debt repayment, while only actually repaying 1000 USD.and the user being liquidated lost 850 usd because of that .

Root Cause

• The root of the problem is that _validateSeizedAssetValue doesn't update the borrowed amount after each iteration, while _repayPositionDebt does through actual repayments. This inconsistency, combined with allowing duplicate pool entries and the use of type(uint256).max, creates the exploit opportunity.

Impact

• This vulnerability allows malicious liquidators to unfairly seize all collateral from users being liquidated, even when repaying only a portion of the debt. The impact is particularly severe for positions with lower Loan-to-Value (LTV) ratios (ex :for a 50% LTV , and 10% liquidation bonus , an attacker could steal 40% of user collateral), where users stand to lose significantly more collateral than they should under normal liquidation conditions.

Code Snippet

• https://github.com/sherlock-audit/2024-08-sentiment-v2/blob/25a0c8aeaddec273c5318540059165696591ecfb/protocol-v2/src/PositionManager.sol#L430-L445
• https://github.com/sherlock-audit/2024-08-sentiment-v2/blob/25a0c8aeaddec273c5318540059165696591ecfb/protocol-v2/src/RiskModule.sol#L111-L120
• https://github.com/sherlock-audit/2024-08-sentiment-v2/blob/25a0c8aeaddec273c5318540059165696591ecfb/protocol-v2/src/RiskModule.sol#L129-L160

Tool used

Manual Review

Recommendation

• To prevent the exploitation of duplicate poolIds in the debtData array, implement a check to ensure each poolId is unique. Here's a suggested modification to the _validateSeizedAssetValue function :

+ error RiskModule_DuplicatePoolId();
function _validateSeizedAssetValue(address position, DebtData[] calldata debtData, AssetData[] calldata assetData, uint256 discount) internal view {
    uint256 debtRepaidValue;
    uint256 debtLength = debtData.length;
+   uint lastPoolId;
    for (uint256 i; i < debtLength; ++i) {
        uint256 poolId = debtData[i].poolId;
+       if(poolId <= lastPoolId) revert RiskModule_DuplicatePoolId();
        uint256 amt = debtData[i].amt;
        if (amt == type(uint256).max) amt = pool.getBorrowsOf(poolId, position);
        address poolAsset = pool.getPoolAssetFor(poolId);
        IOracle oracle = IOracle(riskEngine.getOracleFor(poolAsset));
        debtRepaidValue += oracle.getValueInEth(poolAsset, amt);
+        lastPoolId = poolId;
    }
    // ... rest of the function
}

• This change ensures that each poolId is unique and in ascending order, preventing the vulnerability caused by duplicate entries.Just for Liquidators , they should pass the debtData poolIds sorted from smallest to largest.

Duplicate of #556