Emmanuel - User would liquidate his account to sidestep takerInvariant modifier

[sherlock-admin]

Emmanuel

medium

User would liquidate his account to sidestep takerInvariant modifier

Summary

A single user could open a massive maker position, using the maximum leverage possible(and possibly reach the maker limit), and when a lot of takers open take positions, maker would liquidate his position, effectively bypassing the taker invariant and losing nothing apart from position fees.
This would cause takers to be charged extremely high funding fees(at the maxRate), and takers that are not actively monitoring their positions will be greatly affected.

Vulnerability Detail

In the closeMakeFor function, there is a modifier called takerInvariant.

function closeMakeFor(
        address account,
        UFixed18 amount
    )
        public
        nonReentrant
        notPaused
        onlyAccountOrMultiInvoker(account)
        settleForAccount(account)
        takerInvariant
        closeInvariant(account)
        liquidationInvariant(account)
    {
        _closeMake(account, amount);
    }

This modifier prevents makers from closing their positions if it would make the global maker open positions to fall below the global taker open positions.
A malicious maker can easily sidestep this by liquidating his own account.
Liquidating an account pays the liquidator a fee from the account's collateral, and then forcefully closes all open maker and taker positions for that account.

function closeAll(address account) external onlyCollateral notClosed settleForAccount(account) {
        AccountPosition storage accountPosition = _positions[account];
        Position memory p = accountPosition.position.next(_positions[account].pre);

        // Close all positions
        _closeMake(account, p.maker);
        _closeTake(account, p.taker);

        // Mark liquidation to lock position
        accountPosition.liquidation = true; 
    }

This would make the open maker positions to drop significantly below the open taker position, and greatly increase the funding fee and utilization ratio.

ATTACK SCENARIO

• A new Product(ETH-Long) is launched on arbitrum with the following configurations:
  • 20x max leverage(5% maintenance)
  • makerFee = 0
  • takerFee = 0.015
  • liquidationFee = 20%
  • minRate = 4%
  • maxRate = 120%
  • targetRate = 12%
  • targetUtilization = 80%
  • makerLimit = 4000 Eth
  • ETH price = 1750 USD
  • Coll Token = USDC
  • max liquidity(USD) = 4000*1750 = $7,000,000
• Whale initially supplies 350k USDC of collateral(~200ETH), and opens a maker position of 3000ETH($5.25mn), at 15x leverage.
• After 2 weeks of activity, global open maker position goes up to 3429ETH($6mn), and because fundingFee is low, people are incentivized to open taker positions, so global open taker position gets to 2743ETH($4.8mn) at 80% utilization. Now, rate of fundingFee is 12%
• Now, Whale should only be able to close up to 686ETH($1.2mn) of his maker position using the closeMakeFor function because of the takerInvariant modifier.
• Whale decides to withdraw 87.5k USDC(~50ETH), bringing his total collateral to 262.5k USDC, and his leverage to 20x(which is the max leverage)
• If price of ETH temporarily goes up to 1755 USD, totalMaintenance=3000 * 1755 * 5% = $263250. Because his totalCollateral is 262500 USDC(which is less than totalMaintenance), his account becomes liquidatable.
• Whale liquidates his account, he receives liquidationFee*totalMaintenance = 20% * 263250 = 52650USDC, and his maker position of 3000ETH gets closed. Now, he can withdraw his remaining collateral(262500-52650=209850)USDC because he has no open positions.
• Global taker position is now 2743ETH($4.8mn), and global maker position is 429ETH($750k)
• Whale has succeeded in bypassing the takerInvaraiant modifier, which was to prevent him from closing his maker position if it would make global maker position less than global taker position.

Consequently,

• Funding fees would now be very high(120%), so the currently open taker positions will be greatly penalized, and takers who are not actively monitoring their position could lose a lot.
• Whale would want to gain from the high funding fees, so he would open a maker position that would still keep the global maker position less than the global taker position(e.g. collateral of 232750USDC at 15x leverage, open position = ~2000ETH($3.5mn)) so that taker positions will keep getting charged at the funding fee maxRate.

Impact

User will close his maker position when he shouldn't be allowed to, and it would cause open taker positions to be greatly impacted. And those who are not actively monitoring their open taker positions will suffer loss due to high funding fees.

Code Snippet

