search

sherlock-audit / 2024-05-elfi-protocol-judging

11 stars 7 forks source link

ZeroTrust - In Cross Margin mode, the user’s profit calculation is incorrect. #273

Open sherlock-admin4 opened 4 months ago

sherlock-admin4 commented 4 months ago

ZeroTrust

High

In Cross Margin mode, the user’s profit calculation is incorrect.

Summary

In Cross Margin mode, the user’s profit calculation is incorrect.

Vulnerability Detail

We know that isolated and cross margin are different. When a position is created, in isolated mode, the corresponding assets need to be transferred from the user’s wallet to the MarketVault, while in cross margin mode, the user only needs to have sufficient collateral in the PortfolioVault (any supported collateral will do). For example, with 1x leverage going long on WETH-USDC, the position size is 1 WETH, and the price of WETH is 1000 USD.

function decreasePosition(Position.Props storage position, DecreasePositionParams calldata params) external {
        int256 totalPnlInUsd = PositionQueryProcess.getPositionUnPnl(position, params.executePrice.toInt256(), false);
        Symbol.Props memory symbolProps = Symbol.load(params.symbol);
        AppConfig.SymbolConfig memory symbolConfig = AppConfig.getSymbolConfig(params.symbol);
        FeeProcess.updateBorrowingFee(position, symbolProps.stakeToken);
        FeeProcess.updateFundingFee(position);
@>>        DecreasePositionCache memory cache = _updateDecreasePosition(
            position,
            params.decreaseQty,
            totalPnlInUsd,
            params.executePrice.toInt256(),
            symbolConfig.closeFeeRate,
            params.isLiquidation,
            params.isCrossMargin
        );
       //skip .........

    }
   function _updateDecreasePosition(
        Position.Props storage position,
        uint256 decreaseQty,
        int256 pnlInUsd,
        int256 executePrice,
        uint256 closeFeeRate,
        bool isLiquidation,
        bool isCrossMargin
    ) internal view returns (DecreasePositionCache memory cache) {
        cache.position = position;
        cache.executePrice = executePrice;
        int256 tokenPrice = OracleProcess.getLatestUsdPrice(position.marginToken, false);
        cache.marginTokenPrice = tokenPrice.toUint256();
        uint8 tokenDecimals = TokenUtils.decimals(position.marginToken);
        if (position.qty == decreaseQty) {
@>>            cache.decreaseMargin = cache.position.initialMargin;
            cache.decreaseMarginInUsd = cache.position.initialMarginInUsd;
            cache.unHoldPoolAmount = cache.position.holdPoolAmount;
            (cache.settledBorrowingFee, cache.settledBorrowingFeeInUsd) = FeeQueryProcess.calcBorrowingFee(
                decreaseQty,
                position
            );
            cache.settledFundingFee = cache.position.positionFee.realizedFundingFee;
            cache.settledFundingFeeInUsd = cache.position.positionFee.realizedFundingFeeInUsd;

            cache.closeFeeInUsd = cache.position.positionFee.closeFeeInUsd;
            cache.closeFee = FeeQueryProcess.calcCloseFee(tokenDecimals, cache.closeFeeInUsd, tokenPrice.toUint256());
            cache.settledFee =
                cache.settledBorrowingFee.toInt256() +
                cache.settledFundingFee +
                cache.closeFee.toInt256();
//skip .......
            {
                cache.settledMargin = CalUtils.usdToTokenInt(
                    cache.position.initialMarginInUsd.toInt256() - _getPosFee(cache) + pnlInUsd,
                    TokenUtils.decimals(cache.position.marginToken),
                    tokenPrice
                );
@>>             cache.recordPnlToken = cache.settledMargin - cache.decreaseMargin.toInt256();
                cache.poolPnlToken =
                    cache.decreaseMargin.toInt256() -
                    CalUtils.usdToTokenInt(
                        cache.position.initialMarginInUsd.toInt256() + pnlInUsd,
                        TokenUtils.decimals(cache.position.marginToken),
                        tokenPrice
                    );
            }
            cache.realizedPnl = CalUtils.tokenToUsdInt(
                cache.recordPnlToken,
                TokenUtils.decimals(cache.position.marginToken),
                tokenPrice
            );
            console2.log("cache.position.initialMarginInUsd is", cache.position.initialMarginInUsd);
            console2.log("cache.settledMargin is ", cache.settledMargin);

            console2.log("cache.recordPnlToken is ", cache.recordPnlToken);
            console2.log("cache.poolPnlToken is ", cache.poolPnlToken);
            console2.log("cache.realizedPnl is ", cache.realizedPnl);

        } 
        //skip ......    

        return cache;
    }

