There is a race condition between setPendingRedemptionBalance() and completeRedemptions()

[code423n4]

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding.
There is a race condition between setPendingRedemptionBalance() and completeRedemptions(). A a result, a redeemer might redeem more tokens than he/she deserves.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.
https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/contracts/cash/CashManager.sol#L707-L713
https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/contracts/cash/CashManager.sol#L851-L855

The race condition is illustrates as follows:
Suppose a user has redemptionInfoPerEpoch[epochToService].addressToBurnAmt[redeemer] = 1000, and
setPendingRedemptionBalance() likes to change the balance to 1500. Instead of allowing the redeemer to redeem either 1000 or 1500 (depending on when setPendingRedemptionBalance() is called), it is possible that a redeemer might redeem 1000+1500 CASH see below:

1. there are two MANAGER_ADMINs A and B, and they are not well communicating and coordinating, A will call completeRedemptions() and B will call setPendingRedemptionBalance().
2. Both A and B see that redemptionInfoPerEpoch[epochToService].addressToBurnAmt[redeemer] = 1000.
3. A calls completeRedemptions(), and now the redeemer gets 1000 CASH.
4. B calls setPendingRedemptionBalance() and now redemptionInfoPerEpoch[epochToService].addressToBurnAmt[redeemer] = 1500``.
5. The redeemer requests A to call completeRedemptions() again, and the redeemer gets another 1500 CASH.
6. Finally the redeemer gets 1000+1500 = 2500 CASH.

Tools Used

Remix

Recommended Mitigation Steps

To avoid this race condition, setPendingRedemptionBalance() needs to have the oldBalance as the argument and check whether it is equal to redemptionInfoPerEpoch[epochToService].addressToBurnAmt[redeemer] and only proceed when they are equal.

 function setPendingRedemptionBalance(
    address user,
    uint256 epoch,
    uint256 oldBalance,
    uint256 newBalance
  ) external updateEpoch onlyRole(MANAGER_ADMIN) {
    if (epoch > currentEpoch) {
      revert CannotServiceFutureEpoch();
    }
    uint256 previousBalance = redemptionInfoPerEpoch[epoch].addressToBurnAmt[
      user
    ];

    if(previousBalance != oldBalance) revert OldBalanceError();

    // Increment or decrement total burned for the epoch based on whether we
    // are increasing or decreasing the balance.
    if (newBalance < previousBalance) {
      redemptionInfoPerEpoch[epoch].totalBurned -= previousBalance - newBalance;
    } 
    else {
      redemptionInfoPerEpoch[epoch].totalBurned += newBalance - previousBalance;
    }
    redemptionInfoPerEpoch[epoch].addressToBurnAmt[user] = newBalance;
    emit PendingRedemptionBalanceSet(
      user,
      epoch,
      newBalance,
      redemptionInfoPerEpoch[epoch].totalBurned
    );
  }

[c4-judge]

trust1995 marked the issue as primary issue

[trust1995]

Would like sponsor's opinion if setPendingRedemptionBalance can be used in a way that this impact could occur.

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-sponsor]

ali2251 marked the issue as sponsor confirmed