0x73696d616f - DoSed liquidations as `PrizeVault::liquidatableBalanceOf()` does not take into account the `mintLimit` when the token out is the asset

[sherlock-admin4]

0x73696d616f

medium

DoSed liquidations as PrizeVault::liquidatableBalanceOf() does not take into account the mintLimit when the token out is the asset

Summary

PrizeVault::liquidatableBalanceOf() is called in TpdaLiquidationPair::_availableBalance() to get the maximum amount to liquidate, which will be incorrect when _tokenOut is the asset of the PrizeVault, due to not taking the minted yield fee into account. Thus, it will overestimate the amount to liquidate and revert.

Vulnerability Detail

TpdaLiquidationPair::_availableBalance() is called in TpdaLiquidationPair::swapExactAmountOut() to revert if the amount to liquidate exceeds the maximum and in TpdaLiquidationPair::maxAmountOut() to get the maximum liquidatable amount. Thus, users or smart contracts will call TpdaLiquidationPair::maxAmountOut() to get the maximum amount out and then TpdaLiquidationPair::swapExactAmountOut() with this amount to liquidate.

Compute how much yield is available using the maxAmountOut function on the Liquidation Pair. This function returns the maximum number of tokens you can swap out.

However, this is going to revert whenever the minted yield fee exceeds the mint limit, as PrizeVault::liquidatableBalanceOf() does not consider it when the asset to liquidate is the asset of the PrizeVault. Consider PrizeVault::liquidatableBalanceOf():

function liquidatableBalanceOf(address _tokenOut) external view returns (uint256) {
    ...
    } else if (_tokenOut == address(_asset)) { //@audit missing yield percentage for mintLimit
        // Liquidation of yield assets is capped at the max yield vault withdraw plus any latent balance.
        _maxAmountOut = _maxYieldVaultWithdraw() + _asset.balanceOf(address(this));
    }
    ...
}

As can be seen from the code snipped above, the minted yield fee is not taken into account and the mint limit is not calculated. On PrizeVault::transferTokensOut(), a mint fee given by _yieldFee = (_amountOut * FEE_PRECISION) / (FEE_PRECISION - _yieldFeePercentage) - _amountOut; is always minted and the limit is enforced at the end of the function _enforceMintLimit(_totalDebtBefore, _yieldFee);. Thus, without limiting the liquidatable assets to the amount that would trigger a yield fee that reaches the mint limit, liquidations will be DoSed.

Impact

DoSed liquidations when the asset out is the asset of the PrizeVault.

Code Snippet

https://github.com/sherlock-audit/2024-05-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L693-L696

Tool used

Manual Review

Vscode

Recommendation

The correct formula can be obtained by inverting _yieldFee = (_amountOut * FEE_PRECISION) / (FEE_PRECISION - _yieldFeePercentage) - _amountOut;, leading to:

function liquidatableBalanceOf(address _tokenOut) external view returns (uint256) {
    ...
    } 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));
        // Limit to the fee amount
        uint256  mintLimitDueToFee = (FEE_PRECISION - yieldFeePercentage) * _mintLimit(_totalDebt) / yieldFeePercentage;
       _maxAmountOut = _maxAmountOut >= mintLimitDueToFee ? mintLimitDueToFee : _maxAmountOut;
    }
    ...
}

[sherlock-admin2]

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

infect3d commented:

the proposed remediation is in fact what is already done in the return statement

[nevillehuang]

If mint limit is reached then liquidations are capped per comments

[0x73696d616f]

Escalate

This issue is valid because the docs state exactly how liquidations happen, and they will fail for some states, when they should go through.

Compute how much yield is available using the maxAmountOut function on the Liquidation Pair. This function returns the maximum number of tokens you can swap out.

So we can agree there is DoS, as it will revert when the number of shares is close to the mint limit. Liquidations use the tpda mechanism, so the price picked and the bot that gets awarded depend on being extremely timely in the liquidation. Thus, we conclude that liquidations are time sensitive.

For these 2 reasons, this issue is valid.

[sherlock-admin3]