However, in _updateDecreasePosition, cache.recordPnlToken = cache.settledMargin - cache.decreaseMargin.toInt256(), where cache.decreaseMargin = cache.position.initialMargin, causing cache.recordPnlToken to be nearly zero. This is incorrect in cross margin mode, because in cross margin mode, the initialMargin (with) is not invested in the market. Therefore, cache.recordPnlToken = cache.settledMargin.

poc

For example, with 1x leverage going long on WETH-USDC, the position size is 1 WETH, and the price of WETH is 1000 USD. When the price of WETH rises to 2000 USD, closing the position makes it more evident.

function testCrossMarginOrderExecute() public{
        ethPrice = 1000e8;
        usdcPrice = 101e6;
        OracleProcess.OracleParam[] memory oracles = getOracles(ethPrice, usdcPrice);

        userDeposit();
        depositWETH();

        openCrossMarginOrder();

        //after a day
        skip(1 days);
        ethPrice = 2000e8;
        closeCrossMarginLongPosition();

        getPoolWithOracle(oracles);

    }

Test the base code to verify this.

    totalPnlInUsd is  998900000000000000000
    cache.position.initialMarginInUsd is 998900000000000000000
    cache.settledMargin is  997387665400000000
    cache.recordPnlToken is  -1512334600000000
     cache.poolPnlToken is  0
    cache.realizedPnl is  -3024669200000000000

It can be seen that the profit is a negative value close to zero, which is obviously incorrect.

Impact

This causes financial loss for either the user or the protocol.

Code Snippet

https://github.com/sherlock-audit/2024-05-elfi-protocol/blob/main/elfi-perp-contracts/contracts/process/DecreasePositionProcess.sol#L60

https://github.com/sherlock-audit/2024-05-elfi-protocol/blob/main/elfi-perp-contracts/contracts/process/DecreasePositionProcess.sol#L206

Tool used

Manual Review

Recommendation

Distinguish between the handling methods for isolated mode and cross margin mode.

ZeroTrust01 commented 4 months ago

Hey, @nevillehuang

The sponsor’s responses: @ZeroTrust01 is right so finally gets 1WETH (Both cross and isolate)

have at least confirmed that this issue is valid. In cross margin mode, if you invest 1000 USD(eg Whether the collateral is 1 BTC or 1000 USDT in the PortfolioVault) and end up with 1 WETH (2000 USD), my PoC proves that the system’s calculation is incorrect, resulting in a negative profit.

As for issue 272, the discussion is about having 1 WBTC(Using 1000 USDT as an example would be more appropriate) in cross margin mode. Assuming you invest one WETH (1000 USD) with 1x leverage to go long, should this 1000 USD be borrowed from the LpPool, or can a portion of the 1 WBTC’s value (1000 USD) directly participate in the market trading?

272 and 273 are different issues with different focuses. The root causes in the code are also different.

0502lian commented 4 months ago

Escalate This should be a valid issue.

A very simple scenario: Assumption: Initially, the price of WETH is 1000 USD, and the price at closing the position is 2000 USD.

They both go long with 1x leverage in the WETH-USDC market. The final profit situation is as follows(has already been confirmed by the sponsor):

In this case, because using 1 leverage, whether it is cross mode or isolated mode, there is no borrowing from the LP. So finally, the user gets 1 WETH (Both cross and isolated).

