c4-bot-3 commented 2 months ago

Lines of code

https://github.com/code-423n4/2024-05-predy/blob/main/src/libraries/logic/LiquidationLogic.sol#L99

Vulnerability details

Impact

When a vault is being liquidated, the remaining margin may fail to transfer to original trader due to USDT blocklist

Bug Description

First, the contest README states that USDT is supported by the protocol. USDT has a blocklist feature where blocklisted users would not be able to receive tokens, and the transaction would revert.

When a traders opens a position, and the position becomes insolvent, liquidators may come and liquidate this position. If there is remaining margin in the vault, it would be sent to the vaule.recipient, which is the trader himself.

However, if the trader becomes blocked by USDT after he opens the position, his position would not be fully liquidatable, which means it may become bad debt for the protocol. Note that some may argue that users can just liquidate 99.9999% of the position, however, this is another design issue that incorrectly incentivizes liquidators which I created a separate issue for.

The key here is that upon liquidation finish, the remaining margin is sent back to the trader, and since it may be unsendable due to USDT blocklist, the position may be unliquidated forever.

    function liquidate(
        uint256 vaultId,
        uint256 closeRatio,
        GlobalDataLibrary.GlobalData storage globalData,
        bytes memory settlementData
    ) external returns (IPredyPool.TradeResult memory tradeResult) {
        ...
        bool hasPosition;

        (tradeResult.minMargin,, hasPosition,) =
            PositionCalculator.calculateMinMargin(pairStatus, vault, DataType.FeeAmount(0, 0));

        ...

        uint256 sentMarginAmount = 0;

        if (!hasPosition) {
            int256 remainingMargin = vault.margin;

            if (remainingMargin > 0) {
                if (vault.recipient != address(0)) {
                    // Send the remaining margin to the recipient.
                    vault.margin = 0;

                    sentMarginAmount = uint256(remainingMargin);

>                   ERC20(pairStatus.quotePool.token).safeTransfer(vault.recipient, sentMarginAmount);
                }
            } else if (remainingMargin < 0) {
                vault.margin = 0;

                // To prevent the liquidator from unfairly profiting through arbitrage trades in the AMM and passing losses onto the protocol,
                // any losses that cannot be covered by the vault must be compensated by the liquidator
                ERC20(pairStatus.quotePool.token).safeTransferFrom(msg.sender, address(this), uint256(-remainingMargin));
            }
        }
        ...
    }

Proof of Concept

N/A

Tools Used

Manual review

Recommended Mitigation Steps

Don't transfer the quoteToken back to the trader. The remaining margin can be used to supply the PairStatus quoteToken pool, and simply mint the supply tokens to the trader. The trader can withdraw the tokens himself later.

Assessed type

Token-Transfer

c4-judge commented 2 months ago

alex-ppg marked the issue as partial-75

pkqs90 commented 2 months ago

Hey @alex-ppg, I'd like to point out that in both GammaTradeMarket and PerpMarket, the owner of the vault is the market, and not the user itself. Also these markets cannot process vaults that they aren't owners of. So users can't update the vault.recipient, as stated in the issue this one is duplicated to https://github.com/code-423n4/2024-05-predy-findings/issues/42. Also, I'm curious why this issue is only duplicate-75, as it does identify the way liquidation is affected, as stated in your comment https://github.com/code-423n4/2024-05-predy-findings/issues/42#issuecomment-2197299337.

alex-ppg commented 2 months ago

Hey @pkqs90, thanks for your insightful contributions to the PJQA process! I have reviewed this submission and will reinstate a full reward. The intention of the system is to not support only those two markets, and the issue can manifest itself if the PredyPool is integrated in some other way, so I feel like the primary submission still details the recipient-related flaw adequately.