If completeRedemptions is called multiple times to redeem one epoch, the complex calculation may result in incorrect redemptions

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/contracts/cash/CashManager.sol#L707-L727

Vulnerability details

Impact

When MANAGER_ADMIN calls the completeRedemptions function, it requires that both redeemers and refundees have KYC. If the redeemer loses KYC, the redeemer's address will not appear in redeemers and refundees, otherwise completeRedemptions will fail.

  function completeRedemptions(
    address[] calldata redeemers,
    address[] calldata refundees,
    uint256 collateralAmountToDist,
    uint256 epochToService,
    uint256 fees
  ) external override updateEpoch onlyRole(MANAGER_ADMIN) {
    _checkAddressesKYC(redeemers);
    _checkAddressesKYC(refundees);

If too many users request redemptions or if users lose their KYC, completeRedemptions will have to be called multiple times.
However, completeRedemptions uses totalBurned as a calculation factor when calculating the redeemer's share, the complex calculation may result in incorrect redemptions

    uint256 quantityBurned = redemptionInfoPerEpoch[epochToService]
      .totalBurned - refundedAmt;
    uint256 amountToDist = collateralAmountToDist - fees;
    _processRedemption(redeemers, amountToDist, quantityBurned, epochToService);

Consider the following case.
alice/bob/charlie each request 10000 cash redemptions, totalBurned == 30000.
Later charlie loses KYC for some reason and does not know when it will be resumed.
MANAGER_ADMIN considers redeeming for alice/bob first.
Consider a redemption fee of 1% and cash:usdc = 1:1, at which point alice/bob should get 9900 usdc each.
The collateralAmountToDist parameter that needs to be provided to make completeRedemptions work as expected requires a complex calculation.
The fees parameter is fixed at 200.
(collateralAmountToDist - fees) * redeem[alice] / (totalBurned - refundedAmt) = (collateralAmountToDist - 200)* 10000 /30000 = 9990
collateralAmountToDist = 29970.

  function _processRedemption(
    address[] calldata redeemers,
    uint256 amountToDist, // 298
    uint256 quantityBurned,  // 300
    uint256 epochToService
  ) private {
    uint256 size = redeemers.length;
    for (uint256 i = 0; i < size; ++i) {
      address redeemer = redeemers[i];
      uint256 cashAmountReturned = redemptionInfoPerEpoch[epochToService]
        .addressToBurnAmt[redeemer];
      redemptionInfoPerEpoch[epochToService].addressToBurnAmt[redeemer] = 0;
      uint256 collateralAmountDue = (amountToDist * cashAmountReturned) /
        quantityBurned;

This is a simplified case, the real case is more complicated. This is because fees need to take into account the address where the redemption actually took place, but totalBurned contains the user who lost the KYC.

Another possible scenario is that the collateralAmountToDist parameter is directly proportional to the fees without complex calculations, which could result in the user losing assets or charging more fees(Consider the case of 20000/200, 30000/300).

Proof of Concept

https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/contracts/cash/CashManager.sol#L707-L727
https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/contracts/cash/CashManager.sol#L743-L770

Tools Used

None

Recommended Mitigation Steps

Consider adding an claimedCach parameter to the completeRedemptions function to indicate the number of tokens that will be redeemed, and use claimedCach instead of totalBurned as the calculation factor to calculate the share of redeemers
and require that the claimedCash be equal to the addressToBurnAmt of redeemers

[c4-judge]

trust1995 marked the issue as duplicate of #325

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-judge]

trust1995 changed the severity to 3 (High Risk)