LTDingZhen - Malicious User can create a position that nobody can liquidate

[sherlock-admin]

LTDingZhen

high

Malicious User can create a position that nobody can liquidate

Summary

In function executeClose, after burning an NFT, it will only detect and clear existing limit orders, but not detect existing delayed orders. This allows an attacker to add margin and increase position size to a tokenID where no NFT exists to create a position that cannot be liquidated. The platform risks having bad debt that cannot be eliminated.

Vulnerability Detail

In flatcoin protocol, user can close a position by calling announceLimitOrder or announceLeverageClose. When keepers try to execute the limit order or the delayed order, function executeClose in LeverageModule is called. In executeClose, there is a check for cancellation of any existing limit orders on the position, but does not cancel an existing Delayed order if executeClose is called by executeLimitOrder()!

function executeClose(
    address _account,
    address _keeper,
    FlatcoinStructs.Order calldata _order
) external whenNotPaused onlyAuthorizedModule returns (int256 settledMargin) {
        ...
        // Delete position storage
        vault.deletePosition(announcedClose.tokenId);
    }

    // Cancel any existing limit order on the position
    ILimitOrder(vault.moduleAddress(FlatcoinModuleKeys._LIMIT_ORDER_KEY)).cancelExistingLimitOrder(
        announcedClose.tokenId
    );

    // A position NFT has to be unlocked before burning otherwise, the transfer to address(0) will fail.
    _unlock(announcedClose.tokenId);
    _burn(announcedClose.tokenId);

    vault.updateStableCollateralTotal(int256(announcedClose.tradeFee)); // pay the trade fee to stable LPs

    // Settle the collateral.
    vault.sendCollateral({to: _keeper, amount: _order.keeperFee}); // pay the keeper their fee
    vault.sendCollateral({to: _account, amount: uint256(settledMargin) - totalFee}); // transfer remaining amount to the trader

    emit FlatcoinEvents.LeverageClose(announcedClose.tokenId);
}

When performing leverage adjustments, since the validation of the caller's possession of the NFT is performed at announceLeverageAdjust, executeAdjust does not have a test for the existence of a current position, and allows a position with all parameters 0 to operate normally.

Consider path below:

1. Bob has a position and calls announceLimitOrder to create a limit order that can be executed immediately.
2. Bob calls announceLeverageAdjust to create a delayed order to add some margin and increase position size. Since the limit order has not been executed at this time, all validations in announceLeverageAdjust will pass.
3. a keeper execute the limit order to close Bob's position, and clear its storage. At this point, since only the presence of a limit order is checked, the delayed order is still valid.
4. a keeper execute the delayed order, "create" a new position on the same tokenID. Because the NFT has been destroyed, its owner is address 0. Nobody can liquidate or close this position because OZ's ERC721 doesn't allow burning non-existent token.

Impact

Attackers can create positions which nobody can liquidate, so there is a risk that the platform will incur bad debts that cannot be eliminated.

Code Snippet

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/LeverageModule.sol#L255-L317 https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/LeverageModule.sol#L147-L248

Tool used

Manual Review

Recommendation

In function executeClose, only cancel any existing limit order on the position is not enough. delayed order should also be canceled.

[sherlock-admin]

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

takarez commented:

valid: high(10)

[rashtrakoff]

I think the better solution would be to check if a position exists by either checking the owner of the position (should not be the null address) or the size of the position (should be strictly greater than 0).

[nevillehuang]

Escalate

@rashtrakoff I believe this is possibly misjudged by me as valid based on the PoC provided by a watson mentioned here

[sherlock-admin2]

Escalate

@rashtrakoff I believe this is possibly misjudged by me as valid based on the PoC provided by a watson mentioned here

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[0xjuaan]

POC here, was placed in Liquidate.t.sol

function test_cantLiquidatePosition() public {
        setWethPrice(1000e8);

        uint256 tokenId = announceAndExecuteDepositAndLeverageOpen({
            traderAccount: alice,
            keeperAccount: keeper,
            depositAmount: 100e18,
            margin: 10e18,
            additionalSize: 30e18,
            oraclePrice: 1000e8,
            keeperFeeAmount: 0
        });

        vm.prank(alice);
        limitOrderProxy.announceLimitOrder(tokenId, 750e8, 1250e8);

        // user announces order
        announceAdjustLeverage(alice, tokenId, 3e18, 3e18, 0); //e Alice adds 3e18 margin, 1e18 size

        skip(uint256(vaultProxy.minExecutabilityAge())); // must reach minimum executability time

        bytes[] memory priceUpdateData = getPriceUpdateData(1000e8);
        limitOrderProxy.executeLimitOrder{value: 1}(tokenId, priceUpdateData);

        // This reverts, due to [ERC721: invalid token ID]
        executeAdjustLeverage(keeper, alice, 1000e8);
    }

[securitygrid]

Escalate This issue is not valid beacause the attack described in the report would not be successful. The attack scenario described in the report would not be successful. There are two situations for an adjustment order:

1. If announcedAdjust.additionalSizeAdjustment is greater than 0, tx will revert here.
2. If announcedAdjust.additionalSizeAdjustment is equal to 0, it means that the position only has the margin provided by the attacker and does not have additionalSize. This will not lead to bad debts. Moreover, it is self-harm for the attacker.

[sherlock-admin2]

Escalate This issue is not valid beacause the attack described in the report would not be successful. The attack scenario described in the report would not be successful. There are two situations for an adjustment order:

1. If announcedAdjust.additionalSizeAdjustment is greater than 0, tx will revert here.
2. If announcedAdjust.additionalSizeAdjustment is equal to 0, it means that the position only has the margin provided by the attacker and does not have additionalSize. This will not lead to bad debts. Moreover, it is self-harm for the attacker.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[RealLTDingZhen]

Good catch. This issue is invalid. Even if announcedAdjust.additionalSizeAdjustment = 0 could pass the check, checkLeverageCriteria(newMargin, newAdditionalSize) would fail. So the path is not possible.

[MAKEOUTHILL6]

I agree this is invalid.

[Czar102]

Planning to accept the escalation and invalidate the issue.

[Czar102]

Result: Invalid Has duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• nevillehuang: accepted
• securitygrid: rejected

[securitygrid]

@Czar102 why was my escalation rejected?

[Czar102]

@securitygrid This is because the first escalation meant the same outcome (even though didn't state it explicitly), and only one escalation can be accepted on an issue not to punish the Lead Judge twice for the same mistake, so I accepted the first one.

[securitygrid]

@Czar102 Doesn't this affect my escalation ratio?

[sherlock-admin4]

The protocol team fixed this issue in PR/commit https://github.com/dhedge/flatcoin-v1/pull/295.