ZeroTrust - In Cross Margin mode, the calculation for users borrowing from the pool is incorrect

[sherlock-admin3]

ZeroTrust

High

In Cross Margin mode, the calculation for users borrowing from the pool is incorrect

Summary

In Cross Margin mode, the calculation for users borrowing from the pool 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.

• In isolated mode, when establishing the position, 1 WETH is transferred to the MarketVault, so the borrowing is 0.
• In cross margin mode, assuming the collateral in the PortfolioVault is 10,000 USDC, no funds are transferred when creating the position. Thus, 1000 USD is borrowed from the LpPool and used to purchase 1 WETH at the price of 1000 USD.

When the price of WETH rises to 2000 USD, closing the position makes it more evident.

• 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.

function increasePosition(
        Position.Props storage position,
        Symbol.Props memory symbolProps,
        IncreasePositionParams calldata params
    ) external {
        uint256 fee = FeeQueryProcess.calcOpenFee(params.increaseMargin, params.leverage, symbolProps.code);
        FeeProcess.chargeTradingFee(fee, symbolProps.code, FeeProcess.FEE_OPEN_POSITION, params.marginToken, position);
        if (params.isCrossMargin) {
            Account.Props storage accountProps = Account.load(position.account);
            accountProps.unUseToken(params.marginToken, fee, Account.UpdateSource.CHARGE_OPEN_FEE);
            accountProps.subTokenWithLiability(params.marginToken, fee, Account.UpdateSource.CHARGE_OPEN_FEE);
        }

        uint256 increaseMargin = params.increaseMargin - fee;
        uint256 increaseMarginFromBalance = params.increaseMarginFromBalance > fee
            ? params.increaseMarginFromBalance - fee
            : 0;

        uint8 tokenDecimals = TokenUtils.decimals(params.marginToken);
        uint256 increaseQty = CalUtils.tokenToUsd(
            CalUtils.mulRate(increaseMargin, params.leverage),
            tokenDecimals,
            params.marginTokenPrice
        );

        AppConfig.SymbolConfig memory symbolConfig = AppConfig.getSymbolConfig(symbolProps.code);
        if (position.qty == 0) {
            position.marginToken = params.marginToken;
            position.entryPrice = params.indexTokenPrice;
            position.initialMargin = increaseMargin;
            position.initialMarginInUsd = CalUtils.tokenToUsd(increaseMargin, tokenDecimals, params.marginTokenPrice);
            position.initialMarginInUsdFromBalance = CalUtils.tokenToUsd(
                increaseMarginFromBalance,
                tokenDecimals,
                params.marginTokenPrice
            );
            position.positionFee.closeFeeInUsd = CalUtils.mulRate(increaseQty, symbolConfig.closeFeeRate);
            position.qty = increaseQty;
            position.leverage = params.leverage;
            position.realizedPnl = -(CalUtils.tokenToUsd(fee, tokenDecimals, params.marginTokenPrice).toInt256());
            position.positionFee.openBorrowingFeePerToken = MarketQueryProcess.getCumulativeBorrowingFeePerToken(
                symbolProps.stakeToken,
                params.isLong,
                params.marginToken
            );
            position.positionFee.openFundingFeePerQty = MarketQueryProcess.getFundingFeePerQty(
                symbolProps.code,
                params.isLong
            );
        } else {
            FeeProcess.updateBorrowingFee(position, symbolProps.stakeToken);
            FeeProcess.updateFundingFee(position);
            position.entryPrice = CalUtils.computeAvgEntryPrice(
                position.qty,
                position.entryPrice,
                increaseQty,
                params.indexTokenPrice,
                symbolConfig.tickSize,
                params.isLong
            );
            position.initialMargin += increaseMargin;
            position.initialMarginInUsd += CalUtils.tokenToUsd(increaseMargin, tokenDecimals, params.marginTokenPrice);
            position.initialMarginInUsdFromBalance += CalUtils.tokenToUsd(
                increaseMarginFromBalance,
                tokenDecimals,
                params.marginTokenPrice
            );
            position.positionFee.closeFeeInUsd += CalUtils.mulRate(increaseQty, symbolConfig.closeFeeRate);
            position.qty += increaseQty;
            position.realizedPnl += -(CalUtils.tokenToUsd(fee, tokenDecimals, params.marginTokenPrice).toInt256());
        }

        position.lastUpdateTime = ChainUtils.currentTimestamp();
@>>       uint256 increaseHoldAmount = CalUtils.mulRate(increaseMargin, (params.leverage - 1 * CalUtils.RATE_PRECISION));
@>>        position.holdPoolAmount += increaseHoldAmount;

        /// update & verify OI
        MarketProcess.updateMarketOI(
            MarketProcess.UpdateOIParams(
                true,
                symbolProps.code,
                params.marginToken,
                increaseQty,
                params.indexTokenPrice,
                params.isLong
            )
        );

@>>        LpPoolProcess.holdPoolAmount(symbolProps.stakeToken, position.marginToken, increaseHoldAmount, params.isLong);

        position.emitOpenPositionUpdateEvent(
            params.requestId,
            Position.PositionUpdateFrom.ORDER_INCREASE,
            params.indexTokenPrice,
            fee
        );
    }

