sherlock-audit / 2023-12-flatmoney-judging

11 stars 9 forks source link

0xMAKEOUTHILL - User can create NON-liquidable and unclosable leverage positions leading to protocol insolvency #137

Closed sherlock-admin closed 8 months ago

sherlock-admin commented 8 months ago

0xMAKEOUTHILL

high

User can create NON-liquidable and unclosable leverage positions leading to protocol insolvency

Summary

It is possible for a user to create NON-liquidable and unclosable leverage positions which will lead to insolvency for the protocol

Vulnerability Detail

User announce a leverage position by announceLeverageOpen:

function announceLeverageOpen(
        uint256 margin,
        uint256 additionalSize,
        uint256 maxFillPrice,
        uint256 keeperFee
    ) external whenNotPaused {

        ILeverageModule leverageModule = ILeverageModule(vault.moduleAddress(FlatcoinModuleKeys._LEVERAGE_MODULE_KEY));

        // settles vault fees, checks the keeper fee is in valid range,
        // if there is an ongoing announcement order - cancel it if expired,
        // creates the executableTime = block.timestamp + min.exectuableTime()
        uint64 executableAtTime = _prepareAnnouncementOrder(keeperFee);

        vault.checkSkewMax({additionalSkew: additionalSize});

        leverageModule.checkLeverageCriteria(margin, additionalSize);

        (uint256 currentPrice, ) = IOracleModule(vault.moduleAddress(FlatcoinModuleKeys._ORACLE_MODULE_KEY)).getPrice();

        if (maxFillPrice < currentPrice) revert FlatcoinErrors.MaxFillPriceTooLow(maxFillPrice, currentPrice);

        uint256 tradeFee = leverageModule.getTradeFee(additionalSize);

        if (
            ILiquidationModule(vault.moduleAddress(FlatcoinModuleKeys._LIQUIDATION_MODULE_KEY)).getLiquidationMargin(
                additionalSize,
                maxFillPrice
            ) >= margin
        ) revert FlatcoinErrors.PositionCreatesBadDebt();

        _announcedOrder[msg.sender] = FlatcoinStructs.Order({
            orderType: FlatcoinStructs.OrderType.LeverageOpen,
            orderData: abi.encode(
                FlatcoinStructs.AnnouncedLeverageOpen({
                    margin: margin,
                    additionalSize: additionalSize,
                    maxFillPrice: maxFillPrice,
                    tradeFee: tradeFee
                })
            ),
            keeperFee: keeperFee,
            executableAtTime: executableAtTime
        });

        // Sends collateral to the delayed order contract first before it is settled by keepers and sent to the vault
        vault.collateral().safeTransferFrom(msg.sender, address(this), margin + keeperFee + tradeFee);

        emit FlatcoinEvents.OrderAnnounced({
            account: msg.sender,
            orderType: FlatcoinStructs.OrderType.LeverageOpen,
            keeperFee: keeperFee
        });
    }

The code is very straight forward, the only thing go get here is that in order for the announced order to be executed the minimum executableTime must have passed. Then a keeper decides to execute the position by executeOrder with the address of the account that wants to open the current position:

function executeOrder(
        address account,
        bytes[] calldata priceUpdateData
    )
        external
        payable
        nonReentrant
        whenNotPaused
        updatePythPrice(vault, msg.sender, priceUpdateData)
        orderInvariantChecks(vault)
    {

        // Settle funding fees before executing any order.
        // This is to avoid error related to max caps or max skew reached when the market has been skewed to one side for a long time.
        // This is more important in case the we allow for limit orders in the future.
        vault.settleFundingFees();

        FlatcoinStructs.OrderType orderType = _announcedOrder[account].orderType;

        // If there is no order in store, just return.
        if (orderType == FlatcoinStructs.OrderType.None) return;

        if (orderType == FlatcoinStructs.OrderType.StableDeposit) {
            _executeStableDeposit(account);
        } else if (orderType == FlatcoinStructs.OrderType.StableWithdraw) {
            _executeStableWithdraw(account);
        } else if (orderType == FlatcoinStructs.OrderType.LeverageOpen) {
            _executeLeverageOpen(account);
        } else if (orderType == FlatcoinStructs.OrderType.LeverageClose) {
            _executeLeverageClose(account);
        } else if (orderType == FlatcoinStructs.OrderType.LeverageAdjust) {
            _executeLeverageAdjust(account);
        }
    }

