A design flaw in `RedemptionFacet::disputeRedemption()` disincentivizes good disputers from doing their job, leading to the depeg of the `dUSD` stable asset

[c4-bot-3]

Lines of code

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

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

For this reason, the disputeRedemption() is a core protector of the dUSD stable asset. However, I noticed the function was misdesigned, as all disputing parameters will be stored in plaintext.

This way, an attacker or MEV bot can easily observe/monitor the dispute transaction in the mempool, then front-run a legitimate disputer and take free awards. As a result, the legitimate disputer will spend a gas fee without receiving any award.

Moreover, the design flaw in question can disincentivize good disputers from doing their job, making the shortRecords with high CRs redeemed and closed positions more easily. When the shortRecords with high CRs are redeemed and closed positions instead of the shortRecords with low CRs, the Ditto protocol can confront bad debts easier, eventually leading to the depeg of the dUSD stable asset.

With the scenario described above, this issue deserves a high severity rating. For the solution, please refer to the Recommended Mitigation Steps section below.

Proof of Concept

As you can see in the below snippet, the disputeRedemption() receives disputing parameters (i.e., address asset, address redeemer, uint8 incorrectIndex, address disputeShorter, uint8 disputeShortId) in the plaintext form.

Thus, an attacker or MEV bot can easily steal all disputing parameters while the transaction is pending in the mempool and submit their own transaction with a higher gas fee instead.

This issue will finally disincentivize good disputers from doing their job in protecting the stability of the dUSD asset.

    //@audit -- An attacker or MEV bot can easily observe the dispute transaction in the mempool and front-run a legitimate disputer as all parameters are in plaintext (free money!!)
@>  function disputeRedemption(address asset, address redeemer, uint8 incorrectIndex, address disputeShorter, uint8 disputeShortId)
        external
        isNotFrozen(asset)
        nonReentrant
    {
        ...
    }

• An attacker or MEV bot can easily observe the dispute transaction in the mempool and front-run a legitimate disputer as all parameters are in plaintext (free money!!): https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/RedemptionFacet.sol#L224

Tools Used

Manual Review

Recommended Mitigation Steps

Apply the Commit-Reveal scheme (refs: https://ethglobal.com/showcase/commit-reveal-schema-exploration-nj9v7, https://www.gitcoin.co/blog/commit-reveal-scheme-on-ethereum) or Submarine Commitments scheme (ref: https://libsubmarine.org) to conceal good disputers' disputing parameters from attackers or MEV bots.

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #231

[raymondfam]

See #231.

[c4-judge]

hansfriese marked the issue as unsatisfactory: Out of scope

[serial-coder]

Hi @hansfriese,

As per the comment of the lookout in #231:

issues related to front-running are known issues per readme.

And, as per the audit readme:

Issues related to front-running: can front-run someone's order, liquidation, the chainlink/uniswap oracle update.

Appeal

1. The readme didn't explicitly specify the "redemption" feature as a known issue.
2. The "redemption" is a new feature that has never been audited elsewhere. So, how can this issue be invalidated as an already known issue?
3. If this was an already known issue, why did the sponsor choose the design that made another explicitly known issue in the first place? (please refer to the following to better understand its impacts)

The following is an excerpt from the Impact section:

This way, an attacker or MEV bot can easily observe/monitor the dispute transaction in the mempool, then front-run a legitimate disputer and take free awards. As a result, the legitimate disputer will spend a gas fee without receiving any award.

Moreover, the design flaw in question can disincentivize good disputers from doing their job, making the shortRecords with high CRs redeemed and closed positions more easily. When the shortRecords with high CRs are redeemed and closed positions instead of the shortRecords with low CRs, the Ditto protocol can confront bad debts easier, eventually leading to the depeg of the dUSD stable asset.

This issue disincentivizes good disputers from doing their job. Subsequently, the shortRecords with high CRs will be redeemed and closed positions instead of the shortRecords with low CRs. Finally, this issue can lead to the collapse of the dUSD stable asset.

For this reason, this issue should never be flagged as an already known issue and deserves a high severity rating.

[hansfriese]

I agree the redemption part wasn't included in the known issues(by fault). But I still believe it's fair to treat it as OOS.

FYI, here is the sponsor's reply.

My view is that it should be still considered out of scope. I mostly just copied it from the last audit so I didn't mean to exclude redemptions from frontrunning even if it's not there. I have no mechanisms for dealing with that issue in any of the system and have no plans to. I think having to change code to commit reveal as everyone suggests isn't helpful (this happens in every audit it seems), and the user/protocol can use something like flashbots to get around it. Also plenty of protocols would have the same issue around timing. In the particular case of redemptions, a bot doesn't even need to wait around for a dispute to come through, they should just calculate the dispute themselves based on an invalid slate and call it, no need to frontrun. it doesn't prevent people from calling dispute, esp if it's their own short. It feels like my takeaway for future audits is more that for "known issues" I should be less explicit so that it covers more scope? I'll have to just put "frontrunning" or generic words. I don't get that the implication of not specifying it means someone can report frontrunning so easily, because something involves timing.