Redeemer will pay extra `redemptionFee` when effective disputes occurs

[c4-bot-7]

Lines of code

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

Vulnerability details

Impact

When a redeemer submits proposals for redemption, they are charged a redemptionFee based on the total collateral RedemptionFacet#L204 summed across all the shortRecords proposals (p.totalColRedeemed and p.totalAmountProposed):

File: RedemptionFacet.sol
055:      */
056:     function proposeRedemption(
057:         address asset,
058:         MTypes.ProposalInput[] calldata proposalInput,
059:         uint88 redemptionAmount,
060:         uint88 maxRedemptionFee
061:     ) external isNotFrozen(asset) nonReentrant {
...
...
204:         uint88 redemptionFee = calculateRedemptionFee(asset, p.totalColRedeemed, p.totalAmountProposed);
205:         if (redemptionFee > maxRedemptionFee) revert Errors.RedemptionFeeTooHigh();
206: 
207:         STypes.VaultUser storage VaultUser = s.vaultUser[Asset.vault][msg.sender];
208:         if (VaultUser.ethEscrowed < redemptionFee) revert Errors.InsufficientETHEscrowed();
209:         VaultUser.ethEscrowed -= redemptionFee;
210:         emit Events.ProposeRedemption(p.asset, msg.sender);
211:     }

The issue arises when there are effective disputes, as the total amounts of collateral decrease since this collateral is returned to the shortRecord (RedemptionFacet#L267-L268):

File: RedemptionFacet.sol
224:     function disputeRedemption(address asset, address redeemer, uint8 incorrectIndex, address disputeShorter, uint8 disputeShortId)
225:         external
226:         isNotFrozen(asset)
227:         nonReentrant
228:     {
...
...
261:             // @dev All proposals from the incorrectIndex onward will be removed
262:             // @dev Thus the proposer can only redeem a portion of their original slate
263:             for (uint256 i = incorrectIndex; i < decodedProposalData.length; i++) {
264:                 currentProposal = decodedProposalData[i];
265: 
266:                 STypes.ShortRecord storage currentSR = s.shortRecords[d.asset][currentProposal.shorter][currentProposal.shortId];
267:                 currentSR.collateral += currentProposal.colRedeemed;
268:                 currentSR.ercDebt += currentProposal.ercDebtRedeemed;
269: 
270:                 d.incorrectCollateral += currentProposal.colRedeemed;
271:                 d.incorrectErcDebt += currentProposal.ercDebtRedeemed;
272:             }
...
...

Consequently, the redemptionFee paid by the redeemer would be higher when there are effective disputes. If the redeemer is already paying a penalty, it would be appropriate to refund the fee for the shortRecords proposals that will no longer be processed due to an effective dispute.

Proof of Concept

Consider the following scenario:

1. UserA submits proposals shortRecordsId 1 and 3, totaling a collateral to receive, for example, 10 ETH (5 ETH collateral for each proposal). Based on this number, the corresponding fee is calculated using the function calculateRedemptionFee(asset, p.totalColRedeemed, p.totalAmountProposed);
2. A Disputer submits shortRecordId=2 for dispute, and it is correct, causing shortRecordId=3 to be removed from UserA's slate, leaving only shortRecordId=1. This results in the redeemer receiving only 5 ETH as collateral for shortRecordId=1.
3. UserA calls claimRedemption, receiving only 5 ETH as collateral.

In the end, the user paid a redemptionFee as if they were going to receive 10 ETH, but ultimately only received 5 ETH. The redeemer is paying excess fees.

Tools used

Manual review

Recommended Mitigation Steps

Ensure that the redemption fee calculation accounts for effective disputes, adjusting the fee accordingly to prevent overcharging redeemer when disputes occur. If a redeemer is already paying a penalty, consider refunding the fee for the proposals that will not be processed due to an effective dispute. This adjustment will ensure fairness and transparency in the redemption process.

Assessed type

Context

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

That's the intended design discouraging the formulation of invalid proposals.

[c4-judge]

hansfriese marked the issue as unsatisfactory: Invalid