Which in return calls _executeLeverageOpen:

function _executeLeverageOpen(address account) internal returns (uint256 tokenId) {

        FlatcoinStructs.Order memory order = _announcedOrder[account];

        FlatcoinStructs.AnnouncedLeverageOpen memory announcedOpen = abi.decode(
            order.orderData,
            (FlatcoinStructs.AnnouncedLeverageOpen)
        );

        _prepareExecutionOrder(account, order.executableAtTime);

        vault.collateral().safeTransfer({
            to: address(vault),
            value: announcedOpen.margin + announcedOpen.tradeFee + order.keeperFee
        }); // transfer collateral + fees to the vault

        tokenId = ILeverageModule(vault.moduleAddress(FlatcoinModuleKeys._LEVERAGE_MODULE_KEY)).executeOpen({
            account: account,
            keeper: msg.sender,
            order: order
        });

        emit FlatcoinEvents.OrderExecuted({account: account, orderType: order.orderType, keeperFee: order.keeperFee});
    }

So here - _prepareExecutionOrder makes sure the min executable time has passed in order to initiate the execution, which is continued by executeOpen in the LeverageModule:

function executeOpen(
        address _account,
        address _keeper,
        FlatcoinStructs.Order calldata _order
    ) external whenNotPaused onlyAuthorizedModule returns (uint256 _newTokenId) {

        // Make sure the oracle price is after the order executability time
        //block.timestamp - executableAtTime
        uint32 maxAge = _getMaxAge(_order.executableAtTime);

        FlatcoinStructs.AnnouncedLeverageOpen memory announcedOpen = abi.decode(
            _order.orderData,
            (FlatcoinStructs.AnnouncedLeverageOpen)
        );

        // Check that buy price doesn't exceed requested price.
        (uint256 entryPrice, ) = IOracleModule(vault.moduleAddress(FlatcoinModuleKeys._ORACLE_MODULE_KEY)).getPrice({
            maxAge: maxAge
        });

        if (entryPrice > announcedOpen.maxFillPrice)
            revert FlatcoinErrors.HighSlippage(entryPrice, announcedOpen.maxFillPrice);

        vault.checkSkewMax({additionalSkew: announcedOpen.additionalSize});

        {
            // The margin change is equal to funding fees accrued to longs and the margin deposited by the trader.
            vault.updateGlobalPositionData({
                price: entryPrice,
                marginDelta: int256(announcedOpen.margin),
                additionalSizeDelta: int256(announcedOpen.additionalSize)
            });

            _newTokenId = _mint(_account);

            vault.setPosition(
                FlatcoinStructs.Position({
                    lastPrice: entryPrice,
                    marginDeposited: announcedOpen.margin,
                    additionalSize: announcedOpen.additionalSize,
                    entryCumulativeFunding: vault.cumulativeFundingRate()
                }),
                _newTokenId
            );
        }

        // Check that the new position isn't immediately liquidatable.
        if (
            ILiquidationModule(vault.moduleAddress(FlatcoinModuleKeys._LIQUIDATION_MODULE_KEY)).canLiquidate(
                _newTokenId
            )
        ) revert FlatcoinErrors.PositionCreatesBadDebt();

        // Mint points
        IPointsModule pointsModule = IPointsModule(vault.moduleAddress(FlatcoinModuleKeys._POINTS_MODULE_KEY));
        pointsModule.mintLeverageOpen(_account, announcedOpen.additionalSize);

        // Settle the collateral
        vault.updateStableCollateralTotal(int256(announcedOpen.tradeFee)); // pay the trade fee to stable LPs
        vault.sendCollateral({to: _keeper, amount: _order.keeperFee}); // pay the keeper their fee

        emit FlatcoinEvents.LeverageOpen(_account, _newTokenId);
    }

So far so good, nothing complicated, we just went through the process of opening a leverage position.

Now the interesting part begins.

User decides to adjust his position, so he calls announceLeverageAdjust:

