c4-bot-3 commented 5 months ago

Lines of code

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

Vulnerability details

When multiple users attempt to dispute the same redemption proposal simultaneously, race condition will occur in the following scenario:

a. User A and User B both identify an incorrect redemption proposal and decide to dispute it. b. User A and User B both retrieve the redeemer's asset user record and the disputing ShortRecord. c. User A and User B both check the validity of their disputes and find them to be valid. d. User A and User B both proceed to remove the incorrect proposals and update the state variables.

The fact that the disputeRedemption function does not handle concurrent disputes properly. If User A and User B's transactions are executed in quick succession or included in the same block, the state updates performed by one user may be overwritten by the other, leading to inconsistent or incorrect dispute resolutions.

// Remove the incorrect proposals and update state variables
259:  if (disputeCR < incorrectProposal.CR && disputeSR.updatedAt + C.DISPUTE_REDEMPTION_BUFFER <= redeemerAssetUser.timeProposed) {
// ...

This performs the state updates without considering the possibility of concurrent disputes.

disputeRedemption function is to allow users to dispute redemption proposals by providing a ShortRecord with a lower collateral ratio (CR) than the one being disputed.

asset: The address of the asset being disputed.
redeemer: The address of the user who proposed the redemption.
incorrectIndex: The index of the incorrect proposal in the redeemer's proposal list.
disputeShorter: The address of the shorter associated with the disputing ShortRecord.
disputeShortId: The ID of the disputing ShortRecord.

RedemptionFacet.sol#disputeRedemption

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

    // Check if the dispute is valid
    if (disputeCR < incorrectProposal.CR && disputeSR.updatedAt + C.DISPUTE_REDEMPTION_BUFFER <= redeemerAssetUser.timeProposed) {      <----------@
        // Remove the incorrect proposals and update state variables
        // ...
    } else {
        revert Errors.InvalidRedemptionDispute();
    }
}

The function first retrieves the necessary data, such as the redeemer's asset user record (redeemerAssetUser) and the disputing ShortRecord (disputeSR). It then checks if the dispute is valid by comparing the CR of the disputing ShortRecord with the CR of the incorrect proposal. If the dispute is valid, the function removes the incorrect proposals from the redeemer's proposal list and updates the relevant state variables.

The disputeRedemption function is expected to allow users to dispute redemption proposals one at a time. When a user submits a valid dispute, the function should remove the incorrect proposals from the redeemer's proposal list, update the relevant state variables, and ensure that the dispute resolution is accurate and consistent.

The expected input values for the function are:

asset: A valid asset address.
redeemer: The address of the user who proposed the redemption.
incorrectIndex: A valid index of the incorrect proposal in the redeemer's proposal list.
disputeShorter: The address of the shorter associated with a valid disputing ShortRecord.
disputeShortId: The ID of a valid disputing ShortRecord.

The intended outcome is to successfully resolve the dispute by removing the incorrect proposals and updating the state variables accurately.

The disputeRedemption function deviates from the expected behavior when multiple users attempt to dispute the same proposal simultaneously:

a. Multiple users may successfully pass the validity check for their disputes. b. The state updates performed by one user may be overwritten or interfered with by the updates performed by another user. c. The final state of the redeemer's proposal list and the associated state variables may be inconsistent or incorrect. d. Some valid disputes may be lost or not properly recorded due to the race condition.

As a result, the dispute resolution process becomes unreliable, and the integrity of the redemption mechanism is compromised.

Impact

The state of the redeemer's proposal list and the associated state variables may become inconsistent or corrupted due to concurrent dispute attempts.

Tools Used

Manual Review

Recommended Mitigation Steps

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

+    // Acquire a lock for the redeemer's proposal list
+    require(lock.acquire(redeemer), "Dispute lock failed");

>    // Check if the dispute is valid
    if (disputeCR < incorrectProposal.CR && disputeSR.updatedAt + C.DISPUTE_REDEMPTION_BUFFER <= redeemerAssetUser.timeProposed) {
>        // Remove the incorrect proposals and update state variables
        // ...

+        // Release the lock
+        lock.release(redeemer);
    } else {
        // Release the lock and revert
+        lock.release(redeemer);
        revert Errors.InvalidRedemptionDispute();
    }
}

b. Queue System:

Implement a queue system to handle multiple dispute attempts for the same redemption proposal.
When a dispute is submitted, add it to a queue associated with the specific proposal.
Process the disputes in the queue one by one, ensuring that each dispute is fully resolved before moving on to the next one.

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

+    // Add the dispute to the queue
+    disputeQueue[redeemer].push(DisputeData({
+        asset: asset,
+        incorrectIndex: incorrectIndex,
+        disputeShorter: disputeShorter,
+        disputeShortId: disputeShortId
    }));

+    // Process the dispute queue
+    processDisputeQueue(redeemer);
}

function processDisputeQueue(address redeemer) internal {
    // Process disputes in the queue one by one
    while (disputeQueue[redeemer].length > 0) {
        DisputeData memory dispute = disputeQueue[redeemer][0];

        // Check if the dispute is valid
        if (disputeCR < incorrectProposal.CR && disputeSR.updatedAt + C.DISPUTE_REDEMPTION_BUFFER <= redeemerAssetUser.timeProposed) {
            // Remove the incorrect proposals and update state variables
            // ...
        }

        // Remove the processed dispute from the queue
        disputeQueue[redeemer].pop();
    }
}

Assessed type

Context

c4-pre-sort commented 5 months ago

raymondfam marked the issue as insufficient quality report

c4-pre-sort commented 5 months ago

raymondfam marked the issue as duplicate of #231

raymondfam commented 5 months ago

See #231.

c4-judge commented 4 months ago

hansfriese marked the issue as unsatisfactory: Out of scope

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