https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/collateral/Collateral.sol#L123-L132
https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/product/Product.sol#L362-L372
https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/product/Product.sol#L334
https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/product/Product.sol#L535-L544

Tool used

Manual Review

Recommendation

Consider implementing any of these:

• Protocol should receive a share of liquidation fee: This would disincentivize users from wanting to liquidate their own accounts, and they would want to keep their positions healthy and over-collateralized
• Let there be a maker limit on each account: In addition to the global maker limit, there should be maker limit for each account which may be capped at 5% of global maker limit. This would decentralize liquidity provisioning.

[arjun-io]

This is working as intended. The market dynamics (namely funding rate curve) should incentivize more makers to come into the market in this case, although it is possible for a malicious maker to temporarily increase funding rates in situations where there is not other LPs (or vaults).

[mstpr]

Escalate for 10 USDC

I think this is an invalid issue.

First of all, it's a great example. However, I would consider this as a great trader than an attacker. There are many assumptions and too many risk factors to pull of such thing and it is definitely a very custom scenario.

Whale deposits tons of maker position and waits for utilization to come to 80%. Since this is very unlikely to happen in a block it will happen over time and meanwhile whale is subject to risk for the entire time since makers are taking the opposite direction of the trade. If everything goes well to plan then whale can pull off this action which again in order to do that whale needs to liquidate itself which is another risk. I think there are too much custom assumptions that are most likely never happens in real world scenarios.

[sherlock-admin]

Escalate for 10 USDC

I think this is an invalid issue.

First of all, it's a great example. However, I would consider this as a great trader than an attacker. There are many assumptions and too many risk factors to pull of such thing and it is definitely a very custom scenario.

Whale deposits tons of maker position and waits for utilization to come to 80%. Since this is very unlikely to happen in a block it will happen over time and meanwhile whale is subject to risk for the entire time since makers are taking the opposite direction of the trade. If everything goes well to plan then whale can pull off this action which again in order to do that whale needs to liquidate itself which is another risk. I think there are too much custom assumptions that are most likely never happens in real world scenarios.

You've created a valid escalation for 10 USDC!

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.

[flowercrimson]

I think the issue is valid. It points to opportunities to exploit during liquidation because market liquidity protection is completely bypassed.

The escalation is incorrect in saying that whales are discouraged/unlikely to perform such attacks because of the risks of losing to traders. All makers and traders have risks losing to each other, just because there is inherent risk in trading, doesn’t prevent one from becoming a maker. In addition, makers will also be further compensated with position fees.

To me, this issue provides a possible scenario where makers can be malicious and there could be more than one malicious makers.

Funding rates might not be sufficient when there are no other LPs, and also because it is designed under the assumption that takerOI>makerOI are edge cases and ‘very rare situations’ according to docs. When whale makers can be liquidated in a malicious manner, takerOI>makerOI is no longer edge case, it would become a regular occurrence for malicious makers to profit, creating cycles of penalization on takers.

[KenzoAgada]

I think the submission is valid and I agree with flowercrimson's points above.

Additionally, the original escalation said:

I would consider this as a great trader than an attacker

This reminds me of the Mango Markets exploit, where the exploiter called it "a very profitable trading strategy"... Yes, there was no smart contract hack or bug, but the economics allowed manipulation.
The situation is not the same here, but there is similarity. The issue throws light on a possible strategy that subverts protocol expectations for fair functioning. I think that the scenario that Emmanuel described is not unlikely enough to be totally dismissed.
This will cause unsuspecting users to lose funds (pay fees) in a way the protocol didn't intend to (which is why there is takerInvariant).
Even if the protocol "accepts the risk", I think this is a good finding that deserves to be pointed out.
So I think the first escalation is invalid and a medium severity is appropriate.

[jacksanford1]

@mstpr Anything else to add after seeing @flowercrimson and @KenzoAgada's responses?

[mstpr]

@jacksanford1 The reason why I said great trader than an attacker is because this scenario is not a planned attack scenario. This is a very custom scenario overall.

I think the main problem described here is that, in some cases where it is not possible to exit the market users may put theirselves to liquidation and hope to liquidate themselves to exit the market. Which is correct. However, can you block this from happening?