/// @notice Announces leverage adjust intent for keepers to execute at offchain oracle price.
    /// @param tokenId The ERC721 token ID of the position.
    /// @param marginAdjustment The amount of margin to deposit or withdraw.
    /// @param additionalSizeAdjustment The amount of additional size to increase or decrease.
    /// @param fillPrice The price at which the trade can be executed.
    /// @param keeperFee The fee the user is paying for keeper transaction execution (in collateral tokens).
    function announceLeverageAdjust(
        uint256 tokenId,
        int256 marginAdjustment,
        int256 additionalSizeAdjustment,
        uint256 fillPrice,
        uint256 keeperFee
    ) external whenNotPaused {

        // settles vault fees, checks the keeper fee is in valid range,
        // if there is an ongoing announcement order - cancel it if expired,
        // creates the executableTime - block.timestamp + min.exectuableTime()
        uint64 executableAtTime = _prepareAnnouncementOrder(keeperFee);

        // If both adjustable parameters are zero, there is nothing to adjust
        if (marginAdjustment == 0 && additionalSizeAdjustment == 0)
            revert FlatcoinErrors.ZeroValue("marginAdjustment|additionalSizeAdjustment");

        ILeverageModule leverageModule = ILeverageModule(vault.moduleAddress(FlatcoinModuleKeys._LEVERAGE_MODULE_KEY));

        // Check that the caller is the owner of the token
        if (leverageModule.ownerOf(tokenId) != msg.sender) revert FlatcoinErrors.NotTokenOwner(tokenId, msg.sender);

        // Trade fee is calculated based on additional size change
        uint256 totalFee;

        {

            uint256 tradeFee;

            (uint256 currentPrice, ) = IOracleModule(vault.moduleAddress(FlatcoinModuleKeys._ORACLE_MODULE_KEY))
                .getPrice();

            // Means increasing or decreasing additional size
            if (additionalSizeAdjustment >= 0) {

                // If additionalSizeAdjustment equals zero, trade fee is zero as well
                tradeFee = leverageModule.getTradeFee(uint256(additionalSizeAdjustment));
                vault.checkSkewMax(uint256(additionalSizeAdjustment));

                if (fillPrice < currentPrice) revert FlatcoinErrors.MaxFillPriceTooLow(fillPrice, currentPrice);

            } else {

                tradeFee = leverageModule.getTradeFee(uint256(additionalSizeAdjustment * -1));

                if (fillPrice > currentPrice) revert FlatcoinErrors.MinFillPriceTooHigh(fillPrice, currentPrice);
            }

            totalFee = tradeFee + keeperFee;
        }

        {

            // New additional size will be either bigger or smaller than current additional size
            // depends on if additionalSizeAdjustment is positive or negative.
            int256 newAdditionalSize = int256(vault.getPosition(tokenId).additionalSize) + additionalSizeAdjustment;

            // If user withdraws margin or changes additional size with no changes to margin, fees are charged from their existing margin.
            int256 newMarginAfterSettlement = leverageModule.getPositionSummary(tokenId).marginAfterSettlement +
                ((marginAdjustment > 0) ? marginAdjustment : marginAdjustment - int256(totalFee));

            // New margin or size can't be negative, which means that they want to withdraw more than they deposited or not enough to pay the fees
            if (newMarginAfterSettlement < 0 || newAdditionalSize < 0)
                revert FlatcoinErrors.ValueNotPositive("newMarginAfterSettlement|newAdditionalSize");

            if (
                ILiquidationModule(vault.moduleAddress(FlatcoinModuleKeys._LIQUIDATION_MODULE_KEY))
                    .getLiquidationMargin(uint256(newAdditionalSize), fillPrice) >= uint256(newMarginAfterSettlement)
            ) revert FlatcoinErrors.PositionCreatesBadDebt();

            // New values can't be less than min margin and min/max leverage requirements.
            leverageModule.checkLeverageCriteria(uint256(newMarginAfterSettlement), uint256(newAdditionalSize));
        }

        _announcedOrder[msg.sender] = FlatcoinStructs.Order({
            orderType: FlatcoinStructs.OrderType.LeverageAdjust,
            orderData: abi.encode(
                FlatcoinStructs.AnnouncedLeverageAdjust({
                    tokenId: tokenId,
                    marginAdjustment: marginAdjustment,
                    additionalSizeAdjustment: additionalSizeAdjustment,
                    fillPrice: fillPrice,
                    tradeFee: totalFee - keeperFee,
                    totalFee: totalFee
                })
            ),
            keeperFee: keeperFee,
            executableAtTime: executableAtTime
        });

        // Lock the NFT belonging to this position so that it can't be transferred to someone else.
        // Locking doesn't require an approval from the leverage trader.
        leverageModule.lock(tokenId);

        // If user increases margin, fees are charged from their account.
        if (marginAdjustment > 0) {
            // Sending positive margin adjustment and both fees from the user to the delayed order contract.
            vault.collateral().safeTransferFrom(msg.sender, address(this), uint256(marginAdjustment) + totalFee);
        }

        emit FlatcoinEvents.OrderAnnounced({
            account: msg.sender,
            orderType: FlatcoinStructs.OrderType.LeverageAdjust,
            keeperFee: keeperFee
        });
    }

