Attacker exploiting reentrancy can claim excessive collateral, disrupting redemption process.

[c4-bot-2]

Lines of code

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

Vulnerability details

Summary & Impact

claimRedemption allows a redeemer to claim the collateral from the verified redemption candidates after the dispute period has passed. It takes the following parameter:

• asset: The market that will be impacted.

As can be seen, the function performs the following key steps:

1. Retrieves the redeemer's asset user and vault user structs.
2. Checks if the redeemer has a valid redemption proposal and if the dispute period has elapsed.
3. Reads the redemption proposal data using SSTORE2.
4. Iterates through the redemption proposal data and calls the _claimRemainingCollateral function for each verified redemption candidate.
5. Updates the redeemer's vault user struct with the total redeemed collateral.
6. Deletes the redeemer's asset user struct's SSTORE2 pointer.

RedemptionFacet.sol::claimRedemption

function claimRedemption(address asset) external isNotFrozen(asset) nonReentrant {
    uint256 vault = s.asset[asset].vault;
    STypes.AssetUser storage redeemerAssetUser = s.assetUser[asset][msg.sender];
    STypes.VaultUser storage redeemerVaultUser = s.vaultUser[vault][msg.sender];
    if (redeemerAssetUser.SSTORE2Pointer == address(0)) revert Errors.InvalidRedemption();
    if (LibOrders.getOffsetTime() < redeemerAssetUser.timeToDispute) revert Errors.TimeToDisputeHasNotElapsed();

    MTypes.ProposalData[] memory decodedProposalData =
        LibBytes.readProposalData(redeemerAssetUser.SSTORE2Pointer, redeemerAssetUser.slateLength);

    uint88 totalColRedeemed;
    for (uint256 i = 0; i < decodedProposalData.length; i++) {
        MTypes.ProposalData memory currentProposal = decodedProposalData[i];
        totalColRedeemed += currentProposal.colRedeemed;
        _claimRemainingCollateral({
            asset: asset,
            vault: vault,
            shorter: currentProposal.shorter,
            shortId: currentProposal.shortId
        });
    }
    redeemerVaultUser.ethEscrowed += totalColRedeemed;
    delete redeemerAssetUser.SSTORE2Pointer;
    emit Events.ClaimRedemption(asset, msg.sender);
}

Had it been there are proper reentrancy guards in the claimRedemption function. Although the function is marked with the nonReentrant modifier, it still calls an external function (_claimRemainingCollateral) before updating the redeemer's vault user struct and deleting the asset user struct's SSTORE2 pointer.

If the _claimRemainingCollateral function or any other function called within the loop is malicious or can be controlled by an attacker, it could potentially exploit the reentrancy vulnerability by recursively calling the claimRedemption function before the state is fully updated.

RedemptionFacet.sol::L324-L329

_claimRemainingCollateral({
    asset: asset,
    vault: vault,
    shorter: currentProposal.shorter,
    shortId: currentProposal.shortId
});

By recursively calling the claimRedemption function within the _claimRemainingCollateral function or any other function called in the loop. This can lead to unexpected behavior and potential manipulation of the redemption outcomes.

For example, an attacker can create a malicious contract that implements a fallback function or a specific function that is called by _claimRemainingCollateral. Inside this function, the attacker can recursively call claimRedemption with the same or different parameters, leading to multiple redemptions before the state is fully updated.

Impact

By exploiting the reentrancy vulnerability, attackers can potentially claim more collateral than intended or disrupt the redemption process for other users.

Tools Used

Vs

Recommended Mitigation Steps

a. Apply the checks-effects-interactions pattern:

• Perform all necessary checks and validations first.
• Update the state variables and perform internal actions.
• Interact with external contracts or functions last.

b. Use a reentrancy guard modifier:

• Implement a reentrancy guard modifier that prevents recursive calls to the function.
• Apply the modifier to the claimRedemption function.

bool private locked;

modifier noReentrant() {
    require(!locked, "Reentrancy detected");
    locked = true;
    _;
    locked = false;
}

function claimRedemption(address asset) external isNotFrozen(asset) noReentrant {
    uint256 vault = s.asset[asset].vault;
    STypes.AssetUser storage redeemerAssetUser = s.assetUser[asset][msg.sender];
    STypes.VaultUser storage redeemerVaultUser = s.vaultUser[vault][msg.sender];
    if (redeemerAssetUser.SSTORE2Pointer == address(0)) revert Errors.InvalidRedemption();
    if (LibOrders.getOffsetTime() < redeemerAssetUser.timeToDispute) revert Errors.TimeToDisputeHasNotElapsed();

    MTypes.ProposalData[] memory decodedProposalData =
        LibBytes.readProposalData(redeemerAssetUser.SSTORE2Pointer, redeemerAssetUser.slateLength);

    uint88 totalColRedeemed;
    for (uint256 i = 0; i < decodedProposalData.length; i++) {
        MTypes.ProposalData memory currentProposal = decodedProposalData[i];
        totalColRedeemed += currentProposal.colRedeemed;
    }

    // Update state variables first
    redeemerVaultUser.ethEscrowed += totalColRedeemed;
    delete redeemerAssetUser.SSTORE2Pointer;

    // Interact with external contracts last
    for (uint256 i = 0; i < decodedProposalData.length; i++) {
        MTypes.ProposalData memory currentProposal = decodedProposalData[i];
        _claimRemainingCollateral({
            asset: asset,
            vault: vault,
            shorter: currentProposal.shorter,
            shortId: currentProposal.shortId
        });
    }

    emit Events.ClaimRedemption(asset, msg.sender);
}

In this modified version:

• The noReentrant modifier is added to prevent recursive calls to the function.
• The state variables (redeemerVaultUser.ethEscrowed and redeemerAssetUser.SSTORE2Pointer) are updated before interacting with external contracts.
• The _claimRemainingCollateral function is called after updating the state variables.

Assessed type

Reentrancy

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #69

[raymondfam]

See #69. Illogical reentrancy exploit.

[c4-judge]

hansfriese marked the issue as unsatisfactory: Invalid