aman - `_executeDecreaseOrder` pass `isCrossMargin=false` is not correct crossMargin value

[sherlock-admin3]

aman

Medium

_executeDecreaseOrder pass isCrossMargin=false is not correct crossMargin value

Summary

While processing the decreasef position order we passed isCrossMargin=false with out considering that the position isCrossMargin=true.

Vulnerability Detail

While in case of autoReducePositions we passed the correct value from position.isCrossMargin.

function _executeDecreaseOrder(
        uint256 orderId,
        Order.OrderInfo memory order,
        Symbol.Props memory symbolProps
    ) internal {
        address account = order.account;
        bool isLong = Order.Side.LONG == order.orderSide;

        Position.Props storage position = Position.load(account, order.symbol, order.marginToken, order.isCrossMargin);
        position.checkExists();

        if (position.isLong == isLong) {
            revert Errors.DecreaseOrderSideInvalid();
        }

        if (position.qty < order.qty) {
            order.qty = position.qty;
        }

        MarketProcess.updateMarketFundingFeeRate(symbolProps.code);
        MarketProcess.updatePoolBorrowingFeeRate(symbolProps.stakeToken, position.isLong, order.marginToken);

        uint256 executionPrice = _getExecutionPrice(order, symbolProps.indexToken);

        position.decreasePosition(
            DecreasePositionProcess.DecreasePositionParams(
                orderId,
                order.symbol,
                false,
@>              false,
                order.marginToken,
                order.qty,
                executionPrice
            )
        );
        Account.load(order.account).delOrder(orderId);
        emit OrderFilledEvent(orderId, order, block.timestamp, executionPrice);
    }

Impact

The Protocol will consider crossMargin position as isolate Position.

Code Snippet

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

Tool used

Manual Review

Recommendation

pass correct value of isCrossMargin in decreasePosition

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/0xCedar/elfi-perp-contracts/pull/31

[nevillehuang]

Request poc

This seems correctly described but LSW comments seems to be accurate:

autoReducePositions always send isLiquidation as false which the actual "isCrossMargin" value is only used if the "isLiquidation" is true, so no impact here.

[sherlock-admin4]

PoC requested from @amankakar

Requests remaining: 6

[amankakar]

Request poc

This seems correctly described but LSW comments seems to be accurate:

autoReducePositions always send isLiquidation as false which the actual "isCrossMargin" value is only used if the "isLiquidation" is true, so no impact here.

I agree with LSW's comment. While it won't cause any issues with the current implementation, it could lead to problems in the future. As mentioned in the contest's ReadMe, Watson could report a issue, which posing risks for future integrations.

Should potential issues, like broken assumptions about function behavior, be reported if they could pose risks in future integrations, even if they might not be an issue in the context of the scope? If yes, can you elaborate on properties/invariants that should hold?

yes

[nevillehuang]

Per consistency with beefy escalations, as seen here, future integrations must be explicitly stated if not it won't be considered.

Future issues: Issues that result out of a future integration/implementation that was not mentioned in the docs/README or because of a future change in the code (as a fix to another issue) are not valid issues.