Now keep in mind some things:

1 - User still owns the tokenID(position) 2 - An announced order struct is constructed with the current parameters of adjusting the leverage size and the margin of the current tokenID(position)

_announcedOrder[msg.sender] = FlatcoinStructs.Order({
            orderType: FlatcoinStructs.OrderType.LeverageAdjust,
            orderData: abi.encode(
                FlatcoinStructs.AnnouncedLeverageAdjust({
                    tokenId: tokenId,
                    marginAdjustment: marginAdjustment,
                    additionalSizeAdjustment: additionalSizeAdjustment,
                    fillPrice: fillPrice,
                    tradeFee: totalFee - keeperFee,
                    totalFee: totalFee
                })
            ),
            keeperFee: keeperFee,
            executableAtTime: executableAtTime
        });

Now while this particular user has an already open position user AND AN ANNOUNCED ADJUSTMENT OF HIS POSITION (keep in mind announced, not yet executed to make any effect), he can create a limit order on his position to set his loss and profit stoppers.

In LimitOrder:

function announceLimitOrder(uint256 tokenId, uint256 priceLowerThreshold, uint256 priceUpperThreshold) external {

        uint64 executableAtTime = _prepareAnnouncementOrder();

        address positionOwner = _checkPositionOwner(tokenId);

        _checkThresholds(priceLowerThreshold, priceUpperThreshold);

        uint256 tradeFee = ILeverageModule(vault.moduleAddress(FlatcoinModuleKeys._LEVERAGE_MODULE_KEY)).getTradeFee(
            vault.getPosition(tokenId).additionalSize
        );

        _limitOrderClose[tokenId] = FlatcoinStructs.Order({
            orderType: FlatcoinStructs.OrderType.LimitClose,
            orderData: abi.encode(
                FlatcoinStructs.LimitClose(tokenId, priceLowerThreshold, priceUpperThreshold, tradeFee)
            ),
            keeperFee: 0, // Not applicable for limit orders. Keeper fee will be determined at execution time.
            executableAtTime: executableAtTime
        });

        // Lock the NFT belonging to this position so that it can't be transferred to someone else.
        ILeverageModule(vault.moduleAddress(FlatcoinModuleKeys._LEVERAGE_MODULE_KEY)).lock(tokenId);

        emit FlatcoinEvents.OrderAnnounced({
            account: positionOwner,
            orderType: FlatcoinStructs.OrderType.LimitClose,
            keeperFee: 0
        });
    }

Here the user announces a limit order for his position, he can make the priceLowerThreshold and priceUpperThreshold so that it can almost immediately be eligible for execution before his announced order becomes executed.

So executeLimitOrder is called on his current position:

function executeLimitOrder(
        uint256 tokenId,
        bytes[] calldata priceUpdateData
    ) external payable nonReentrant updatePythPrice(vault, msg.sender, priceUpdateData) orderInvariantChecks(vault) {

        _checkLimitCloseOrder(tokenId);

        vault.settleFundingFees();

        _closePosition(tokenId);
    }
function _closePosition(uint256 tokenId) internal {

        ILeverageModule leverageModule = ILeverageModule(vault.moduleAddress(FlatcoinModuleKeys._LEVERAGE_MODULE_KEY));

        FlatcoinStructs.Order memory order = _limitOrderClose[tokenId];

        FlatcoinStructs.LimitClose memory _limitOrder = abi.decode(
            _limitOrderClose[tokenId].orderData,
            (FlatcoinStructs.LimitClose)
        );

        address account = leverageModule.ownerOf(tokenId);

        (uint256 price, ) = IOracleModule(vault.moduleAddress(FlatcoinModuleKeys._ORACLE_MODULE_KEY)).getPrice();

        // Check that the minimum time delay is reached before execution
        if (block.timestamp < order.executableAtTime)
            revert FlatcoinErrors.ExecutableTimeNotReached(order.executableAtTime);

        uint256 minFillPrice;

        if (price <= _limitOrder.priceLowerThreshold) {
            minFillPrice = 0; // can execute below lower limit price threshold
        } else if (price >= _limitOrder.priceUpperThreshold) {
            minFillPrice = _limitOrder.priceUpperThreshold;
        } else {
            revert FlatcoinErrors.LimitOrderPriceNotInRange(
                price,
                _limitOrder.priceLowerThreshold,
                _limitOrder.priceUpperThreshold
            );
        }

        // Delete the order tracker from storage.
        delete _limitOrderClose[tokenId];

        order.orderData = abi.encode(
            FlatcoinStructs.AnnouncedLeverageClose({
                tokenId: tokenId,
                minFillPrice: minFillPrice,
                tradeFee: _limitOrder.tradeFee
            })
        );

        order.keeperFee = IKeeperFee(vault.moduleAddress(FlatcoinModuleKeys._KEEPER_FEE_MODULE_KEY)).getKeeperFee();

        leverageModule.executeClose({account: account, keeper: msg.sender, order: order});

        emit FlatcoinEvents.OrderExecuted({account: account, orderType: order.orderType, keeperFee: order.keeperFee});
    }

Assume all the checks pass, so we go straight into closing his order by calling executeClose:

function executeClose(
        address _account,
        address _keeper,
        FlatcoinStructs.Order calldata _order
    ) external whenNotPaused onlyAuthorizedModule returns (int256 settledMargin) {

        FlatcoinStructs.AnnouncedLeverageClose memory announcedClose = abi.decode(
            _order.orderData,
            (FlatcoinStructs.AnnouncedLeverageClose)
        );

        FlatcoinStructs.Position memory position = vault.getPosition(announcedClose.tokenId);

        // Make sure the oracle price is after the order executability time
        uint32 maxAge = _getMaxAge(_order.executableAtTime);

        // check that sell price doesn't exceed requested price
        (uint256 exitPrice, ) = IOracleModule(vault.moduleAddress(FlatcoinModuleKeys._ORACLE_MODULE_KEY)).getPrice({
            maxAge: maxAge
        });

        if (exitPrice < announcedClose.minFillPrice)
            revert FlatcoinErrors.HighSlippage(exitPrice, announcedClose.minFillPrice);

        uint256 totalFee;

        {

            FlatcoinStructs.PositionSummary memory positionSummary = PerpMath._getPositionSummary(
                position,
                vault.cumulativeFundingRate(),
                exitPrice
            );

            settledMargin = positionSummary.marginAfterSettlement;
            totalFee = announcedClose.tradeFee + _order.keeperFee;

            if (settledMargin <= 0) revert FlatcoinErrors.ValueNotPositive("settledMargin");
            // Make sure there is enough margin in the position to pay the keeper fee
            if (settledMargin < int256(totalFee)) revert FlatcoinErrors.NotEnoughMarginForFees(settledMargin, totalFee);

            vault.updateGlobalPositionData({
                price: exitPrice,
                marginDelta: -settledMargin,
                additionalSizeDelta: -int256(position.additionalSize)
            });

            // Delete position storage
            vault.deletePosition(announcedClose.tokenId);
        }

        // Cancel any existing limit order on the position
        ILimitOrder(vault.moduleAddress(FlatcoinModuleKeys._LIMIT_ORDER_KEY)).cancelExistingLimitOrder(
            announcedClose.tokenId
        );

        // A position NFT has to be unlocked before burning otherwise, the transfer to address(0) will fail.
        _unlock(announcedClose.tokenId);
        _burn(announcedClose.tokenId);

        vault.updateStableCollateralTotal(int256(announcedClose.tradeFee)); // pay the trade fee to stable LPs

        // Settle the collateral.
        vault.sendCollateral({to: _keeper, amount: _order.keeperFee}); // pay the keeper their fee
        vault.sendCollateral({to: _account, amount: uint256(settledMargin) - totalFee}); // transfer remaining amount to the trader

        emit FlatcoinEvents.LeverageClose(announcedClose.tokenId);
    }

