The same deposited tokens can be used as margins for multiple positions

Summary

When a user deposits or withdraws tokens of any valid token type T through AccountFacet.deposit/createWithdrawRequest, these tokens can be used to increase/decrease the initialMarginInUsdFromBalance field for any number of positions whose marginToken is T. However, the same deposited/withdrawn tokens should only be counted into/out of the margin of at most one cross-margin position.

Vulnerability Detail

The AccountFacet.deposit/executeWithdraw function calls AssetsProcess.deposit/withdraw, which eventually calls PositionMarginProcess.updateAllPositionFromBalanceMargin. In this function, the fourth parameter of updatePositionFromBalanceMargin is the variable amount, which means that the same deposited/withdrawn tokens are counted into/out of the margin of multiple positions whose marginToken is the same as the token argument (by increasing/decreasing their initialMarginInUsdFromBalance fields). As a matter of factor, the value of the fourth argument of the call to updatePositionFromBalanceMargin should be reduceAmount.

    function updateAllPositionFromBalanceMargin(
        uint256 requestId,
        address account,
        address token,
        int256 amount,
        bytes32 originPositionKey
    ) external {
        if (amount == 0) {
            return;
        }
        bytes32[] memory positionKeys = Account.load(account).getAllPosition();
        int256 reduceAmount = amount;
        for (uint256 i; i < positionKeys.length; i++) {
            Position.Props storage position = Position.load(positionKeys[i]);
            if (token == position.marginToken && position.isCrossMargin) {
                int256 changeAmount = updatePositionFromBalanceMargin(
                    position,
                    originPositionKey.length > 0 && originPositionKey == position.key,
                    requestId,
                    amount
                ).toInt256();
                reduceAmount = amount > 0 ? reduceAmount - changeAmount : reduceAmount + changeAmount;
                if (reduceAmount == 0) {
                    break;
                }
            }
        }
    }
    function updatePositionFromBalanceMargin(
        Position.Props storage position,
        bool needSendEvent,
        uint256 requestId,
        int256 amount
    ) public returns (uint256 changeAmount) {
        if (position.initialMarginInUsd == position.initialMarginInUsdFromBalance || amount == 0) {
            changeAmount = 0;
            return 0;
        }
        if (amount > 0) {
            uint256 borrowMargin = (position.initialMarginInUsd - position.initialMarginInUsdFromBalance)
                .mul(position.initialMargin)
                .div(position.initialMarginInUsd);
            changeAmount = amount.toUint256().min(borrowMargin);
            position.initialMarginInUsdFromBalance += changeAmount.mul(position.initialMarginInUsd).div(
                position.initialMargin
            );
        } else {
            uint256 addBorrowMarginInUsd = (-amount).toUint256().mul(position.initialMarginInUsd).div(
                position.initialMargin
            );
            if (position.initialMarginInUsdFromBalance <= addBorrowMarginInUsd) {
                position.initialMarginInUsdFromBalance = 0;
                changeAmount = position.initialMarginInUsdFromBalance.mul(position.initialMargin).div(
                    position.initialMarginInUsd
                );
            } else {
                position.initialMarginInUsdFromBalance -= addBorrowMarginInUsd;
                changeAmount = (-amount).toUint256();
            }
        }
        if (needSendEvent && changeAmount > 0) {
            position.emitPositionUpdateEvent(requestId, Position.PositionUpdateFrom.DEPOSIT, 0);
        }
    }

As evidence, if we first deposit tokens and then increase a position, we find that when some tokens are used as the margin of one position ("used as margin" means counted into the initialMarginInUsdFromBalance here), the usedAmount value increases by initialMarginInUsd (initialMarginInUsd >= initialMarginInUsdFromBalance), thus preventing the same tokens from being used as margin for other positions. This should be the expected behavior.

    function _executeIncreaseOrderMargin(
        Order.OrderInfo memory order,
        Account.Props storage accountProps,
        uint256 marginTokenPrice
    ) internal returns (uint256 orderMargin, uint256 orderMarginFromBalance) {
        address marginToken = order.marginToken;
        address account = accountProps.owner;
        if (order.isCrossMargin) {
            // ......
            orderMargin = CalUtils.usdToToken(
                order.orderMargin,
                TokenUtils.decimals(marginToken),
                marginTokenPrice
            );
            orderMarginFromBalance = accountProps.useToken(
                marginToken,
                orderMargin,
                false,
                Account.UpdateSource.INCREASE_POSITION
            );
        } else {
            // ......
        }
    }

    function useToken(Props storage self, address token, uint256 amount) external returns (uint256 useFromBalance) {
        return useToken(self, token, amount, false, UpdateSource.DEFAULT);
    }
    function useToken(
        Props storage self,
        address token,
        uint256 amount,
        bool isCheck,
        UpdateSource source
    ) public returns (uint256 useFromBalance) {
        if (!self.tokens.contains(token)) {
            self.tokens.add(token);
        }
        TokenBalance storage balance = self.tokenBalances[token];
        require(!isCheck || balance.amount >= balance.usedAmount + amount, "use token failed with amount not enough");
        TokenBalance memory preBalance = balance;
        if (balance.amount >= balance.usedAmount + amount) {
            balance.usedAmount += amount;
            useFromBalance = amount;
        } else if (balance.amount > balance.usedAmount) {
            useFromBalance = balance.amount - balance.usedAmount;
            balance.usedAmount += amount;
        } else {
            balance.usedAmount += amount;
            useFromBalance = 0;
        }
        emit AccountTokenUpdateEvent(self.owner, token, preBalance, balance, source);
    }

Impact

The same deposited/withdrawn tokens can be counted into the updating of the initialMarginInUsdFromBalance field for multiple positions. The contract will thus overestimate/underestimate the risk of affected crossMargin positions. Users can profit accordingly.

Code Snippet

Tool used

Manual Review

Recommendation

Fixed the fourth parameter of the updatePositionFromBalanceMargin function call in the updateAllPositionFromBalanceMargin function to reduceAmount.

Duplicate of #35

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

link - The same deposited tokens can be used as margins for multiple positions #265