Escalate

This issue is valid because the docs state exactly how liquidations happen, and they will fail for some states, when they should go through.

Compute how much yield is available using the maxAmountOut function on the Liquidation Pair. This function returns the maximum number of tokens you can swap out.

So we can agree there is DoS, as it will revert when the number of shares is close to the mint limit. Liquidations use the tpda mechanism, so the price picked and the bot that gets awarded depend on being extremely timely in the liquidation. Thus, we conclude that liquidations are time sensitive.

For these 2 reasons, this issue is valid.

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.

[WangSecurity]

which will be incorrect when _tokenOut is the asset of the PrizeVault

To clarify this line, you mean the asset of the underlying yieldVault, correct?

As I see the yield fee is taken here in the return statement of the function?

[0x73696d616f]

Yes _tokenOut is the asset of the underlying vault. The problem is that when this asset is picked, a fee is still minted and the mint limit is enforced, see here. But liquidatableBalanceOf() does not take into account that a fee is minted even if the asset picked is _tokenOut. You can see here that the mint limit is not checked. So it overestimates the maximum liquidatable balance and makes liquidations fail. The maximum liquidatable balance should be capped to take into account the fee, with the exact recommendation given in the issue.

[InfectedIsm]

I may have missed something @0x73696d616f, but isn't it what is done in the final return statement?

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

We see that the return value of liquidatableBalanceOf is: _maxAmountOut * (FEE_PRECISION - yieldFeePercentage) / FEE_PRECISION (1)

So the amount that is returned by liquidatableBalanceOf() is reduced by the fee that will be taken.

And in fact, if you plug (1) as _amountOut into : (2) _yieldFee = (_amountOut * FEE_PRECISION) / (FEE_PRECISION - _yieldFeePercentage) - _amountOut

you get _yieldFee = fee * _maxAmountOut (3)

If we rewrite (1) in a more readable way we have liquidatableBalanceOf = _maxAmountOut * ( 1 - fee) (4)

And if you add (3) the fee amount + (4) _amountOut that will be sent to user, we get: maxAmountOut * fee + maxAmountOut * (1 - fee) == maxAmountOut * (fee + 1 - fee) == maxAmountOut

So everything seems to fit correctly ?

[0x73696d616f]

Yes, that part is correct, but the limit is not actually enforced, if you notice regardless of the mint limit, it always returns a value bigger than 0. Example, maxAmountOut = 10000 (there is 10000 yield to be liquidated) there are 10 shares left to the mint limit yield fee is 1% so it would return 10000*99/100 = 9900 then, the fee to mint is 9900*100/(100 - 1) - 9900 = 100 and 100 > 10, so it reverts

the correct max amount is, (100 - 1)*10/1 = 990, as mentioned in the issue if we do 990*100/(100 - 1) - 990 = 10, which is the max shares that can be minted

[InfectedIsm]

Got it thanks, so when the total prize vault shares is close to TWAB_SUPPLY_LIMIT , then it is possible that the fee to mint based on the maximum liquidatable underlying asset amount is greater than the mintLimit = TWAB_SUPPLY_LIMIT - _existingShares, thus causing a revert during the _enforceMintLimit call The liquidation pair smoothingFactor coupled with the relatively low value of the fee means that the vault must be really close the the TWAB_SUPPLY_LIMIT for this to happen, but when vault is near 100% of its capacity it can happen more frequently, or even make it impossible to liquidate when TWAB_SUPPLY_LIMIT is reached This could be addressed by manually setting the yield fees to 0 if it happens, though manual intervention would still be required.

[WangSecurity]

And last clarification, excuse me if a silly one, but want to confirm if it's correct:

This issue doesn't require the mintLimit to be reached, but to be very close to it? I see that it can happen even if it's not close to the limit, but will be more often?

[0x73696d616f]

It gets more likely the closer we are to the limit, as the value required to trigger it decreases. In the example here, the 990 tokens would not be liquidated, as it tries to liquidate way more and reverts.

[WangSecurity]