Things to NOTE here:

1 - Most important is that the tokenId is burned meaning this position no longer has an owner 2 - The whole position is deleted

vault.deletePosition(announcedClose.tokenId);

Now did you forget that we still have an adjusting order waiting to be executed? Let's get back to it.

Assume that a keeper has called executeOrder and we go straight into here:

function _executeLeverageAdjust(address account) internal {

        FlatcoinStructs.Order memory order = _announcedOrder[account];

        FlatcoinStructs.AnnouncedLeverageAdjust memory leverageAdjust = abi.decode(
            order.orderData,
            (FlatcoinStructs.AnnouncedLeverageAdjust)
        );

        _prepareExecutionOrder(account, order.executableAtTime);

        ILeverageModule(vault.moduleAddress(FlatcoinModuleKeys._LEVERAGE_MODULE_KEY)).executeAdjust({
            account: account,
            keeper: msg.sender,
            order: order
        });

        if (leverageAdjust.marginAdjustment > 0) {
            // Sending positive margin adjustment and fees from delayed order contract to the vault
            vault.collateral().safeTransfer({
                to: address(vault),
                value: uint256(leverageAdjust.marginAdjustment) + leverageAdjust.tradeFee + order.keeperFee
            });
        }

        emit FlatcoinEvents.OrderExecuted({account: account, orderType: order.orderType, keeperFee: order.keeperFee});
    }

Now as you can see here the first line returns a struct of our adjustment, next it prepares it for execution by checking the min executable time has passed and we go straight to executeAdjust:

function executeAdjust(
        address _account,
        address _keeper,
        FlatcoinStructs.Order calldata _order
    ) external whenNotPaused onlyAuthorizedModule {

        uint32 maxAge = _getMaxAge(_order.executableAtTime);

 @---->       //@note position will only work with the values announced in the order
        FlatcoinStructs.AnnouncedLeverageAdjust memory announcedAdjust = abi.decode(
            _order.orderData,
            (FlatcoinStructs.AnnouncedLeverageAdjust)
        );

        (uint256 adjustPrice, ) = IOracleModule(vault.moduleAddress(FlatcoinModuleKeys._ORACLE_MODULE_KEY)).getPrice({
            maxAge: maxAge
        });

        if (announcedAdjust.additionalSizeAdjustment >= 0) {

            // Given that the size of a position is being increased, it's necessary to check that
            // it doesn't exceed the max skew limit.
            vault.checkSkewMax(uint256(announcedAdjust.additionalSizeAdjustment));

            if (adjustPrice > announcedAdjust.fillPrice)
                revert FlatcoinErrors.HighSlippage(adjustPrice, announcedAdjust.fillPrice);

        } else {
            if (adjustPrice < announcedAdjust.fillPrice)
                revert FlatcoinErrors.HighSlippage(adjustPrice, announcedAdjust.fillPrice);
        }

@---->    //@note everything using position.(some value) will return 0 since the position has already been fully wiped out
        FlatcoinStructs.Position memory position = vault.getPosition(announcedAdjust.tokenId);

        // Fees come out from the margin if the margin is being reduced or remains unchanged (meaning the size is being modified).
        int256 marginAdjustment = (announcedAdjust.marginAdjustment > 0)
            ? announcedAdjust.marginAdjustment
            : announcedAdjust.marginAdjustment - int256(announcedAdjust.totalFee);

        vault.updateGlobalPositionData({
            price: adjustPrice,
            marginDelta: marginAdjustment,
            additionalSizeDelta: announcedAdjust.additionalSizeAdjustment
        });

        int256 cumulativeFunding = vault.cumulativeFundingRate();

        // This accounts for the profit loss and funding fees accrued till now.

@----> // examples of dummy values 
        // 2e18 + 0 = 2e18
        uint256 newMargin = (marginAdjustment +
            PerpMath
                ._getPositionSummary({position: position, nextFundingEntry: cumulativeFunding, price: adjustPrice})
                .marginAfterSettlement).toUint256();

@----> //examples of dummy values
        // 0 + 3e18 = 3e18
        uint256 newAdditionalSize = (int256(position.additionalSize) + announcedAdjust.additionalSizeAdjustment)
            .toUint256();

        // Check that the new position isn't immediately liquidatable.
        if (
            newMargin <=
            ILiquidationModule(vault.moduleAddress(FlatcoinModuleKeys._LIQUIDATION_MODULE_KEY)).getLiquidationMargin(
                newAdditionalSize,
                adjustPrice
            )
        ) revert FlatcoinErrors.PositionCreatesBadDebt();

        // Check that the leverage isn't too high.
        checkLeverageCriteria(newMargin, newAdditionalSize);

        vault.setPosition(
            FlatcoinStructs.Position({
                lastPrice: adjustPrice,
                marginDeposited: newMargin,
                additionalSize: newAdditionalSize,
                entryCumulativeFunding: cumulativeFunding
            }),
            announcedAdjust.tokenId
        );

        // Unlock the position token to allow for transfers.
        _unlock(announcedAdjust.tokenId);

        // Mint points.
        if (announcedAdjust.additionalSizeAdjustment > 0) {

            address positionOwner = ownerOf(announcedAdjust.tokenId);
            IPointsModule pointsModule = IPointsModule(vault.moduleAddress(FlatcoinModuleKeys._POINTS_MODULE_KEY));

            pointsModule.mintLeverageOpen(positionOwner, uint256(announcedAdjust.additionalSizeAdjustment));
        }

        if (announcedAdjust.tradeFee > 0) vault.updateStableCollateralTotal(int256(announcedAdjust.tradeFee));

        // Sending keeper fee from order contract to the executor.
        vault.sendCollateral({to: _keeper, amount: _order.keeperFee});

        if (announcedAdjust.marginAdjustment < 0) {
            // We send the user that much margin they requested during announceLeverageAdjust().
            // However their remaining margin is reduced by the fees.
            // It is accounted in announceLeverageAdjust().
            uint256 marginToWithdraw = uint256(announcedAdjust.marginAdjustment * -1);

            // Withdrawing margin from the vault and sending it to the user.
            vault.sendCollateral({to: _account, amount: marginToWithdraw});
        }

        emit FlatcoinEvents.LeverageAdjust(announcedAdjust.tokenId);
    }

