sherlock-audit / 2024-05-pooltogether-judging

13 stars 8 forks source link

MiloTruck - `TpdaLiquidationPair.swapExactAmountOut()` can be DOSed by a vault's mint limit #39

Open sherlock-admin3 opened 4 months ago

sherlock-admin3 commented 4 months ago

MiloTruck

medium

TpdaLiquidationPair.swapExactAmountOut() can be DOSed by a vault's mint limit

Summary

By repeatedly DOSing TpdaLiquidationPair.swapExactAmountOut() for a period of time, an attacker can swap the liquidatable balancce in a vault for profit.

Vulnerability Detail

When liquidation bots call TpdaLiquidationPair.swapExactAmountOut(), they specify the amount of tokens they wish to receive in _amountOut. _amountOut is then checked against the available balance to swap from the vault:

TpdaLiquidationPair.sol#L141-L144

        uint256 availableOut = _availableBalance();
        if (_amountOut > availableOut) {
            revert InsufficientBalance(_amountOut, availableOut);
        }

The available balance to swap is determined by the liquidatable balance of the vault:   TpdaLiquidationPair.sol#L184-L186

    function _availableBalance() internal returns (uint256) {
        return ((1e18 - smoothingFactor) * source.liquidatableBalanceOf(address(_tokenOut))) / 1e18;
    }

However, when the output token from the swap (ie. tokenOut) is vault shares, PrizeVault.liquidatableBalanceOf() is restricted by the mint limit:

PrizeVault.sol#L687-L709

    function liquidatableBalanceOf(address _tokenOut) external view returns (uint256) {
        uint256 _totalDebt = totalDebt();
        uint256 _maxAmountOut;
        if (_tokenOut == address(this)) {
            // Liquidation of vault shares is capped to the mint limit.
            _maxAmountOut = _mintLimit(_totalDebt);
        } else if (_tokenOut == address(_asset)) {
            // Liquidation of yield assets is capped at the max yield vault withdraw plus any latent balance.
            _maxAmountOut = _maxYieldVaultWithdraw() + _asset.balanceOf(address(this));
        } else {
            return 0;
        }

        // The liquid yield is limited by the max that can be minted or withdrawn, depending on
        // `_tokenOut`.
        uint256 _availableYield = _availableYieldBalance(totalPreciseAssets(), _totalDebt);
        uint256 _liquidYield = _availableYield >= _maxAmountOut ? _maxAmountOut : _availableYield;

        // The final balance is computed by taking the liquid yield and multiplying it by
        // (1 - yieldFeePercentage), rounding down, to ensure that enough yield is left for
        // the yield fee.
        return _liquidYield.mulDiv(FEE_PRECISION - yieldFeePercentage, FEE_PRECISION);
    }

This means that if the amount of shares minted is close to type(uint96).max, the available balance in the vault (ie. _liquidYield) will be restricted by the remaining number of shares that can be minted.

However, an attacker can take advantage of this to force all calls to TpdaLiquidationPair.swapExactAmountOut() to revert:

Note that the type(uint96).max mint limit is reachable for tokens with low value. For example, PEPE has 18 decimals and a current price of $0.00001468, so type(uint96).max is equal to $1,163,070 worth of PEPE. For tokens with a higher value, the attacker can borrow funds in the front-run transaction, and back-run the victim's transaction to return the funds.

This is an issue as the price paid by liquidation bots for the liquidatable balance decreases linearly over time:

TpdaLiquidationPair.sol#L191-L195

        uint256 elapsedTime = block.timestamp - lastAuctionAt;
        if (elapsedTime == 0) {
            return type(uint192).max;
        }
        uint192 price = uint192((targetAuctionPeriod * lastAuctionPrice) / elapsedTime);

As such, an attacker can repeatedly perform this attack (or deposit sufficient funds until the mint limit is 0) to prevent any liquidation bot from swapping the liquidatable balance. After the price has decreased sufficiently, the attacker can then swap the liquidatable balance for himself at a profit.

Impact

By depositing funds into a vault to reach the mint limit, an attacker can DOS all calls to TpdaLiquidationPair.swapExactAmountOut() and prevent liquidation bots from swapping the vault's liquidatable balance. This allows the attacker to purchase the liquidatable balance at a discount, which causes a loss of funds for users in the vault.

