search

sherlock-audit / 2024-05-pooltogether-judging

13 stars 8 forks source link

MiloTruck - Price formula in `TpdaLiquidationPair._computePrice()` does not account for a jump in liquidatable balance #38

Open sherlock-admin2 opened 4 months ago

sherlock-admin2 commented 4 months ago

MiloTruck

medium

Price formula in TpdaLiquidationPair._computePrice() does not account for a jump in liquidatable balance

Summary

The linearly decreasing auction formula in TpdaLiquidationPair._computePrice() does not account for sudden increases in the vault's liquidatable balance, causing the price returned to be much lower than it should be.

Vulnerability Detail

The price paid by liquidation bots to liquidate the asset balance in a vault is calculated in TpdaLiquidationPair._computePrice() as shown:

TpdaLiquidationPair.sol#L191-L195

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

As seen from above, the price paid decreases linearly over time from targetAuctionPeriod * lastAuctionPrice to 0. This allows the liquidation pair to naturally find a price that liquidation bots are willing to pay for the current liquidatable balance in the vault, which is calculated as shown:

TpdaLiquidationPair.sol#L184-L186

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

The vault's liquidatable balance is determined by liquidatableBalanceOf():

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);
    }

Apart from the yield vault suddenly gaining yield, there are two reasons why the vault's liquidatable balance might suddenly increase:

However, since the vault's liquidatable balance is not included in price calculation and the price decreases linearly, TpdaLiquidationPair._computePrice() cannot accurately account for instantaneous increases in liquidatableBalanceOf().

Using (1) to illustrate this:

As seen from above, when a sudden increase in liquidatable balance occurs, the price in _computePrice() could be too low due to the linearly decreasing auction.

Impact

When liquidatableBalanceOf() suddenly increases due to an increase in the mint limit or a reduction in yieldFeePercentage, liquidation bots can swap the liquidatable balance in the vault for less than its actual value. This causes a loss of funds for the vault, and by extension, all users in the vault.

Code Snippet

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

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

Tool used

Manual Review

Recommendation

Consider using an alternative auction formula in _computePrice(), ideally one that takes into account the current liquidatable balance in the vault.

sherlock-admin4 commented 4 months ago

1 comment(s) were left on this issue during the judging contest.

infect3d commented:

that is the reason of the MIN_PRICE state variable

trmid commented 4 months ago

If the yield source is expected to have large jumps in yield (by design or by the prize vault owner's actions) the prize vault owner can either:

  1. set a suitable smoothingFactor on the liquidation pair to mitigate the effect within a reasonable efficiency target
  2. pause liquidations during times of anticipated flux and set a new liquidation pair when ready (this would be good practice if suddenly lowering the yield fee percentage).
0xjuaan commented 4 months ago

Escalate

This issue should be informational

This is clearly a design decision by the protocol

The sponsor has also suggested some ways the prizeVault owner can mitigate this in the vault setup

Even if there is the odd upward fluctuation of yield the current design of the auction system ensures it will correct itself for future liquidations

sherlock-admin3 commented 4 months ago

Escalate

This issue should be informational

This is clearly a design decision by the protocol

The sponsor has also suggested some ways the prizeVault owner can mitigate this in the vault setup

Even if there is the odd upward fluctuation of yield the current design of the auction system ensures it will correct itself for future liquidations

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

MiloTruck commented 4 months ago

This is clearly a design decision by the protocol

The sponsor has also suggested some ways the prizeVault owner can mitigate this in the vault setup

I don't see how the auction mechanism being a design decision invalidates this issue. I've clearly pointed out how this design decision is incapable of handling upward yield fluctuations, which is an actual problem.

The sponsor has pointed out ways which, in my opinion, only partially mitigate the problem presented. The owner has to either has to temporarily stop liquidations or reduce the percentage of yield that can be liquidated to smooth out the upward fluctuation. Note that both ways are a form of owner intervention, and I don't believe it is documented anywhere on how the owner should deal with upward yield fluctuations.

Even if there is the odd upward fluctuation of yield the current design of the auction system ensures it will correct itself for future liquidations

The prize vault still loses funds for the current liquidation, so this doesn't invalidate anything.

nevillehuang commented 4 months ago

Agree with @MiloTruck comments.

WangSecurity commented 4 months ago

As the report says, the issue may cause loss of funds to users, hence, I believe design decision rule is not appropriate here. Planning to reject the escalation and leave the issue as it is.

WangSecurity commented 4 months ago

Result: Medium Has duplicates

sherlock-admin4 commented 4 months ago

Escalations have been resolved successfully!

Escalation status: