When a user’s borrow position becomes insolvent within cooldown period, the position cannot be liquidated

[c4-bot-2]

Lines of code

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

Vulnerability details

Impact

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

When a borrowed position becomes deliquent during a cooldown period, the position cannot be liquidated, this will lead to bad debt in the protocol until the cooldown is over.

Vulnerability Details

Currently the cooldown period is 1 hr but it can go as high as 6 hr. When a user adds liquidity by calling depositCollateralAndIncreaseShare(...) in the CollateralAndLiquidity.sol contract, an internal call is made to

_depositLiquidityAndIncreaseShare(...) as shown below

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

        ...
        }

this in turn makes a call to the StakingRewards._increaseUserShare(...) function which executes with useCoolDown 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)
        {
        ...

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

        ...

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

When the user makes a borrow immediately after increasing their shares and the collateralisation decreases, all calls to CollateralAndLiquidity.liquidateUser(...) will revert.

Coded POC

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

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

        (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 Liquidity", addedLiquidityIncrement + aliceInitialAddedLiquidityIncrement );

        // Deposit extra so alice can withdraw all her 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" );
        }

        vm.warp( block.timestamp + 1001);

        // @audit Within an hour of borrowing the price of collateral crashes
        // @audit this can go up to six hours with same results
        _crashCollateralPrice();

        vm.prank(bob);
        vm.expectRevert("Must wait for the cooldown to expire");
        collateralAndLiquidity.liquidateUser(alice);

    }

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
        {
        ...

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

        ...
        }

Assessed type

Timing

[c4-judge]

Picodes marked the issue as duplicate of #312

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes removed the grade

[c4-judge]

Picodes marked the issue as satisfactory