Thank you for that clarification, with that I agree that it's valid issue. Planning to accept the escalation and validate with medium severity.

[Hash01011122]

@WangSecurity Shouldn't this be low severity, as the likelihood of this to occur is very low.

Firstly, for this to occur is _tokenOut should be the asset PrizeVault,

Secondly, fee to mint based on the maximum liquidatable underlying asset amount is greater than _enforceMintLimit, thus causing a revert during the _enforceMintLimit call

Thirdly, Watson assumes that protocol wouldn't react if maxAmountOut and _enforceMintLimit happens to create this scenario which can be ensured when protocol manually sets the yield fees to 0 as pointed out @InfectedIsm

Likelihood of this to occur is very low and with no impact.

Also, check out @MiloTruck comment here: https://github.com/sherlock-audit/2024-05-pooltogether-judging/issues/39#issuecomment-2182180107

[0x73696d616f]

@Hash01011122

Firstly, for this to occur is _tokenOut should be the asset PrizeVault

This does not decrease the likelihood, it's a perfectly valid choice.

Secondly, fee to mint based on the maximum liquidatable underlying asset amount is greater than _enforceMintLimit, thus causing a revert during the _enforceMintLimit call

This is the only requirement, but still means a considerable amount of tokens could not be liquidated. Without this requirement, it would be a high.

Thirdly, Watson assumes that protocol wouldn't react if maxAmountOut and _enforceMintLimit happens to create this scenario which can be ensured when protocol manually sets the yield fees to 0 as pointed out @InfectedIsm

This changes nothing because the function is time sensitive so it's crucial for bots to ensure this does not revert. If the bot reverts due to this, it would not get the price itself, taking a loss and the price will be different.

Likelihood of this to occur is very low and with no impact.

It's not very low as the limit may be reached and it causes DoS and is time sensitive (and loss of funds for users of the vault, as they would get less tokens in return, as the price would keep going down), hence medium is appropriate.

Also, check out @MiloTruck comment here: https://github.com/sherlock-audit/2024-05-pooltogether-judging/issues/39#issuecomment-2182180107

It has no relevance that another watson says very vaguely that the issue is low.

[Hash01011122]

This changes nothing because the function is time sensitive

Any idea how much time sensitive it really is @0x73696d616f @trmid @nevillehuang??

[0x73696d616f]

@Hash01011122 yes, look at the way the tpda liquidation works, there are 2 reasons:

1. the price keeps decreasing, so if it is DoSed vault users will receive less prize tokens in return of the liquidation.
2. the first bot that calls the function to liquidate should get the profit, but it doesn't and the profit goes to someone else.

[Hash01011122]

@0x73696d616f I didn't quite understand what you commented, would you mind elaborating this one please.

[0x73696d616f]

@Hash01011122 no problem, the liquidation is performed by the liquidation pair. This liquidation pair computes the price here, uint192 price = uint192((targetAuctionPeriod * lastAuctionPrice) / elapsedTime);. The longer elapsedTime, the smaller the price becomes. So DoSing liquidations will decrease the price. This price, is how many tokens are sent to the prize pool as a contribution from the respective vault that is being liquidated. So by decreasing the price, the vault that is liquidated will get a lower contribution to the prize pool, decreasing prizes for all users of that vault.

For point 2, bots are racing in this game to try to liquidate in great conditions. Ideally, each bot wants a price as low as possible, but the catch is that if they wait too much, other bot may liquidate instead. Thus, as the first bot that tries to liquidate reverts, it will not get the prize, when it should.

[MiloTruck]

Going to add context as to why I think this is valid, but low severity.

Liquidations occur through swapExactAmountOut(), which essentially does the following:

• swapAmountIn is the amount of prize tokens that has to be transferred in. This is determined by _computePrice(), a linearly decreasing auction that decreases the price from infinity to near-zero over time.
• The liquidator specifies _amountOut, which is the amount of yield tokens he wants to receive.
• The liquidator pays swapAmountIn of prize tokens and receives _amountOut of yield tokens.

