`MintFees` can change by admin any time, lead to user may spend more fee than expected

[code423n4]

Lines of code

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

Vulnerability details

function requestMint(
    uint256 collateralAmountIn
  )
    external
    override
    updateEpoch
    nonReentrant
    whenNotPaused
    checkKYC(msg.sender)
  {
    if (collateralAmountIn < minimumDepositAmount) {
      revert MintRequestAmountTooSmall();
    }

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

    _checkAndUpdateMintLimit(depositValueAfterFees);

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

    mintRequestsPerEpoch[currentEpoch][msg.sender] += depositValueAfterFees;

    emit MintRequested(
      msg.sender,
      currentEpoch,
      collateralAmountIn,
      depositValueAfterFees,
      feesInCollateral
    );
  }

Impact

Users who request mint during the period when the admin is actively adjusting MintFees cannot clearly limit the maximum range of MintFees, resulting in completing the transaction with unexpected trading conditions.

Proof of Concept

Given:

• mintFee = 0

1. Alice call requestMint() with collateralAmountIn = 10,000 try deposit 10,000 collateral

2. Admin call setMintFee() change mintFee to 1,000 bps, or 10%, with higher gas price than Alice

3. mintFee change transaction was executed faster than Alice's tx because it was given a higher gas price.

4. When Alice's requestMint() is executed, a 10% mint fee will be charged which is not what Alice expected when she submitted the transaction. If the fee is higher than 1%, Alice will not submit the transaction.

Recommended Mitigation Steps

requestMint() should privede a minDepositValueAfterFees as slippage control

[c4-judge]

trust1995 changed the severity to QA (Quality Assurance)

[c4-judge]

trust1995 marked the issue as grade-b