Improper Input Validation in `setRedeemLimit()` Function

[code423n4]

Lines of code

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

Vulnerability details

Impact

The setRedeemLimit function currently does not have any validation on the input _redeemLimit value. An attacker could potentially pass in a very large value, causing the redeemLimit variable to be set to a very large number and potentially allowing the attacker to redeem more tokens than intended.

Proof of Concept

An attacker, Bob, could potentially call the setRedeemLimit function and pass in a very high value for _redeemLimit, allowing him to redeem a large amount of cash tokens beyond the intended limit. If the function does not properly check and validate the input passed in by the attacker, it would allow Bob to redeem an arbitrarily large amount of cash tokens.

However, if a Bob with the MANAGER_ADMIN role can set the redeemLimit to zero, which would cause the check if (amount > redeemLimit - currentRedeemAmount) in the _checkAndUpdateRedeemLimit function to always fail, effectively disabling the redeem function.

Tools Used

manual review

Recommended Mitigation Steps

before

  function setRedeemLimit(
    uint256 _redeemLimit
  ) external onlyRole(MANAGER_ADMIN) {
    uint256 oldRedeemLimit = redeemLimit;
    redeemLimit = _redeemLimit;
    emit RedeemLimitSet(oldRedeemLimit, _redeemLimit);
  }

after

function setRedeemLimit(uint256 _redeemLimit) external onlyRole(MANAGER_ADMIN) {
    require(_redeemLimit > MINIMUM_REDEEM_LIMIT, "Redeem limit is too small");
    require(_redeemLimit < MAXIMUM_REDEEM_LIMIT, "Redeem limit is too large");
    uint256 oldRedeemLimit = redeemLimit;
    redeemLimit = _redeemLimit;
    emit RedeemLimitSet(oldRedeemLimit, _redeemLimit);
}

MINIMUM_REDEEM_LIMIT and MAXIMUM_REDEEM_LIMIT are global variables that have been set to reasonable values. The require statements check that the redeem limit is within the specified range, and if not, the function reverts and the transaction is invalidated.

In terms of the severity of the issue, it would likely be considered a medium severity issue as it could potentially be used to drain the contract's balance of tokens, but it would likely require a malicious actor to already have a significant amount of tokens and would not allow them to gain more than they already have.

[c4-judge]

trust1995 marked the issue as unsatisfactory: Out of scope

[c4-sponsor]

ypatil12 marked the issue as sponsor disputed

[ypatil12]

We assume MANAGER_ADMIN is not malicious