USDS repaid will not be transferred to Liquidizer, but Liquidizer will still burn the amount of USDS in upkeep, causing Liquidizer always draining protocol owned liquidity

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/CollateralAndLiquidity.sol#L125-L128

Vulnerability details

Impact

Liquidizer will constantly be in shortage of USDS to burn during performUpKeep(), and will always drain POL(protocol owned liquidity) during upkeep.

This is due to the accounting of usdsThatShouldBeBurned being out of sync with the actual token transfer (USDS + WBTC + WETH).

Proof of Concept

Liquidizer.sol is supposed to burn USDS and Salt tokens during each performUpKeep(). The problem is in the USDS flow.

Two sources add to usdsThatShouldBeBurned in Liquidizer.sol: a) During repayUSDS(), the same amount of USDS repaid is increased in usdsThatShouldBeBurned; b) when a user's borrow is undercollateralized and liquidated, their collateral tokens(WBTC, WETH) are transferred to Liquidizer, and the debt amount of USDS is increased in usdsThatShouldBeBurned.

The issue is in flow a):usdsThatShouldBeBurned is increased but no USDS tokens are transferred to Liquidizer.sol. This causes a continuous deficiency in USDS in Liquidzier.

//src/stable/CollateralAndLiquidity.sol
    function repayUSDS(uint256 amountRepaid) external nonReentrant {
...
        // Have the user send the USDS to the USDS contract so that it can later be burned (on USDS.performUpkeep)
        //@audit : this only transfer USDS from caller to CollateralAndLiquidity.sol, but not to liquidizer
|>      usds.safeTransferFrom(msg.sender, address(usds), amountRepaid);

        // Have USDS remember that the USDS should be burned
        //@audit :this only increases the state variable burnable amount in Liquidizer.sol, but doesn't actually transfer the received USDS token to Liquidizer.sol
|>      liquidizer.incrementBurnableUSDS(amountRepaid);
...

(https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/CollateralAndLiquidity.sol#L125-L128)

As a reference for a correct flow: b) both transfers the token(WBTC+WETH) to Liquidizer and increases usdsThatShouldBeBurned in Liquidizer, such that during liquidizer upkeep, WBTC and WETH will be converted to USDS and then burned. In contrast, a) will not transfer any USDS for liquidizer to burn.

//src/stable/CollateralAndLiquidity.sol
    function liquidateUser( address wallet ) external nonReentrant
...
        // Send the remaining WBTC and WETH to the Liquidizer contract so that the tokens can be converted to USDS and burned (on Liquidizer.performUpkeep)
        wbtc.safeTransfer( address(liquidizer), reclaimedWBTC - rewardedWBTC );
        weth.safeTransfer( address(liquidizer), reclaimedWETH - rewardedWETH );

        // Have the Liquidizer contract remember the amount of USDS that will need to be burned.
        uint256 originallyBorrowedUSDS = usdsBorrowedByUsers[wallet];
        liquidizer.incrementBurnableUSDS(originallyBorrowedUSDS);
...

(https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/CollateralAndLiquidity.sol#L176-L181)

If we compare a) and b), we know in performUpKeep() a) causes usdsThatShouldBeBurned to inflate with no underlying usds transferred, which causes usdsThatShouldBeBurned > usdsBalance in _possiblyBurnUSDS(). And else body will run which withdraw protocol owned liquidity (dao.withdrawPOL(salt, usds, PERCENT_POL_TO_WITHDRAW) and dao.withdrawPOL(dai, usds, PERCENT_POL_TO_WITHDRAW);).

//src/stable/Liquidizer.sol
    function _possiblyBurnUSDS() internal {
...
        if (usdsBalance >= usdsThatShouldBeBurned) {
...
        } else {
...
|>          dao.withdrawPOL(salt, usds, PERCENT_POL_TO_WITHDRAW);
|>          dao.withdrawPOL(dai, usds, PERCENT_POL_TO_WITHDRAW);

(https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/Liquidizer.sol#L123-L124)

As a result, we see due to not enough usdsBalance to burn, Liquidizer will keep withdrawing POL to compensate.

This violates the intention of (1) having extra usds as a burnerable buffer in liquidizer. (Code comment: // Extra USDS (beyond usdsThatShouldBeBurned) remains in this contract as a burnable buffer in the event of undercollateralized liquidation.); (2) protocol POL will be drained continuously during up keep, when it is unnecessary, causing protocol loss of funds steadily.

Tools Used

Manual Review

Recommended Mitigation Steps

In repayUSDS(), transfer user repaid USDS to Liquidizer.sol.

Assessed type

Error

[c4-judge]

Picodes marked the issue as duplicate of #618

[c4-judge]

Picodes marked the issue as satisfactory