Restriction on multiple collateral deposits in a short period of time poses liquidation risk on `CollateralAndLiquidity.sol`

[c4-bot-3]

Lines of code

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

Vulnerability details

Impact

In the CollateralAndLiquidity.sol contract, users can deposit wBTC and wETH to borrow USDS. The protocol includes a liquidation procedure if the collateral value falls below the required minimum. The issue arises with the CollateralAndLiquidity::depositCollateralAndIncreaseShare function, which does not allow multiple deposits to save a user's position and prevent liquidation. Consider the following scenario:

1. A user creates a position, deposits wBTC and wETH, and borrows USDS.
2. Due to market volatility, the value of wETH decreases, making the user's position susceptible to liquidation. The user attempts to deposit additional collateral to avoid liquidation using the CollateralAndLiquidity::depositCollateralAndIncreaseShare function.
3. While the user gains some time, the value of wETH continues to fall. The user wishes to deposit more collateral, but the transaction is reverted as the CollateralAndLiquidity::depositCollateralAndIncreaseShare function does not allow multiple deposits in a short period of time.

The user loses his collateral because the protocol does not permit multiple collateral deposits to overcollateralize the position.

Proof of Concept

A test case has been provided to illustrate the issue. In this scenario, the user, Alice, attempts to deposit additional collateral to avoid liquidation. However, due to the current implementation, the transaction is reverted, preventing Alice from saving her position:

1. Alice deposit and borrow the max amount.
2. The collateral price drops and Alice can be liquidated.
3. Alice deposit more collateral in order to avoid liquidation.
4. The price drops more and Alice can be liquidated again.
5. Alice wants to deposit more collateral in order to avoid liquidation but the transaction will be reverted by Must wait for the cooldown to expire.

// Filename: src/stable/tests/CollateralAndLiquidity.t.sol:TestCollateral
// $ forge test --match-test "testUserMultipleDepositsInLiquidation" --rpc-url https://yourrpc.com -vv 
//
    function testUserMultipleDepositsInLiquidation() public {
        // Liquidatable user can't deposit multiple time in order to save his position
        //
        // Have bob deposit so alice can withdraw everything without DUST reserves restriction
        _depositHalfCollateralAndBorrowMax(bob);
        //
        // 1. Alice deposit and borrow the max amount
        vm.startPrank( alice );
        collateralAndLiquidity.depositCollateralAndIncreaseShare(1e8, 1e18, 0, block.timestamp, true );
        uint256 maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);
        collateralAndLiquidity.borrowUSDS( maxUSDS );
        vm.stopPrank();
        // Check if Alice has a position and she is not be liquidated
        assertTrue(_userHasCollateral(alice));
        assertFalse(collateralAndLiquidity.canUserBeLiquidated(alice));
        vm.warp( block.timestamp + 1 days );
        //
        // 2. The collateral price drop and Alice can be liquidated
        vm.startPrank( DEPLOYER );
        forcedPriceFeed.setBTCPrice( forcedPriceFeed.getPriceBTC() * 20 / 100);
        forcedPriceFeed.setETHPrice( forcedPriceFeed.getPriceETH() * 20 / 100 );
        vm.stopPrank();
        assertTrue(collateralAndLiquidity.canUserBeLiquidated(alice));
        //
        // 3. Alice deposit more collateral in order to avoid liquidation
        vm.prank(alice);
        collateralAndLiquidity.depositCollateralAndIncreaseShare(2e8, 2e18, 0, block.timestamp, true);
        assertFalse(collateralAndLiquidity.canUserBeLiquidated(alice));
        //
        // 4. The price drops more and Alice can be liquidated again
        vm.startPrank( DEPLOYER );
        forcedPriceFeed.setBTCPrice( forcedPriceFeed.getPriceBTC() * 20 / 100);
        forcedPriceFeed.setETHPrice( forcedPriceFeed.getPriceETH() * 20 / 100 );
        vm.stopPrank();
        assertTrue(collateralAndLiquidity.canUserBeLiquidated(alice));
        //
        // 5. Alice wants to deposit more collateral in order to avoid liquidation but the transaction will fail
        vm.expectRevert( "Must wait for the cooldown to expire" );
        vm.prank(alice);
        collateralAndLiquidity.depositCollateralAndIncreaseShare(2e8, 2e18, 0, block.timestamp, true);
    }

Tools used

Manual review

Recommended Mitigation Steps

The restriction on multiple collateral deposits is implemented to prevent malicious users from obtaining more rewards, as explained in the following comment:

    // Minimum time between increasing and decreasing user share in SharedRewards contracts.
    // Prevents reward hunting where users could frontrun reward distributions and then immediately withdraw.
    // Range: 15 minutes to 6 hours with an adjustment of 15 minutes

However, this limitation has consequences for users attempting to deposit more collateral to avoid liquidation. It is recommended that users be allowed to deposit collateral without any restrictions. To address concerns related to reward distribution, consider implementing a solution where the user's staking time duration is taken into account, and rewards are calculated based on time rather than allowing or disallowing multiple colleteral deposits. This approach would balance the need for security and user flexibility.

Assessed type

Invalid Validation

[c4-judge]

Picodes marked the issue as duplicate of #963

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)