If there is only one USDS borrower, he can never be liquidated

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L187

Vulnerability details

Bug Description

The Salty protocol offers the USDS stablecoin, collateralized by WETH/WBTC. Users can deposit collateral and borrow USDS against it. A key safeguard mechanism is liquidation, which is activated when the collateral value drops below a certain threshold (typically 110%). However, a significant flaw exists in the liquidation process, particularly when there is only one user borrowing USDS.

The process to liquidate a user involves calling the liquidateUser() function, which in turn calls pools.removeLiquidity() to withdraw the user's collateral. However, the pools.removeLiquidity() function checks if the remaining reserves after withdrawal are below the DUST threshold and reverts if they are.

require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve0 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal");

This check creates a situation where, if all the collateral for USDS is held by a single user, it becomes impossible to liquidate this user. Any attempt to liquidate would reduce the reserves below DUST, causing the transaction to revert

Impact

This flaw means the only holder of USDS can evade liquidation, potentially leading to bad debt in the protocol that cannot be recouped or mitigated.

Proof of Concept

The scenario can be demonstrated as follows:

• Alice is the sole depositor of collateral and borrows the maximum allowable USDS.
• Another user, Bob, attempts to liquidate Alice when her collateral value crashes.
• However, the liquidation process reverts because removing Alice's collateral would reduce the reserves below the DUST threshold.

function testLiquidateOnlyHolder() public {
    //------------------------- SETUP --------------------------------------------------

    // Total needs to be worth at least $2500
    uint256 depositedWBTC = ( 1000 ether *10**8) / priceAggregator.getPriceBTC();
    uint256 depositedWETH = ( 1000 ether *10**18) / priceAggregator.getPriceETH();

    (uint256 reserveWBTC, uint256 reserveWETH) = pools.getPoolReserves(wbtc, weth);

    //------------------------ TEST---------------------------------------------------------

    // Alice deposits collateral
    vm.startPrank(alice);
    collateralAndLiquidity.depositCollateralAndIncreaseShare( depositedWBTC * 2, depositedWETH * 2, 0, block.timestamp, false );
    vm.stopPrank();

    //Alcie borrows the USDS
    vm.startPrank(alice);
    vm.warp( block.timestamp + 1 hours);

    uint256 maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);
    collateralAndLiquidity.borrowUSDS( maxUSDS );
    vm.stopPrank();

    // Try and fail to liquidate alice
    vm.expectRevert( "User cannot be liquidated" );
    vm.prank(bob);
    collateralAndLiquidity.liquidateUser(alice);

    // Artificially crash the collateral price
    _crashCollateralPrice();

    // Bob tries liquidating Alice's position but it reverts due to the reserves falling below dust if she gets liquidated
    vm.prank(bob);
    vm.expectRevert();
    collateralAndLiquidity.liquidateUser(alice);
}

The test must be added to /stable/tests/CollateralAndLiquidity.t.sol and can be run by calling COVERAGE="yes" NETWORK="sep" forge test -vvvv --rpc-url https://rpc.sepolia.org --match-test "testLiquidateOnlyHolder".

Tools Used

Manual Review

Recommended Mitigation Steps

To resolve this issue, the logic in pools.removeLiquidity() needs to be adjusted. The function should allow the withdrawal of the last collateral, even if it reduces the reserves to zero. This adjustment can be implemented with a conditional check that permits reserves to be either above DUST or exactly zero:

require(((reserves.reserve0 >= PoolUtils.DUST && reserves.reserve0 >= PoolUtils.DUST) || (reserves.reserve0 == 0 && reserves.reserve0 == 0)), "Insufficient reserves after liquidity removal");

With this change, the protocol will maintain its ability to liquidate the sole holder of USDS, ensuring the safeguard against bad debt remains effective.

Assessed type

Other

[c4-judge]

Picodes marked the issue as duplicate of #278

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as selected for report

[c4-sponsor]

othernet-global (sponsor) acknowledged

[othernet-global]

The stablecoin framework: /stablecoin, /price_feed, WBTC/WETH collateral, PriceAggregator, price feeds and USDS have been removed: https://github.com/othernet-global/salty-io/commit/88b7fd1f3f5e037a155424a85275efd79f3e9bf9