When a closed ShortRecord is canceled redemption at disputeRedemption, the user loses the collateral

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/RedemptionFacet.sol#L264-L268

Vulnerability details

Impact

The user loses collateral.

Proof of Concept

If the Redemption requested ShortRecord is closed for various reasons and then canceled redemption by disputeRedemption, the user cannot get the collateral back because the ShortRecord has already been closed.

function disputeRedemption(address asset, address redeemer, uint8 incorrectIndex, address disputeShorter, uint8 disputeShortId)
    external
    isNotFrozen(asset)
    nonReentrant
{
    ...
    MTypes.ProposalData[] memory decodedProposalData =
        LibBytes.readProposalData(redeemerAssetUser.SSTORE2Pointer, redeemerAssetUser.slateLength);

    ...

    if (disputeCR < incorrectProposal.CR && disputeSR.updatedAt + C.DISPUTE_REDEMPTION_BUFFER <= redeemerAssetUser.timeProposed)
    {
        // @dev All proposals from the incorrectIndex onward will be removed
        // @dev Thus the proposer can only redeem a portion of their original slate
        for (uint256 i = incorrectIndex; i < decodedProposalData.length; i++) {
@>          currentProposal = decodedProposalData[i];

@>          STypes.ShortRecord storage currentSR = s.shortRecords[d.asset][currentProposal.shorter][currentProposal.shortId]; // The currentSR might be closed
@>          currentSR.collateral += currentProposal.colRedeemed;
@>          currentSR.ercDebt += currentProposal.ercDebtRedeemed;

            d.incorrectCollateral += currentProposal.colRedeemed;
            d.incorrectErcDebt += currentProposal.ercDebtRedeemed;
        }

        ...
    } ...
}

For example:

1. Alice ShortRecord #3 is in proposesRedemption . There is remaining ercDebt and collateral in ShortRecord #3 due to redemptionAmount.
2. Alice calls exitShortWallet . Now the ShortRecord #3 is closed.
3. Bob calls disputeRedemption, canceling Alice's ShortRecord #3 redemption. It returns ercDebt and collateral to ShortRecord #3.
4. But Alice's ShortRecord #3 is already closed. Alice can't handle the collateral or ercDebt of this short.

There are many scenarios where ShortRecord #3 could be closed unintentionally. For example, if the same ShortRecord is called proposeRedemption twice due to redemptionAmount , first claim could close ShortRecord #3. Even if a dispute occurs in the other proposal later, Alice cannot handle the returned funds. Since it's closed, it can't be requested to redeem or liquidate again.

This is PoC. Add it to the Redemption.t.sol file.

function test_PoC_AlreadyClosedSR() public {
    address shorter = sender;
    uint88 _redemptionAmounts = DEFAULT_AMOUNT + DEFAULT_AMOUNT / 2; // C.SHORT_STARTING_ID + 2 partial redeemed

    makeShorts({singleShorter: true});

    MTypes.ProposalInput[] memory proposalInputs =
        makeProposalInputsForDispute({shortId1: C.SHORT_STARTING_ID, shortId2: C.SHORT_STARTING_ID + 2});

    address redeemer = receiver;

    checkEscrowed({redeemer: redeemer, ercEscrowed: DEF_REDEMPTION_AMOUNT});

    changeUpdateAtAndSkipTime({shortId: C.SHORT_STARTING_ID + 2});

    setETHAndProposeShorts({redeemer: redeemer, proposalInputs: proposalInputs, redemptionAmount: _redemptionAmounts});

    checkRedemptionSSTORE(redeemer, proposalInputs, IS_FULL, IS_PARTIAL);

    // C.SHORT_STARTING_ID + 2 is closed
    fundLimitAskOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, receiver);
    exitShort(C.SHORT_STARTING_ID + 2, DEFAULT_AMOUNT / 2, DEFAULT_PRICE, shorter);
    assertSR(getShortRecord(sender, C.SHORT_STARTING_ID + 2).status, SR.Closed);

    uint88 ercDebtBefore = getShortRecord(sender, C.SHORT_STARTING_ID + 2).ercDebt;
    uint88 collateralBefore = getShortRecord(sender, C.SHORT_STARTING_ID + 2).collateral;

    address disputer = extra;
    vm.prank(disputer);

    diamond.disputeRedemption({
        asset: asset,
        redeemer: redeemer,
        incorrectIndex: 1,
        disputeShorter: sender,
        disputeShortId: C.SHORT_STARTING_ID + 1
    });

    uint88 ercDebtAfter = getShortRecord(sender, C.SHORT_STARTING_ID + 2).ercDebt;
    uint88 collateralAfter = getShortRecord(sender, C.SHORT_STARTING_ID + 2).collateral;

    assertSR(getShortRecord(sender, C.SHORT_STARTING_ID + 2).status, SR.Closed); // the canceled SR is closed
    assertGt(ercDebtAfter, ercDebtBefore);
    assertGt(collateralAfter, collateralBefore); // SR get collatral back, but user cannot handle this
}

Tools Used

Manual Review

Recommended Mitigation Steps

Prevent redeeming ShortRecord from being closed.

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #35

[raymondfam]

See #35.

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-judge]

hansfriese changed the severity to 3 (High Risk)