Platform: Code4rena
Start Date: 11/01/2023
Pot Size: $60,500 USDC
Total HM: 6
Participants: 69
Period: 6 days
Judge: Trust
Total Solo HM: 2
Id: 204
League: ETH
Rank: 2/69
Findings: 2
Award: $7,517.41
🌟 Selected for report: 1
🚀 Solo Findings: 1
🌟 Selected for report: hansfriese
7481.1721 USDC - $7,481.17
Sanctioned user's funds are locked
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: }
Manual Review
Assuming that the MANAGER_ADMIN can be trusted, I suggest removing KYC check for the redeemers and refundees.
#0 - c4-judge
2023-01-22T16:37:34Z
trust1995 marked the issue as primary issue
#1 - c4-judge
2023-01-22T16:37:38Z
trust1995 marked the issue as satisfactory
#2 - ali2251
2023-01-31T15:48:16Z
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
#3 - c4-sponsor
2023-01-31T15:48:26Z
ali2251 marked the issue as sponsor disputed
#4 - trust1995
2023-02-01T07:26:27Z
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.
#5 - c4-judge
2023-02-01T07:49:42Z
trust1995 marked the issue as selected for report
#6 - cameronclifton
2023-02-22T16:16:22Z
@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?
#7 - trust1995
2023-02-23T18:47:46Z
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.
#8 - cameronclifton
2023-02-23T19:49:29Z
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 [....]"
#9 - hansfriese
2023-02-24T12:36:41Z
@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
🌟 Selected for report: CodingNameKiki
Also found by: 0x1f8b, 0x52, 0x5rings, 0xAgro, 0xSmartContract, 0xcm, 0xkato, 2997ms, Aymen0909, BClabs, BPZ, BRONZEDISC, Bauer, Bnke0x0, Deekshith99, IllIllI, Josiah, Kaysoft, RaymondFam, Rolezn, SaeedAlipoor01988, Tajobin, Udsen, Viktor_Cortess, adriro, arialblack14, betweenETHlines, btk, chaduke, chrisdior4, cryptphi, csanuragjain, cygaar, defsec, descharre, erictee, gzeon, hansfriese, horsefacts, joestakey, koxuan, lukris02, luxartvinsec, nicobevi, oyc_109, pavankv, peanuts, rbserver, scokaf, shark, tnevler, tsvetanovv, zaskoh
36.2377 USDC - $36.24
The protocol can not be used with some collaterals.
In the initialization process, the protocol calculates the decimalsMultiplier that are used later to convert from the collateral token amount to cash token amount.
CashManager.sol 179: decimalsMultiplier = 180: 10 ** 181: (IERC20Metadata(_cash).decimals() - 182: IERC20Metadata(_collateral).decimals());
This implementation reverts for the collaterals with decimals greater than 18 and the protocol can not support some collaterals, it is an unnecessary contract level restriction for the future expansion.
Manual Review
Add a new parameter to store the relationship between the two token decimals and use it properly for conversions.
#0 - c4-judge
2023-01-23T14:17:43Z
trust1995 changed the severity to QA (Quality Assurance)
#1 - c4-judge
2023-01-23T14:17:48Z
trust1995 marked the issue as grade-b