`Liquidizer` will always call `dao.withdrawPOL` and burn the pool's liquidity

[c4-bot-2]

Lines of code

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

Vulnerability details

Summary

CollateralAndLiquidity.repayUSDS calls liquidizer.incrementBurnableUSDS. Each repayUSDS will increase Liquidizer::usdsThatShouldBeBurned without transferring USDS there. This will lead to withdrawals from SALT/USDS and DAI/USDS pools, burning of USDS and SALT.

Vulnerability Details

During normal usage of the contracts, CollateralAndLiquidity.repayUSDS will be called frequently. Each repayUSDS call will increase Liquidizer::usdsThatShouldBeBurned without transferring USDS to Liquidizer.

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

        usds.safeTransferFrom(msg.sender, address(usds), amountRepaid);

        // Have USDS remember that the USDS should be burned
        liquidizer.incrementBurnableUSDS( amountRepaid );

incrementBurnableUSDS increases usdsThatShouldBeBurned https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/Liquidizer.sol#L85

        usdsThatShouldBeBurned += usdsToBurn;

Upkeep.step1 calls liquidizer().performUpkeep(), which calls _possiblyBurnUSDS() https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/Liquidizer.sol#L107-L125

uint256 usdsBalance = usds.balanceOf(address(this));
if ( usdsBalance >= usdsThatShouldBeBurned )
    {
    // Burn only up to usdsThatShouldBeBurned.
    // Leftover USDS will be kept in this contract in case it needs to be burned later.
    _burnUSDS( usdsThatShouldBeBurned );
    usdsThatShouldBeBurned = 0;
    }
else
    {
    // The entire usdsBalance will be burned - but there will still be an outstanding balance to burn later
    _burnUSDS( usdsBalance );
    usdsThatShouldBeBurned -= usdsBalance;

    // As there is a shortfall in the amount of USDS that can be burned, liquidate some Protocol Owned Liquidity and
    // send the underlying tokens here to be swapped to USDS
    dao.withdrawPOL(salt, usds, PERCENT_POL_TO_WITHDRAW);
    dao.withdrawPOL(dai, usds, PERCENT_POL_TO_WITHDRAW);
    }

repayUSDS never transfers USDS to Liquidizer, which will result in the condition usdsBalance >= usdsThatShouldBeBurned being false. This will trigger the execution of dao.withdrawPOL.

Given that PERCENT_POL_TO_WITHDRAW is set to 1, after 1000 upkeeps, the liquidity in the pool will decrease by a factor of 1/0.99^1000, which is approximately 23,163 times. This liquidity will then be burned in _burnUSDS in _possiblyBurnUSDS and salt.burnTokensInContract() in Liquidizer.performUpkeep.

Impact

• The SALT/USDS and DAI/USDS pools are constantly being depleted, potentially leaving insufficient liquidity to cover undercollateralized liquidations. This could lead to several issues:
  • Accrual of bad debt.
  • High slippage in these pools.
  • Reduced stability of the DAI/USDS pair.
  • Loss of liquidity, equating to a financial loss for the DAO, from the pools.
• There is excessive burning of USDS and SALT.
• WBTC, WETH, and DAI keep being exchanged for USDS (as indicated in PoolUtils._placeInternalSwap within Liquidizer.performUpkeep), and the USDS acquired from these exchanges is subsequently burned.

Proof of Concept

Put the code in src/stable/tests/USDSBurnedTwice.t.sol and run COVERAGE="yes" forge test -f wss://ethereum-sepolia.publicnode.com -vvv --mt testNumberOfOpenPositions2

// SPDX-License-Identifier: BUSL 1.1
pragma solidity =0.8.22;

import "../../dev/Deployment.sol";
import "../../price_feed/tests/ForcedPriceFeed.sol";
import {TestCollateral} from "./CollateralAndLiquidity.t.sol";

contract USDSBurnedTwice is TestCollateral
    {
    constructor()
    TestCollateral()
        {
        }

    // A unit test for collateral.numberOfUsersWithBorrowedUSDS to verify that it returns the correct number of open positions.
    function testNumberOfOpenPositions2() public {
        // Alice, Bob and Charlie each deposit and borrow
        _depositCollateralAndBorrowMax(alice);
        vm.warp( block.timestamp + 1 days );

        uint usdsThatShouldBeBurnedBefore = liquidizer.usdsThatShouldBeBurned();
        uint usdsOnUsdsBefore = usds.balanceOf(address(usds));
        uint usdsOnLiquidizerBefore = usds.balanceOf(address(liquidizer));

        uint256 aliceBorrowedAmount = usds.balanceOf(alice);
        vm.startPrank(alice);
        collateralAndLiquidity.repayUSDS(aliceBorrowedAmount);

        // Repeat as many times as they wish
        for (uint i; i<10; i++){
            collateralAndLiquidity.borrowUSDS( collateralAndLiquidity.maxBorrowableUSDS(alice) );
            collateralAndLiquidity.repayUSDS(usds.balanceOf(alice));
        }

        uint usdsThatShouldBeBurnedAfter = liquidizer.usdsThatShouldBeBurned();
        uint usdsOnUsdsAfter = usds.balanceOf(address(usds)); 
        uint usdsOnLiquidizerAfter = usds.balanceOf(address(liquidizer)); 
        uint maxBorrowable = collateralAndLiquidity.maxBorrowableUSDS(alice);               
        console.log("usdsThatShouldBeBurnedBefore: %e", usdsThatShouldBeBurnedBefore);
        console.log("usdsThatShouldBeBurnedAfter: %e", usdsThatShouldBeBurnedAfter);
        console.log("usdsOnUsdsBefore: %e", usdsOnUsdsBefore);
        console.log("usdsOnUsdsAfter: %e", usdsOnUsdsAfter);
        console.log("usdsOnLiquidizerBefore: %e", usdsOnLiquidizerBefore);
        console.log("usdsOnLiquidizerAfter: %e", usdsOnLiquidizerAfter);
        // Print maxBorrowable to show that usdsThatShouldBeBurnedAfter = 11 * maxBorrowable
        // (1 for _depositCollateralAndBorrowMax and 10 in for-loop)
        console.log("maxBorrowable: %e", maxBorrowable);

        // Now every upkeep we will withdraw liquidity, depleating pools and leading to 
        // everything else described in "Impact"
        upkeep.performUpkeep();

        console.log("usdsOnUsds after upkeep: %e", usds.balanceOf(address(usds)));
        // Note: now the pools are empty, but you can see the logic in _possiblyBurnUSDS
        // Because usdsThatShouldBeBurned grow uncontrollably it will always be > usdsBalance of liquidizer
        console.log("usdsThatShouldBeBurned after upkeep: %e", liquidizer.usdsThatShouldBeBurned());
        console.log("usdsOnLiquidizer after upkeep: %e", usds.balanceOf(address(liquidizer)));
    }

}

Tools Used

Manual review

Recommended Mitigation Steps

Consider rewriting the logic and possibly removing liquidizer.incrementBurnableUSDS( amountRepaid ) from CollateralAndLiquidity.repayUSDS.

Assessed type

Error

[c4-judge]

Picodes marked the issue as primary issue

[c4-judge]

Picodes marked the issue as duplicate of #240

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as duplicate of #137