search

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

0 stars 0 forks source link

A user can claim all his collateral before redemption time #3

Closed c4-bot-5 closed 3 months ago

c4-bot-5 commented 3 months ago

Lines of code

https://github.com/code-423n4/2024-07-dittoeth/blob/ca3c5bf8e13d0df6a2c1f8a9c66ad95bbad35bce/contracts/facets/ClaimRedemptionFacet.sol#L80-L87

Vulnerability details

Summary

A malicious user can set up a proposal on his Short Record and decide not to claim his collateral or redeemed asset when the dispute time ends. If that Short Record is proposed again he can claim the collateral before dispute time is over for the new proposal.

Description

After a redemption is proposed, a dispute time has to pass before the redeemer can call claimRedemption() to collect his asset and the shorter's remaining collateral is distributed to him. If the redeemer does not collect his asset, the shorter can call claimRemainingCollateral() before the time expires to claim his remaining collateral.

claimRemainingCollateral() calls _claimRemainingCollateral() and the call to _claimRemainingCollateral() allows the user to claim all the collateral in the Short Record if the if condition below is satisfied.

ClaimRedemptionFacet.sol#L80-L87

❌      if (shortRecord.ercDebt == 0 && shortRecord.status == SR.FullyFilled) {
            // @dev Refund shorter the remaining collateral only if fully redeemed and not claimed already
c          uint88 collateral = shortRecord.collateral;
❌          s.vaultUser[vault][shorter].ethEscrowed += collateral;
            // @dev Shorter shouldn't lose any unclaimed yield because dispute time > YIELD_DELAY_SECONDS
            LibSRUtil.disburseCollateral(asset, shorter, collateral, shortRecord.dethYieldRate, shortRecord.updatedAt);
            LibShortRecord.deleteShortRecord(asset, shorter, shortId);
        }

The issue is, if there are other pending redemption proposals for that same Short Record, the shorter can claim all the collateral in the Short Record before the dispute times of those proposals expire. Thus, if one of the proposals gets disputed it gets sent to the (Treasury Asset Protection Pool) TAPP Short Record. This may make the TAPP liquidatable if it does not have enough collateral to cover the new debt.

All the collateral may get claimed in any of these scenarios:

  1. A malicious user creates a (Short Record) SR, proposes a redemption on a portion of the debt and never claims it. If the full SR gets proposed or it is reused and gets proposed again, he can claim all the collateral before the dispute time expires.
  2. The redeemer calls claimRedemption() to claim his asset. If any of the Short Records he proposed has another ongoing proposal but satisfies the if condition in _claimRemainingCollateral() the shorter gets all the collateral.

Impact

This affects the protocol in two ways:

  1. A user can claim his collateral before the dispute time of a redemption proposal expires.
  2. It will increase the debt in TAPP Short Record if the proposal is disputed.

Proof of Concept

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

    // import {STypes, MTypes, O, SR} from "contracts/libraries/DataTypes.sol";
    function test_claimBeforeRedemptionTime() public {
        // create a short record by matching a bid order with a short order
        fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, receiver);
        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, shortOrderId: 0});

        address redeemer = receiver;
        checkEscrowed({redeemer: redeemer, ercEscrowed: DEFAULT_AMOUNT});   

        uint88 redeemAmount = DEFAULT_AMOUNT/2;
        // reduce price of asset to make the short record redeemable
        _setETH(900 ether);
        proposeRedemption(proposalInputs, redeemAmount, redeemer);

        (, uint32 timeToDispute,,, ) = getSlate(redeemer);
        skip(timeToDispute); // skip time so that first proposal becomes redeemable

        redeemer = extra;
        depositUsd(redeemer, redeemAmount);
        // propose redemption again to propose the full debt
        proposeRedemption(proposalInputs, redeemAmount, redeemer);        

        (, timeToDispute,,, ) = getSlate(redeemer);
        assertTrue(timeToDispute > 0);
        uint remainingCollateral = diamond.getShortRecord(asset, sender, C.SHORT_STARTING_ID).collateral;
        vm.prank(sender);
        // sender claims collateral before the dipsute time of the second proposal expires
        diamond.claimRemainingCollateral(asset, receiver, 0, C.SHORT_STARTING_ID);        
        assertEq(diamond.getVaultUserStruct(vault, sender).ethEscrowed, remainingCollateral);
    }

Tools Used

Manual Analysis

Recommended Mitigation Steps

Consider allowing users to claim collateral from their Short Record only when there are no pending proposals under dispute time on that Short Record.

Assessed type

Other

c4-judge commented 3 months ago

hansfriese marked the issue as primary issue

ditto-eth commented 3 months ago

there is already a mitigation in place for this unlikely scenario: https://github.com/code-423n4/2024-07-dittoeth/blob/ca3c5bf8e13d0df6a2c1f8a9c66ad95bbad35bce/contracts/facets/DisputeRedemptionFacet.sol#L86-L88

nonseodion commented 3 months ago

@ditto-eth the mitigation transfers debt to the TAPP Short Record, but transferring debt to the TAPP will also require some extra collateral of the TAPP to be used in covering that debt. Thus reducing the CR of the TAPP. This may allow the TAPP to get liquidated. Thus this should be done if completely necessary and avoided if it can be.

When Short Records have their ercDebt == 0 they can't be exited because CR cannot be calculated in exitShort() (division by zero). If the Short Record is partially filled it can still be exited if it gets filled during the proposal. This somewhat unlikely scenario of SRs being exited reduces the frequency of sending debt to TAPP. But with the issue above, a malicious user can ensure debt is sent to TAPP.

ditto-eth commented 3 months ago

the point of the TAPP SR is to be liquidated so that risky debt can get cleared from the system. the TAPP SR simply consolidates risky debt so that it's easier to identify and liquidate more of the risky debt in the system.

disputeRedemption() also enforces fees in a way where an attacker cannot benefit from an exploit

nonseodion commented 3 months ago

If risky debt can be avoided it should be.

The issue does not occur only maliciously and does not require the attacker to make a proposal.

hansfriese commented 3 months ago

Intended behavior.

c4-judge commented 3 months ago

hansfriese marked the issue as unsatisfactory: Invalid