The above 10,000 USDC collateral could entirely be 10 BTC (or other non-market trading pair tokens). It can be seen that the code in increaseHoldAmount does not distinguish between isolated mode and cross margin mode, which is therefore incorrect.

Impact

This results in financial loss for the protocol borrow fee.

Code Snippet

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

Tool used

Manual Review

Recommendation

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

[nevillehuang]

request poc

[sherlock-admin4]

PoC requested from @ZeroTrust01

Requests remaining: 11

[nevillehuang]

Very similar to issue #273.

[ZeroTrust01]

POC:
In a market for a trading pair (such as WETH-USDC), only the two tokens of the trading pair are involved in transactions, with any shortfall provided by the pool market through lending. Suppose the price of WETH is 3000 USD and the price of USDC is 1 USD.

In isolated mode:

•	If a user uses 1 WETH to open a 2x leverage long position, they borrow funds of 1 WETH (3000 USD) from the TokenPool.
•	If a user uses 3000 USDC to open a 2x leverage short position, they borrow funds of 3000 USDC (3000 USD) from the USDPool.

So the borrow fee is like: 3000USD*BORROW_FEE_RATE

However, in cross margin mode:

•	If a user uses 1 BTC (60000 USD) as collateral and opens a 2x leverage long position, they should borrow funds of 2 WETH (6000 USD).
•	If a user opens a 2x leverage short position, they borrow funds of 6000 USDC (6000 USD).

In this case, the user doesn’t use the funds of the trading pair directly; instead, they use BTC and position value as collateral to borrow from the pool.
So the borrow fee is like: 6000USD*BORROW_FEE_RATE
In addition, in cross margin mode the collateral is held in the PortfolioVault, which is just like a collateralized lending market.

Similarly, an example of 1x leverage is as follows:
In isolated mode:

•	If a user uses 1 WETH to open a 1x leverage long position, they borrow funds of 0 WETH (3000 USD) from the TokenPool.
•	If a user uses 3000 USDC to open a 1x leverage short position, they borrow funds of 0 USDC (0 USD) from the USDPool.

So the borrow fee is like: 0USD*BORROW_FEE_RATE

However, in cross margin mode:

•	If a user uses 1 BTC (60000 USD) as collateral and opens a 1x leverage long position, they should borrow funds of 1 WETH (3000 USD).
•	If a user opens a 1x leverage short position, they borrow funds of 3000 USDC (3000 USD).

In this case, the user doesn’t use the funds of the trading pair directly; instead, they use BTC and position value as collateral to borrow from the pool.
So the borrow fee is like: 3000USD*BORROW_FEE_RATE

[nevillehuang]

@ZeroTrust01 Sponsor comments, could be invalid

All transactions are ultimately settled in ETH, and the principal amount remains the same.

[ZeroTrust01]

This issue concerns the source of funds used for trading. Regardless of the currency(token) in which the trade is ultimately settled, it involves understanding how much funds the user has borrowed and how much fees they need to pay.
As I mentioned above, in isolated mode and cross margin mode, the funds borrowed from pool(tokenPool or USDPool) by the user are different.

[0xELFi02]

@ZeroTrust01 is right

[0xELFi02]

• 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 gets 1WETH (Both cross and isolate)

[nevillehuang]

@0xELFi02 Not quite sure if I am getting your point. You are agreeing this is a valid issue?

[0xELFi02]

@nevillehuang No, It should be invalid

[ZeroTrust01]

• 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 gets 1WETH (Both cross and isolate)

In a market for a trading pair (such as WETH-USDC), only the two tokens of the trading pair are involved in trade.
Let's only consider using 1 leverage
Suppose the price of WBTC is 60,000 USD, the price of WETH is 1,000 USD, and the price of USDC is 1 USD.

In isolated mode, a user can go long with one WETH or go short with 1,000 USDC.

In cross margin mode, however, even if a user has one WBTC(Using 1000 USDT as an example would be more appropriate), I doubt they can directly use a portion of its value (1,000 USD) to participate in the market directly If it’s not using WBTC as collateral to borrow WETH or USDC.

For a trading pair market, only the two tokens of the pair can directly participate in trading. This is the focus of our discussion.

For example, in a USD-Tesla stock trading pair, even if my account has a large amount of euros, can I directly use my euros to buy Tesla stock?

If you believe that in a WETH-USDC trading pair market, more than just WETH and USDC can directly participate in the trading, then I have nothing further to add.
@nevillehuang @0xELFi02
Thanks!

[0502lian]

Escalate

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.

I think 1000 USDT can not directly participate in trading in a market for a trading pair (such as WETH-USDC).
He must use 1000 USDT as collateral to borrow WETH (1000 USD) or USDC (1000 USD) in order to participate in market trading.
So this should be a valid issue.

[sherlock-admin3]

Escalate

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.

