c4-bot-7 commented 2 months ago

Lines of code

https://github.com/code-423n4/2024-07-dittoeth/blob/ca3c5bf8e13d0df6a2c1f8a9c66ad95bbad35bce/contracts/facets/DisputeRedemptionFacet.sol#L93-L101

Vulnerability details

Description

When a proposal is disputed, the Short Records (SRs) from the disputed proposal can each be merged with their former SR or they get added to the TAPP.

They will get merged with their former SR in the else block below if the SR is not closed.

DisputeRedemptionFacet.sol#L83-L118

088:                     if (currentSR.status == SR.Closed) {
089:                         // @dev If the protocol created a new SR with the proposed amount, it would have been at or near 1:1 (1x CR), thus making it instantly liquidatable
090:                         // @dev Additionally, the shorter would not lose anything in losing the proposed collateral since they also no longer owe the debt
091:                         // @dev Thus more streamlined to give back the proposed amounts to the TAPP, and safer to consolidate risky debt into one position
092:                         STypes.ShortRecord storage tappSR = s.shortRecords[d.asset][address(this)][C.SHORT_STARTING_ID];
093:                         LibShortRecord.merge({
094:                             short: tappSR,
095:                             ercDebt: currentProposal.ercDebtRedeemed,
096:                             ercDebtSocialized: currentProposal.ercDebtRedeemed.mul(d.ercDebtRate),
097:                             collateral: currentProposal.colRedeemed,
098:                             yield: currentProposal.colRedeemed.mul(d.dethYieldRate),
099:                             creationTime: d.protocolTime,
100:                             ercDebtFee: currentProposal.ercDebtFee
101:                         });
102:                     } else {
103:                         // @dev Returns the proposed collateral and debt to the SR
104:                         // @dev Also valid in the case where a proposed SR is closed and re-used prior to disputing
105:                         LibShortRecord.merge({
106:                             short: currentSR,
107:                             ercDebt: currentProposal.ercDebtRedeemed,
108:                             ercDebtSocialized: currentProposal.ercDebtRedeemed.mul(d.ercDebtRate),
109:                             collateral: currentProposal.colRedeemed,
110:                             yield: currentProposal.colRedeemed.mul(d.dethYieldRate),
111:                             creationTime: d.protocolTime,
112:                             ercDebtFee: currentProposal.ercDebtFee
113:                         });
114:                     }

SRs can be reused and if the disputed SR is merged with a reused SR it may produce a liquidatable SR. Consider the following scenario:

A filled SR with a debt and collateral value of 5000 USD and 8500 USD (CR = 1.7) respectively has 3000 USD proposed for redemption. (Max Redemption Collateral Ratio = 2.0)
After it is proposed the owner exits the SR and creates a new short order with 2000 USD debt and CR of 2.0 which reuses the exited SR. 500 USD of the short order gets filled and it has 1500 USD (1000 USD from short order + 500 USD from bid) worth of collateral.
The proposal gets disputed and is merged with the reused SR making it have a debt of 3500 USD and collateral value of 4500 USD (1500 USD from SR + 3000 USD from disputed SR). The position now has a CR of 1.28 making the position liquidatable. (Liquidation CR = 1.5)

Impact

Proposal disputes can create liquidatable positions.

Proof of Concept

The test can be run in Redemption.t.sol file.

    function test_DisputeAndCreateBadSR() public {
        // add limit bids to fill future shorts
        fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, receiver);
        fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, receiver);

        // add short to be used in dispute
        fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, sender);
        vm.prank(sender);
        // decrease collateral so dispute SR has less CR than proposed SR
        decreaseCollateral(C.SHORT_STARTING_ID, 1 ether); 
        // skip time so dispute SR has enought time to be used for dispute
        skip(70 minutes);

        // create the SR to be used for proposal
        fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, sender);
        // prepare proposal input
        MTypes.ProposalInput[] memory proposalInputs = new MTypes.ProposalInput[](1);
        proposalInputs[0] = MTypes.ProposalInput({shorter: sender, shortId: C.SHORT_STARTING_ID+1, shortOrderId: 1});

        address redeemer = receiver; 
        uint88 remainingDebt = 2000 ether;
        uint88 redeemAmount = DEFAULT_AMOUNT-2000 ether;
        depositUsd(receiver, redeemAmount); // send DUSD to redeemer for redemption DUSD

        // reduce price of asset to make the short record redeemable
        _setETH(1000 ether);
        proposeRedemption(proposalInputs, redeemAmount, redeemer);

        // exit the proposed SR
        depositUsd(sender, remainingDebt);
        exitShortErcEscrowed(C.SHORT_STARTING_ID + 1, remainingDebt, sender);

        // reuse the proposed SR by creating a new short order
        fundLimitBidOpt(0.001 ether, 1000 ether, receiver);
        fundLimitShortOpt(0.001 ether, 2000 ether, sender);

        address disputer = extra;
        vm.prank(disputer);
        // dispute the redemption to merge the proposed SR and the reused SR
        diamond.disputeRedemption({
            asset: asset,
            redeemer: redeemer,
            incorrectIndex: 0,
            disputeShorter: sender,
            disputeShortId: C.SHORT_STARTING_ID
        });

        // resulting SR is liquidatable
        uint liquidationCR = diamond.getAssetStruct(asset).liquidationCR;
        assertLt(diamond.getCollateralRatio(asset, getShortRecord(sender, 2)), liquidationCR * 10**18);
    }

Tools Used

Manual Analysis

Recommended Mitigation Steps

Consider simulating the merge and adding a collateral ratio check on that simulation before merging with the old SR. If the collateral ratio is not high enough, merge with TAPP.

Assessed type

Other

c4-judge commented 2 months ago

hansfriese marked the issue as primary issue

ditto-eth commented 2 months ago

The impact seems negligible because a healthy liquidation is fine for the system. The resulting SR would have CR > 1 because the returning ercDebt/collateral is equal to 1 and gets averaged with a CR > 1. Also not planning to fix because this situation only happens from user error:

users can and should check any possible disputes and call dispute themselves and wait for resolution. if the shorter did that in this scenario would not be possible
The shorter is for some reason exiting and then re-creating a new SR during the time that their old SR was part of a dispute period

hansfriese commented 2 months ago

Agree with the sponsor. QA at best as it pertains to the user's abnormal behavior.