search

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

0 stars 0 forks source link

No expiration deadline leads to legitimate redemption proposals being unexpectedly disputed #264

Closed c4-bot-7 closed 3 months ago

c4-bot-7 commented 3 months ago

Lines of code

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

Vulnerability details

Introduction

The redemption mechanism enables a redeemer to redeem their ercEscrowed for ethCollateral on target shortRecords. Ideally, the redeemer must submit a redemption proposal containing shortRecords with the lowest collateral ratio (CR), as redeeming shortRecords with high CRs can eventually lead to stability issues of the dUSD stable asset to depeg easier.

To protect shortRecords with high CRs from being redeemed, the redemption mechanism provides a dispute mechanism by way of the RedemptionFacet::disputeRedemption(), where a disputer can provide a single shortRecord with a CR lower than a shortRecord in the target proposal. If valid, all invalid shortRecords (with higher CRs) in the proposal are removed, the proposer (redeemer) gets a penalty, and the disputer is awarded based on the difference in incorrect debt (the proposer's applied penalty).

Impact

The disputeRedemption() applies the C.DISPUTE_REDEMPTION_BUFFER constant of 3600 seconds (i.e., 1 hour) to mitigate the case that the proposal takes time to transact and prevent front-running attacks.

However, the 1-hour time buffer may not be sufficient to protect the legitimate proposal from being disputed in case the proposal is pending in the mempool for an extended period due to network congestion, etc. Furthermore, increasing the C.DISPUTE_REDEMPTION_BUFFER to be longer is also ineffective.

The root cause of this issue is the lack of an expiration deadline for the redemption proposal. Without a deadline, a proposer (redeemer) cannot designate how long a proposal can be active. Hence, the proposal can be pending in the mempool indefinitely, and it will always be valid to be processed by the RedemptionFacet::proposeRedemption() once the transaction is executed.

Furthermore, a malicious miner/validator can hold the transaction until they favor it or they can make a profit (e.g., a malicious validator, in turn, becomes a disputer).

For the solution, please refer to the Recommended Mitigation Steps section below.

Proof of Concept

In the disputeRedemption(), the C.DISPUTE_REDEMPTION_BUFFER constant of 3600 seconds (1 hour) is applied to mitigate when a proposal takes time to transact and to prevent front-running attacks.

Nevertheless, it may not be sufficient to protect the legitimate proposal from being disputed if it has been pending in the mempool for an extended period due to network congestion, etc.

Therefore, adding a proposer-controllable expiration deadline for the redemption proposal is the solution.

    function disputeRedemption(address asset, address redeemer, uint8 incorrectIndex, address disputeShorter, uint8 disputeShortId)
        external
        isNotFrozen(asset)
        nonReentrant
    {
        ...

        //@audit -- The C.DISPUTE_REDEMPTION_BUFFER (3600 seconds) is applied in case the proposal takes time to transact 
        //          and to prevent front-running attacks, but it may not be sufficient to protect the legitimate proposal from being disputed
        //          in case the proposal is pending in the mempool for an extended period due to network congestion, etc.
@>      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

            ...

        }

        ...
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Add a proposer-controllable expiration deadline for the redemption proposal, enabling a proposer (redeemer) to designate a lifetime of their transaction.

    function proposeRedemption(
        address asset,
        MTypes.ProposalInput[] calldata proposalInput,
        uint88 redemptionAmount,
-       uint88 maxRedemptionFee
+       uint88 maxRedemptionFee,
+       uint256 deadline
    ) external isNotFrozen(asset) nonReentrant {

+       if (block.timestamp > deadline) {
+           revert Errors.ProposalExpired(deadline);
+       }

        ...
    }

Assessed type

Other

c4-pre-sort commented 3 months ago

raymondfam marked the issue as insufficient quality report

c4-pre-sort commented 3 months ago

raymondfam marked the issue as primary issue

raymondfam commented 3 months ago

That's the intended design. For as long as disputeCR < incorrectProposal.CR is false, the second condition disputeSR.updatedAt + C.DISPUTE_REDEMPTION_BUFFER <= redeemerAssetUser.timeProposed will be inconsequential.

c4-judge commented 3 months ago

hansfriese marked the issue as unsatisfactory: Invalid

serial-coder commented 3 months ago

Hi @hansfriese,

Appeal

Point 1

I don't understand why the judge marked this issue invalid. Could you please have a second look?

Point 2

As per the lookout's comment:

That's the intended design. For as long as disputeCR < incorrectProposal.CR is false, the second condition disputeSR.updatedAt + C.DISPUTE_REDEMPTION_BUFFER <= redeemerAssetUser.timeProposed will be inconsequential.

You misunderstood and were missing the point of the issue.

I didn't raise this issue because the if statement, if (disputeCR < incorrectProposal.CR && disputeSR.updatedAt + C.DISPUTE_REDEMPTION_BUFFER <= redeemerAssetUser.timeProposed) { ... }, in the disputeRedemption() is incorrect or misdesigned. In other words, the if statement in question is correct and was designed properly.

My intention in mentioning the if statement was to point out that the C.DISPUTE_REDEMPTION_BUFFER constant of the 1-hour time buffer is not sufficient to address legitimate redemption proposals from being disputed if the proposals were pending in the mempool for an extended period due to network congestion, etc.

Please have a second look at the following from the Impact section:

The disputeRedemption() applies the C.DISPUTE_REDEMPTION_BUFFER constant of 3600 seconds (i.e., 1 hour) to mitigate the case that the proposal takes time to transact and prevent front-running attacks.

However, the 1-hour time buffer may not be sufficient to protect the legitimate proposal from being disputed in case the proposal is pending in the mempool for an extended period due to network congestion, etc. Furthermore, increasing the C.DISPUTE_REDEMPTION_BUFFER to be longer is also ineffective.

The root cause of this issue is the lack of an expiration deadline for the redemption proposal. Without a deadline, a proposer (redeemer) cannot designate how long a proposal can be active. Hence, the proposal can be pending in the mempool indefinitely, and it will always be valid to be processed by the RedemptionFacet::proposeRedemption() once the transaction is executed.

Furthermore, a malicious miner/validator can hold the transaction until they favor it or they can make a profit (e.g., a malicious validator, in turn, becomes a disputer).

For instance, if the proposals were pending in the mempool for 1.5 hours due to network congestion, the proposals can be disputed unexpectedly. From the proposers' point of view, they didn't expect that their proposals would be pending in the mempool for that long period. This leads to unexpected disputes on legitimate redemption proposals, which can be addressed/mitigated if the proposers can set an expiration deadline for their proposals while submitting transactions.

The snippet below presents the recommended code that enables a proposer to designate a lifetime of their proposal, the same idea as in the permit() of OZ's ERC20Permit contract. This way, a proposal transaction will revert if the deadline set by a proposer has been reached.

    function proposeRedemption(
        address asset,
        MTypes.ProposalInput[] calldata proposalInput,
        uint88 redemptionAmount,
-       uint88 maxRedemptionFee
+       uint88 maxRedemptionFee,
+       uint256 deadline
    ) external isNotFrozen(asset) nonReentrant {

+       if (block.timestamp > deadline) {
+           revert Errors.ProposalExpired(deadline);
+       }

        ...
    }
hansfriese commented 3 months ago

I agree it says a valid concern but I think QA is more appropriate due to the below requirements.

c4-judge commented 3 months ago

hansfriese removed the grade

c4-judge commented 3 months ago

hansfriese changed the severity to QA (Quality Assurance)

c4-judge commented 3 months ago

hansfriese marked the issue as grade-b