Admin should be able to refund or redeem the sanctioned users

[code423n4]

Lines of code

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

Vulnerability details

Impact

Sanctioned user's funds are locked

Proof of Concept

It is understood that the sanctioned users can not mint nor redeem because the functions requestMint() and requestRedemption() are protected by the modifier checkKYC().
And it is also understood that the protocol team knows about this.
But I still believe the admin should be able to refund or redeem those funds.
And it is not possible for now because the KYC is checked for the redeemers and refundees in the function completeRedemptions().
So as long as the user becomes unverified (due to several reasons including the signature expiry), the funds are completely locked and even the admin has no control over it.

CashManager.sol
707:   function completeRedemptions(
708:     address[] calldata redeemers,
709:     address[] calldata refundees,
710:     uint256 collateralAmountToDist,
711:     uint256 epochToService,
712:     uint256 fees
713:   ) external override updateEpoch onlyRole(MANAGER_ADMIN) {
714:     _checkAddressesKYC(redeemers);
715:     _checkAddressesKYC(refundees);
716:     if (epochToService >= currentEpoch) {
717:       revert MustServicePastEpoch();
718:     }
719:     // Calculate the total quantity of shares tokens burned w/n an epoch
720:     uint256 refundedAmt = _processRefund(refundees, epochToService);
721:     uint256 quantityBurned = redemptionInfoPerEpoch[epochToService]
722:       .totalBurned - refundedAmt;
723:     uint256 amountToDist = collateralAmountToDist - fees;
724:     _processRedemption(redeemers, amountToDist, quantityBurned, epochToService);
725:     collateral.safeTransferFrom(assetSender, feeRecipient, fees);
726:     emit RedemptionFeesCollected(feeRecipient, fees, epochToService);
727:   }

Tools Used

Manual Review

Recommended Mitigation Steps

Assuming that the MANAGER_ADMIN can be trusted, I suggest removing KYC check for the redeemers and refundees.

[c4-judge]

trust1995 marked the issue as primary issue

[c4-judge]

trust1995 marked the issue as satisfactory

[ali2251]

Its not in scope as mentioned in README, specifically in Not in scope ->

KYC/Sanction related edge cases specifically when a user’s KYC status or Sanction status changes in between different actions, leaving them at risk of their funds being locked in the protocols or being liquidated in Flux

[c4-sponsor]

ali2251 marked the issue as sponsor disputed

[trust1995]

Its not in scope as mentioned in README, specifically in Not in scope ->

KYC/Sanction related edge cases specifically when a user’s KYC status or Sanction status changes in between different actions, leaving them at risk of their funds being locked in the protocols or being liquidated in Flux

I don't believe this clause includes the described case, i.e. even admin cannot move the locked funds.

[c4-judge]

trust1995 marked the issue as selected for report

[cameronclifton]

@trust1995 the out of scope section includes the words:
"[...] when a user’s KYC status or Sanction status changes in between different actions, leaving them at risk of their funds being locked in the protocols [....]"

Can you please explain how this is not the exact same scenario that we declare out of scope?

[trust1995]

The KYC edge cases disclaimer does not include funds being permanently locked in the protocol. It relates to the fact that when users lose their KYC or are sanctioned, their funds are locked in (from their perspective). The fact those funds can't be claimed by delegated authorities is concerning.

[cameronclifton]

While we see your concern, this is the intended design and we feel you are misinterpreting the out of scope section. There is nothing in the out of scope section that has an exception clause relating to whose perspective the funds are locked from.

Maybe an example could help clear up the miscommunication going on - Could you please explain an "in scope" scenario here that is not covered by the following?

""[...] when a user’s KYC status or Sanction status changes in between different actions, leaving them at risk of their funds being locked in the protocols [....]"

[hansfriese]

@cameronclifton I will explain the original intention of this submission as an auditor.
As I mentioned in the finding details (and the judge @trust1995 also commented), my main point was that the funds are permanently locked in the protocol. As far as I understand, the scope clarification says it is not in scope that the funds are locked in the user's perspective. The two example cases show this point clearly.
I wanted to point out that there should be a way to handle those funds NOT from the user's perspective, but from the protocol admin's perspective. I believe it is not a good practice to let funds be locked PERMANENTLY.

KYC/Sanction related edge cases specifically when a user’s KYC status or Sanction status changes in between different actions, leaving them at risk of their funds being locked in the protocols or being liquidated in Flux.

• If someone gets sanctioned they can not supply collateral (CASH or stablecoin)
• If someone loses KYC status they can not repay borrow or have someone repay borrow on behalf of them