What we are doing here is that basically we can use the adjust to simply create a "new" order but this time without an actual somebody that owns that tokenId(position).

Impact

Now since we have an outgoing leverage position BUT WITHOUT ANYONE OWNING THE TOKENID, this position can't be closed and can't be liquidated, here is why:

Let's say we want to close the order, that's possible only if the caller of the announceLeverageClose is the owner of the tokenID

if (leverageModule.ownerOf(tokenId) != msg.sender) revert FlatcoinErrors.NotTokenOwner(tokenId, msg.sender);

So this way of getting rid of that position won't work.

Okay so lets try to liquidate it, it won't require any ownership things right?

Well lets look at the liquidation process:

/// @notice Function to liquidate a position.
    /// @dev One could directly call this method instead of `liquidate(uint256, bytes[])` if they don't want to update the Pyth price.
    /// @param tokenId The token ID of the leverage position.
    function liquidate(uint256 tokenId) public nonReentrant whenNotPaused liquidationInvariantChecks(vault, tokenId) {

        FlatcoinStructs.Position memory position = vault.getPosition(tokenId);

        (uint256 currentPrice, ) = IOracleModule(vault.moduleAddress(FlatcoinModuleKeys._ORACLE_MODULE_KEY)).getPrice();

        // Settle funding fees accrued till now.
        vault.settleFundingFees();

        // Check if the position can indeed be liquidated.
        if (!canLiquidate(tokenId)) revert FlatcoinErrors.CannotLiquidate(tokenId);

        FlatcoinStructs.PositionSummary memory positionSummary = PerpMath._getPositionSummary(
            position,
            vault.cumulativeFundingRate(),
            currentPrice
        );

        // Check that the total margin deposited by the long traders is not -ve.
        // To get this amount, we will have to account for the PnL and funding fees accrued.
        int256 settledMargin = positionSummary.marginAfterSettlement;

        uint256 liquidatorFee;

        // If the settled margin is greater than 0, send a portion (or all) of the margin to the liquidator and LPs.
        if (settledMargin > 0) {

            // Calculate the liquidation fees to be sent to the caller.
            uint256 expectedLiquidationFee = PerpMath._liquidationFee(
                position.additionalSize,
                liquidationFeeRatio,
                liquidationFeeLowerBound,
                liquidationFeeUpperBound,
                currentPrice
            );

            uint256 remainingMargin;

            // Calculate the remaining margin after accounting for liquidation fees.
            // If the settled margin is less than the liquidation fee, then the liquidator fee is the settled margin.
            if (uint256(settledMargin) > expectedLiquidationFee) {
                liquidatorFee = expectedLiquidationFee;
                remainingMargin = uint256(settledMargin) - expectedLiquidationFee;
            } else {
                liquidatorFee = uint256(settledMargin);
            }

            // Adjust the stable collateral total to account for user's remaining margin.
            // If the remaining margin is greater than 0, this goes to the LPs.
            // Note that {`remainingMargin` - `profitLoss`} is the same as {`marginDeposited` + `accruedFunding`}.
            vault.updateStableCollateralTotal(int256(remainingMargin) - positionSummary.profitLoss);

            // Send the liquidator fee to the caller of the function.
            // If the liquidation fee is greater than the remaining margin, then send the remaining margin.
            vault.sendCollateral(msg.sender, liquidatorFee);
        } else {
            // If the settled margin is -ve then the LPs have to bear the cost.
            // Adjust the stable collateral total to account for user's profit/loss and the negative margin.
            // Note: We are adding `settledMargin` and `profitLoss` instead of subtracting because of their sign (which will be -ve).
            vault.updateStableCollateralTotal(settledMargin - positionSummary.profitLoss);
        }

        // Update the global position data.
        // Note that we are only accounting for `globalMarginDelta`, `marginDeposited` and `userAccruedFunding`.
        // and not the PnL of the user when altering `marginDepositedTotal`.
        // This is because the PnL is already accounted for in the `stableCollateralTotal`.
        // So when the PnL is +ve (the trader made profits), the trader takes the profit along with the margin deposited.
        // When the PnL is -ve, the trader loses a portion of the margin deposited to the LPs and the rest is again taken along.
        // In neither case, the PnL is added/subtracted to/from the `marginDepositedTotal`.
        // Now we are subtracting `userAccruedFunding` in the below function call because:
        //      `globalMarginDelta` = `userAccruedFunding` + Funding accrued by the rest of the long traders.
        // And this accrued funding is being taken away from the system (if +ve) or given to LPs (if -ve).
        // When the `userAccruedFunding` is +ve, the user takes away the funding fees earned.
        // When it's negative, the user pays the funding fees to the LPs and their margin is reduced.
        // So the `marginDepositedTotal` is added with `userAccruedFunding` in the below function call as the user has paid for their share
        // of funding fees.
        vault.updateGlobalPositionData({
            price: position.lastPrice,
            marginDelta: -(int256(position.marginDeposited) + positionSummary.accruedFunding),
            additionalSizeDelta: -int256(position.additionalSize) // Since position is being closed, additionalSizeDelta should be negative.
        });

        // Delete position storage
        vault.deletePosition(tokenId);

        // Cancel any limit orders associated with the position
        ILimitOrder(vault.moduleAddress(FlatcoinModuleKeys._LIMIT_ORDER_KEY)).cancelExistingLimitOrder(tokenId);

        // If the position token is locked because of an announced order, it should still be liquidatable
        ILeverageModule leverageModule = ILeverageModule(vault.moduleAddress(FlatcoinModuleKeys._LEVERAGE_MODULE_KEY));
        leverageModule.unlock(tokenId);
@---->        //@audit This will revert since this tokenId has already been burned. 
        leverageModule.burn(tokenId);

        emit FlatcoinEvents.PositionLiquidated(tokenId, msg.sender, liquidatorFee);
    }

Won't go through the whole function, just look at the last lines, there is leverageModule.burn(tokenId); which will revert since this tokenId has already been burned when we closed the position. So liquidations are impossible too.

Code Snippet

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/DelayedOrder.sol#L217-L311

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/LeverageModule.sol#L147-L248

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/LimitOrder.sol#L58-L84

Tool used

Manual Review

Recommendation

In the execution process of adjusting a position, you can add a check to ensure the owner of a tokenId != address(0)

Duplicate of #227

sherlock-admin commented 8 months ago

1 comment(s) were left on this issue during the judging contest.

takarez commented:

valid: user can create a position that nobody can liquidate; high(10)