Setting mintFee to zero will break underlying tokens that don't support zero transfers

[code423n4]

Lines of code

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

Vulnerability details

Impact

Minting will break if mintFee is set to zero

Proof of Concept

uint256 feesInCollateral = _getMintFees(collateralAmountIn);
uint256 depositValueAfterFees = collateralAmountIn - feesInCollateral;

_checkAndUpdateMintLimit(depositValueAfterFees);

collateral.safeTransferFrom(msg.sender, feeRecipient, feesInCollateral);

CashManager#requestMint attempts to transfer fee to feeRecipient even if there is no fee to transfer (i.e. mintFee == 0). This will break minting for tokens that do not support zero value transfers if mintFee == 0.

Tools Used

Manual Review

Recommended Mitigation Steps

Only transfer fees if there are fees to transfer:

    uint256 feesInCollateral = _getMintFees(collateralAmountIn);
    uint256 depositValueAfterFees = collateralAmountIn - feesInCollateral;


    _checkAndUpdateMintLimit(depositValueAfterFees);

-   collateral.safeTransferFrom(msg.sender, feeRecipient, feesInCollateral);

+   if(feesInCollateral != 0) {
+       collateral.safeTransferFrom(msg.sender, feeRecipient, feesInCollateral);
+   }

[c4-judge]

trust1995 changed the severity to QA (Quality Assurance)

[c4-judge]

trust1995 marked the issue as grade-b