Code Snippet

https://github.com/sherlock-audit/2024-05-pooltogether/blob/1aa1b8c028b659585e4c7a6b9b652fb075f86db3/pt-v5-tpda-liquidator/src/TpdaLiquidationPair.sol#L141-L144

https://github.com/sherlock-audit/2024-05-pooltogether/blob/1aa1b8c028b659585e4c7a6b9b652fb075f86db3/pt-v5-tpda-liquidator/src/TpdaLiquidationPair.sol#L184-L186

https://github.com/sherlock-audit/2024-05-pooltogether/blob/1aa1b8c028b659585e4c7a6b9b652fb075f86db3/pt-v5-vault/src/PrizeVault.sol#L687-L709

https://github.com/sherlock-audit/2024-05-pooltogether/blob/1aa1b8c028b659585e4c7a6b9b652fb075f86db3/pt-v5-tpda-liquidator/src/TpdaLiquidationPair.sol#L191-L195

Tool used

Manual Review

Recommendation

Consider implementing liquidatableBalanceOf() and/or _availableBalance() such that it is not restricted by the vault's mint limit.

For example, consider adding a _tokenOut parameter to TpdaLiquidationPair.swapExactAmountOut() for callers to specify the output token. This would allow liquidation bots to swap for the vault's asset token, which is not restricted by the mint limit, when the mint limit is reached.

trmid commented 4 months ago

If the prize vault owner is deploying with an asset that is expected to be close to the TWAB limit in deposits, they can set a liquidation pair that liquidates the yield as assets instead of shares to bypass this limit.

However, it would not be advisable to deploy a prize vault with an asset that is expected to reach the clearly documented TWAB limits.

nevillehuang commented 4 months ago

@WangSecurity @trmid I overlooked this issue. This should be a duplicate of #19, still considering validity and severity

Hash01011122 commented 4 months ago

@nevillehuang This appears to be dup of #88 not #19

MiloTruck commented 4 months ago

This appears to be dup of #88 not #19

It's not a dupe of #88, #88 is describing how the calculation in liquidatableBalanceOf() doesn't take into account the mint limit when _tokenOut == address(_asset). I intentionally didn't submit it since I believe it's low severity.

This issue describes how you can intentionally hit the mint limit to prevent liquidations from occurring.

MiloTruck commented 4 months ago

I overlooked this issue. This should be a duplicate of #19, still considering validity and severity

@nevillehuang Would just like to highlight that the main bug here is being able to DOS liquidations due to the mint limit, being able to reduce the auction price is just one of the impacts this could have. This is not to be confused with #38, which doesn't need liquidations to be DOSed to occur.

Perhaps this issue is more similar to #22 than #19.

0x73696d616f commented 4 months ago

This is not a duplicate of #88, it's a duplicate of #19. I also did not intentionally submit this issue because it is a design choice.

This one, #19 and all the dupes that depend on reaching the mint limit and bots not being able to liquidate because an attacker is DoSing them or similar are at most medium because:

  1. The owner can set liquidations to be based on the asset out.
  2. If any user withdraws, bots can liquidate anyway.
  3. Bots can use flashbots.
  4. For these issues to be considered, a large % of the pool is required by the attacker.
  5. DoS requiring the attacker to keep DoSing are considered 1 block DoSes.

From these points, we can see that it is impossible to keep the DoS for a long period, so it is either considered a design choice or medium.

0xspearmint1 commented 4 months ago
  1. Even if the owner sets liquidations to be based on asset out the _enforceMintLimit function is still called and will revert the liquidation, as long as yieldFee is > 0 which it will be.
if (_tokenOut == address(_asset)) {
            _enforceMintLimit(_totalDebtBefore, _yieldFee); 
WangSecurity commented 4 months ago

I agree this should be a duplicate of #19 (even though it may be more similar to #22, #22 is a duplicate of #19 itself). Since there is no escalation, planning to just duplicate it.

WangSecurity commented 4 months ago

Based on the discussion under #19, this report will be the main of the new family. Medium severity, because the attacker has to constantly keep the mint limit reached, which even with small tokens (SHIB or LADYS) needs large capital (flash loans cannot be used).