For cross margin mode: 1 WETH(2000usd) - 1000 usdt = 0.5 WETH However, the PoC shows that in cross margin mode, the profit is 0, which is obviously incorrect.

sherlock-admin3 commented 4 months ago

Escalate This should be a valid issue.

A very simple scenario: Assumption: Initially, the price of WETH is 1000 USD, and the price at closing the position is 2000 USD.

  • In isolated mode, the user’s initial capital is 1 WETH.
  • In cross margin mode, the user’s initial capital is 1000 USDT.

They both go long with 1x leverage in the WETH-USDC market. The final profit situation is as follows(has already been confirmed by the sponsor):

  • In isolated mode: The user profits 1000 USD (2000 USD - 1000 USD initial capital) and finally still gets their original 1 WETH (2000 USD), which is used for trading.
  • In cross margin mode: The user profits 1000 USD (2000 USD - 1000 USD initial borrowed funds) and finally gets 0.5 WETH.

In this case, because using 1 leverage, whether it is cross mode or isolated mode, there is no borrowing from the LP. So finally, the user gets 1 WETH (Both cross and isolated).

For cross margin mode: 1 WETH(2000usd) - 1000 usdt = 0.5 WETH However, the PoC shows that in cross margin mode, the profit is 0, which is obviously incorrect.

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.

0xELFi commented 4 months ago

We separate the consideration of funds into two parts: one is the user's own margin assets, and the other is the user's position profit. Let's first look at the profit part: regardless of whether it is isolated or cross, their profit is (2000USD-1000USD)/2000 = 0.5 WETH.

Margin asset part: since we price the margin in USD at the moment the user opens the position, both isolated and cross margin are 1000USD. Relative to the latest WETH price of 2000USD, the margin becomes 0.5 ETH. So for both cross and isolated users, the settledMargin is 0.5WETH + 0.5WETH = 1 WETH. The user actually invests 1 WETH. For the user, if we disregard the fee issue, the recordPnlToken should be 0.

The only difference between isolated and cross margin here is: with cross margin, the user uses their own assets as collateral to borrow 1 WETH to go long on ETHUSD. For the system in terms of opening and closing positions, it is the same as isolated margin, meaning the user uses 1 WETH to go long on ETHUSD.

0502lian commented 4 months ago

You said that “The only difference between isolated and cross margin here is: with cross margin, the user uses their own assets as collateral to borrow 1 WETH to go long on ETHUSD.” This is exactly what issue 272 points out: Isolated Mode: borrows 0, Cross Margin Mode: borrows 1 WETH. But you say it is invalid.

WangSecurity commented 4 months ago

Let's focus on this issue here and keep the discussion about #272 there. As I understand this comment the protocol is working as it should, but i might be missing something, so please correct me.

0502lian commented 4 months ago

We separate the consideration of funds into two parts: one is the user's own margin assets, and the other is the user's position profit. Let's first look at the profit part: regardless of whether it is isolated or cross, their profit is (2000USD-1000USD)/2000 = 0.5 WETH.

Margin asset part: since we price the margin in USD at the moment the user opens the position, both isolated and cross margin are 1000USD. Relative to the latest WETH price of 2000USD, the margin becomes 0.5 ETH. So for both cross and isolated users, the settledMargin is 0.5WETH + 0.5WETH = 1 WETH. The user actually invests 1 WETH. For the user, if we disregard the fee issue, the recordPnlToken should be 0.

The only difference between isolated and cross margin here is: with cross margin, the user uses their own assets as collateral to borrow 1 WETH to go long on ETHUSD. For the system in terms of opening and closing positions, it is the same as isolated margin, meaning the user uses 1 WETH to go long on ETHUSD.

@WangSecurity

The sponsor’s statement that “The only difference between isolated and cross margin here is: with cross margin, the user uses their own assets as collateral to borrow 1 WETH to go long on ETHUSD. For the system in terms of opening and closing positions, it is the same as isolated margin, meaning the user uses 1 WETH to go long on ETHUSD” is not a fact, but just their idea.

