code423n4 commented 1 year ago

Lines of code

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

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
  uint256 refundedAmt = _processRefund(refundees, epochToService);

  ////////////////////////////////////////////////////////////////
  // @audit If one epoch need at least 2 calls,
  // `quantityBurned` will be wrong because `totalBurned` is not updated
  ////////////////////////////////////////////////////////////////
  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.

Basically, function completeRedemptions() first process all the refund, then distribute collateralAmountToDist substracted fees to all the redeemers based on the amount of CASH they burned and total burned CASH. However, since after refunding, totalBurned of epoch is not updated, it will results in wrong value of totalBurned for next calls.

Proof of Concept

Consider the scenario

Assuming Alice, Bob and Caleb both sended redemption requests in epoch X with following amount: 100, 100, 100. So totalBurned = 300.
Admin does the redemption and only requests of Alice and Bob can be serviced, Caleb's request will be refunded. And let's assume exchange rate is 1-1 and fees = 0
Admin call completeRedemptions([Alice], [Caleb], 200, X, 0), in this calls, Alice and Caleb will received the correct amount of collateral back. Note that totalBurned is not updated so its value is still totalBurned = 300.

Admin call completeRedemptions([Bob], 200, X, 0). Now we can see

quantityBurned = totalBurned - refundedAmt = 300
amountToDist = 200 - fees = 200
collateralAmountDue = (amountToDist * cashAmountReturned) / quantityBurned 
= (200 * 100) / 300 = 66.67

At the end, Bob only received 66.67 collateral token back when he should receive 100 token like Alice

Tools Used

Manual Review

Recommended Mitigation Steps

Consider updating value of redemptionInfoPerEpoch[epochToService].totalBurned after each time function completeRedemptions() is called.

c4-judge commented 1 year ago

trust1995 marked the issue as duplicate of #325

c4-judge commented 1 year ago

trust1995 marked the issue as satisfactory

tom2o17 commented 1 year ago

Second call should have collateralAmountToDist == 300

cc @ypatil12

c4-sponsor commented 1 year ago

tom2o17 marked the issue as sponsor acknowledged

code-423n4 / 2023-01-ondo-findings