code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

Function completeRedemptions() is used by admin account to distribute collateral to users and also to refund redemption requests if the redemption cannot be serviced.

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 (epochToService >= currentEpoch) {
    revert MustServicePastEpoch();
  }
  // Calculate the total quantity of shares tokens burned w/n an epoch

  // @audit Wrong value of quantityBurned if `refundees` list is empty
  uint256 refundedAmt = _processRefund(refundees, epochToService);
  uint256 quantityBurned = redemptionInfoPerEpoch[epochToService]
    .totalBurned - refundedAmt; 
  uint256 amountToDist = collateralAmountToDist - fees;
  _processRedemption(redeemers, amountToDist, quantityBurned, epochToService);
  collateral.safeTransferFrom(assetSender, feeRecipient, fees);
  emit RedemptionFeesCollected(feeRecipient, fees, epochToService);
}

Since in each epoch there might be many users send redemption requests, so to avoid breaking block gas limit, sponsor confirmed that each epoch will require multiple calls to function completeRedemptions() with collateralAmountToDist stay constant in every call.

If function completeRedemptions() is called with empty refundees in any TX, every redeemers in that call will receive less collateral back than expected

Proof of Concept

Consider the scenario

Assuming Alice, Bob both sended redemption requests in epoch X with following amount: 100, 100. So totalBurned = 200.
Admin does the redemption and only requests of Alice can be serviced, bob's request will be refunded. And let's assume exchange rate is 1-1 and fees = 0
Admin call completeRedemptions([], [Bob], 100, X, 0) first to process all refuding. Bob will be minted 100 CASH back.
Admin call completeRedemptions([Alice], [], 100, X, 0), in this calls, because refundees list is empty, so quantityBurned will be calculated wrongly
```
quantityBurned = totalBurned - 0 = 200
amountToDist = 100
Alice_received = (100 * 100 / 200) = 50
```

So Alice only receive 50 instead of 100 collateral token back.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding a totalRefundAmt input param instead of calculating it using function _processRefund() in every call

c4-judge commented 1 year ago

trust1995 marked the issue as duplicate of #325