User can't decrease their collateral to a level where they are liquidatable. They can decrease the collateral up to liquidation threshold + 1 at minimum. Then, since the market is reporting in settlements, user needs and hopes that the price declines tiny bit more such that the next settlement user is liquidatable. Meanwhile, user needs to make sure that he will be able to liquidate hisself. If someone else liquidates the position, there is no profit or benefit of doing this action. If there is a robust liquidator community then user takes a great risk.

I would say this is valid if only user could have done this actions in 1 tx.
User notices he can't exit the market. Reduce some collateral and liquidates hisself in 1 tx.

I think everything works as intended. User doesn't close the position here, user decrease its collateral and hopes to liquidate itself before others.

Another take to the issue:
Whale above scenario might say "I don't care about who liquidates me and takes the liquidation fee I need to exit this position because being a maker in current market will be worse than me to just give up the liquidation fee right now" . Well, obviously this is a trading choice of the whale then. Pay the liquidation fee and exit your position our wait for the utilization to go down and exit your position.

[jacksanford1]

@flowercrimson @KenzoAgada Any response to @mstpr's latest comment?

[Emedudu]

The issue here is that takerInvariant modifier will not serve its intended purpose.

takerInvariant modifier was put in place to ensure that open maker positions does not go lower than open taker positions.

Of course, it is the right of users to close their open positions, but according to Perennial's rules(takerInvariant modifier), they shouldn't be able to close their positions such that the global open positions will be less than global taker positions.

I don't think protocol would have added takerInvariant to closeMakeFor function if they are comfortable with global maker positions dropping below taker positions.

The reason why I said great trader than an attacker is because this scenario is not a planned attack scenario. This is a very custom scenario overall.

The protocol put takerInvariant in place to prevent this scenario from happening, whether it was a planned attack scenario or not.

But now, instead of using the normal door way, which is closeMakeFor function (which would enforce the rules and restrict a user from closing his position), User goes through "liquidation" door way(which would have about same effect as closeMakeFor and bypass the rules).

This would also lead to HIGH funding fees which would be unfair to all open taker positions.

[KenzoAgada]

@mstpr in your reply you kind of reframed the discussion to about "user may want to liquidate themselves to exit the market". But what you haven't touched upon in your explanation is the problem of the forced high funding fee.
Looking at the original issue's example, the whale can "force" takers to pay the maximum high maxRate.
He can then take another maker position himself and make himself the beneficiary of the payment.
Isn't this valid as a possible attack path (/a very profitable trading strategy 🙂)?

I understand there are some requirements, such as no other large maker positions and the attacker/trader liquidating himself (though depending on params that might be less necessary). So it's not a high severity issue. And I understand that the sponsor says this is working as intended and the market should take care of it. But should does not guarantee it... It seems to me that the issue describes a valid possible oversight/exploit of the mechanism.

[jacksanford1]

@arjun-io Do you still feel the same way about this issue?

[arjun-io]

Our view is this is working as intended as per my initial comment (reposted below). Part of a healthy market is numerous LPs (directly or indirectly through the vault).

This is working as intended. The market dynamics (namely funding rate curve) should incentivize more makers to come into the market in this case, although it is possible for a malicious maker to temporarily increase funding rates in situations where there is not other LPs (or vaults).

One note: The protocol+market can receive a portion of the close fees which would disincentivize this behavior (similar to the liquidation fee share as described in the recommendation). Capping LPs as a % of the maker limit doesn't really achieve anything, as it creates a bad UX for good actors and bad actors can simply split positions across multiple accounts

[jacksanford1]

This is a really tricky issue. I think the Mango Markets example actually doesn't apply here, and if this were an issue similar to Mango Markets then it would be Low. The Mango Markets exploit was poorly chosen collateral (MNGO token) and/or a poorly chosen LTV for that collateral. Those are not smart contract issues, and the Mango Markets protocol actually functioned exactly as it was intended to function from a smart contract perspective.

But coming back to the original Impact of this issue:

User will close his maker position when he shouldn't be allowed to, and it would cause open taker positions to be greatly impacted. And those who are not actively monitoring their open taker positions will suffer loss due to high funding fees.

If it's true that the user shouldn't be allowed to close his position (i.e. not an intended functionality of the smart contracts) AND if it's true that it could cause potentially large losses for other users, then it should be a Medium.

Unless there is further discussion around either of those aspects (intended functionality or loss potential to users) then this should be a Medium imo.

[jacksanford1]

Result:
Medium
Has duplicates
See previous message for reasoning.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• mstpr: rejected