It's important to realize that liquidatableBalanceOf() is only used as the upper limit for _amountOut:

TpdaLiquidationPair.sol#L141-L144

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

This means that when a liquidator calls swapExactAmountOut(), he can specify anything from 0 to the value returned by liquidatableBalanceOf() multiplied by the smoothing factor (ie. TpdaLiquidationPair.maxAmountOut()).

The issue here describes how liquidatableBalanceOf() doesn't account for the mint limit, so it will return a liquidatable balance higher than the actual amount of yield tokens that can be transferred out. In this case, if swapExactAmountOut() is called with _amountOut = maxAmountOut(), it will revert as the issue has stated.

However, just because swapExactAmountOut() can revert when you pass a certain _amountOut value doesn't mean it will DOS liquidations. liquidatableBalanceOf() only returns the maximum liquidatable balance, so swapExactAmountOut() can always be called with a smaller _amountOut value so that it doesn't revert.

Assuming the vault is close to the mint limit, the scenario is:

• liquidatableBalanceOf() returns an inflated liquidatable balance.
• The "actual liquidatable balance" (ie. a value for _amountOut at which the mint limit won't be hit and swapExactAmountOut() passes) is smaller than liquidatableBalanceOf(). Note that this is the value liquidatableBalanceOf() would return if the recommendation was applied.

So what would happen is liquidators would just call swapExactAmountOut() with the actual liquidatable balance, such that the liquidation process doesn't revert. And the linearly decreasing auction mechanism ensures that a price for this actual liquidatable balance will always be found.

To lay it out simply, something like this would occur (assume the smoothing factor is 100% for simplicity):

1. _computePrice() decreases from infinity to a fair price for the liquidatable balance returned by maxAmountOut().
2. Liquidator calls swapExactAmountOut() with _amountOut = maxAmountOut(), but realizes it reverts.
3. Liquidators waits for _computePrice() to decrease further until a fair price is reached for the vault's actual liquidatable balance.
4. Liquidator calls swapExactAmountOut() with _amountOut as the actual liquidatable balance, which doesn't revert.

The only impact here is that maxAmountOut() returns an inflated value, so liquidation bots calling swapExactAmountOut() in (2) waste gas. However, nearly all bots simulate their transactions beforehand, so the bot would simply realize that the transaction reverts and not send it.

You could argue that maxAmountOut() returns a wrong value and bots won't be able to figure out what _amountOut should be in (4), but this is equivalent to saying a view function not used anywhere else in the code returning the wrong value is medium severity.

[0x73696d616f]

Liquidator calls swapExactAmountOut() with _amountOut = maxAmountOut(), but realizes it reverts.

This is enough to cause problems, as it is an extremely time sensitive function. The price keeps going down and liquidations remain DoSed as it is a key function and part of the liquidation flow, explicitly stated by the protocol (it makes sense because bots want as much amount as possible so they will always call it before liquidating), so this is the source of truth, regardless of workarounds (which will not work as bots don't have much time to fix this, auctions last 6 hours, so they will not fix in time and the price drops too much)

Compute how much yield is available using the maxAmountOut function on the Liquidation Pair. This function returns the maximum number of tokens you can swap out.

[WangSecurity]

Firstly, I agree that liquidation is a time-sensitive function. Secondly, as said in the docs, maxAmountOut is function that has to be called to determine the max amount that can be liquidated. With these two factors together, I believe this issue is indeed medium.

The decision remains the same, accept the escalation and upgrade severity to medium.

Also, @Hash01011122, H/M severity don't take likelihood, only impact and constraints.

[0x73696d616f]

The decision remains the same, accept the escalation and leave the issue as it is.

Think you mean upgrade to medium

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/GenerationSoftware/pt-v5-vault/pull/114

[WangSecurity]

@nevillehuang @0x73696d616f are there any duplicates?

[WangSecurity]

Result: Medium Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• 0x73696d616f: accepted

[nevillehuang]

@WangSecurity Not as far as I know.

[10xhash]

Fixed. Now the amount is restricted such that the corresponding fee minted will fall within the mint limit

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.