I think 1000 USDT can not directly participate in trading in a market for a trading pair (such as WETH-USDC).
He must use 1000 USDT as collateral to borrow WETH (1000 USD) or USDC (1000 USD) in order to participate in market trading.
So this should be a valid issue.

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.

[0xELFi02]

@0502lian "in cross margin mode the collateral is held in the PortfolioVault, which is just like a collateralized lending market.
", this is right from @ZeroTrust01 above.

Let's focus on the controversial points, let's discuss the "A very simple scenario" above you mentioned:
If there is only 1 pair eth/usd, in cross, you have 1 btc and deposit as collateral. can you long or short eth/usd now? my answer is yes.
use long position as a example, if got profit , you'll get eth, if you loss, you'll get eth liability.

[0502lian]

@0502lian "in cross margin mode the collateral is held in the PortfolioVault, which is just like a collateralized lending market. ", this is right from @ZeroTrust01 above.

Let's focus on the controversial points, let's discuss the "A very simple scenario" above you mentioned: If there is only 1 pair eth/usd, in cross, you have 1 btc and deposit as collateral. can you long or short eth/usd now? my answer is yes. use long position as a example, if got profit , you'll get eth, if you loss, you'll get eth liability.

So use long position(1 leverage) as a example , do you borrow `eth' or not ???
If not, where is the one eth for trading?

[WangSecurity]

@0xELFi02 could you clarify this part:

So use long position(1 leverage) as a example , do you borrow `eth' or not ???
If not, where is the one eth for trading?

[0xELFi]

With 1 leverage, the user does not need to borrow funds. For isolate margin, the principal comes from the user's own wallet. For cross margin mode, the principal is borrowed through collateral, meaning the margin principal is also effectively borrowed

[0502lian]

With 1 leverage, the user does not need to borrow funds. For isolate margin, the principal comes from the user's own wallet. For cross margin mode, the principal is borrowed through collateral, meaning the margin principal is also effectively borrowed

Let’s focus our discussion on users in the cross margin mode scenario.

Just as you said "the principal is borrowed through collateral".
Borrowing requires a lender, doesn’t it? Can you help us by telling who the lender is? If the amount borrowed is 1e18 WETH, then if the user’s position(or account) increases by 1e18 WETH, whose account decreases by 1e18 WETH?

[mstpr]

Escalate

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.

I think 1000 USDT can not directly participate in trading in a market for a trading pair (such as WETH-USDC). He must use 1000 USDT as collateral to borrow WETH (1000 USD) or USDC (1000 USD) in order to participate in market trading. So this should be a valid issue.

Please correct me if I am wrong:

Both of the trades isolated/cross the initial margin is same. However, when you close the position in isolated mode you get the entire settle margin which is profit + initial margin which is 1.5WETH. When closing the cross position you only get the profit (naturally) which is 0.5WETH. Then you can withdraw your cross funds if you like and you can end up as same as the isolated one. I don't see the problem here

[0502lian]

Escalate
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.

I think 1000 USDT can not directly participate in trading in a market for a trading pair (such as WETH-USDC). He must use 1000 USDT as collateral to borrow WETH (1000 USD) or USDC (1000 USD) in order to participate in market trading. So this should be a valid issue.

Please correct me if I am wrong:

Both of the trades isolated/cross the initial margin is same. However, when you close the position in isolated mode you get the entire settle margin which is profit + initial margin which is 1.5WETH. When closing the cross position you only get the profit (naturally) which is 0.5WETH. Then you can withdraw your cross funds if you like and you can end up as same as the isolated one. I don't see the problem here

We talked about 1x leverage here.
you get the entire settle margin which is profit + initial margin which is 1.5WETH. When closing the cross position you only get the profit (naturally) which is 0.5WETH.—— Is your calculation based on 1x leverage?

[mstpr]

If we are talking about 1x Leverage then I don't see any difference between this issue and the other one.

[WangSecurity]

I'm not sure what the issue here is exactly. The example from the report looks correct and how it should be, no?

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 the isolated case, the user receives 1 WETH (2000 USD), because when they opened a position 1 WETH (1000 USD) was transferred from the user into the contract, so in the end, they receive their initial USD value (1000 USD == 0.5 WETH) and profit in USD (1000 USD == 0.5 WETH), though WETH value didn't change.

In the cross case, the user didn't provide 1 WETH, but opened a position of 1 WETH (1000 USD), with collateral in another token (say USDC). When closing the position, they got their profit value in USD (1000 USD == 0.5 WETH), but didn't receive initial value cause they didn't provide any.

The impact of the report is that the borrowing fee wasn't taken, but based on the comment here there is no borrowing with 1x leverage, hence, no borrowing fee. So the impact is incorrect and report seems to be just explaining how the protocol works.

Planning to reject the escalation and leave the issue as it is.

[0502lian]

Agree.
I no longer insist that this issue is valid; it seems more like a suggestion. They need to do a real borrow for initial margin.
The protocol did not borrow tokens in the correct manner but instead created WETH out of None (it wasn’t borrowed or bought; it was just created), which led to the results seen in issue 273.

[WangSecurity]

Result:
Invalid
Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• 0502lian: rejected