USDS borrower cannot add collateral during cooldown even to avoid liquidation

[c4-bot-9]

Lines of code

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

Vulnerability details

USDS borrows are collateralized by liquidity in the WBTC/WETH pool.

Users add liquidity by calling CollateralAndLiquidity.depositCollateralAndIncreaseShare().

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

This function calls Liquidity._depositLiquidityAndIncreaseShare(), which calls StakingRewards._increaseUserShare() with the useCooldown parameter set to true.

function _depositLiquidityAndIncreaseShare( IERC20 tokenA, IERC20 tokenB, uint256 maxAmountA, uint256 maxAmountB, uint256 minLiquidityReceived, bool useZapping ) internal returns (uint256 addedAmountA, uint256 addedAmountB, uint256 addedLiquidity)
    {

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

    [...]
    }

function _increaseUserShare( address wallet, bytes32 poolID, uint256 increaseShareAmount, bool useCooldown ) internal
    {
    [...]

    UserShareInfo storage user = _userShareInfo[wallet][poolID];

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

    [...]
    }

The cooldown period also applies when removing liquidity from the pool.

The cooldown period ensures that users have to wait at least until the cooldown period has passed after adding or removing liquidity before they can add or remove liquidity again. Considering that the WBTC/WETH liquidity serves as collateral for USDS borrows, adding collateral is affected by the cooldown period.

The cooldown period is set in the StakingConfig contract to be 1 hour by default (https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/StakingConfig.sol#L31) but can be change to a value between 15 minutes and 6 hours by the DAO (https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/StakingConfig.sol#L85-L99).

Impact

If a user becomes liquidatable during the cooldown period, they are unable to add collateral and might be liquidated directly after the cooldown period ends unless they manage to frontrun the liquidation by adding collateral (users cannot be liquidated during the cooldown period). The user might have to pay an additional gas fee or MEV bribe to ensure that their transaction to add collateral is included before a possible liquidation transaction by another user who might be also be financially incentivized to pay a higher gas fee or MEV bribe to collect the liquidation reward.

Alternatively the user might repay some of their USDS borrow to make their borrow healthy again, however that might require them to swap WETH/WBTC for USDS which adds fees and possibly taxable events.

Proof of Concept

There is an existing test that demonstrates that users cannot deposit collateral during the cooldown period: https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/tests/CollateralAndLiquidity.t.sol#L390-L392.

Recommended Mitigation Steps

• Allow StakingRewards._increaseUserShare() to be also called during the cooldown period. The cooldown period is supposed to avoid users quickly depositing and withdrawing to steal rewards. This goal should also be achievable by just disallowing withdrawals during the cooldown period, but allowing additional deposits.

Assessed type

Other

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

othernet-global (sponsor) disputed

[othernet-global]

Yes, this is acceptable as the default cooldown is only one hour and the chance of liquidation within a user's first hour of holding the position is negligible.

[Picodes]

The suggested mitigation makes sense. But it also seems acceptable to require a sufficient over-collateralization for 1 hour as long as it's stated clearly. I'll downgrade to Low.

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

Picodes marked the issue as grade-c