This is why I pointed out in other issues that they need to actually borrow. Borrowing requires a lender, doesn’t it? Can the sponsor @0xELFi help us by telling who the lender is? If the amount borrowed is 1e18 WETH, then if the user’s position increases by 1e18 WETH, whose account decreases by 1e18 WETH? Thank you .

WangSecurity commented 4 months ago

This is why I pointed out in other issues that they need to actually borrow. Borrowing requires a lender, doesn’t it? Can the sponsor @0xELFi help us by telling who the lender is? If the amount borrowed is 1e18 WETH, then if the user’s position increases by 1e18 WETH, whose account decreases by 1e18 WETH? Thank you

As I understand you again talk about #272 and let's keep the discussion about it under #272. As I understand this report is only a design recommendation and not a real issue. But if I'm missing why this one is a valid issue, please correct me.

0502lian commented 4 months ago

This is why I pointed out in other issues that they need to actually borrow. Borrowing requires a lender, doesn’t it? Can the sponsor @0xELFi help us by telling who the lender is? If the amount borrowed is 1e18 WETH, then if the user’s position increases by 1e18 WETH, whose account decreases by 1e18 WETH? Thank you

As I understand you again talk about #272 and let's keep the discussion about it under #272. As I understand this report is only a design recommendation and not a real issue. But if I'm missing why this one is a valid issue, please correct me.

This is definitely a serious issue. User's profit have been lost. I have proven the loss of funds using PoC and test code.

A very simple scenario:

However, the Proof of Code shows that in cross margin mode, the profit is 0. The profit being 0 is clearly incorrect.

Anyone with trading experience would immediately know that the profit of 0 is wrong. Everyone can verify that the description of the simple scenario is factual.

It is very important for the judgment of this issue to note that some statements made by the sponsor are not based on the factual code.

"Margin asset part: since we price the margin in USD at the moment the user opens the position, both isolated and cross margin are 1000USD. Relative to the latest WETH price of 2000USD, the margin becomes 0.5 ETH. So for both cross and isolated users, the settledMargin is 0.5WETH + 0.5WETH = 1 WETH. The user actually invests 1 WETH. For the user, if we disregard the fee issue, the recordPnlToken should be 0."

The user actually invests 1 WETH. — This is not true; the cross margin user invests 1000 USDT.

"The only difference between isolated and cross margin here is: with cross margin, the user uses their own assets as collateral to borrow 1 WETH to go long on ETHUSD. For the system in terms of opening and closing positions, it is the same as isolated margin, meaning the user uses 1 WETH to go long on ETHUSD."

The user uses their own assets as collateral to borrow 1 WETH to go long on ETHUSD. — This is not true, they did not borrow 1 WETH from anyone. The sponsor cannot specify who the lender is, the process of the borrow transaction, or whose account decreased by 1 WETH.

I have already provided PoC and test results. However, the sponsor refutes me based on assertions that are not true of the code. Moreover, the proof of concept I provided is very simple and should be understandable to everyone, especially for those with token trading experience—it’s clear at a glance.

WangSecurity commented 4 months ago

Thank you for such a thorough response, I believe you're correct here and this issue is indeed valid. As I understand it will happen is almost every trade due to incorrect formula, correct?

0502lian commented 4 months ago

Thank you for such a thorough response, I believe you're correct here and this issue is indeed valid. As I understand it will happen is almost every trade due to incorrect formula, correct?

Yes, it will happen is almost every trade due to incorrect formula in Cross Margin Mode. ElFi申辩证据 The sponsor actually admitted this issue during the competition.

WangSecurity commented 4 months ago

Thank you very much, planning to accept the escalation and validate the issue with high severity, cause the constraints are not extreme.

mstpr commented 4 months ago

@0xELFi @0xELFi02 @nevillehuang @WangSecurity This issue looks a valid high to me. I am wondering why it has the "Sponsor Disputed" tag?

WangSecurity commented 4 months ago

Result: High Unique

sherlock-admin2 commented 4 months ago

Escalations have been resolved successfully!

Escalation status: