A borrower can lose their collateral by a `back-running` attack - right after the borrower would borrow USDS

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/CollateralAndLiquidity.sol#L145 https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/CollateralAndLiquidity.sol#L304 https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/CollateralAndLiquidity.sol#L227 https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/CollateralAndLiquidity.sol#L232-L233 https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/CollateralAndLiquidity.sol#L235 https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/CollateralAndLiquidity.sol#L202-L203 https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/CollateralAndLiquidity.sol#L205 https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/StakingRewards.sol#L89

Vulnerability details

Impact

When the CollateralAndLiquidity#liquidateUser() would be called by a liquidator, whether or not a given wallet (borrower) can be liquidated would be checked via the CollateralAndLiquidity#canUserBeLiquidated() like this: https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/CollateralAndLiquidity.sol#L145

    function liquidateUser( address wallet ) external nonReentrant
        {
        ...
        // First, make sure that the user's collateral ratio is below the required level
        require( canUserBeLiquidated(wallet), "User cannot be liquidated" ); ///<--------- @audit

Within the CollateralAndLiquidity#canUserBeLiquidated(), the CollateralAndLiquidity#userCollateralValueInUSD() would be called to calculate the userCollateralValue. Then, the userCollateralValue would be used to make sure whether or not the user's position is under collateralized like this: https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/CollateralAndLiquidity.sol#L304

    // Confirm that a user can be liquidated - that they have borrowed USDS and that their collateral value / borrowedUSDS ratio is less than the minimum required
    function canUserBeLiquidated( address wallet ) public view returns (bool)
        {
        // Check the current collateral ratio for the user
        uint256 usdsBorrowedAmount = usdsBorrowedByUsers[wallet];
        if ( usdsBorrowedAmount == 0 )
            return false;

        uint256 userCollateralValue = userCollateralValueInUSD(wallet); ///<--------------- @audit

        // Make sure the user's position is under collateralized
        return (( userCollateralValue * 100 ) / usdsBorrowedAmount) < stableConfig.minimumCollateralRatioPercent(); ///<--------------- @audit
        }

Within the CollateralAndLiquidity#userCollateralValueInUSD(), the collateralPoolID of the totalShares (totalShares[collateralPoolID]) would be stored into the totalCollateralShares. (NOTE：This collateralPoolID is the poolID of the WBTC/ETH pool) Then, the userWBTC and the userWETH would be calculated by using the totalCollateralShares and so on. Finally, the underlyingTokenValueInUSD() would be called like this: https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/CollateralAndLiquidity.sol#L227 https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/CollateralAndLiquidity.sol#L232-L233 https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/CollateralAndLiquidity.sol#L235

    // The current market value of the user's collateral in USD
    // Returns the value with 18 decimals
    function userCollateralValueInUSD( address wallet ) public view returns (uint256)
        {
        uint256 userCollateralAmount = userShareForPool( wallet, collateralPoolID );
        if ( userCollateralAmount == 0 )
            return 0;

        uint256 totalCollateralShares = totalShares[collateralPoolID]; ///<--------- @audit

        // Determine how much collateral share the user currently has
        (uint256 reservesWBTC, uint256 reservesWETH) = pools.getPoolReserves(wbtc, weth);

        uint256 userWBTC = (reservesWBTC * userCollateralAmount ) / totalCollateralShares; ///<--------- @audit
        uint256 userWETH = (reservesWETH * userCollateralAmount ) / totalCollateralShares;  ///<--------- @audit

        return underlyingTokenValueInUSD( userWBTC, userWETH ); ///<--------- @audit
        }

Within the CollateralAndLiquidity#underlyingTokenValueInUSD(), the btcValue and the ethValue would be calculated based on the given amountBTC and amountETH. Then, the sum of the btcValue and the ethValue (btcValue + ethValue) would be returned as a collateral value in USD of a given amountBTC and amountETH like this: https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/CollateralAndLiquidity.sol#L202-L203 https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/CollateralAndLiquidity.sol#L205

    // The current market value in USD for a given amount of BTC and ETH using the PriceAggregator
    // Returns the value with 18 decimals
    function underlyingTokenValueInUSD( uint256 amountBTC, uint256 amountETH ) public view returns (uint256)
        {
        // Prices from the price feed have 18 decimals
        uint256 btcPrice = priceAggregator.getPriceBTC();
        uint256 ethPrice = priceAggregator.getPriceETH();

        // Keep the 18 decimals from the price and remove the decimals from the token balance
        uint256 btcValue = ( amountBTC * btcPrice ) / wbtcTenToTheDecimals;
        uint256 ethValue = ( amountETH * ethPrice ) / wethTenToTheDecimals;

        return btcValue + ethValue; ///<--------- @audit
        }

When a user would call the StakingRewards#_increaseUserShare() via the CollateralAndLiquidity#depositCollateralAndIncreaseShare(), a given poolID of the totalShares storage would be increased like this: https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/StakingRewards.sol#L89

    // Increase a user's share for the given whitelisted pool.
    function _increaseUserShare( address wallet, bytes32 poolID, uint256 increaseShareAmount, bool useCooldown ) internal
        {
        ...
        // Update the deposit balances
        user.userShare += uint128(increaseShareAmount);
        totalShares[poolID] = existingTotalShares + increaseShareAmount;  ///<----------- @audit

Within the CollateralAndLiquidity#userCollateralValueInUSD() above, the totalCollateralShares, which the collateralPoolID of the totalShares (totalShares[collateralPoolID]) is stored, would be used for the userWBTC calculation and the userWETH calculation.

This is problematic because the totalCollateralShares (totalShares[collateralPoolID]) can intentionally be surged by a flashloan attack. Once the totalCollateralShares would be surged, the userWBTC and the userWETH would dramatically be decreased. As a result, the btcValue and the ethValue would also dramatically be decreased, meaning that the collateral value (btcValue + ethValue) in USD of all borrowing positions can dramatically be decreased as well. In this case, a lot of borrowing positions can immediately be moved to a "un-healthy" (liquidatable) status and then the malicious actor can liquidate these "un-healthy borrowing positions by repeatedly calling the CollateralAndLiquidity#liquidateUser().

This allow a malicious actor to gain a liquidation rewards by intentionally manipulating the totalCollateralShares and liquidating a victim borrower's debt position by a back-running attack - right after a borrower would borrow USDS via the CollateralAndLiquidity#borrowUSDS().

Due to that, the victim borrower would would lose their collateral (WBTC and WETH), which the borrower deposited.

Proof of Concept

Assumption:

• Let's say Alice is a victim borrower and Bob is a malicious actor:

Attack scenario: The malicious actor can this attack above in a single block:

• 0/ Alice would deposit WBTC and WETH into the WBTC/WETH Pool via the CollateralAndLiquidity#depositCollateralAndIncreaseShare().

  • But, she has not been borrow any USDS yet. (She has no debt position yet)

• 1/ Alice would borrow the amount of USDS via the CollateralAndLiquidity#borrowUSDS().

• 2/ Bob would monitor the TX of the step 1/ in the mempool and he would back-run it with calling the CollateralAndLiquidity#depositCollateralAndIncreaseShare() with the large amount of WBTC and WETH and the CollateralAndLiquidity#liquidateUser(). This TX would cause the following state changes:

  • At this time, the StakingRewards#_increaseUserShare() would be called inside the CollateralAndLiquidity#depositCollateralAndIncreaseShare(). Hence, the totalShares[collateralPoolID] (totalCollateralShares) would dramatically be increased.
  • Thereby, the userWBTC and the userWETH of all existing debt positions in the CollateralAndLiquidity#userCollateralValueInUSD() would dramatically be dramatically decreased.
  • As a result, the btcValue and the ethValue of all existing debt positions in the CollateralAndLiquidity#underlyingTokenValueInUSD() would also dramatically be dramatically decreased. This means that the collateral value in USD (btcValue + ethValue) of all debt positions, which is returned from the CollateralAndLiquidity#underlyingTokenValueInUSD(), can dramatically be decreased as well. https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/CollateralAndLiquidity.sol#L232-L233 https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/CollateralAndLiquidity.sol#L202-L205

    
    /// Function: CollateralAndLiquidity#userCollateralValueInUSD()

uint256 userWBTC = (reservesWBTC userCollateralAmount ) / totalCollateralShares;
uint256 userWETH = (reservesWETH userCollateralAmount ) / totalCollateralShares;

```solidity
/// Function: CollateralAndLiquidity#underlyingTokenValueInUSD()
/// NOTE："userWBTC" and "userWETH" would be stored into the "amountBTC" and "amountETH" respectively.

uint256 btcValue = ( amountBTC * btcPrice ) / wbtcTenToTheDecimals; 
uint256 ethValue = ( amountETH * ethPrice ) / wethTenToTheDecimals;

return btcValue + ethValue;

• 3/ Alice's TX (step 1/) would be executed first.

• 4/ Right after, Bob's TX (step 2/) would be executed as a back-run.

• 5/ Due to the step 4/, Alice's debt positions can immediately be moved to a un-healthy (liquidatable) status, which their collateral value in USD (btcValue + ethValue) is below 110% of the liquidation threshold. And then, her un-healthy (liquidatable) debt position would be liquidated by the CollateralAndLiquidity#liquidateUser() call, which is included in the Bob's back-run TX.

• 6/ Bob would receive the liquidation rewards from Alice's un-healthy debt positions that he liquidated.

• 7/ Alice would would lose their collateral (WBTC and WETH), which she deposited when the step 0/.

Tools Used

• Foundry

Recommended Mitigation Steps

Consider adding a grace period for a few blocks after a debt position would be moved to the un-healthy (liquidatable) status - so that this type of a back-running attack can be avoided.

Assessed type

MEV

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

othernet-global (sponsor) disputed

[othernet-global]

The ratio of wbtc/collateralShares before liquidate is added is the same as the ratio of (wbtc+addedWBTC)/(collateralShares + addCollateralShares). The same applies to weth.

When a user adds liquidity or collateral, it does not dilute the tokens held by any previous user.

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid