Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/CollateralAndLiquidity.sol#L154 https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/CollateralAndLiquidity.sol#L70-L76 https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/Liquidity.sol#L107 https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L146-L147 https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/StakingRewards.sol#L64-L71 https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/StakingRewards.sol#L104-L111

Vulnerability details

Impact

The CollateralAndLiquidity.liquidateUser function is used to liquidate a position which has fallen under the minimum collateral ratio. During the liquidateUser function execution the StakingRewards._decreaseUserShare function is called to decrease the user's share of collateral as it has been liquidated and they no longer have it, as shown below:

    _decreaseUserShare( wallet, collateralPoolID, userCollateralAmount, true );

As it is evident from the above code snippet the useCooldown bool is set to true. Hence the StakingRewards._decreaseUserShare function will perform the following check to ensure that the cool down period has expired.

    if ( useCooldown ) //@audit-info - check if the cooldown is mentioned
    if ( msg.sender != address(exchangeConfig.dao()) ) // DAO doesn't use the cooldown
        {
        require( block.timestamp >= user.cooldownExpiration, "Must wait for the cooldown to expire" );
        //@audit-info - ensure hte cool down period has passed
        // Update the cooldown expiration for future transactions
        user.cooldownExpiration = block.timestamp + stakingConfig.modificationCooldown();
        }

if the block.timestamp < user.cooldownExpiration the transaction will revert and the liquidation won't be executed.

A malicious borrower can exploit the user.cooldownExpiration to revert the liquidation attempts and thus keep his position alive until he can get his collateralization ratio above the minimumCollateralRatioPercent or can wait till his collateralization ratio is below < 100 thus incurring loss on the protocol. The malicious borrower can manipulate the user.cooldownExpiration to prevent the liquidation as explained below:

A malicious borrower calls the CollateralAndLiquidity.depositCollateralAndIncreaseShare and deposits collateral (WBTC/WETH) worth of USD 10,000 and borrows USDS 5000. (consider the initialCollateralRatioPercent = 200).
This will update the user.cooldownExpiration since the StakingRewards._increaseUserShare is called. The user.cooldownExpiration = block.timestamp + stakingConfig.modificationCooldown() is executed and the user.cooldownExpiration is set to one hour from the transaction execution time (block.timestamp).
The malicious borrower does not attempt to deposit or withdraw any collateral after this initial transaction and hence after one hour his user.cooldownExpiration will be expired.
After some time the deposited collateral value of the malicious borrower, in USD, drops significantly to USD 5450. This makes the collateralization ratio = (5450 / 5000) * 100 = 109 and it is < minimumCollateralRatioPercent (Assume minimumCollateralRatioPercent is the default value of 110).
A liquidator sees the opportunity and attempts to liquidate the malicious borrower.
Malicious borrower can easily front-run the liquidation transaction by calling the CollateralAndLiquidity.depositCollateralAndIncreaseShare function by depositing WBTC and WETH just more than PoolUtils.DUST (100) value. For example 101 tokens considering 18 decimal places.
This will call the StakingRewards._increaseUserShare and the user.cooldownExpiration will be incremented by another 1 hour.
As a result when the liquidation transaction is being executed the block.timestamp < user.cooldownExpiration will occur since the user.cooldownExpiration has not expired and the liquidation transaction will revert.
The malicious borrower can keep on reverting the liquidation transaction by front-running the liquidation and updating the user.cooldownExpiration by depositing WBTC and WETH tokens just more than the PoolUtils.DUST amount of 100 (Eg : 101).

The following impacts are possible as a result of the above attack vector:

This will put the protocol in danger since if the existing collateral value of the borrower falls steeply and collateralization ration goes below 100 then even if the liquidation is successfully executed the protocol will not able to fully recover the lent amount and thus results in loss of funds to the protocol.
Furthermore the malicious borrower can delay the liquidation as much as he wants and then can later gets his liquidation ratio above the minimumCollateralRatioPercent such that his position is secured thus getting unfair advantage.
The liquidator who is trying to liquidate this undercollateralized position will get his transaction reverted thus incurring transaction gas loss thus resulting in loss of funds to him rather than getting his due liquidation reward.

Proof of Concept

        _decreaseUserShare( wallet, collateralPoolID, userCollateralAmount, true );

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

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

https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/CollateralAndLiquidity.sol#L70-L76

        _increaseUserShare( msg.sender, poolID, addedLiquidity, true );

https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/Liquidity.sol#L107

        require( maxAmountA > PoolUtils.DUST, "The amount of tokenA to add is too small" );
        require( maxAmountB > PoolUtils.DUST, "The amount of tokenB to add is too small" );

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

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

https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/StakingRewards.sol#L64-L71

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

https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/StakingRewards.sol#L104-L111

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

The useCooldown bool value is used to prevent reward hunting (ie - quickly depositing and withdrawing large amounts of liquidity to snipe rewards as they arrive). And there is no reward hunting expected to be performed during the liquidaiton. Hence there is no advantage to be gained by setting the useCooldown bool value to true during the liquidation when calling the CollateralAndLiquidity.liquidateUser. Since the liquidation can be called by anyone other than the wallet which is being liquidated itself. Hence there is no undue advantage a user can gain via reward hunting during the liquidation. Hence we can set the useCooldown bool value to false when calling the StakingRewards._decreaseUserShare function inside the CollateralAndLiquidity.liquidateUser.

Hence the updated _decreaseUserShare function call is as shown below:

    _decreaseUserShare( wallet, collateralPoolID, userCollateralAmount, false );

This will ensure no cool down period check is performed during the liquidation transaction thus not letting a malicious user to revert the liquidation transaction by updating the user.cooldownExpiration timestamp by depositing WBTC and WETH just more than the DUST amount.

Assessed type

Other

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

MALICIOUS BORROWER CAN AVOID LIQUIDATION BY UPDATING THE `user.cooldownExpiration` TIMESTAMP THUS INCURRING LOSS OF FUNDS ON THE PROTOCOL #903