Borrowers can cause DOS by frontrunning Liquidations to increase their collateral amount by a small amount thereby increasing their cooldown periods which causes liquidation transactions to revert.

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/Liquidity.sol#L104-L108
https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/CollateralAndLiquidity.sol#L153-L155
https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/StakingRewards.sol#L104-L112

Vulnerability details

Impact

The cooldown period is used to prevent users from reward hunting where users could frontrun reward distributions and then immediately withdraw. However, borrowers can use the cooldown period to their advantage by frontrunning liquidation and increasing their positions with a minimal amount therefore increasing their cooldown period. This forces liquidators to wait for cooldown period to expire of which borrowers can then continously repeat the same trick thereby causing DOS . Now the default cooldown period is 1 hour with a range from 15 minutes to 6 hours ( adjustable by the DAO ) which is more than the acceptable DOS of 15 minutes.

Proof of Concept

Assume a user Bob deposited collateral and borrowed a given amount of USDS. The price of his collateral fell massively and his position is open for liquidation. A liquidator Alice sees the chance to liquidate Bob's position and submits a transaction to do so. Bob monitoring the mempool, frontruns alice and deposits a small amount i.e( 0.00001wbtc & 0.0001eth ) to increase his position and thereby increases his cooldown period. since Bob's cooldown period is checked during liquidation, Alice's tx does not pass the check and reverts.Let's examine the code below.
To increase his collateral a Bob calls :

           function depositCollateralAndIncreaseShare( uint256 maxAmountWBTC, uint256 maxAmountWETH, uint256 minLiquidityReceived, uint256 deadline, bool useZapping )  external nonReentrant ensureNotExpired(deadline)  returns  (uint256 addedAmountWBTC, uint256 addedAmountWETH, uint256  addedLiquidity)
            {
            // Have the user deposit the specified WBTC/WETH liquidity and 
              increase their collateral share
            (addedAmountWBTC, addedAmountWETH, addedLiquidity) = 
            _depositLiquidityAndIncreaseShare( wbtc, weth, maxAmountWBTC, 
            maxAmountWETH, minLiquidityReceived, useZapping );


            emit CollateralDeposited(msg.sender, addedAmountWBTC, addedAmountWETH, addedLiquidity);
            }

which then calls ;

          function _depositLiquidityAndIncreaseShare( IERC20 tokenA, IERC20  tokenB, uint256 maxAmountA, uint256 maxAmountB, uint256 minLiquidityReceived, bool useZapping ) internal returns (uint256 addedAmountA, uint256 addedAmountB, uint256 addedLiquidity)
	{
	require( exchangeConfig.walletHasAccess(msg.sender), "Sender does 
            not have exchange access" );
            
           // Transfer the specified maximum amount of tokens from the user
	tokenA.safeTransferFrom(msg.sender, address(this), maxAmountA );
	tokenB.safeTransferFrom(msg.sender, address(this), maxAmountB );

	// Balance the token amounts by swapping one to the other before adding the liquidity?
	if ( useZapping )
		(maxAmountA, maxAmountB) = _dualZapInLiquidity(tokenA, tokenB, maxAmountA, maxAmountB );

	// Approve the liquidity to add
	tokenA.approve( address(pools), maxAmountA );
	tokenB.approve( address(pools), maxAmountB );

	// Deposit the specified liquidity into the Pools contract
	// The added liquidity will be owned by this contract. (external call to Pools contract)
	bytes32 poolID = PoolUtils._poolID( tokenA, tokenB );
	(addedAmountA, addedAmountB, addedLiquidity) = pools.addLiquidity( tokenA, tokenB, maxAmountA, maxAmountB, minLiquidityReceived, totalShares[poolID]);

	// Increase the user's liquidity share by the amount of addedLiquidity.
	// Cooldown is specified to prevent reward hunting (ie - quickly depositing and withdrawing large amounts of liquidity to snipe rewards as they arrive)
	// _increaseUserShare confirms the pool as whitelisted as well.
	_increaseUserShare( msg.sender, poolID, addedLiquidity, true );

	// If any of the user's tokens were not used, then send them back
	if ( addedAmountA < maxAmountA )
		tokenA.safeTransfer( msg.sender, maxAmountA - addedAmountA );

	if ( addedAmountB < maxAmountB )
		tokenB.safeTransfer( msg.sender, maxAmountB - addedAmountB );

	emit LiquidityDeposited(msg.sender, address(tokenA), address(tokenB), addedAmountA, addedAmountB, addedLiquidity);
    }

Focusing on the increase user share part, we can see increase user share function has to be called with a true boolean value as stated in the comments before it meaning the Bob's cooldown period will be increased for future transactions. With that in mind , lets' see the process of liquidating Bob .
Alice calls ;

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

	// First, make sure that the user's collateral ratio is below the required level
	require( canUserBeLiquidated(wallet), "User cannot be liquidated" );

	uint256 userCollateralAmount = userShareForPool( wallet, collateralPoolID );

	// Withdraw the liquidated collateral from the liquidity pool.
	// The liquidity is owned by this contract so when it is withdrawn it will be reclaimed by this contract.
	(uint256 reclaimedWBTC, uint256 reclaimedWETH) = pools.removeLiquidity(wbtc, weth, userCollateralAmount, 0, 0, totalShares[collateralPoolID] );

	// Decrease the user's share of collateral as it has been liquidated and they no longer have it.
	_decreaseUserShare( wallet, collateralPoolID, userCollateralAmount, true );
         .....

After removing liquidity , the Bob's share is supposed to be decreased and as we can see cooldown period is specified which is checked by the code below.

    function _decreaseUserShare( address wallet, bytes32 poolID, uint256 decreaseShareAmount, bool useCooldown ) internal
            {
            require( decreaseShareAmount != 0, "Cannot decrease zero share" );


            UserShareInfo storage user = _userShareInfo[wallet][poolID];
            require( decreaseShareAmount <= user.userShare, "Cannot decrease more than existing user share" );


            if ( useCooldown )
            if ( msg.sender != address(exchangeConfig.dao()) ) // DAO doesn't use the cooldown
                    {
                    require( block.timestamp >= user.cooldownExpiration, "Must wait for the cooldown to expire" );


                    // Update the cooldown expiration for future transactions
                    user.cooldownExpiration = block.timestamp + stakingConfig.modificationCooldown();
                    }
                 ...

Therefore, since Bob's cooldown period was increased when he added their collateral the require check above will fail causing the transaction to revert and the Bob cannot be liquidated. This can happen continously and due to that liquidations are DOS'ed. This can be worse if cooldown period is set at maximum time which is 6 hours.

Tools Used

VS

Recommended Mitigation Steps

Since cooldown period is important, consider allowing the DAO to liquidate malicious borrowers since it does not use cooldown .

Assessed type

DoS

[c4-judge]

Picodes marked the issue as duplicate of #312

[c4-judge]

Picodes marked the issue as satisfactory